Merge lp:~frankban/charms/precise/juju-gui/guiserver-auth-fixes into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Authentication fixes.

Fixed an error in the guiserver authentication
when the request id is set to 0.

Moved the auth_backend from the app to the
handler: it is only relevant in the WebSocketHandler
context.

Some clean up in the tests.

https://codereview.appspot.com/12612043/

[Francesco Banconi (frankban)]

Reviewers: mp+179014_code.launchpad.net,

Message:
Please take a look.

Description:
Authentication fixes.

Fixed an error in the guiserver authentication
when the request id is set to 0.

Moved the auth_backend from the app to the
handler: it is only relevant in the WebSocketHandler
context.

Some clean up in the tests.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/guiserver-auth-fixes/+merge/179014

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/12612043/

Affected files:
   A [revision details]
   M server/guiserver/apps.py
   M server/guiserver/auth.py
   M server/guiserver/handlers.py
   M server/guiserver/tests/test_apps.py
   M server/guiserver/tests/test_auth.py
   M server/guiserver/tests/test_handlers.py
   M server/guiserver/tests/test_manage.py

[Benjamin Saller (bcsaller)]

LGTM

The whole 'is not None' things gets us all from time to time.

Thanks

https://codereview.appspot.com/12612043/

[Madison Scott-Clary (makyo)]

Did not QA, but LGTM, thanks!

https://codereview.appspot.com/12612043/

[Francesco Banconi (frankban)]

*** Submitted:

Authentication fixes.

Fixed an error in the guiserver authentication
when the request id is set to 0.

Moved the auth_backend from the app to the
handler: it is only relevant in the WebSocketHandler
context.

Some clean up in the tests.

R=benjamin.saller, matthew.scott
CC=
https://codereview.appspot.com/12612043

https://codereview.appspot.com/12612043/

[Francesco Banconi (frankban)]

Thank you both for the reviews!

https://codereview.appspot.com/12612043/

=== modified file 'server/guiserver/apps.py'
--- server/guiserver/apps.py	2013-07-25 11:33:00 +0000
+++ server/guiserver/apps.py	2013-08-07 16:40:41 +0000
@@ -36,12 +36,16 @@
     # Set up static paths.
     guiroot = options.guiroot
     static_path = os.path.join(guiroot, 'juju-ui')
-    # Set up the authentication backend.
-    auth_backend = auth.get_backend(options.apiversion)
     # Set up handlers.
+    websocket_handler_options = {
+        # The Juju API backend url.
+        'apiurl': options.apiurl,
+        # The backend to use for user authentication.
+        'auth_backend': auth.get_backend(options.apiversion),
+    }
     server_handlers = [
         # Handle WebSocket connections.
-        (r'^/ws$', handlers.WebSocketHandler, {'apiurl': options.apiurl}),
+        (r'^/ws$', handlers.WebSocketHandler, websocket_handler_options),
         # Handle static files.
         (r'^/juju-ui/(.*)', web.StaticFileHandler, {'path': static_path}),
         (r'^/(favicon\.ico)$', web.StaticFileHandler, {'path': guiroot}),
@@ -56,8 +60,7 @@
         # Any other path is served by index.html.
         (r'^/(.*)', handlers.IndexHandler, {'path': guiroot}),
     )
-    return web.Application(
-        server_handlers, debug=options.debug, auth_backend=auth_backend)
+    return web.Application(server_handlers, debug=options.debug)
 
 
 def redirector():
 
=== modified file 'server/guiserver/auth.py'
--- server/guiserver/auth.py	2013-07-25 15:42:46 +0000
+++ server/guiserver/auth.py	2013-08-07 16:40:41 +0000
@@ -21,10 +21,10 @@
     - User: a simple data structure representing a logged in or anonymous user;
     - authentication backends (GoBackend and PythonBackend): any object
       implementing the following interface:
-        - get_request_id(data);
-        - request_is_login(data);
-        - get_credentials(data);
-        - login_succeeded(data).
+        - get_request_id(data) -> id or None;
+        - request_is_login(data) -> bool;
+        - get_credentials(data) -> (str, str);
+        - login_succeeded(data) -> bool.
       The only purpose of auth backends is to provide the logic to parse
       requests' data based on the API implementation currently in use. Backends
       don't know anything about the authentication process or the current user,
@@ -36,6 +36,8 @@
       if the authentication succeeds.
 """
 
+import logging
+
 
 class User(object):
     """The current WebSocket user."""
@@ -57,6 +59,9 @@
         username = self.username or 'anonymous'
         return '<User: {} ({})>'.format(username, status)
 
+    def __str__(self):
+        return self.username
+
 
 class AuthMiddleware(object):
     """Handle user authentication.
@@ -84,7 +89,7 @@
         """
         backend = self._backend
         request_id = backend.get_request_id(data)
-        if request_id and backend.request_is_login(data):
+        if request_id is not None and backend.request_is_login(data):
             self._request_id = request_id
             credentials = backend.get_credentials(data)
             self._user.username, self._user.password = credentials
@@ -100,6 +105,7 @@
         if request_id == self._request_id:
             logged_in = self._backend.login_succeeded(data)
             if logged_in:
+                logging.info('auth: user {} logged in'.format(self._user))
                 self._user.is_authenticated = True
             else:
                 self._user.username = self._user.password = ''
 
=== modified file 'server/guiserver/handlers.py'
--- server/guiserver/handlers.py	2013-07-24 16:22:21 +0000
+++ server/guiserver/handlers.py	2013-08-07 16:40:41 +0000
@@ -64,8 +64,13 @@
     """
 
     @gen.coroutine
-    def initialize(self, apiurl, io_loop=None):
-        """Create a new WebSocket client and connect it to the Juju API."""
+    def initialize(self, apiurl, auth_backend, io_loop=None):
+        """Initialize the WebSocket server.
+
+        Create a new WebSocket client and connect it to the Juju API.
+        Set up the authentication system.
+        Handle the queued messages.
+        """
         if io_loop is None:
             io_loop = IOLoop.current()
         self._io_loop = io_loop
@@ -76,7 +81,6 @@
         self._juju_message_queue = queue = deque()
         # Set up the authentication infrastructure.
         self.user = User()
-        auth_backend = self.application.settings['auth_backend']
         self.auth = AuthMiddleware(self.user, auth_backend)
         # Juju requires the Origin header to be included in the WebSocket
         # client handshake request. Propagate the client origin if present;
 
=== modified file 'server/guiserver/tests/test_apps.py'
--- server/guiserver/tests/test_apps.py	2013-07-26 09:45:42 +0000
+++ server/guiserver/tests/test_apps.py	2013-08-07 16:40:41 +0000
@@ -22,7 +22,6 @@
 
 from guiserver import (
     apps,
-    auth,
     handlers,
 )
 
@@ -90,12 +89,6 @@
         spec = self.get_url_spec(app, r'^/test/(.*)$')
         self.assertIsNone(spec)
 
-    def test_auth_backend(self):
-        # The server settings include the currently used auth backend.
-        app = self.get_app(apiversion='go')
-        self.assertIn('auth_backend', app.settings)
-        self.assertIsInstance(app.settings['auth_backend'], auth.GoBackend)
-
 
 class TestRedirector(AppsTestMixin, unittest.TestCase):
 
 
=== modified file 'server/guiserver/tests/test_auth.py'
--- server/guiserver/tests/test_auth.py	2013-07-25 15:42:46 +0000
+++ server/guiserver/tests/test_auth.py	2013-08-07 16:40:41 +0000
@@ -18,6 +18,8 @@
 
 import unittest
 
+from tornado.testing import LogTrapTestCase
+
 from guiserver import auth
 from guiserver.tests import helpers
 
@@ -44,6 +46,11 @@
         expected = '<User: anonymous (not authenticated)>'
         self.assertEqual(expected, repr(user))
 
+    def test_str(self):
+        # The string representation of an user is correctly generated.
+        user = auth.User(username='the-doctor')
+        self.assertEqual('the-doctor', str(user))
+
 
 class AuthMiddlewareTestMixin(object):
     """Include tests for the AuthMiddleware.
@@ -118,15 +125,23 @@
         self.assert_user('user2', 'passwd2', True)
         self.assertFalse(self.auth.in_progress())
 
+    def test_request_id_is_zero(self):
+        # The authentication process starts if a login request is processed
+        # and the request id is zero.
+        request = self.make_login_request(request_id=0)
+        self.auth.process_request(request)
+        self.assertTrue(self.auth.in_progress())
+
 
 class TestGoAuthMiddleware(
-        helpers.GoAPITestMixin, AuthMiddlewareTestMixin, unittest.TestCase):
+        helpers.GoAPITestMixin, AuthMiddlewareTestMixin,
+        LogTrapTestCase, unittest.TestCase):
     pass
 
 
 class TestPythonAuthMiddleware(
         helpers.PythonAPITestMixin, AuthMiddlewareTestMixin,
-        unittest.TestCase):
+        LogTrapTestCase, unittest.TestCase):
     pass
 
 
 
=== modified file 'server/guiserver/tests/test_handlers.py'
--- server/guiserver/tests/test_handlers.py	2013-07-25 15:42:46 +0000
+++ server/guiserver/tests/test_handlers.py	2013-08-07 16:40:41 +0000
@@ -24,6 +24,7 @@
 import mock
 from tornado import (
     concurrent,
+    gen,
     web,
 )
 from tornado.testing import (
@@ -46,6 +47,7 @@
 class WebSocketHandlerTestMixin(object):
     """Base set up for all the WebSocketHandler test cases."""
 
+    auth_backend = auth.get_backend(manage.DEFAULT_API_VERSION)
     hello_message = json.dumps({'hello': 'world'})
 
     def get_app(self):
@@ -57,20 +59,21 @@
         #   ws-client -> ws-server -> ws-forwarding-client -> ws-echo-server
         # Messages arriving to the echo server are returned back to the client:
         #   ws-echo-server -> ws-forwarding-client -> ws-server -> ws-client
-        self.echo_server_address = self.get_wss_url('/echo')
-        self.echo_server_closed_future = concurrent.Future()
+        self.apiurl = self.get_wss_url('/echo')
+        self.api_close_future = concurrent.Future()
         echo_options = {
-            'close_future': self.echo_server_closed_future,
+            'close_future': self.api_close_future,
             'io_loop': self.io_loop,
         }
         ws_options = {
-            'apiurl': self.echo_server_address,
+            'apiurl': self.apiurl,
+            'auth_backend': self.auth_backend,
             'io_loop': self.io_loop,
         }
         return web.Application([
             (r'/echo', helpers.EchoWebSocketHandler, echo_options),
             (r'/ws', handlers.WebSocketHandler, ws_options),
-        ], auth_backend=auth.get_backend(manage.DEFAULT_API_VERSION))
+        ])
 
     def make_client(self):
         """Return a WebSocket client ready to be connected to the server."""
@@ -90,6 +93,17 @@
             handler.ws_connection = mock.Mock()
         return handler
 
+    @gen.coroutine
+    def make_initialized_handler(
+            self, apiurl=None, headers=None, mock_protocol=False):
+        """Create and return an initialized WebSocketHandler instance."""
+        if apiurl is None:
+            apiurl = self.apiurl
+        handler = self.make_handler(
+            headers=headers, mock_protocol=mock_protocol)
+        yield handler.initialize(apiurl, self.auth_backend, self.io_loop)
+        raise gen.Return(handler)
+
 
 class TestWebSocketHandlerConnection(
         WebSocketHandlerTestMixin, helpers.WSSTestMixin, LogTrapTestCase,
@@ -107,8 +121,7 @@
     def test_initialization(self):
         # A WebSocket client is created and connected when the handler is
         # initialized.
-        handler = self.make_handler()
-        yield handler.initialize(self.echo_server_address, self.io_loop)
+        handler = yield self.make_initialized_handler()
         self.assertTrue(handler.connected)
         self.assertTrue(handler.juju_connected)
         self.assertIsInstance(
@@ -120,38 +133,36 @@
     def test_juju_connection_failure(self):
         # If the connection to the Juju API server does not succeed, an
         # error is reported and the client is disconnected.
-        handler = self.make_handler()
         expected_log = '.*unable to connect to the Juju API'
         with ExpectLog('', expected_log, required=True):
-            yield handler.initialize(
-                'wss://127.0.0.1/does-not-exist', self.io_loop)
+            handler = yield self.make_initialized_handler(
+                apiurl='wss://127.0.0.1/no-such')
         self.assertFalse(handler.connected)
         self.assertFalse(handler.juju_connected)
 
     @gen_test
     def test_juju_connection_propagated_request_headers(self):
         # The Origin header is propagated to the client connection.
-        handler = self.make_handler(headers={'Origin': 'https://example.com'})
-        yield handler.initialize(self.echo_server_address, self.io_loop)
+        expected = {'Origin': 'https://example.com'}
+        handler = yield self.make_initialized_handler(headers=expected)
         headers = handler.juju_connection.request.headers
         self.assertIn('Origin', headers)
-        self.assertEqual('https://example.com', headers['Origin'])
+        self.assertEqual(expected['Origin'], headers['Origin'])
 
     @gen_test
     def test_juju_connection_default_request_headers(self):
         # The default Origin header is included in the client connection
         # handshake if not found in the original request.
-        handler = self.make_handler()
-        yield handler.initialize(self.echo_server_address, self.io_loop)
+        handler = yield self.make_initialized_handler()
         headers = handler.juju_connection.request.headers
         self.assertIn('Origin', headers)
         self.assertEqual(self.get_url('/echo'), headers['Origin'])
 
+    @gen_test
     def test_client_callback(self):
         # The WebSocket client is created passing the proper callback.
-        handler = self.make_handler()
         with self.mock_websocket_connect() as mock_websocket_connect:
-            handler.initialize(self.echo_server_address, self.io_loop)
+            handler = yield self.make_initialized_handler()
         self.assertEqual(1, mock_websocket_connect.call_count)
         self.assertIn(
             handler.on_juju_message, mock_websocket_connect.call_args[0])
@@ -161,7 +172,7 @@
         # The proxy connection is terminated when the client disconnects.
         client = yield self.make_client()
         yield client.close()
-        yield self.echo_server_closed_future
+        yield self.api_close_future
 
     @gen_test
     def test_connection_closed_by_server(self):
@@ -171,7 +182,7 @@
         expected_log = '.*Juju API unexpectedly disconnected'
         with ExpectLog('', expected_log, required=True):
             # Fire the Future in order to force an echo server disconnection.
-            self.echo_server_closed_future.set_result(None)
+            self.api_close_future.set_result(None)
             message = yield client.read_message()
         self.assertIsNone(message)
 
@@ -183,16 +194,15 @@
     @mock.patch('guiserver.clients.WebSocketClientConnection')
     def test_from_browser_to_juju(self, mock_juju_connection):
         # A message from the browser is forwarded to the remote server.
-        handler = self.make_handler()
-        yield handler.initialize(self.echo_server_address, self.io_loop)
+        handler = yield self.make_initialized_handler()
         handler.on_message(self.hello_message)
         mock_juju_connection.write_message.assert_called_once_with(
             self.hello_message)
 
+    @gen_test
     def test_from_juju_to_browser(self):
         # A message from the remote server is returned to the browser.
-        handler = self.make_handler()
-        handler.initialize(self.echo_server_address, self.io_loop)
+        handler = yield self.make_initialized_handler()
         with mock.patch('guiserver.handlers.WebSocketHandler.write_message'):
             handler.on_juju_message(self.hello_message)
             handler.write_message.assert_called_once_with(self.hello_message)
@@ -205,7 +215,7 @@
         mock_path = 'guiserver.clients.WebSocketClientConnection.write_message'
         with mock.patch(mock_path) as mock_write_message:
             initialization = handler.initialize(
-                self.echo_server_address, self.io_loop)
+                self.apiurl, self.auth_backend, self.io_loop)
             handler.on_message(self.hello_message)
             self.assertFalse(mock_write_message.called)
             yield initialization
@@ -246,7 +256,7 @@
     def setUp(self):
         super(TestWebSocketHandlerAuthentication, self).setUp()
         self.handler = self.make_handler(mock_protocol=True)
-        self.handler.initialize(self.echo_server_address, self.io_loop)
+        self.handler.initialize(self.apiurl, self.auth_backend, self.io_loop)
 
     def send_login_request(self):
         """Create a login request and send it to the handler."""
@@ -292,7 +302,7 @@
         self.assertFalse(self.handler.auth.in_progress())
 
 
-class TestIndexHandler(AsyncHTTPTestCase, LogTrapTestCase):
+class TestIndexHandler(LogTrapTestCase, AsyncHTTPTestCase):
 
     def setUp(self):
         # Set up a static path with an index.html in it.
@@ -328,7 +338,7 @@
         self.ensure_index('/:flag:/activated/?my=query')
 
 
-class TestHttpsRedirectHandler(AsyncHTTPTestCase, LogTrapTestCase):
+class TestHttpsRedirectHandler(LogTrapTestCase, AsyncHTTPTestCase):
 
     def get_app(self):
         return web.Application([(r'.*', handlers.HttpsRedirectHandler)])
 
=== modified file 'server/guiserver/tests/test_manage.py'
--- server/guiserver/tests/test_manage.py	2013-07-31 10:29:56 +0000
+++ server/guiserver/tests/test_manage.py	2013-08-07 16:40:41 +0000
@@ -21,6 +21,7 @@
 import unittest
 
 import mock
+from tornado.testing import LogTrapTestCase
 
 from guiserver import manage
 
@@ -138,7 +139,7 @@
             self.assertEqual(expected, manage._get_ssl_options())
 
 
-class TestRun(unittest.TestCase):
+class TestRun(LogTrapTestCase, unittest.TestCase):
 
     def mock_and_run(self, **kwargs):
         """Run the application after mocking the IO loop and the options/apps.