Bug 29799

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.

I fixed my branch according to your comments; another round of review welcome :)

+        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.

(In reply to comment #3)

Thanks for the review, I just updated my branch according to your comments.

Cool, ship it.

(In future, you could squash the remove-close() and go-back-to-UTF-8 patches into the relevant previous patches.)

(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.