Merge lp:~ricardokirkner/rnr-server/submit-review-with-macaroons into lp:rnr-server
- submit-review-with-macaroons
- Merge into trunk
Proposed by
Ricardo Kirkner
Status: | Merged |
---|---|
Approved by: | Ricardo Kirkner |
Approved revision: | 327 |
Merged at revision: | 317 |
Proposed branch: | lp:~ricardokirkner/rnr-server/submit-review-with-macaroons |
Merge into: | lp:rnr-server |
Diff against target: |
982 lines (+577/-37) 13 files modified
requirements-devel.txt (+1/-0) requirements.txt (+1/-0) src/clickreviews/forms.py (+2/-2) src/core/api/auth/__init__.py (+2/-0) src/core/api/auth/macaroon.py (+82/-0) src/core/api/auth/sso.py (+2/-2) src/core/tests/doubles.py (+119/-0) src/core/tests/helpers.py (+42/-0) src/core/tests/test_api_auth.py (+121/-2) src/core/tests/test_utilities.py (+123/-15) src/core/utilities.py (+40/-6) src/reviewsapp/api/urls.py (+4/-3) src/reviewsapp/tests/test_handlers.py (+38/-7) |
To merge this branch: | bzr merge lp:~ricardokirkner/rnr-server/submit-review-with-macaroons |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Fabián Ezequiel Gallina (community) | Approve | ||
Review via email: mp+294817@code.launchpad.net |
Commit message
support submitting reviews using macaroon based authentication
Description of the change
To post a comment you must log in.
Revision history for this message
Fabián Ezequiel Gallina (fgallina) wrote : | # |
- 322. By Ricardo Kirkner
-
improvements per review
Revision history for this message
Ricardo Kirkner (ricardokirkner) : | # |
Revision history for this message
Fabián Ezequiel Gallina (fgallina) wrote : | # |
added question and we are good to go.
- 323. By Ricardo Kirkner
-
switch api from GET to POST
- 324. By Ricardo Kirkner
-
remove unnecessary decoration
- 325. By Ricardo Kirkner
-
unify code into update_
or_create_ user_from_ sso - 326. By Ricardo Kirkner
-
updated code based on changes on the verify_acl api
Revision history for this message
Fabián Ezequiel Gallina (fgallina) wrote : | # |
Looking good, added minor comments.
- 327. By Ricardo Kirkner
-
minor changes per review
Revision history for this message
Ricardo Kirkner (ricardokirkner) : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'requirements-devel.txt' |
2 | --- requirements-devel.txt 2015-10-26 15:12:50 +0000 |
3 | +++ requirements-devel.txt 2016-05-17 14:32:36 +0000 |
4 | @@ -10,5 +10,6 @@ |
5 | paste==1.7.5.1 |
6 | pep8==1.5.7 |
7 | piston-mini-client |
8 | +responses==0.5.1 |
9 | testtools==0.9.34 |
10 | wsgi-intercept==0.5.1 |
11 | |
12 | === modified file 'requirements.txt' |
13 | --- requirements.txt 2015-10-26 15:11:49 +0000 |
14 | +++ requirements.txt 2016-05-17 14:32:36 +0000 |
15 | @@ -12,6 +12,7 @@ |
16 | oops-dictconfig==0.0.4 |
17 | oops-wsgi==0.0.11 # bzr+ssh://bazaar.launchpad.net/~ubuntuone-pqm-team/python-oops-wsgi/stable@39 |
18 | |
19 | +requests==2.6.0 |
20 | requests-oauthlib==0.5.0 |
21 | south==1.0.2 |
22 | txstatsd==1.0.0 |
23 | |
24 | === modified file 'src/clickreviews/forms.py' |
25 | --- src/clickreviews/forms.py 2015-04-17 12:39:41 +0000 |
26 | +++ src/clickreviews/forms.py 2016-05-17 14:32:36 +0000 |
27 | @@ -14,7 +14,7 @@ |
28 | from clickreviews.utilities import verify_click_package_published |
29 | from core.forms import JSONErrorMixin |
30 | from core.models import BaseModeration |
31 | -from core.utilities import update_or_create_user_from_oauth |
32 | +from core.utilities import update_or_create_user_from_sso |
33 | |
34 | |
35 | class ClickPackageReviewForm(forms.ModelForm, JSONErrorMixin): |
36 | @@ -239,7 +239,7 @@ |
37 | |
38 | def clean_moderator(self): |
39 | openid = self.cleaned_data['moderator'] |
40 | - user, __ = update_or_create_user_from_oauth(openid) |
41 | + user, __ = update_or_create_user_from_sso(openid) |
42 | if user is None: |
43 | self.add_error( |
44 | 'moderator', _('User given as moderator not found.')) |
45 | |
46 | === modified file 'src/core/api/auth/__init__.py' |
47 | --- src/core/api/auth/__init__.py 2014-09-24 14:57:52 +0000 |
48 | +++ src/core/api/auth/__init__.py 2016-05-17 14:32:36 +0000 |
49 | @@ -1,8 +1,10 @@ |
50 | from core.api.auth.delegated_sso import DelegatedSSOAuthentication |
51 | +from core.api.auth.macaroon import MacaroonsAuthentication |
52 | from core.api.auth.sso import SSOOAuthAuthentication |
53 | |
54 | |
55 | __all__ = [ |
56 | 'DelegatedSSOAuthentication', |
57 | + 'MacaroonsAuthentication', |
58 | 'SSOOAuthAuthentication', |
59 | ] |
60 | |
61 | === added file 'src/core/api/auth/macaroon.py' |
62 | --- src/core/api/auth/macaroon.py 1970-01-01 00:00:00 +0000 |
63 | +++ src/core/api/auth/macaroon.py 2016-05-17 14:32:36 +0000 |
64 | @@ -0,0 +1,82 @@ |
65 | +# This file is part of Software Center Ratings and Reviews |
66 | +# Copyright (C) 2016 Canonical Ltd. |
67 | +# |
68 | +# This program is free software: you can redistribute it and/or modify |
69 | +# it under the terms of the GNU Affero General Public License as |
70 | +# published by the Free Software Foundation, either version 3 of the |
71 | +# License, or (at your option) any later version. |
72 | +# |
73 | +# This program is distributed in the hope that it will be useful, |
74 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
75 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
76 | +# GNU Affero General Public License for more details. |
77 | +# |
78 | +# You should have received a copy of the GNU Affero General Public License |
79 | +# along with this program. If not, see <http://www.gnu.org/licenses/>. |
80 | + |
81 | +"""Macaroon authenticator for Piston.""" |
82 | + |
83 | +from __future__ import absolute_import |
84 | + |
85 | +import logging |
86 | + |
87 | +from django.http import HttpResponse |
88 | +from piston.authentication import OAuthAuthentication |
89 | + |
90 | +from core.utilities import ( |
91 | + update_or_create_user_from_sso, |
92 | + web_services, |
93 | +) |
94 | + |
95 | + |
96 | +logger = logging.getLogger(__name__) |
97 | + |
98 | + |
99 | +def has_macaroon_bits(authorization): |
100 | + """Return whether there are Macaroon bits in authorization. |
101 | + |
102 | + Macaroon bits are detected by ensuring the authorization starts with |
103 | + 'Macaroon'. |
104 | + |
105 | + """ |
106 | + has_bits = ( |
107 | + authorization and authorization.split()[0].lower() == 'macaroon') |
108 | + return has_bits |
109 | + |
110 | + |
111 | +class MacaroonsAuthentication(OAuthAuthentication): |
112 | + """ This class is a Piston Authentication class. |
113 | + |
114 | + See http://bitbucket.org/jespern/django-piston/wiki/Documentation for |
115 | + more info. |
116 | + """ |
117 | + def __init__(self, realm='API'): |
118 | + self.realm = realm |
119 | + |
120 | + def challenge(self): |
121 | + """ We define our own challenge so that we can provide our own realm |
122 | + and return a simple error message instead of rendering a template. |
123 | + """ |
124 | + resp = HttpResponse("Authorization Required") |
125 | + resp['WWW-Authenticate'] = 'Macaroon realm="%s"' % self.realm |
126 | + resp.status_code = 401 |
127 | + return resp |
128 | + |
129 | + def is_authenticated(self, request): |
130 | + authorization = request.META.get('HTTP_AUTHORIZATION', None) |
131 | + if not has_macaroon_bits(authorization): |
132 | + # skip network request as macaroon headers are missing |
133 | + return False |
134 | + data = web_services.verify_acl(request) |
135 | + account = data and data.get('account', {}) or {} |
136 | + verified = account.get('verified', False) |
137 | + if not verified: |
138 | + return False |
139 | + |
140 | + # get or create user account |
141 | + user, _ = update_or_create_user_from_sso( |
142 | + account.get('openid'), account=account) |
143 | + |
144 | + # inject user into request |
145 | + request.user = user |
146 | + return True |
147 | |
148 | === modified file 'src/core/api/auth/sso.py' |
149 | --- src/core/api/auth/sso.py 2015-10-09 18:54:55 +0000 |
150 | +++ src/core/api/auth/sso.py 2016-05-17 14:32:36 +0000 |
151 | @@ -25,7 +25,7 @@ |
152 | |
153 | from core.utilities import ( |
154 | web_services, |
155 | - update_or_create_user_from_oauth, |
156 | + update_or_create_user_from_sso, |
157 | ) |
158 | |
159 | |
160 | @@ -93,7 +93,7 @@ |
161 | |
162 | openid = oauth_request.get_parameter('oauth_consumer_key') |
163 | |
164 | - user, _ = update_or_create_user_from_oauth(openid) |
165 | + user, _ = update_or_create_user_from_sso(openid) |
166 | if user is not None: |
167 | request.user = user |
168 | else: |
169 | |
170 | === added file 'src/core/tests/doubles.py' |
171 | --- src/core/tests/doubles.py 1970-01-01 00:00:00 +0000 |
172 | +++ src/core/tests/doubles.py 2016-05-17 14:32:36 +0000 |
173 | @@ -0,0 +1,119 @@ |
174 | +from __future__ import absolute_import, unicode_literals |
175 | + |
176 | +import json |
177 | + |
178 | +import responses |
179 | +from django.conf import settings |
180 | +from requests import PreparedRequest |
181 | + |
182 | +from core.utilities import WebServices |
183 | + |
184 | + |
185 | +# XXX: RequestsDouble and RequestsDoubleTestCaseMixin come from |
186 | +# appstore-common, this copypasta (with modifications) is an attempt |
187 | +# to generalize it a bit and later on move the code into its own |
188 | +# module and reuse it across projects. |
189 | +class RequestsDouble(object): |
190 | + |
191 | + def start(self): |
192 | + self.reset() |
193 | + responses.start() |
194 | + |
195 | + def stop(self): |
196 | + # Stopping does not triggers a reset and it should because |
197 | + # responses dirty state can become a problem for other tests. |
198 | + try: |
199 | + self.reset() |
200 | + responses.stop() |
201 | + except RuntimeError: |
202 | + raise RuntimeError( |
203 | + 'Calling stop on a RequestDouble that is not started.' |
204 | + 'This may be because you are instantiating your own instead ' |
205 | + 'of reusing the RequestDouble from your TestCase') |
206 | + |
207 | + def reset(self): |
208 | + responses.reset() |
209 | + |
210 | + def reset_calls(self): |
211 | + responses.calls.reset() |
212 | + |
213 | + def make_body(self, data=None, files=None): |
214 | + req = PreparedRequest() |
215 | + # need to define headers before body |
216 | + req.prepare_headers({}) |
217 | + req.prepare_body(data=data, files=files) |
218 | + return req.body |
219 | + |
220 | + def add_response(self, url, status_code=200, body='', |
221 | + match_querystring=False, method='GET', |
222 | + callback=None, reset=False): |
223 | + if reset: |
224 | + self.reset() |
225 | + if callback: |
226 | + responses.add_callback(method, url, callback=callback, |
227 | + match_querystring=match_querystring) |
228 | + else: |
229 | + responses.add(method, url, match_querystring=match_querystring, |
230 | + status=status_code, body=body) |
231 | + |
232 | + def _call_has_headers(self, call, headers): |
233 | + has_headers = True |
234 | + |
235 | + if headers is not None: |
236 | + # make sure all given headers were included in call |
237 | + for key, value in headers.items(): |
238 | + if call.request.headers.get(key) != value: |
239 | + has_headers = False |
240 | + break |
241 | + |
242 | + return has_headers |
243 | + |
244 | + def find_call(self, url, headers=None, body=None, method=None): |
245 | + """Find calls for url optionally matching headers, body and method. |
246 | + |
247 | + The given headers can be a subset of the actual call headers, |
248 | + body must be an exact match. |
249 | + |
250 | + """ |
251 | + for call in responses.calls: |
252 | + test_body = call.request.body if body is None else body |
253 | + if (call.request.url == url |
254 | + and self._call_has_headers(call, headers) |
255 | + and test_body == call.request.body): |
256 | + if method is not None and call.request.method != method: |
257 | + continue |
258 | + return (url, call.request.headers, call.request.body) |
259 | + |
260 | + def find_calls(self, url, method=None): |
261 | + calls = (c for c in responses.calls if c.request.url == url) |
262 | + if method: |
263 | + calls = (c for c in calls if c.request.method == method) |
264 | + return [(url, c.request.headers, c.request.body) for c in calls] |
265 | + |
266 | + |
267 | +class SCADouble(WebServices): |
268 | + |
269 | + def __init__(self, requests_double, *args, **kwargs): |
270 | + WebServices.__init__(self, *args, **kwargs) |
271 | + self.requests_double = requests_double |
272 | + |
273 | + self.verify_acl_url = ( |
274 | + settings.SCA_HOST_URL.strip('/') + '/dev/api/acl/verify/') |
275 | + |
276 | + def set_verify_acl_response(self, **kwargs): |
277 | + body = { |
278 | + 'allowed': True, |
279 | + 'refresh_required': False, |
280 | + 'account': { |
281 | + 'openid': 'oid1234', |
282 | + 'email': 'user@example.com', |
283 | + 'displayname': 'user1234', |
284 | + 'verified': True, |
285 | + }, |
286 | + 'last_auth': '20160515T12:00:00.000', |
287 | + 'permissions': ['package_access'], |
288 | + } |
289 | + body.update(**kwargs) |
290 | + self.requests_double.add_response( |
291 | + self.verify_acl_url, method='POST', |
292 | + body=json.dumps(body)) |
293 | |
294 | === modified file 'src/core/tests/helpers.py' |
295 | --- src/core/tests/helpers.py 2015-04-22 13:24:20 +0000 |
296 | +++ src/core/tests/helpers.py 2016-05-17 14:32:36 +0000 |
297 | @@ -5,6 +5,7 @@ |
298 | from mock import patch |
299 | |
300 | from core.utilities import web_services |
301 | +from core.tests.doubles import RequestsDouble |
302 | |
303 | |
304 | class SSOTestCaseMixin(object): |
305 | @@ -83,3 +84,44 @@ |
306 | message += "\n\n\nSecond queries:\n" |
307 | message += format_queries(second.captured_queries) |
308 | raise AssertionError(message) |
309 | + |
310 | + |
311 | +# Why a mixin? Because we want to be able to plug this functionality |
312 | +# to any of the variants of TestCases the user may have without |
313 | +# relying on hierarchy (e.g. `unittest.TestCase`, |
314 | +# `django.test.TestCase`). An effect of this is that `patch_requests` |
315 | +# must be explicitly called to activate requests double functionality, |
316 | +# which may be a good thing if you want fine-grained control. |
317 | +class RequestsDoubleTestCaseMixin(object): |
318 | + """Mixin to add requests double functionality. |
319 | + |
320 | + To enable it, `patch_requests` must be called prior your tests are |
321 | + executed, so generally this is a good thing to be placed in the |
322 | + setUp method of your `TestCase` classes unless you wish more |
323 | + fine-grained control. |
324 | + """ |
325 | + |
326 | + def patch_requests(self): |
327 | + """Enable requests double. |
328 | + |
329 | + Takes care of cleaning up itself after a test is run. |
330 | + """ |
331 | + self.requests_double = RequestsDouble() |
332 | + self.requests_double.start() |
333 | + self.addCleanup(self.requests_double.stop) |
334 | + |
335 | + def patch_choose_boundary(self, boundary='theboundary'): |
336 | + return self.patch( |
337 | + 'requests.packages.urllib3.filepost.choose_boundary', |
338 | + return_value=boundary.encode('utf-8')) |
339 | + |
340 | + def assert_call(self, url, headers=None, body=None, method=None): |
341 | + """Assert call to url and headers was executed.""" |
342 | + call = self.requests_double.find_call( |
343 | + url, headers, body, method=method) |
344 | + if call is None: |
345 | + self.fail( |
346 | + "No matching call found. url=%s headers=%s body=%s " |
347 | + "method=%s" % (url, headers, body, method)) |
348 | + |
349 | + |
350 | |
351 | === modified file 'src/core/tests/test_api_auth.py' |
352 | --- src/core/tests/test_api_auth.py 2015-10-09 19:31:57 +0000 |
353 | +++ src/core/tests/test_api_auth.py 2016-05-17 14:32:36 +0000 |
354 | @@ -37,9 +37,11 @@ |
355 | SSOOAuthAuthentication, |
356 | delegated_sso, |
357 | ) |
358 | +from core.api.auth.macaroon import MacaroonsAuthentication, has_macaroon_bits |
359 | from core.api.auth.sso import has_oauth_bits |
360 | +from core.tests.doubles import SCADouble |
361 | from core.tests.factory import TestCaseWithFactory |
362 | -from core.tests.helpers import SSOTestCaseMixin |
363 | +from core.tests.helpers import RequestsDoubleTestCaseMixin, SSOTestCaseMixin |
364 | from core.utilities import full_claimed_id |
365 | |
366 | |
367 | @@ -53,6 +55,7 @@ |
368 | self.method = method |
369 | self.environ = self.META |
370 | self.uri = uri |
371 | + self.user = None |
372 | |
373 | def build_absolute_uri(self): |
374 | return self.uri |
375 | @@ -134,6 +137,17 @@ |
376 | self.assertFalse(result, self.no_oauth % not_allowed) |
377 | |
378 | |
379 | +class HasMacaroonBitsTestCase(TestCase): |
380 | + |
381 | + def test_has_macaroon_bits(self): |
382 | + for authorization in ('Macaroon zaraza', 'macaroon zaraza'): |
383 | + self.assertTrue(has_macaroon_bits(authorization)) |
384 | + |
385 | + def test_has_not_macaroon_bits(self): |
386 | + for authorization in ('OAuth zaraza', 'Auth macaroon zaraza'): |
387 | + self.assertFalse(has_macaroon_bits(authorization)) |
388 | + |
389 | + |
390 | class SSOOAuthAuthenticationTestCase(BaseAuthenticationTestCase): |
391 | |
392 | def _get_auth(self): |
393 | @@ -221,7 +235,7 @@ |
394 | self.user.delete() |
395 | self.factory.makeUser() |
396 | |
397 | - get_method = 'core.api.auth.sso.update_or_create_user_from_oauth' |
398 | + get_method = 'core.api.auth.sso.update_or_create_user_from_sso' |
399 | with patch(get_method) as mock_get_user: |
400 | mock_get_user.return_value = (None, False) |
401 | response = self.get_resource(request) |
402 | @@ -669,3 +683,108 @@ |
403 | request = self.make_request('POST', self.signature, body='wrong') |
404 | response = self.post_resource(request) |
405 | self.assertEqual(httplib.UNAUTHORIZED, response.status_code) |
406 | + |
407 | + |
408 | +class MacaroonsAuthenticationTestCase(BaseAuthenticationTestCase, |
409 | + RequestsDoubleTestCaseMixin): |
410 | + |
411 | + def setUp(self): |
412 | + super(MacaroonsAuthenticationTestCase, self).setUp() |
413 | + self.patch_requests() |
414 | + |
415 | + self.sca_double = SCADouble(self.requests_double) |
416 | + p = patch('core.utilities.web_services', new=self.sca_double) |
417 | + p.start() |
418 | + self.addCleanup(p.stop) |
419 | + |
420 | + def _get_auth(self): |
421 | + return MacaroonsAuthentication(realm="Ratings and Reviews") |
422 | + |
423 | + def make_request(self): |
424 | + request = MockRequest(method='POST') |
425 | + authorization = 'Macaroon root="%s", discharge="%s"' % ( |
426 | + 'root-macaroon-data', 'discharge-macaroon-data') |
427 | + request.META['HTTP_AUTHORIZATION'] = authorization |
428 | + return request |
429 | + |
430 | + def test_no_auth_fails(self): |
431 | + request = MockRequest() |
432 | + assert not request.META.get('HTTP_AUTHORIZATION') |
433 | + |
434 | + response = self.post_resource(request) |
435 | + |
436 | + self.assertEqual(httplib.UNAUTHORIZED, response.status_code) |
437 | + self.assertEqual('Macaroon realm="Ratings and Reviews"', |
438 | + response['WWW-Authenticate']) |
439 | + |
440 | + def test_no_macaroon_authorization_header(self): |
441 | + request = self.make_request() |
442 | + request.META['HTTP_AUTHORIZATION'] = 'Basic realm="foo"' |
443 | + |
444 | + response = self.post_resource(request) |
445 | + self.assertEqual(httplib.UNAUTHORIZED, response.status_code) |
446 | + |
447 | + def test_success(self): |
448 | + self.sca_double.set_verify_acl_response() |
449 | + request = self.make_request() |
450 | + |
451 | + response = self.post_resource(request) |
452 | + self.assertEqual(httplib.CREATED, response.status_code) |
453 | + |
454 | + def test_failure(self): |
455 | + self.requests_double.add_response( |
456 | + self.sca_double.verify_acl_url, method='POST', status_code=400) |
457 | + request = self.make_request() |
458 | + |
459 | + response = self.post_resource(request) |
460 | + self.assertEqual(httplib.UNAUTHORIZED, response.status_code) |
461 | + |
462 | + def test_successful_authentication_injects_user(self): |
463 | + self.sca_double.set_verify_acl_response( |
464 | + openid='oid1234', displayname='user1234', |
465 | + email='user@example.com', is_verified=True) |
466 | + request = self.make_request() |
467 | + |
468 | + response = self.auth.is_authenticated(request) |
469 | + self.assertTrue(response) |
470 | + |
471 | + # a new user was created |
472 | + self.assertEqual(User.objects.count(), 2) |
473 | + user = request.user |
474 | + self.assertIsInstance(user, User) |
475 | + self.assertEqual(user.username, 'oid1234') |
476 | + self.assertEqual(user.last_name, 'user1234') |
477 | + self.assertEqual(user.email, 'user@example.com') |
478 | + openid = user.useropenid_set.get().claimed_id.split('/')[-1] |
479 | + self.assertEqual(openid, 'oid1234') |
480 | + |
481 | + def test_do_not_create_user_if_exists(self): |
482 | + account = { |
483 | + 'openid': self.openid, |
484 | + 'displayname': 'user1234', |
485 | + 'email': 'user@example.com', |
486 | + 'verified': True, |
487 | + } |
488 | + self.sca_double.set_verify_acl_response(account=account) |
489 | + request = self.make_request() |
490 | + |
491 | + response = self.auth.is_authenticated(request) |
492 | + self.assertTrue(response) |
493 | + |
494 | + self.assertEqual(request.user, self.user) |
495 | + self.assertEqual(User.objects.count(), 1) |
496 | + |
497 | + def test_do_not_create_user_if_user_not_verified(self): |
498 | + account = { |
499 | + 'openid': self.openid, |
500 | + 'displayname': 'user1234', |
501 | + 'email': 'user@example.com', |
502 | + 'verified': False, |
503 | + } |
504 | + self.sca_double.set_verify_acl_response(account=account) |
505 | + request = self.make_request() |
506 | + |
507 | + response = self.auth.is_authenticated(request) |
508 | + self.assertFalse(response) |
509 | + |
510 | + self.assertIsNone(request.user) |
511 | |
512 | === modified file 'src/core/tests/test_utilities.py' |
513 | --- src/core/tests/test_utilities.py 2015-04-22 19:38:37 +0000 |
514 | +++ src/core/tests/test_utilities.py 2016-05-17 14:32:36 +0000 |
515 | @@ -3,16 +3,20 @@ |
516 | from django.conf import settings |
517 | from django.core.cache import cache |
518 | from django.test import TestCase |
519 | +from django.test.client import RequestFactory |
520 | from django.test.utils import override_settings |
521 | +from django.utils.timezone import now |
522 | from launchpadlib.errors import HTTPError |
523 | from mock import patch, Mock |
524 | from piston_mini_client import PistonResponseObject |
525 | |
526 | +from core.tests.doubles import SCADouble |
527 | +from core.tests.helpers import RequestsDoubleTestCaseMixin |
528 | from core.tests.factory import TestCaseWithFactory |
529 | from core.utilities import ( |
530 | WebServices, |
531 | full_claimed_id, |
532 | - update_or_create_user_from_oauth, |
533 | + update_or_create_user_from_sso, |
534 | web_services, |
535 | ) |
536 | |
537 | @@ -305,10 +309,10 @@ |
538 | self.assertTrue(claimed_id.endswith(consumer_key)) |
539 | |
540 | |
541 | -class UpdateOrCreateUserFromOauthTestCase(TestCaseWithFactory): |
542 | +class UpdateOrCreateUserFromSSOTestCase(TestCaseWithFactory): |
543 | |
544 | def setUp(self): |
545 | - super(UpdateOrCreateUserFromOauthTestCase, self).setUp() |
546 | + super(UpdateOrCreateUserFromSSOTestCase, self).setUp() |
547 | self.user = self.factory.makeUser() |
548 | p = patch('core.utilities.web_services') |
549 | self.mock_web_services = p.start() |
550 | @@ -338,28 +342,28 @@ |
551 | |
552 | def test_missing_account_data_and_missing_user(self): |
553 | self.mock_get_data.return_value = None |
554 | - user, created = update_or_create_user_from_oauth('inexistent') |
555 | + user, created = update_or_create_user_from_sso('inexistent') |
556 | self.assertIsNone(user) |
557 | self.assertFalse(created) |
558 | |
559 | def test_account_data_with_no_email_and_missing_user(self): |
560 | sso_user_data = self.mock_data_for_account(email='') |
561 | self.mock_get_data.return_value = sso_user_data |
562 | - user, created = update_or_create_user_from_oauth('inexistent') |
563 | + user, created = update_or_create_user_from_sso('inexistent') |
564 | self.assertIsNone(user) |
565 | self.assertFalse(created) |
566 | |
567 | def test_account_data_with_no_displayname_and_missing_user(self): |
568 | sso_user_data = self.mock_data_for_account(displayname=None) |
569 | self.mock_get_data.return_value = sso_user_data |
570 | - user, created = update_or_create_user_from_oauth('inexistent') |
571 | + user, created = update_or_create_user_from_sso('inexistent') |
572 | self.assertIsNone(user) |
573 | self.assertFalse(created) |
574 | |
575 | def test_account_data_unverified_and_missing_user(self): |
576 | sso_user_data = self.mock_data_for_account(verified=False) |
577 | self.mock_get_data.return_value = sso_user_data |
578 | - user, created = update_or_create_user_from_oauth('inexistent') |
579 | + user, created = update_or_create_user_from_sso('inexistent') |
580 | self.assertIsNone(user) |
581 | self.assertFalse(created) |
582 | |
583 | @@ -367,7 +371,7 @@ |
584 | self.user.delete() |
585 | sso_user_data = self.mock_data_for_account() |
586 | self.mock_get_data.return_value = sso_user_data |
587 | - user, created = update_or_create_user_from_oauth('inexistent') |
588 | + user, created = update_or_create_user_from_sso('inexistent') |
589 | self.assertIsNotNone(user) |
590 | self.assertTrue(created) |
591 | self.assert_user_data(user, sso_user_data) |
592 | @@ -380,7 +384,7 @@ |
593 | self.factory.makeUser( |
594 | first_name='John', last_name='Does', email='user@email.com', |
595 | open_id=open_id) |
596 | - user, created = update_or_create_user_from_oauth(open_id) |
597 | + user, created = update_or_create_user_from_sso(open_id) |
598 | self.assertIsNotNone(user) |
599 | self.assertFalse(created) |
600 | self.assert_user_data(user, sso_user_data) |
601 | @@ -391,7 +395,7 @@ |
602 | self.factory.makeUser( |
603 | first_name='John', last_name='Does', email='user@email.com', |
604 | open_id=open_id) |
605 | - user, created = update_or_create_user_from_oauth(open_id) |
606 | + user, created = update_or_create_user_from_sso(open_id) |
607 | self.assertIsNotNone(user) |
608 | self.assertFalse(created) |
609 | |
610 | @@ -403,7 +407,7 @@ |
611 | self.factory.makeUser( |
612 | first_name='John', last_name='Does', email='user@email.com', |
613 | open_id=open_id) |
614 | - user, created = update_or_create_user_from_oauth( |
615 | + user, created = update_or_create_user_from_sso( |
616 | open_id, update_window=None) |
617 | self.assertIsNotNone(user) |
618 | self.assertFalse(created) |
619 | @@ -417,7 +421,7 @@ |
620 | user = self.factory.makeUser( |
621 | first_name='John', last_name='Does', email='user@email.com', |
622 | open_id=open_id) |
623 | - logged_user, created = update_or_create_user_from_oauth( |
624 | + logged_user, created = update_or_create_user_from_sso( |
625 | open_id, update_window=60 * 60 * 24) |
626 | self.assertIsNotNone(logged_user) |
627 | self.assertFalse(created) |
628 | @@ -433,7 +437,7 @@ |
629 | self.factory.makeUser( |
630 | first_name='John', last_name='Does', email='user@email.com', |
631 | open_id=open_id) |
632 | - user, created = update_or_create_user_from_oauth( |
633 | + user, created = update_or_create_user_from_sso( |
634 | open_id, update_window=None) |
635 | self.assertIsNotNone(user) |
636 | self.assertFalse(created) |
637 | @@ -448,7 +452,7 @@ |
638 | self.factory.makeUser( |
639 | first_name='John', last_name='Does', email='user@email.com', |
640 | open_id=open_id) |
641 | - user, created = update_or_create_user_from_oauth( |
642 | + user, created = update_or_create_user_from_sso( |
643 | open_id, update_window=None) |
644 | self.assertIsNotNone(user) |
645 | self.assertFalse(created) |
646 | @@ -463,9 +467,113 @@ |
647 | self.factory.makeUser( |
648 | first_name='John', last_name='Does', email='user@email.com', |
649 | open_id=open_id) |
650 | - user, created = update_or_create_user_from_oauth( |
651 | + user, created = update_or_create_user_from_sso( |
652 | open_id, update_window=None) |
653 | self.assertIsNotNone(user) |
654 | self.assertFalse(created) |
655 | self.assertEqual(user.get_full_name(), 'John Does') |
656 | self.assertEqual(user.email, 'user@email.com') |
657 | + |
658 | + def test_get_user_from_account_data(self): |
659 | + openid = 'oid1234' |
660 | + sso_user_data = self.mock_data_for_account(openid=openid) |
661 | + |
662 | + db_user = self.factory.makeUser( |
663 | + email=sso_user_data['email'], open_id=openid) |
664 | + |
665 | + user, created = update_or_create_user_from_sso( |
666 | + openid, account=sso_user_data) |
667 | + self.assertFalse(created) |
668 | + self.assertEqual(user, db_user) |
669 | + |
670 | + def test_create_user_from_account_data(self): |
671 | + openid = 'oid1234' |
672 | + sso_user_data = self.mock_data_for_account(openid=openid) |
673 | + |
674 | + user, created = update_or_create_user_from_sso( |
675 | + openid, account=sso_user_data) |
676 | + self.assertTrue(created) |
677 | + self.assertEqual(user.email, sso_user_data['email']) |
678 | + self.assertEqual(user.useropenid_set.first().claimed_id, |
679 | + full_claimed_id(openid)) |
680 | + |
681 | + |
682 | +class VerifyACLTestCase(TestCaseWithFactory, RequestsDoubleTestCaseMixin): |
683 | + |
684 | + def setUp(self): |
685 | + super(VerifyACLTestCase, self).setUp() |
686 | + self.patch_requests() |
687 | + self.sca = WebServices() |
688 | + self.sca_double = SCADouble(self.requests_double) |
689 | + p = patch('core.utilities.logger') |
690 | + self.mock_logger = p.start() |
691 | + self.addCleanup(p.stop) |
692 | + |
693 | + def make_request(self): |
694 | + request = RequestFactory().post('/') |
695 | + authorization = 'Macaroon root="%s", discharge="%s"' % ( |
696 | + 'root-macaroon-data', 'discharge-macaroon-data') |
697 | + request.META['HTTP_AUTHORIZATION'] = authorization |
698 | + return request |
699 | + |
700 | + def assert_verify_acl_response(self, data, **kwargs): |
701 | + expected = { |
702 | + 'allowed': True, |
703 | + 'refresh_required': False, |
704 | + 'account': { |
705 | + 'openid': 'oid1234', |
706 | + 'email': 'user@example.com', |
707 | + 'displayname': 'user1234', |
708 | + 'verified': True, |
709 | + }, |
710 | + 'last_auth': '20160515T12:00:00.000', |
711 | + 'permissions': ['package_access'], |
712 | + } |
713 | + expected.update(**kwargs) |
714 | + self.assertEqual(data, expected) |
715 | + |
716 | + def test_verify_acl_allowed(self): |
717 | + self.sca_double.set_verify_acl_response() |
718 | + request = self.make_request() |
719 | + data = self.sca.verify_acl(request) |
720 | + self.assert_verify_acl_response(data) |
721 | + |
722 | + def test_verify_acl_not_allowed(self): |
723 | + self.sca_double.set_verify_acl_response(allowed=False) |
724 | + request = self.make_request() |
725 | + data = self.sca.verify_acl(request) |
726 | + self.assertIsNone(data) |
727 | + |
728 | + def test_verify_acl_failed(self): |
729 | + self.requests_double.add_response( |
730 | + self.sca_double.verify_acl_url, |
731 | + method='POST', body='some error', status_code=500) |
732 | + request = self.make_request() |
733 | + data = self.sca.verify_acl(request) |
734 | + self.assertIsNone(data) |
735 | + self.mock_logger.warn.assert_called_once_with( |
736 | + 'Failed to verify acl. Response: some error (500)') |
737 | + |
738 | + def test_verify_acl_asks_for_right_permission(self): |
739 | + self.sca_double.set_verify_acl_response(permissions=['package_upload']) |
740 | + request = self.make_request() |
741 | + data = self.sca.verify_acl(request) |
742 | + self.assertIsNone(data) |
743 | + |
744 | + def test_verify_acl_returns_account_data(self): |
745 | + expected = { |
746 | + 'allowed': True, |
747 | + 'refresh_required': False, |
748 | + 'account': { |
749 | + 'openid': 'openid', |
750 | + 'displayname': 'username', |
751 | + 'email': 'user@example.com', |
752 | + 'verified': True, |
753 | + }, |
754 | + 'last_auth': now().isoformat(), |
755 | + 'permissions': ['package_access'], |
756 | + } |
757 | + self.sca_double.set_verify_acl_response(**expected) |
758 | + request = self.make_request() |
759 | + data = self.sca.verify_acl(request) |
760 | + self.assert_verify_acl_response(data, **expected) |
761 | |
762 | === modified file 'src/core/utilities.py' |
763 | --- src/core/utilities.py 2014-10-03 17:06:49 +0000 |
764 | +++ src/core/utilities.py 2016-05-17 14:32:36 +0000 |
765 | @@ -1,9 +1,11 @@ |
766 | +import json |
767 | import sys |
768 | import time |
769 | |
770 | import logging |
771 | from httplib2 import ServerNotFoundError |
772 | |
773 | +import requests |
774 | from django.conf import settings |
775 | from django.contrib.auth.models import User |
776 | from django.core.cache import cache |
777 | @@ -189,6 +191,28 @@ |
778 | |
779 | return pubs |
780 | |
781 | + def verify_acl(self, request): |
782 | + data = { |
783 | + 'auth_data': json.dumps({ |
784 | + 'http_url': request.build_absolute_uri(), |
785 | + 'http_method': request.method, |
786 | + 'authorization': request.META.get('HTTP_AUTHORIZATION', None), |
787 | + }), |
788 | + } |
789 | + url = settings.SCA_HOST_URL.strip('/') + '/dev/api/acl/verify/' |
790 | + response = requests.post(url, data=json.dumps(data)) |
791 | + if response.ok: |
792 | + data = response.json() |
793 | + if data.get('allowed', False): |
794 | + if 'package_access' in data.get('permissions', []): |
795 | + # macaroon verified alright |
796 | + return data |
797 | + # acl verification failed, either request was not allowed, |
798 | + # or lacked necessary permissions |
799 | + return |
800 | + logger.warn('Failed to verify acl. Response: {} ({})'.format( |
801 | + response.content, response.status_code)) |
802 | + |
803 | |
804 | web_services = WebServices() |
805 | |
806 | @@ -226,7 +250,7 @@ |
807 | |
808 | |
809 | @transaction.commit_on_success |
810 | -def _get_or_create_user_from_oauth(consumer_key): |
811 | +def _get_or_create_user_from_sso(consumer_key, account=None): |
812 | claimed_id = full_claimed_id(consumer_key) |
813 | created = False |
814 | |
815 | @@ -236,8 +260,9 @@ |
816 | user = None |
817 | |
818 | if user is None: |
819 | - account = web_services.get_data_for_account( |
820 | - openid_identifier=consumer_key) |
821 | + if account is None: |
822 | + account = web_services.get_data_for_account( |
823 | + openid_identifier=consumer_key) |
824 | account_data = _get_sso_account_data(account) |
825 | |
826 | if account_data is None: |
827 | @@ -271,8 +296,8 @@ |
828 | return user, created |
829 | |
830 | |
831 | -def update_or_create_user_from_oauth( |
832 | - consumer_key, update_window=settings.SSO_UPDATE_WINDOW): |
833 | +def update_or_create_user_from_sso( |
834 | + consumer_key, update_window=settings.SSO_UPDATE_WINDOW, account=None): |
835 | """Update or create user using SSO account data. |
836 | |
837 | This function tries to retrieve a `User` from the database using |
838 | @@ -283,15 +308,24 @@ |
839 | given `update_window`. If `update_window` is `None`, SSO data is |
840 | always updated. |
841 | |
842 | + If the optional `account` parameter is provided, SSO will not be contacted |
843 | + for retrieving the initial user data, but the provided `account` data |
844 | + will be used. |
845 | + |
846 | Returns: |
847 | A two-element tuple, where the first element is the |
848 | created/updated user or None and the last one is a boolean |
849 | especifying whether the user was created or not. |
850 | |
851 | """ |
852 | - user, created = _get_or_create_user_from_oauth(consumer_key) |
853 | + user, created = _get_or_create_user_from_sso( |
854 | + consumer_key, account=account) |
855 | |
856 | if user is not None: |
857 | + # check for update even if account data was provided |
858 | + # account data is only used for the initial user creation; |
859 | + # after that, wether the login is 'fresh' or not (from rnr perspective) |
860 | + # is orthogonal to how the original data was retrieved |
861 | if update_window is None: |
862 | should_update = True |
863 | else: |
864 | |
865 | === modified file 'src/reviewsapp/api/urls.py' |
866 | --- src/reviewsapp/api/urls.py 2014-10-09 19:32:45 +0000 |
867 | +++ src/reviewsapp/api/urls.py 2016-05-17 14:32:36 +0000 |
868 | @@ -22,7 +22,7 @@ |
869 | ) |
870 | |
871 | from piston.resource import Resource |
872 | -from core.api.auth import SSOOAuthAuthentication |
873 | +from core.api.auth import MacaroonsAuthentication, SSOOAuthAuthentication |
874 | from core.api.decorators import ( |
875 | gzip_content, |
876 | vary_only_on_accept, |
877 | @@ -44,6 +44,7 @@ |
878 | ) |
879 | |
880 | auth = SSOOAuthAuthentication(realm="Ratings and Reviews") |
881 | +macaroons_auth = MacaroonsAuthentication(realm="Ratings and Reviews") |
882 | |
883 | |
884 | # The server status resource should never be cached. |
885 | @@ -99,8 +100,8 @@ |
886 | |
887 | # The following POST handlers have POST data and will not be cached. |
888 | # The .__name__ hack is needed to support Django 1.1 |
889 | -submit_review_resource = CSRFExemptResource(handler=SubmitReviewHandler, |
890 | - authentication=auth) |
891 | +submit_review_resource = CSRFExemptResource( |
892 | + handler=SubmitReviewHandler, authentication=(macaroons_auth, auth)) |
893 | modify_review_resource = CSRFExemptResource(handler=ModifyReviewHandler, |
894 | authentication=auth) |
895 | flag_review_resource = CSRFExemptResource(handler=FlagReviewHandler, |
896 | |
897 | === modified file 'src/reviewsapp/tests/test_handlers.py' |
898 | --- src/reviewsapp/tests/test_handlers.py 2016-04-04 04:10:58 +0000 |
899 | +++ src/reviewsapp/tests/test_handlers.py 2016-05-17 14:32:36 +0000 |
900 | @@ -56,7 +56,11 @@ |
901 | from piston.emitters import Emitter |
902 | from piston.handler import typemapper |
903 | |
904 | -from core.tests.helpers import assert_no_extra_queries_after |
905 | +from core.tests.doubles import SCADouble |
906 | +from core.tests.helpers import ( |
907 | + RequestsDoubleTestCaseMixin, |
908 | + assert_no_extra_queries_after, |
909 | +) |
910 | from reviewsapp.api.handlers import ( |
911 | Django13JSONEncoder, |
912 | Django13JSONEmitter, |
913 | @@ -983,7 +987,8 @@ |
914 | self.assertEqual(response['vary'], 'Accept') |
915 | |
916 | |
917 | -class SubmitReviewHandlerTestCase(TestCaseWithFactory): |
918 | +class SubmitReviewHandlerTestCase(TestCaseWithFactory, |
919 | + RequestsDoubleTestCaseMixin): |
920 | |
921 | def setUp(self): |
922 | super(SubmitReviewHandlerTestCase, self).setUp() |
923 | @@ -1000,13 +1005,26 @@ |
924 | } |
925 | self.factory.makeArch('i386') |
926 | |
927 | - def _post_new_review(self, data, user=None, verify_result=True): |
928 | + self.patch_requests() |
929 | + self.sca_double = SCADouble(self.requests_double) |
930 | + p = patch('core.utilities.web_services', new=self.sca_double) |
931 | + p.start() |
932 | + self.addCleanup(p.stop) |
933 | + |
934 | + def _post_new_review(self, data, user=None, verify_result=True, |
935 | + auth='oauth'): |
936 | # Post a review as an authenticated user. |
937 | url = reverse('rnr-api-reviews') |
938 | |
939 | - if user is None: |
940 | - user = self.factory.makeUser() |
941 | - self.client.login(username=user.username, password='test') |
942 | + headers = {} |
943 | + if auth == 'macaroon': |
944 | + authorization = 'Macaroon root="%s", discharge="%s"' % ( |
945 | + 'root-macaroon-data', 'discharge-macaroon-data') |
946 | + headers['HTTP_AUTHORIZATION'] = authorization |
947 | + else: |
948 | + if user is None: |
949 | + user = self.factory.makeUser() |
950 | + self.client.login(username=user.username, password='test') |
951 | |
952 | # Ensure we don't hit the network for our tests. |
953 | is_authed_fn = ('core.api.auth.SSOOAuthAuthentication.' |
954 | @@ -1022,7 +1040,8 @@ |
955 | with patch(sca_verify_fn) as mock_sca_verify_method: |
956 | mock_sca_verify_method.return_value = verify_result |
957 | response = self.client.post( |
958 | - url, data=data, content_type='application/json') |
959 | + url, data=data, content_type='application/json', |
960 | + **headers) |
961 | return response |
962 | |
963 | def test_bogus_data(self): |
964 | @@ -1058,6 +1077,18 @@ |
965 | response_dict = simplejson.loads(response.content) |
966 | self.assertEqual('inkscape', response_dict['package_name']) |
967 | |
968 | + def test_create_using_macaroon_auth(self): |
969 | + self.sca_double.set_verify_acl_response( |
970 | + is_verified=True, openid='oid1234', |
971 | + email='user@example.com', displayname='user1234') |
972 | + response = self._post_new_review(simplejson.dumps(self.required_data), |
973 | + auth='macaroon') |
974 | + self.assertEqual(httplib.OK, response.status_code) |
975 | + self.assertEqual('application/json; charset=utf-8', |
976 | + response['content-type']) |
977 | + response_dict = simplejson.loads(response.content) |
978 | + self.assertEqual('inkscape', response_dict['package_name']) |
979 | + |
980 | def test_creates_repository_and_software_item(self): |
981 | # Submitting a review for a software item or repository of which |
982 | # we don't yet have a record will create those records (after |
added few comments, otherwise is looking good.