Juju Charms Collection
juju-gui package

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

  1. Precise Pangolin (12.04)
  2. guiserver-auth-fixes
  3. Merge into trunk
Proposed by Francesco Banconi
Status: Merged
Merged at revision: 86
Proposed branch: lp:~frankban/charms/precise/juju-gui/guiserver-auth-fixes
Merge into: lp:~juju-gui/charms/precise/juju-gui/trunk
Diff against target: 409 lines (+82/-50)
7 files modified
server/guiserver/apps.py (+8/-5)
server/guiserver/auth.py (+11/-5)
server/guiserver/handlers.py (+7/-3)
server/guiserver/tests/test_apps.py (+0/-7)
server/guiserver/tests/test_auth.py (+17/-2)
server/guiserver/tests/test_handlers.py (+37/-27)
server/guiserver/tests/test_manage.py (+2/-1)
To merge this branch: bzr merge lp:~frankban/charms/precise/juju-gui/guiserver-auth-fixes
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+179014@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

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

Revision history for this message
Benjamin Saller (bcsaller) wrote :

LGTM

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

Thanks

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

Revision history for this message
Madison Scott-Clary (makyo) wrote :

Did not QA, but LGTM, thanks!

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

Revision history for this message
Francesco Banconi (frankban) wrote :

*** 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/

Revision history for this message
Francesco Banconi (frankban) wrote :

Thank you both for the reviews!

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'server/guiserver/apps.py'
2--- server/guiserver/apps.py 2013-07-25 11:33:00 +0000
3+++ server/guiserver/apps.py 2013-08-07 16:40:41 +0000
4@@ -36,12 +36,16 @@
5 # Set up static paths.
6 guiroot = options.guiroot
7 static_path = os.path.join(guiroot, 'juju-ui')
8- # Set up the authentication backend.
9- auth_backend = auth.get_backend(options.apiversion)
10 # Set up handlers.
11+ websocket_handler_options = {
12+ # The Juju API backend url.
13+ 'apiurl': options.apiurl,
14+ # The backend to use for user authentication.
15+ 'auth_backend': auth.get_backend(options.apiversion),
16+ }
17 server_handlers = [
18 # Handle WebSocket connections.
19- (r'^/ws$', handlers.WebSocketHandler, {'apiurl': options.apiurl}),
20+ (r'^/ws$', handlers.WebSocketHandler, websocket_handler_options),
21 # Handle static files.
22 (r'^/juju-ui/(.*)', web.StaticFileHandler, {'path': static_path}),
23 (r'^/(favicon\.ico)$', web.StaticFileHandler, {'path': guiroot}),
24@@ -56,8 +60,7 @@
25 # Any other path is served by index.html.
26 (r'^/(.*)', handlers.IndexHandler, {'path': guiroot}),
27 )
28- return web.Application(
29- server_handlers, debug=options.debug, auth_backend=auth_backend)
30+ return web.Application(server_handlers, debug=options.debug)
31
32
33 def redirector():
34
35=== modified file 'server/guiserver/auth.py'
36--- server/guiserver/auth.py 2013-07-25 15:42:46 +0000
37+++ server/guiserver/auth.py 2013-08-07 16:40:41 +0000
38@@ -21,10 +21,10 @@
39 - User: a simple data structure representing a logged in or anonymous user;
40 - authentication backends (GoBackend and PythonBackend): any object
41 implementing the following interface:
42- - get_request_id(data);
43- - request_is_login(data);
44- - get_credentials(data);
45- - login_succeeded(data).
46+ - get_request_id(data) -> id or None;
47+ - request_is_login(data) -> bool;
48+ - get_credentials(data) -> (str, str);
49+ - login_succeeded(data) -> bool.
50 The only purpose of auth backends is to provide the logic to parse
51 requests' data based on the API implementation currently in use. Backends
52 don't know anything about the authentication process or the current user,
53@@ -36,6 +36,8 @@
54 if the authentication succeeds.
55 """
56
57+import logging
58+
59
60 class User(object):
61 """The current WebSocket user."""
62@@ -57,6 +59,9 @@
63 username = self.username or 'anonymous'
64 return '<User: {} ({})>'.format(username, status)
65
66+ def __str__(self):
67+ return self.username
68+
69
70 class AuthMiddleware(object):
71 """Handle user authentication.
72@@ -84,7 +89,7 @@
73 """
74 backend = self._backend
75 request_id = backend.get_request_id(data)
76- if request_id and backend.request_is_login(data):
77+ if request_id is not None and backend.request_is_login(data):
78 self._request_id = request_id
79 credentials = backend.get_credentials(data)
80 self._user.username, self._user.password = credentials
81@@ -100,6 +105,7 @@
82 if request_id == self._request_id:
83 logged_in = self._backend.login_succeeded(data)
84 if logged_in:
85+ logging.info('auth: user {} logged in'.format(self._user))
86 self._user.is_authenticated = True
87 else:
88 self._user.username = self._user.password = ''
89
90=== modified file 'server/guiserver/handlers.py'
91--- server/guiserver/handlers.py 2013-07-24 16:22:21 +0000
92+++ server/guiserver/handlers.py 2013-08-07 16:40:41 +0000
93@@ -64,8 +64,13 @@
94 """
95
96 @gen.coroutine
97- def initialize(self, apiurl, io_loop=None):
98- """Create a new WebSocket client and connect it to the Juju API."""
99+ def initialize(self, apiurl, auth_backend, io_loop=None):
100+ """Initialize the WebSocket server.
101+
102+ Create a new WebSocket client and connect it to the Juju API.
103+ Set up the authentication system.
104+ Handle the queued messages.
105+ """
106 if io_loop is None:
107 io_loop = IOLoop.current()
108 self._io_loop = io_loop
109@@ -76,7 +81,6 @@
110 self._juju_message_queue = queue = deque()
111 # Set up the authentication infrastructure.
112 self.user = User()
113- auth_backend = self.application.settings['auth_backend']
114 self.auth = AuthMiddleware(self.user, auth_backend)
115 # Juju requires the Origin header to be included in the WebSocket
116 # client handshake request. Propagate the client origin if present;
117
118=== modified file 'server/guiserver/tests/test_apps.py'
119--- server/guiserver/tests/test_apps.py 2013-07-26 09:45:42 +0000
120+++ server/guiserver/tests/test_apps.py 2013-08-07 16:40:41 +0000
121@@ -22,7 +22,6 @@
122
123 from guiserver import (
124 apps,
125- auth,
126 handlers,
127 )
128
129@@ -90,12 +89,6 @@
130 spec = self.get_url_spec(app, r'^/test/(.*)$')
131 self.assertIsNone(spec)
132
133- def test_auth_backend(self):
134- # The server settings include the currently used auth backend.
135- app = self.get_app(apiversion='go')
136- self.assertIn('auth_backend', app.settings)
137- self.assertIsInstance(app.settings['auth_backend'], auth.GoBackend)
138-
139
140 class TestRedirector(AppsTestMixin, unittest.TestCase):
141
142
143=== modified file 'server/guiserver/tests/test_auth.py'
144--- server/guiserver/tests/test_auth.py 2013-07-25 15:42:46 +0000
145+++ server/guiserver/tests/test_auth.py 2013-08-07 16:40:41 +0000
146@@ -18,6 +18,8 @@
147
148 import unittest
149
150+from tornado.testing import LogTrapTestCase
151+
152 from guiserver import auth
153 from guiserver.tests import helpers
154
155@@ -44,6 +46,11 @@
156 expected = '<User: anonymous (not authenticated)>'
157 self.assertEqual(expected, repr(user))
158
159+ def test_str(self):
160+ # The string representation of an user is correctly generated.
161+ user = auth.User(username='the-doctor')
162+ self.assertEqual('the-doctor', str(user))
163+
164
165 class AuthMiddlewareTestMixin(object):
166 """Include tests for the AuthMiddleware.
167@@ -118,15 +125,23 @@
168 self.assert_user('user2', 'passwd2', True)
169 self.assertFalse(self.auth.in_progress())
170
171+ def test_request_id_is_zero(self):
172+ # The authentication process starts if a login request is processed
173+ # and the request id is zero.
174+ request = self.make_login_request(request_id=0)
175+ self.auth.process_request(request)
176+ self.assertTrue(self.auth.in_progress())
177+
178
179 class TestGoAuthMiddleware(
180- helpers.GoAPITestMixin, AuthMiddlewareTestMixin, unittest.TestCase):
181+ helpers.GoAPITestMixin, AuthMiddlewareTestMixin,
182+ LogTrapTestCase, unittest.TestCase):
183 pass
184
185
186 class TestPythonAuthMiddleware(
187 helpers.PythonAPITestMixin, AuthMiddlewareTestMixin,
188- unittest.TestCase):
189+ LogTrapTestCase, unittest.TestCase):
190 pass
191
192
193
194=== modified file 'server/guiserver/tests/test_handlers.py'
195--- server/guiserver/tests/test_handlers.py 2013-07-25 15:42:46 +0000
196+++ server/guiserver/tests/test_handlers.py 2013-08-07 16:40:41 +0000
197@@ -24,6 +24,7 @@
198 import mock
199 from tornado import (
200 concurrent,
201+ gen,
202 web,
203 )
204 from tornado.testing import (
205@@ -46,6 +47,7 @@
206 class WebSocketHandlerTestMixin(object):
207 """Base set up for all the WebSocketHandler test cases."""
208
209+ auth_backend = auth.get_backend(manage.DEFAULT_API_VERSION)
210 hello_message = json.dumps({'hello': 'world'})
211
212 def get_app(self):
213@@ -57,20 +59,21 @@
214 # ws-client -> ws-server -> ws-forwarding-client -> ws-echo-server
215 # Messages arriving to the echo server are returned back to the client:
216 # ws-echo-server -> ws-forwarding-client -> ws-server -> ws-client
217- self.echo_server_address = self.get_wss_url('/echo')
218- self.echo_server_closed_future = concurrent.Future()
219+ self.apiurl = self.get_wss_url('/echo')
220+ self.api_close_future = concurrent.Future()
221 echo_options = {
222- 'close_future': self.echo_server_closed_future,
223+ 'close_future': self.api_close_future,
224 'io_loop': self.io_loop,
225 }
226 ws_options = {
227- 'apiurl': self.echo_server_address,
228+ 'apiurl': self.apiurl,
229+ 'auth_backend': self.auth_backend,
230 'io_loop': self.io_loop,
231 }
232 return web.Application([
233 (r'/echo', helpers.EchoWebSocketHandler, echo_options),
234 (r'/ws', handlers.WebSocketHandler, ws_options),
235- ], auth_backend=auth.get_backend(manage.DEFAULT_API_VERSION))
236+ ])
237
238 def make_client(self):
239 """Return a WebSocket client ready to be connected to the server."""
240@@ -90,6 +93,17 @@
241 handler.ws_connection = mock.Mock()
242 return handler
243
244+ @gen.coroutine
245+ def make_initialized_handler(
246+ self, apiurl=None, headers=None, mock_protocol=False):
247+ """Create and return an initialized WebSocketHandler instance."""
248+ if apiurl is None:
249+ apiurl = self.apiurl
250+ handler = self.make_handler(
251+ headers=headers, mock_protocol=mock_protocol)
252+ yield handler.initialize(apiurl, self.auth_backend, self.io_loop)
253+ raise gen.Return(handler)
254+
255
256 class TestWebSocketHandlerConnection(
257 WebSocketHandlerTestMixin, helpers.WSSTestMixin, LogTrapTestCase,
258@@ -107,8 +121,7 @@
259 def test_initialization(self):
260 # A WebSocket client is created and connected when the handler is
261 # initialized.
262- handler = self.make_handler()
263- yield handler.initialize(self.echo_server_address, self.io_loop)
264+ handler = yield self.make_initialized_handler()
265 self.assertTrue(handler.connected)
266 self.assertTrue(handler.juju_connected)
267 self.assertIsInstance(
268@@ -120,38 +133,36 @@
269 def test_juju_connection_failure(self):
270 # If the connection to the Juju API server does not succeed, an
271 # error is reported and the client is disconnected.
272- handler = self.make_handler()
273 expected_log = '.*unable to connect to the Juju API'
274 with ExpectLog('', expected_log, required=True):
275- yield handler.initialize(
276- 'wss://127.0.0.1/does-not-exist', self.io_loop)
277+ handler = yield self.make_initialized_handler(
278+ apiurl='wss://127.0.0.1/no-such')
279 self.assertFalse(handler.connected)
280 self.assertFalse(handler.juju_connected)
281
282 @gen_test
283 def test_juju_connection_propagated_request_headers(self):
284 # The Origin header is propagated to the client connection.
285- handler = self.make_handler(headers={'Origin': 'https://example.com'})
286- yield handler.initialize(self.echo_server_address, self.io_loop)
287+ expected = {'Origin': 'https://example.com'}
288+ handler = yield self.make_initialized_handler(headers=expected)
289 headers = handler.juju_connection.request.headers
290 self.assertIn('Origin', headers)
291- self.assertEqual('https://example.com', headers['Origin'])
292+ self.assertEqual(expected['Origin'], headers['Origin'])
293
294 @gen_test
295 def test_juju_connection_default_request_headers(self):
296 # The default Origin header is included in the client connection
297 # handshake if not found in the original request.
298- handler = self.make_handler()
299- yield handler.initialize(self.echo_server_address, self.io_loop)
300+ handler = yield self.make_initialized_handler()
301 headers = handler.juju_connection.request.headers
302 self.assertIn('Origin', headers)
303 self.assertEqual(self.get_url('/echo'), headers['Origin'])
304
305+ @gen_test
306 def test_client_callback(self):
307 # The WebSocket client is created passing the proper callback.
308- handler = self.make_handler()
309 with self.mock_websocket_connect() as mock_websocket_connect:
310- handler.initialize(self.echo_server_address, self.io_loop)
311+ handler = yield self.make_initialized_handler()
312 self.assertEqual(1, mock_websocket_connect.call_count)
313 self.assertIn(
314 handler.on_juju_message, mock_websocket_connect.call_args[0])
315@@ -161,7 +172,7 @@
316 # The proxy connection is terminated when the client disconnects.
317 client = yield self.make_client()
318 yield client.close()
319- yield self.echo_server_closed_future
320+ yield self.api_close_future
321
322 @gen_test
323 def test_connection_closed_by_server(self):
324@@ -171,7 +182,7 @@
325 expected_log = '.*Juju API unexpectedly disconnected'
326 with ExpectLog('', expected_log, required=True):
327 # Fire the Future in order to force an echo server disconnection.
328- self.echo_server_closed_future.set_result(None)
329+ self.api_close_future.set_result(None)
330 message = yield client.read_message()
331 self.assertIsNone(message)
332
333@@ -183,16 +194,15 @@
334 @mock.patch('guiserver.clients.WebSocketClientConnection')
335 def test_from_browser_to_juju(self, mock_juju_connection):
336 # A message from the browser is forwarded to the remote server.
337- handler = self.make_handler()
338- yield handler.initialize(self.echo_server_address, self.io_loop)
339+ handler = yield self.make_initialized_handler()
340 handler.on_message(self.hello_message)
341 mock_juju_connection.write_message.assert_called_once_with(
342 self.hello_message)
343
344+ @gen_test
345 def test_from_juju_to_browser(self):
346 # A message from the remote server is returned to the browser.
347- handler = self.make_handler()
348- handler.initialize(self.echo_server_address, self.io_loop)
349+ handler = yield self.make_initialized_handler()
350 with mock.patch('guiserver.handlers.WebSocketHandler.write_message'):
351 handler.on_juju_message(self.hello_message)
352 handler.write_message.assert_called_once_with(self.hello_message)
353@@ -205,7 +215,7 @@
354 mock_path = 'guiserver.clients.WebSocketClientConnection.write_message'
355 with mock.patch(mock_path) as mock_write_message:
356 initialization = handler.initialize(
357- self.echo_server_address, self.io_loop)
358+ self.apiurl, self.auth_backend, self.io_loop)
359 handler.on_message(self.hello_message)
360 self.assertFalse(mock_write_message.called)
361 yield initialization
362@@ -246,7 +256,7 @@
363 def setUp(self):
364 super(TestWebSocketHandlerAuthentication, self).setUp()
365 self.handler = self.make_handler(mock_protocol=True)
366- self.handler.initialize(self.echo_server_address, self.io_loop)
367+ self.handler.initialize(self.apiurl, self.auth_backend, self.io_loop)
368
369 def send_login_request(self):
370 """Create a login request and send it to the handler."""
371@@ -292,7 +302,7 @@
372 self.assertFalse(self.handler.auth.in_progress())
373
374
375-class TestIndexHandler(AsyncHTTPTestCase, LogTrapTestCase):
376+class TestIndexHandler(LogTrapTestCase, AsyncHTTPTestCase):
377
378 def setUp(self):
379 # Set up a static path with an index.html in it.
380@@ -328,7 +338,7 @@
381 self.ensure_index('/:flag:/activated/?my=query')
382
383
384-class TestHttpsRedirectHandler(AsyncHTTPTestCase, LogTrapTestCase):
385+class TestHttpsRedirectHandler(LogTrapTestCase, AsyncHTTPTestCase):
386
387 def get_app(self):
388 return web.Application([(r'.*', handlers.HttpsRedirectHandler)])
389
390=== modified file 'server/guiserver/tests/test_manage.py'
391--- server/guiserver/tests/test_manage.py 2013-07-31 10:29:56 +0000
392+++ server/guiserver/tests/test_manage.py 2013-08-07 16:40:41 +0000
393@@ -21,6 +21,7 @@
394 import unittest
395
396 import mock
397+from tornado.testing import LogTrapTestCase
398
399 from guiserver import manage
400
401@@ -138,7 +139,7 @@
402 self.assertEqual(expected, manage._get_ssl_options())
403
404
405-class TestRun(unittest.TestCase):
406+class TestRun(LogTrapTestCase, unittest.TestCase):
407
408 def mock_and_run(self, **kwargs):
409 """Run the application after mocking the IO loop and the options/apps.

Subscribers

People subscribed via source and target branches