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

Proposed by Gary Poster on 2013-11-22
Status: Merged
Merged at revision: 136
Proposed branch: lp:~gary/charms/precise/juju-gui/authtoken1
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 349 lines (+306/-4)
2 files modified
server/guiserver/auth.py (+143/-4)
server/guiserver/tests/test_auth.py (+163/-0)
To merge this branch: bzr merge lp:~gary/charms/precise/juju-gui/authtoken1
Reviewer Review Type Date Requested Status
charmers 2013-11-22 Pending
Review via email: mp+196347@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Gary Poster (gary) wrote :

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) wrote :

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/

139. By Gary Poster on 2013-11-23

respond to review

140. By Gary Poster on 2013-11-23

correct grammar

Gary Poster (gary) wrote :

*** 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) wrote :

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

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'server/guiserver/auth.py'
2--- server/guiserver/auth.py 2013-10-17 14:12:16 +0000
3+++ server/guiserver/auth.py 2013-11-23 02:10:22 +0000
4@@ -36,7 +36,11 @@
5 if the authentication succeeds.
6 """
7
8+import datetime
9 import logging
10+import uuid
11+
12+from tornado.ioloop import IOLoop
13
14
15 class User(object):
16@@ -44,10 +48,6 @@
17
18 def __init__(self, username='', password='', is_authenticated=False):
19 self.is_authenticated = is_authenticated
20- # XXX (frankban) YAGNI: the username/password attributes are not
21- # required for now, but they will help handling the HA story, i.e. in
22- # the process of re-authenticating to the API after switching from one
23- # Juju state/API server to another.
24 self.username = username
25 self.password = password
26
27@@ -219,3 +219,142 @@
28 """Return the auth backend instance to use for the given API version."""
29 backend_class = {'go': GoBackend, 'python': PythonBackend}[apiversion]
30 return backend_class()
31+
32+
33+class AuthenticationTokenHandler(object):
34+ """Handle requests related to authentication tokens.
35+
36+ A token creation request looks like the following:
37+
38+ {
39+ 'RequestId': 42,
40+ 'Type': 'GUIToken',
41+ 'Request': 'Create',
42+ 'Params': {},
43+ }
44+
45+ Here is an example of a token creation response.
46+
47+ {
48+ 'RequestId': 42,
49+ 'Response': {
50+ 'Token': 'TOKEN-STRING',
51+ 'Created': '2013-11-21T12:34:46.778866Z',
52+ 'Expires': '2013-11-21T12:36:46.778866Z'
53+ }
54+ }
55+
56+ A token authentication request looks like the following:
57+
58+ {
59+ 'RequestId': 42,
60+ 'Type': 'GUIToken',
61+ 'Request': 'Login',
62+ 'Params': {'Token': 'TOKEN-STRING'},
63+ }
64+
65+ Here is an example of a successful login response:
66+
67+ {
68+ 'RequestId': 42,
69+ 'Response': {'AuthTag': 'user-admin', 'Password': 'ADMIN-SECRET'}
70+ }
71+
72+ A login failure response is like the following:
73+
74+ {
75+ 'RequestId': 42,
76+ 'Error': 'unknown, fulfilled, or expired token',
77+ 'ErrorCode': 'unauthorized access',
78+ 'Response': {},
79+ }
80+
81+ Juju itself might return a failure response like the following, but this
82+ would be difficult or impossible to trigger as of this writing:
83+
84+ {
85+ 'RequestId': 42,
86+ 'Error': 'invalid entity name or password',
87+ 'ErrorCode': 'unauthorized access',
88+ 'Response': {},
89+ }
90+ """
91+
92+ def __init__(self, max_life=datetime.timedelta(minutes=2), io_loop=None):
93+ self._max_life = max_life
94+ if io_loop is None:
95+ io_loop = IOLoop.current()
96+ self._io_loop = io_loop
97+ self._data = {}
98+
99+ def token_requested(self, data):
100+ """Does data represent a token creation request? True or False."""
101+ return (
102+ 'RequestId' in data and
103+ data.get('Type', None) == 'GUIToken' and
104+ data.get('Request', None) == 'Create'
105+ )
106+
107+ def process_token_request(self, data, user, write_message):
108+ """Create a single-use, time-expired token and send it back."""
109+ token = uuid.uuid4().hex
110+
111+ def expire_token():
112+ self._data.pop(token, None)
113+ handle = self._io_loop.add_timeout(self._max_life, expire_token)
114+ now = datetime.datetime.utcnow()
115+ # Stashing these is a security risk. We currently deem this risk to
116+ # be acceptably small. Even keeping an authenticated websocket in
117+ # memory seems to be of a similar risk profile, and we cannot operate
118+ # without that.
119+ self._data[token] = dict(
120+ username=user.username,
121+ password=user.password,
122+ handle=handle
123+ )
124+ write_message({
125+ 'RequestId': data['RequestId'],
126+ 'Response': {
127+ 'Token': token,
128+ 'Created': now.isoformat() + 'Z',
129+ 'Expires': (now + self._max_life).isoformat() + 'Z'
130+ }
131+ })
132+
133+ def authentication_requested(self, data):
134+ """Does data represent a token authentication request? True or False.
135+ """
136+ params = data.get('Params', {})
137+ return (
138+ 'RequestId' in data and
139+ data.get('Type') == 'GUIToken' and
140+ data.get('Request') == 'Login' and
141+ 'Token' in params
142+ )
143+
144+ def process_authentication_request(self, data, write_message):
145+ """Get the credentials for the token, or send an error."""
146+ credentials = self._data.pop(data['Params']['Token'], None)
147+ if credentials is not None:
148+ self._io_loop.remove_timeout(credentials['handle'])
149+ return credentials['username'], credentials['password']
150+ else:
151+ write_message({
152+ 'RequestId': data['RequestId'],
153+ 'Error': 'unknown, fulfilled, or expired token',
154+ 'ErrorCode': 'unauthorized access',
155+ 'Response': {},
156+ })
157+ # None is an explicit return marker to say "I handled this".
158+ # It is returned by default.
159+
160+ def process_authentication_response(self, data, user):
161+ """Make a successful token authentication response.
162+
163+ This includes the username and password so that clients can then use
164+ them. For instance, the GUI stashes them in session storage so that
165+ reloading the page does not require logging in again."""
166+ return {
167+ 'RequestId': data['RequestId'],
168+ 'Response': {'AuthTag': user.username, 'Password': user.password}
169+ }
170
171=== modified file 'server/guiserver/tests/test_auth.py'
172--- server/guiserver/tests/test_auth.py 2013-08-07 16:34:09 +0000
173+++ server/guiserver/tests/test_auth.py 2013-11-23 02:10:22 +0000
174@@ -16,8 +16,10 @@
175
176 """Tests for the Juju GUI server authentication management."""
177
178+import datetime
179 import unittest
180
181+import mock
182 from tornado.testing import LogTrapTestCase
183
184 from guiserver import auth
185@@ -244,3 +246,164 @@
186 for request in requests:
187 is_login = self.backend.request_is_login(request)
188 self.assertFalse(is_login, request)
189+
190+
191+class TestAuthenticationTokenHandler(unittest.TestCase):
192+
193+ def setUp(self):
194+ super(TestAuthenticationTokenHandler, self).setUp()
195+ self.io_loop = mock.Mock()
196+ self.max_life = datetime.timedelta(minutes=1)
197+ self.tokens = auth.AuthenticationTokenHandler(
198+ self.max_life, self.io_loop)
199+
200+ def test_explicit_initialization(self):
201+ # The class accepted the explicit initialization.
202+ self.assertEqual(self.max_life, self.tokens._max_life)
203+ self.assertEqual(self.io_loop, self.tokens._io_loop)
204+ self.assertEqual({}, self.tokens._data)
205+
206+ @mock.patch('tornado.ioloop.IOLoop.current',
207+ mock.Mock(return_value='mockloop'))
208+ def test_default_initialization(self):
209+ # The class has sane initialization defaults.
210+ tokens = auth.AuthenticationTokenHandler()
211+ self.assertEqual(
212+ datetime.timedelta(minutes=2), tokens._max_life)
213+ self.assertEqual('mockloop', tokens._io_loop)
214+
215+ def test_token_requested(self):
216+ # It recognizes a token request.
217+ requests = (
218+ dict(RequestId=42, Type='GUIToken', Request='Create'),
219+ dict(RequestId=22, Type='GUIToken', Request='Create', Params={}))
220+ for request in requests:
221+ is_token_requested = self.tokens.token_requested(request)
222+ self.assertTrue(is_token_requested, request)
223+
224+ def test_not_token_requested(self):
225+ # It rejects invalid token requests.
226+ requests = (
227+ dict(),
228+ dict(Type='GUIToken', Request='Create'),
229+ dict(RequestId=42, Request='Create'),
230+ dict(RequestId=42, Type='GUIToken'))
231+ for request in requests:
232+ token_requested = self.tokens.token_requested(request)
233+ self.assertFalse(token_requested, request)
234+
235+ @mock.patch('uuid.uuid4', mock.Mock(return_value=mock.Mock(hex='DEFACED')))
236+ @mock.patch('datetime.datetime',
237+ mock.Mock(
238+ **{'utcnow.return_value':
239+ datetime.datetime(2013, 11, 21, 21)}))
240+ def test_process_token_request(self):
241+ # It correctly responds to token requests.
242+ user = auth.User('user-admin', 'ADMINSECRET')
243+ write_message = mock.Mock()
244+ data = dict(RequestId=42, Type='GUIToken', Request='Create')
245+ self.tokens.process_token_request(data, user, write_message)
246+ write_message.assert_called_once_with(dict(
247+ RequestId=42,
248+ Response=dict(
249+ Token='DEFACED',
250+ Created='2013-11-21T21:00:00Z',
251+ Expires='2013-11-21T21:01:00Z'
252+ )
253+ ))
254+ self.assertTrue('DEFACED' in self.tokens._data)
255+ self.assertEqual(
256+ {'username', 'password', 'handle'},
257+ set(self.tokens._data['DEFACED'].keys()))
258+ self.assertEqual(
259+ user.username, self.tokens._data['DEFACED']['username'])
260+ self.assertEqual(
261+ user.password, self.tokens._data['DEFACED']['password'])
262+ self.assertEqual(
263+ self.max_life, self.io_loop.add_timeout.call_args[0][0])
264+ self.assertTrue('DEFACED' in self.tokens._data)
265+ expire_token = self.io_loop.add_timeout.call_args[0][1]
266+ expire_token()
267+ self.assertFalse('DEFACED' in self.tokens._data)
268+
269+ def test_authentication_requested(self):
270+ # It recognizes an authentication request.
271+ request = dict(
272+ RequestId=42, Type='GUIToken', Request='Login',
273+ Params={'Token': 'DEFACED'})
274+ auth_requested = self.tokens.authentication_requested(request)
275+ self.assertTrue(auth_requested, request)
276+
277+ def test_not_authentication_requested(self):
278+ # It rejects invalid authentication requests.
279+ requests = (
280+ dict(),
281+ dict(Type='GUIToken', Request='Login', Params={'Token': 'T'}),
282+ dict(RequestId=42, Request='Login', Params={'Token': 'DEFACED'}),
283+ dict(RequestId=42, Type='GUIToken', Params={'Token': 'DEFACED'}),
284+ dict(RequestId=42, Type='GUIToken', Request='Login'),
285+ dict(RequestId=42, Type='GUIToken', Request='Login', Params={}))
286+ for request in requests:
287+ auth_requested = self.tokens.authentication_requested(request)
288+ self.assertFalse(auth_requested, request)
289+
290+ def test_known_authentication_request(self):
291+ # It correctly responds to authentication requests with known tokens.
292+ username = 'user-admin'
293+ password = 'ADMINSECRET'
294+ self.tokens._data['DEFACED'] = dict(
295+ handle='handle marker', username=username, password=password)
296+ request = dict(
297+ RequestId=42, Type='GUIToken', Request='Login',
298+ Params={'Token': 'DEFACED'})
299+ write_message = mock.Mock()
300+ self.assertEqual(
301+ (username, password),
302+ self.tokens.process_authentication_request(request, write_message))
303+ self.io_loop.remove_timeout.assert_called_once_with('handle marker')
304+ self.assertFalse(write_message.called)
305+ self.assertFalse('DEFACED' in self.tokens._data)
306+
307+ def test_unknown_authentication_request(self):
308+ # It correctly rejects authentication requests with unknown tokens.
309+ request = dict(
310+ RequestId=42, Type='GUIToken', Request='Login',
311+ Params={'Token': 'DEFACED'})
312+ write_message = mock.Mock()
313+ self.assertEqual(
314+ None,
315+ self.tokens.process_authentication_request(request, write_message))
316+ self.assertFalse(self.io_loop.remove_timeout.called)
317+ write_message.assert_called_once_with(dict(
318+ RequestId=42,
319+ Error='unknown, fulfilled, or expired token',
320+ ErrorCode='unauthorized access',
321+ Response={}))
322+
323+ @mock.patch('uuid.uuid4', mock.Mock(return_value=mock.Mock(hex='DEFACED')))
324+ @mock.patch('datetime.datetime',
325+ mock.Mock(
326+ **{'utcnow.return_value':
327+ datetime.datetime(2013, 11, 21, 21)}))
328+ def test_token_request_and_authentication_collaborate(self):
329+ # process_token_request and process_authentication_request collaborate.
330+ # This is a small integration test of the two functions' interaction.
331+ user = auth.User('user-admin', 'ADMINSECRET')
332+ write_message = mock.Mock()
333+ request = dict(RequestId=42, Type='GUIToken', Request='Create')
334+ self.tokens.process_token_request(request, user, write_message)
335+ request = dict(
336+ RequestId=43, Type='GUIToken', Request='Login',
337+ Params={'Token': 'DEFACED'})
338+ self.assertEqual(
339+ (user.username, user.password),
340+ self.tokens.process_authentication_request(request, write_message))
341+
342+ def test_process_authentication_response(self):
343+ # It translates a normal authentication success.
344+ user = auth.User('user-admin', 'ADMINSECRET')
345+ response = {'RequestId': 42, 'Response': {}}
346+ self.assertEqual(
347+ dict(RequestId=42,
348+ Response=dict(AuthTag=user.username, Password=user.password)),
349+ self.tokens.process_authentication_response(response, user))

Subscribers

People subscribed via source and target branches

to all changes: