Bug 29799

Summary: Add tests for TLS server channels
Product: Telepathy Reporter: Cosimo Cecchi <cosimoc>
Component: gabbleAssignee: Cosimo Cecchi <cosimoc>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: will
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=shortlog;h=refs/heads/tls-tests-2
Whiteboard: review+
i915 platform: i915 features:

Description Cosimo Cecchi 2010-08-25 08:01:25 UTC
This branch of mine adds them [1].

[1] http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=shortlog;h=refs/heads/tls-tests
Comment 1 Will Thompson 2010-09-14 07:54:26 UTC
Pleases rename the test to tls/server-tls-channel.py (removing the 'test-' prefix)? It's obviously a test. It's in the directory full of tests. (I know other tests violate this.)


+    def auth(self, auth):
+        assert (base64.b64decode(str(auth)) ==
+            '\x00%s\x00%s' % (self.username, self.password))
+
+        success = domish.Element((ns.NS_XMPP_SASL, 'success'))
+        self.xmlstream.send(success)
+        self.xmlstream.reset()
+        self.sasl_authenticated = True

Can't this subclass XmppAuthenticator and chain up to the superclass rather than re-implementing it all? Ditto the streamStarted() method; it seems to copy-paste a lot of code. bindIq() is literally a verbatim copy.

+        file = open(CA_KEY, 'rb')
+        pem_key = file.read()
+        file.close()
+        pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "")

Better written as:

  with open(CA_KEY, 'rb') as file:
      pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "")


+    q.expect_many(
+        EventPattern('dbus-signal', signal='Rejected'),
+        EventPattern('dbus-signal', signal='Closed'),
+        EventPattern('dbus-signal', signal='ChannelClosed'),
+        EventPattern('dbus-signal', signal='ConnectionError',
+                     args=[cs.CERT_UNTRUSTED, {}]),
+        EventPattern('dbus-signal', signal='StatusChanged',
+                     args=[cs.CONN_STATUS_DISCONNECTED, cs.CSR_CERT_UNTRUSTED])
+        )

Don't we expect these to happen in a particular order? expect_many is for ignoring the order of events.


+    # we expect the fallback verification process to connect successfully

There should be a test for when it doesn't, presumably by setting { 'require-encryption': True, 'ignore-ssl-errors': False }. And, why do we expect the fallback verification process to connect successfully in this case? I think we should document exactly what domain the certificate is for, probably in this test, rather than relying on later readers asking you on IRC or digging out a tool to examine certificates.

+    assertEquals (len(rejections), 0)

assertLength(0, rejections)

Python coding style doesn't put a space before the opening paren.
Comment 2 Cosimo Cecchi 2010-09-14 10:32:36 UTC
I fixed my branch according to your comments; another round of review welcome :)
Comment 3 Will Thompson 2010-09-14 10:55:54 UTC
+        with open(CA_KEY, 'rb') as file:
+            pem_key = file.read()
+            file.close()
+            pkey = crypto.load_privatekey(crypto.FILETYPE_PEM, pem_key, "")

You don't need to call close(). That's one of the points of with: it's closed when you leave that scope.

As we discussed on IRC, don't avoid UTF-8, just add coding=utf-8 to the top of the comment.

http://git.collabora.co.uk/?p=user/cosimoc/telepathy-gabble.git;a=commitdiff;h=1fada095b5fb9215a75a5c3a5b18f604ee3abf5b quite visibly shows a spaces vs. tabs mix-up towards the end of the diff.
Comment 4 Cosimo Cecchi 2010-09-15 02:44:19 UTC
(In reply to comment #3)

Thanks for the review, I just updated my branch according to your comments.
Comment 5 Will Thompson 2010-09-15 03:40:37 UTC
Cool, ship it.

(In future, you could squash the remove-close() and go-back-to-UTF-8 patches into the relevant previous patches.)
Comment 6 Cosimo Cecchi 2010-09-15 04:02:01 UTC
(In reply to comment #5)
> Cool, ship it.
> 
> (In future, you could squash the remove-close() and go-back-to-UTF-8 patches
> into the relevant previous patches.)

Thanks, merged to master.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.