Merge lp:~ricardokirkner/rnr-server/submit-review-with-macaroons into lp:rnr-server

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

To post a comment you must log in.
Revision history for this message
Fabián Ezequiel Gallina (fgallina) wrote :

added few comments, otherwise is looking good.

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) :
Revision history for this message
Fabián Ezequiel Gallina (fgallina) wrote :

LGTM

review: Approve

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

Subscribers

People subscribed via source and target branches