Merge lp:~rvb/maas/bug-1437388 into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Add the client object to the list of "active" clients only after authentication succeeds. This fixes a race condition where the notifications would be passed on to a client before it's authenticated.

[Gavin Panella (allenap)]

Looks good, nicely sleuthed. One comment about handing errors, but otherwise it's grand.

review: Approve

[Raphaël Badin (rvb)]

Thanks for the review, I've fixed authenticate (using the decorator @deferred).

[Blake Rouse (blake-rouse)]

Looks good to me as well.

review: Approve

=== modified file 'src/maasserver/websockets/protocol.py'
--- src/maasserver/websockets/protocol.py	2015-04-16 07:38:04 +0000
+++ src/maasserver/websockets/protocol.py	2015-04-23 13:20:47 +0000
@@ -36,7 +36,10 @@
 from maasserver.websockets import handlers
 from maasserver.websockets.listener import PostgresListener
 from maasserver.websockets.websockets import STATUSES
-from provisioningserver.utils.twisted import synchronous
+from provisioningserver.utils.twisted import (
+    deferred,
+    synchronous,
+)
 from twisted.internet import reactor
 from twisted.internet.defer import inlineCallbacks
 from twisted.internet.protocol import (
@@ -89,21 +92,29 @@
 
     def connectionMade(self):
         """Connection has been made to client."""
-        self.factory.clients.append(self)
-
         # Using the provided cookies on the connection request, authenticate
         # the client. If this fails or if the CSRF token can't be found, it
         # will call loseConnection. A websocket connection is only allowed
         # from an authenticated user.
         cookies = self.transport.cookies
-        self.authenticate(
+        d = self.authenticate(
             get_cookie(cookies, 'sessionid'),
             get_cookie(cookies, 'csrftoken'),
         )
 
+        # Only add the client to the list of known clients if/when the
+        # authentication succeeds.
+        def add_client(user):
+            if user is not None:
+                self.factory.clients.append(self)
+        d.addCallback(add_client)
+
     def connectionLost(self, reason):
         """Connection to the client has been lost."""
-        self.factory.clients.remove(self)
+        # If the connection is lost before the authentication happens, the
+        # 'client' will not have been added to the list.
+        if self in self.factory.clients:
+            self.factory.clients.remove(self)
 
     def loseConnection(self, status, reason):
         """Close connection with status and reason."""
@@ -146,6 +157,7 @@
         else:
             return None
 
+    @deferred
     def authenticate(self, session_id, csrftoken):
         """Authenticate the connection.
 
@@ -170,6 +182,7 @@
                 self.user = user
 
             self.processMessages()
+            return self.user
 
         def got_user_error(failure):
             self.loseConnection(

=== modified file 'src/maasserver/websockets/tests/test_protocol.py'
--- src/maasserver/websockets/tests/test_protocol.py	2015-04-16 07:38:04 +0000
+++ src/maasserver/websockets/tests/test_protocol.py	2015-04-23 13:20:47 +0000
@@ -49,6 +49,7 @@
     Is,
     IsInstance,
 )
+from twisted.internet import defer
 from twisted.internet.defer import (
     fail,
     inlineCallbacks,
@@ -90,12 +91,23 @@
         call = protocol.transport.write.call_args_list.pop()
         return json.loads(call[0][0])
 
-    def test_connectionMade_adds_self_to_factory(self):
-        protocol, factory = self.make_protocol()
-        protocol.connectionMade()
-        self.addCleanup(lambda: protocol.connectionLost(""))
-        self.assertItemsEqual(
-            [protocol], factory.clients)
+    def test_connectionMade_adds_self_to_factory_if_auth_succeeds(self):
+        protocol, factory = self.make_protocol()
+        mock_authenticate = self.patch(protocol, "authenticate")
+        user = maas_factory.make_User()
+        mock_authenticate.return_value = defer.succeed(user)
+        protocol.connectionMade()
+        self.addCleanup(lambda: protocol.connectionLost(""))
+        self.assertItemsEqual([protocol], factory.clients)
+
+    def test_connectionMade_doesnt_add_self_to_factory_if_auth_fails(self):
+        protocol, factory = self.make_protocol()
+        mock_authenticate = self.patch(protocol, "authenticate")
+        fake_error = maas_factory.make_name()
+        mock_authenticate.return_value = defer.fail(Exception(fake_error))
+        protocol.connectionMade()
+        self.addCleanup(lambda: protocol.connectionLost(""))
+        self.assertNotIn(protocol, factory.clients)
 
     def test_connectionMade_extracts_sessionid_and_csrftoken(self):
         protocol, factory = self.make_protocol(patch_authenticate=False)
@@ -118,10 +130,16 @@
 
     def test_connectionLost_removes_self_from_factory(self):
         protocol, factory = self.make_protocol()
+        mock_authenticate = self.patch(protocol, "authenticate")
+        mock_authenticate.return_value = defer.succeed(None)
         protocol.connectionMade()
         protocol.connectionLost("")
-        self.assertItemsEqual(
-            [], factory.clients)
+        self.assertItemsEqual([], factory.clients)
+
+    def test_connectionLost_succeeds_if_client_hasnt_been_recorded(self):
+        protocol, factory = self.make_protocol()
+        self.assertIsNone(protocol.connectionLost(""))
+        self.assertItemsEqual([], factory.clients)
 
     def test_loseConnection_writes_to_log(self):
         protocol, factory = self.make_protocol()
@@ -571,8 +589,8 @@
         protocol.transport = MagicMock()
         if user is None:
             user = maas_factory.make_User()
-        protocol.user = user
-        self.patch(protocol, "authenticate")
+        mock_authenticate = self.patch(protocol, "authenticate")
+        mock_authenticate.return_value = defer.succeed(user)
         protocol.connectionMade()
         self.addCleanup(lambda: protocol.connectionLost(""))
         return protocol, factory