Merge lp:~gary/charms/precise/juju-gui/authtoken1 into lp:~juju-gui/charms/precise/juju-gui/trunk

[Gary Poster]

Add authentication token handler class

This is the first branch in a set of what will likely be three. This one simply creates the handler that we will use. There's no QA yet.

https://codereview.appspot.com/31020043/

[Gary Poster (gary)]

Reviewers: mp+196347_code.launchpad.net,

Message:
Please take a look.

Description:
Add authentication token handler class

This is the first branch in a set of what will likely be three. This
one simply creates the handler that we will use. There's no QA yet.

https://code.launchpad.net/~gary/charms/precise/juju-gui/authtoken1/+merge/196347

(do not edit description out of merge proposal)

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

Affected files (+306, -4 lines):
   A [revision details]
   M server/guiserver/auth.py
   M server/guiserver/tests/test_auth.py

[Francesco Banconi (frankban)]

Nice branch Gary. Tests pass and are very nice.
LGTM with only minors/optional changes.
Thank you!

https://codereview.appspot.com/31020043/diff/1/server/guiserver/auth.py
File server/guiserver/auth.py (right):

https://codereview.appspot.com/31020043/diff/1/server/guiserver/auth.py#newcode236
server/guiserver/auth.py:236: Here is an example of a token creation
response. Lifetime is in seconds.
Does "Lifetime is in seconds" still apply?

https://codereview.appspot.com/31020043/diff/1/server/guiserver/auth.py#newcode325
server/guiserver/auth.py:325: """Does data represents a token creation
request? True or False."""
Does data represent a token authentication request?

https://codereview.appspot.com/31020043/diff/1/server/guiserver/tests/test_auth.py
File server/guiserver/tests/test_auth.py (right):

https://codereview.appspot.com/31020043/diff/1/server/guiserver/tests/test_auth.py#newcode325
server/guiserver/tests/test_auth.py:325:
self.io_loop.add_timeout.call_args[0][1]()
Nice! It took me a minute before understanding you are calling the
expire_token() function. Maybe we could retrieve the function and then
call it, or just add a comment?

https://codereview.appspot.com/31020043/diff/1/server/guiserver/tests/test_auth.py#newcode362
server/guiserver/tests/test_auth.py:362:
self.io_loop.remove_timeout.assert_called_with('handle marker')
assert_called_once_with? Very cheap double check.

https://codereview.appspot.com/31020043/diff/1/server/guiserver/tests/test_auth.py#newcode389
server/guiserver/tests/test_auth.py:389: # This is a small integration
test of the two functions' interaction.
Very nice!

https://codereview.appspot.com/31020043/

[lp:~gary/charms/precise/juju-gui/authtoken1]

139. By Gary Poster on 2013-11-23

respond to review

140. By Gary Poster on 2013-11-23

correct grammar

[Gary Poster (gary)]

*** Submitted:

Add authentication token handler class

This is the first branch in a set of what will likely be three. This
one simply creates the handler that we will use. There's no QA yet.

R=frankban
CC=
https://codereview.appspot.com/31020043

https://codereview.appspot.com/31020043/

[Gary Poster (gary)]

Thank you for the very helpful review, Francesco, and for the elegant
pre-imp ideas that guided this branch.

https://codereview.appspot.com/31020043/

=== modified file 'server/guiserver/auth.py'
--- server/guiserver/auth.py	2013-10-17 14:12:16 +0000
+++ server/guiserver/auth.py	2013-11-23 02:10:22 +0000
@@ -36,7 +36,11 @@
       if the authentication succeeds.
 """
 
+import datetime
 import logging
+import uuid
+
+from tornado.ioloop import IOLoop
 
 
 class User(object):
@@ -44,10 +48,6 @@
 
     def __init__(self, username='', password='', is_authenticated=False):
         self.is_authenticated = is_authenticated
-        # XXX (frankban) YAGNI: the username/password attributes are not
-        # required for now, but they will help handling the HA story, i.e. in
-        # the process of re-authenticating to the API after switching from one
-        # Juju state/API server to another.
         self.username = username
         self.password = password
 
@@ -219,3 +219,142 @@
     """Return the auth backend instance to use for the given API version."""
     backend_class = {'go': GoBackend, 'python': PythonBackend}[apiversion]
     return backend_class()
+
+
+class AuthenticationTokenHandler(object):
+    """Handle requests related to authentication tokens.
+
+    A token creation request looks like the following:
+
+        {
+            'RequestId': 42,
+            'Type': 'GUIToken',
+            'Request': 'Create',
+            'Params': {},
+        }
+
+    Here is an example of a token creation response.
+
+        {
+            'RequestId': 42,
+            'Response': {
+                'Token': 'TOKEN-STRING',
+                'Created': '2013-11-21T12:34:46.778866Z',
+                'Expires': '2013-11-21T12:36:46.778866Z'
+            }
+        }
+
+    A token authentication request looks like the following:
+
+        {
+            'RequestId': 42,
+            'Type': 'GUIToken',
+            'Request': 'Login',
+            'Params': {'Token': 'TOKEN-STRING'},
+        }
+
+    Here is an example of a successful login response:
+
+        {
+            'RequestId': 42,
+            'Response': {'AuthTag': 'user-admin', 'Password': 'ADMIN-SECRET'}
+        }
+
+    A login failure response is like the following:
+
+        {
+            'RequestId': 42,
+            'Error': 'unknown, fulfilled, or expired token',
+            'ErrorCode': 'unauthorized access',
+            'Response': {},
+        }
+
+    Juju itself might return a failure response like the following, but this
+    would be difficult or impossible to trigger as of this writing:
+
+        {
+            'RequestId': 42,
+            'Error': 'invalid entity name or password',
+            'ErrorCode': 'unauthorized access',
+            'Response': {},
+        }
+    """
+
+    def __init__(self, max_life=datetime.timedelta(minutes=2), io_loop=None):
+        self._max_life = max_life
+        if io_loop is None:
+            io_loop = IOLoop.current()
+        self._io_loop = io_loop
+        self._data = {}
+
+    def token_requested(self, data):
+        """Does data represent a token creation request?  True or False."""
+        return (
+            'RequestId' in data and
+            data.get('Type', None) == 'GUIToken' and
+            data.get('Request', None) == 'Create'
+        )
+
+    def process_token_request(self, data, user, write_message):
+        """Create a single-use, time-expired token and send it back."""
+        token = uuid.uuid4().hex
+
+        def expire_token():
+            self._data.pop(token, None)
+        handle = self._io_loop.add_timeout(self._max_life, expire_token)
+        now = datetime.datetime.utcnow()
+        # Stashing these is a security risk.  We currently deem this risk to
+        # be acceptably small.  Even keeping an authenticated websocket in
+        # memory seems to be of a similar risk profile, and we cannot operate
+        # without that.
+        self._data[token] = dict(
+            username=user.username,
+            password=user.password,
+            handle=handle
+            )
+        write_message({
+            'RequestId': data['RequestId'],
+            'Response': {
+                'Token': token,
+                'Created': now.isoformat() + 'Z',
+                'Expires': (now + self._max_life).isoformat() + 'Z'
+            }
+        })
+
+    def authentication_requested(self, data):
+        """Does data represent a token authentication request? True or False.
+        """
+        params = data.get('Params', {})
+        return (
+            'RequestId' in data and
+            data.get('Type') == 'GUIToken' and
+            data.get('Request') == 'Login' and
+            'Token' in params
+        )
+
+    def process_authentication_request(self, data, write_message):
+        """Get the credentials for the token, or send an error."""
+        credentials = self._data.pop(data['Params']['Token'], None)
+        if credentials is not None:
+            self._io_loop.remove_timeout(credentials['handle'])
+            return credentials['username'], credentials['password']
+        else:
+            write_message({
+                'RequestId': data['RequestId'],
+                'Error': 'unknown, fulfilled, or expired token',
+                'ErrorCode': 'unauthorized access',
+                'Response': {},
+            })
+            # None is an explicit return marker to say "I handled this".
+            # It is returned by default.
+
+    def process_authentication_response(self, data, user):
+        """Make a successful token authentication response.
+
+        This includes the username and password so that clients can then use
+        them.  For instance, the GUI stashes them in session storage so that
+        reloading the page does not require logging in again."""
+        return {
+            'RequestId': data['RequestId'],
+            'Response': {'AuthTag': user.username, 'Password': user.password}
+        }
 
=== modified file 'server/guiserver/tests/test_auth.py'
--- server/guiserver/tests/test_auth.py	2013-08-07 16:34:09 +0000
+++ server/guiserver/tests/test_auth.py	2013-11-23 02:10:22 +0000
@@ -16,8 +16,10 @@
 
 """Tests for the Juju GUI server authentication management."""
 
+import datetime
 import unittest
 
+import mock
 from tornado.testing import LogTrapTestCase
 
 from guiserver import auth
@@ -244,3 +246,164 @@
         for request in requests:
             is_login = self.backend.request_is_login(request)
             self.assertFalse(is_login, request)
+
+
+class TestAuthenticationTokenHandler(unittest.TestCase):
+
+    def setUp(self):
+        super(TestAuthenticationTokenHandler, self).setUp()
+        self.io_loop = mock.Mock()
+        self.max_life = datetime.timedelta(minutes=1)
+        self.tokens = auth.AuthenticationTokenHandler(
+            self.max_life, self.io_loop)
+
+    def test_explicit_initialization(self):
+        # The class accepted the explicit initialization.
+        self.assertEqual(self.max_life, self.tokens._max_life)
+        self.assertEqual(self.io_loop, self.tokens._io_loop)
+        self.assertEqual({}, self.tokens._data)
+
+    @mock.patch('tornado.ioloop.IOLoop.current',
+                mock.Mock(return_value='mockloop'))
+    def test_default_initialization(self):
+        # The class has sane initialization defaults.
+        tokens = auth.AuthenticationTokenHandler()
+        self.assertEqual(
+            datetime.timedelta(minutes=2), tokens._max_life)
+        self.assertEqual('mockloop', tokens._io_loop)
+
+    def test_token_requested(self):
+        # It recognizes a token request.
+        requests = (
+            dict(RequestId=42, Type='GUIToken', Request='Create'),
+            dict(RequestId=22, Type='GUIToken', Request='Create', Params={}))
+        for request in requests:
+            is_token_requested = self.tokens.token_requested(request)
+            self.assertTrue(is_token_requested, request)
+
+    def test_not_token_requested(self):
+        # It rejects invalid token requests.
+        requests = (
+            dict(),
+            dict(Type='GUIToken', Request='Create'),
+            dict(RequestId=42, Request='Create'),
+            dict(RequestId=42, Type='GUIToken'))
+        for request in requests:
+            token_requested = self.tokens.token_requested(request)
+            self.assertFalse(token_requested, request)
+
+    @mock.patch('uuid.uuid4', mock.Mock(return_value=mock.Mock(hex='DEFACED')))
+    @mock.patch('datetime.datetime',
+                mock.Mock(
+                    **{'utcnow.return_value':
+                       datetime.datetime(2013, 11, 21, 21)}))
+    def test_process_token_request(self):
+        # It correctly responds to token requests.
+        user = auth.User('user-admin', 'ADMINSECRET')
+        write_message = mock.Mock()
+        data = dict(RequestId=42, Type='GUIToken', Request='Create')
+        self.tokens.process_token_request(data, user, write_message)
+        write_message.assert_called_once_with(dict(
+            RequestId=42,
+            Response=dict(
+                Token='DEFACED',
+                Created='2013-11-21T21:00:00Z',
+                Expires='2013-11-21T21:01:00Z'
+            )
+        ))
+        self.assertTrue('DEFACED' in self.tokens._data)
+        self.assertEqual(
+            {'username', 'password', 'handle'},
+            set(self.tokens._data['DEFACED'].keys()))
+        self.assertEqual(
+            user.username, self.tokens._data['DEFACED']['username'])
+        self.assertEqual(
+            user.password, self.tokens._data['DEFACED']['password'])
+        self.assertEqual(
+            self.max_life, self.io_loop.add_timeout.call_args[0][0])
+        self.assertTrue('DEFACED' in self.tokens._data)
+        expire_token = self.io_loop.add_timeout.call_args[0][1]
+        expire_token()
+        self.assertFalse('DEFACED' in self.tokens._data)
+
+    def test_authentication_requested(self):
+        # It recognizes an authentication request.
+        request = dict(
+            RequestId=42, Type='GUIToken', Request='Login',
+            Params={'Token': 'DEFACED'})
+        auth_requested = self.tokens.authentication_requested(request)
+        self.assertTrue(auth_requested, request)
+
+    def test_not_authentication_requested(self):
+        # It rejects invalid authentication requests.
+        requests = (
+            dict(),
+            dict(Type='GUIToken', Request='Login', Params={'Token': 'T'}),
+            dict(RequestId=42, Request='Login', Params={'Token': 'DEFACED'}),
+            dict(RequestId=42, Type='GUIToken', Params={'Token': 'DEFACED'}),
+            dict(RequestId=42, Type='GUIToken', Request='Login'),
+            dict(RequestId=42, Type='GUIToken', Request='Login', Params={}))
+        for request in requests:
+            auth_requested = self.tokens.authentication_requested(request)
+            self.assertFalse(auth_requested, request)
+
+    def test_known_authentication_request(self):
+        # It correctly responds to authentication requests with known tokens.
+        username = 'user-admin'
+        password = 'ADMINSECRET'
+        self.tokens._data['DEFACED'] = dict(
+            handle='handle marker', username=username, password=password)
+        request = dict(
+            RequestId=42, Type='GUIToken', Request='Login',
+            Params={'Token': 'DEFACED'})
+        write_message = mock.Mock()
+        self.assertEqual(
+            (username, password),
+            self.tokens.process_authentication_request(request, write_message))
+        self.io_loop.remove_timeout.assert_called_once_with('handle marker')
+        self.assertFalse(write_message.called)
+        self.assertFalse('DEFACED' in self.tokens._data)
+
+    def test_unknown_authentication_request(self):
+        # It correctly rejects authentication requests with unknown tokens.
+        request = dict(
+            RequestId=42, Type='GUIToken', Request='Login',
+            Params={'Token': 'DEFACED'})
+        write_message = mock.Mock()
+        self.assertEqual(
+            None,
+            self.tokens.process_authentication_request(request, write_message))
+        self.assertFalse(self.io_loop.remove_timeout.called)
+        write_message.assert_called_once_with(dict(
+            RequestId=42,
+            Error='unknown, fulfilled, or expired token',
+            ErrorCode='unauthorized access',
+            Response={}))
+
+    @mock.patch('uuid.uuid4', mock.Mock(return_value=mock.Mock(hex='DEFACED')))
+    @mock.patch('datetime.datetime',
+                mock.Mock(
+                    **{'utcnow.return_value':
+                       datetime.datetime(2013, 11, 21, 21)}))
+    def test_token_request_and_authentication_collaborate(self):
+        # process_token_request and process_authentication_request collaborate.
+        # This is a small integration test of the two functions' interaction.
+        user = auth.User('user-admin', 'ADMINSECRET')
+        write_message = mock.Mock()
+        request = dict(RequestId=42, Type='GUIToken', Request='Create')
+        self.tokens.process_token_request(request, user, write_message)
+        request = dict(
+            RequestId=43, Type='GUIToken', Request='Login',
+            Params={'Token': 'DEFACED'})
+        self.assertEqual(
+            (user.username, user.password),
+            self.tokens.process_authentication_request(request, write_message))
+
+    def test_process_authentication_response(self):
+        # It translates a normal authentication success.
+        user = auth.User('user-admin', 'ADMINSECRET')
+        response = {'RequestId': 42, 'Response': {}}
+        self.assertEqual(
+            dict(RequestId=42,
+                 Response=dict(AuthTag=user.username, Password=user.password)),
+            self.tokens.process_authentication_response(response, user))