Canonical SSO provider

Merge lp:~canonical-isd-hackers/canonical-identity-provider/fix952915 into lp:canonical-identity-provider/release

  1. fix952915
  2. Merge into trunk
Proposed by Simon Davy
Status: Merged
Approved by: David Owen
Approved revision: no longer in the source branch.
Merged at revision: 370
Proposed branch: lp:~canonical-isd-hackers/canonical-identity-provider/fix952915
Merge into: lp:canonical-identity-provider/release
Diff against target: 201 lines (+63/-31)
4 files modified
identityprovider/decorators.py (+6/-4)
identityprovider/tests/unit/test_decorators.py (+34/-13)
identityprovider/tests/unit/test_views_devices.py (+15/-0)
identityprovider/views/devices.py (+8/-14)
To merge this branch: bzr merge lp:~canonical-isd-hackers/canonical-identity-provider/fix952915
Reviewer Review Type Date Requested Status
David Owen (community) Approve
Review via email: mp+97034@code.launchpad.net

Commit message

Fixes #953036 by adding a require_twofactor directive to sso_login_required, to allow views to be 2f protected in a standard manner, and then applying as appropriate.

Description of the change

Fixes #953036 by adding a require_twofactor directive to sso_login_required, to allow views to be 2f protected in a standard manner, and then applying as appropriate

Note: device_addition still does the check manually, as it needs to special case for when there are NO devices.

To post a comment you must log in.
Revision history for this message
David Owen (dsowen) wrote :

#19-22 would be clearer, imo, as:

required = require_twofactor or user_requires_twofactor...

#23: No, not a good place to check the site requirements. The decide view must handle that (and do).

review: Needs Fixing
Revision history for this message
David Owen (dsowen) wrote :

Looks great.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'identityprovider/decorators.py'
--- identityprovider/decorators.py 2012-02-10 15:27:42 +0000
+++ identityprovider/decorators.py 2012-03-12 15:15:33 +0000
@@ -48,15 +48,17 @@
48 return _dont_cache_decorator48 return _dont_cache_decorator
4949
5050
51def sso_login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME, login_url=None):51def sso_login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME,
52 login_url=None, require_twofactor=False):
52 """53 """
53 Decorator the wraps up django's login_required, and also checks for 2f54 Decorator the wraps up django's login_required, and also checks for 2f
54 """55 """
55 def decorator(view_func):56 def decorator(view_func):
56 @wraps(view_func, assigned=available_attrs(view_func))57 @wraps(view_func, assigned=available_attrs(view_func))
57 def _wrapped_view(request, *args, **kwargs):58 def _wrapped_view(request, *args, **kwargs):
58 if (twofactor.user_requires_twofactor_auth(request, request.user) and59 required = (require_twofactor or
59 not twofactor.is_authenticated(request)):60 twofactor.user_requires_twofactor_auth(request, request.user))
61 if required and not twofactor.is_authenticated(request):
60 # only valid reverse arg is token62 # only valid reverse arg is token
61 reverse_args = {}63 reverse_args = {}
62 if 'token' in kwargs and kwargs['token'] != None:64 if 'token' in kwargs and kwargs['token'] != None:
@@ -67,7 +69,7 @@
67 redirect_field_name)69 redirect_field_name)
68 return view_func(request, *args, **kwargs)70 return view_func(request, *args, **kwargs)
69 return django_login_required(71 return django_login_required(
70 _wrapped_view, 72 _wrapped_view,
71 redirect_field_name,73 redirect_field_name,
72 login_url)74 login_url)
7375
7476
=== modified file 'identityprovider/tests/unit/test_decorators.py'
--- identityprovider/tests/unit/test_decorators.py 2012-02-07 16:45:12 +0000
+++ identityprovider/tests/unit/test_decorators.py 2012-03-12 15:15:33 +0000
@@ -65,19 +65,40 @@
6565
66 @patch('identityprovider.decorators.twofactor.is_authenticated')66 @patch('identityprovider.decorators.twofactor.is_authenticated')
67 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')67 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
68 def test_allowed_when_2f_required_and_2f_authed(self, mock_user, mock_auth):68 def test_allowed_when_2f_required_by_user_and_2f_authed(self, mock_user, mock_auth):
69 mock_user.return_value = True69 mock_user.return_value = True
70 mock_auth.return_value = True70 mock_auth.return_value = True
71 view = sso_login_required(self.fake_view, login_url='/login')71 view = sso_login_required(self.fake_view, login_url='/login')
72 response = view(self.mock_request(True))72 response = view(self.mock_request(True))
73 self.assertEqual(response, 'SUCCESS')73 self.assertEqual(response, 'SUCCESS')
7474
75 @patch('identityprovider.decorators.twofactor.is_authenticated')75 @patch('identityprovider.decorators.twofactor.is_authenticated')
76 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')76 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
77 def test_redirected_when_2f_required_and_not_2f_authed(self, mock_user, mock_auth):77 def test_allowed_when_2f_required_by_keyword_and_2f_authed(self, mock_user, mock_auth):
78 mock_user.return_value = True78 mock_user.return_value = False
79 mock_auth.return_value = False79 mock_auth.return_value = True
80 view = sso_login_required(self.fake_view, login_url='/login')80 deco = sso_login_required(require_twofactor=True, login_url='/login')
81 view = deco(self.fake_view)
82 response = view(self.mock_request(True))
83 self.assertEqual(response, 'SUCCESS')
84
85 @patch('identityprovider.decorators.twofactor.is_authenticated')
86 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
87 def test_redirected_when_2f_required_by_user_and_not_2f_authed(self, mock_user, mock_auth):
88 mock_user.return_value = True
89 mock_auth.return_value = False
90 view = sso_login_required(self.fake_view, login_url='/login')
91 response = view(self.mock_request(True, '/target'))
92 self.assertEqual(response.status_code, 302)
93 self.assertEqual(response['Location'], '/two_factor_auth?next=/target')
94
95 @patch('identityprovider.decorators.twofactor.is_authenticated')
96 @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
97 def test_redirected_when_2f_required_by_keyword_and_not_2f_authed(self, mock_user, mock_auth):
98 mock_user.return_value = False
99 mock_auth.return_value = False
100 deco = sso_login_required(require_twofactor=True, login_url='/login')
101 view = deco(self.fake_view)
81 response = view(self.mock_request(True, '/target'))102 response = view(self.mock_request(True, '/target'))
82 self.assertEqual(response.status_code, 302)103 self.assertEqual(response.status_code, 302)
83 self.assertEqual(response['Location'], '/two_factor_auth?next=/target')104 self.assertEqual(response['Location'], '/two_factor_auth?next=/target')
84105
=== modified file 'identityprovider/tests/unit/test_views_devices.py'
--- identityprovider/tests/unit/test_views_devices.py 2012-03-12 11:47:08 +0000
+++ identityprovider/tests/unit/test_views_devices.py 2012-03-12 15:15:33 +0000
@@ -5,6 +5,7 @@
5from django.test import TestCase5from django.test import TestCase
6from lxml.html import fromstring6from lxml.html import fromstring
7from pyquery import PyQuery7from pyquery import PyQuery
8import mock
89
9from identityprovider.models import AuthenticationDevice10from identityprovider.models import AuthenticationDevice
10from identityprovider.views.device_urldata import DEVICE_LIST11from identityprovider.views.device_urldata import DEVICE_LIST
@@ -32,6 +33,14 @@
32 password='test')33 password='test')
3334
34 self.rename_view = reverse('device-rename', args=[self.device.id])35 self.rename_view = reverse('device-rename', args=[self.device.id])
36 self.patch = mock.patch(
37 'identityprovider.models.twofactor.is_authenticated')
38 self.mock_auth = self.patch.__enter__()
39 self.mock_auth.return_value = True
40
41 def tearDown(self):
42 self.patch.__exit__(None, None, None)
43
3544
36 def test_device_rename_get(self):45 def test_device_rename_get(self):
37 response = self.client.get(self.rename_view)46 response = self.client.get(self.rename_view)
@@ -66,3 +75,9 @@
66 # post as new user75 # post as new user
67 response = self.client.post(self.rename_view, {'name': 'bar'})76 response = self.client.post(self.rename_view, {'name': 'bar'})
68 self.assertEqual(response.status_code, 404)77 self.assertEqual(response.status_code, 404)
78
79 def test_device_rename_post_not_authed(self):
80 self.mock_auth.return_value = False
81 response = self.client.post(self.rename_view, {'name': 'bar'})
82 self.assertEqual(response.status_code, 302)
83 self.assertIn(reverse('twofactor'), response['Location'])
6984
=== modified file 'identityprovider/views/devices.py'
--- identityprovider/views/devices.py 2012-03-12 11:09:42 +0000
+++ identityprovider/views/devices.py 2012-03-12 15:15:33 +0000
@@ -31,7 +31,7 @@
3131
3232
33# A decorator33# A decorator
34require_twofactor = switch_is_active('TWOFACTOR')34require_twofactor_enabled = switch_is_active('TWOFACTOR')
3535
3636
37device_types = {37device_types = {
@@ -74,7 +74,7 @@
7474
7575
76@sso_login_required76@sso_login_required
77@require_twofactor77@require_twofactor_enabled
78@allow_only('GET', 'POST')78@allow_only('GET', 'POST')
79def device_list(request):79def device_list(request):
80 if request.method == 'POST':80 if request.method == 'POST':
@@ -113,7 +113,7 @@
113113
114114
115@sso_login_required115@sso_login_required
116@require_twofactor116@require_twofactor_enabled
117@allow_only('GET', 'POST')117@allow_only('GET', 'POST')
118def device_addition(request):118def device_addition(request):
119119
@@ -211,7 +211,7 @@
211211
212212
213@sso_login_required213@sso_login_required
214@require_twofactor214@require_twofactor_enabled
215@allow_only('GET', 'POST')215@allow_only('GET', 'POST')
216def device_verification(request, device_id):216def device_verification(request, device_id):
217 # TODO: Verify that the logged-in user actually owns the device217 # TODO: Verify that the logged-in user actually owns the device
@@ -229,16 +229,10 @@
229 return get_object_or_404(AuthenticationDevice, id=device_id, account=user)229 return get_object_or_404(AuthenticationDevice, id=device_id, account=user)
230230
231231
232@sso_login_required232@sso_login_required(require_twofactor=True)
233@require_twofactor233@require_twofactor_enabled
234@allow_only('GET', 'POST')234@allow_only('GET', 'POST')
235def device_removal(request, device_id):235def device_removal(request, device_id):
236 if not twofactor.is_authenticated(request):
237 return redirect_to_login(
238 request.get_full_path(),
239 reverse('twofactor')
240 )
241
242 device = _get_device_or_404(device_id, request.user)236 device = _get_device_or_404(device_id, request.user)
243237
244 if request.method != 'POST':238 if request.method != 'POST':
@@ -289,11 +283,11 @@
289 device_list_path=reverse(DEVICE_LIST),283 device_list_path=reverse(DEVICE_LIST),
290 form=form))284 form=form))
291285
292device_rename = sso_login_required(DeviceRenameView.as_view())286device_rename = sso_login_required(require_twofactor=True)(DeviceRenameView.as_view())
293287
294288
295@sso_login_required289@sso_login_required
296@require_twofactor290@require_twofactor_enabled
297@allow_only('GET', 'POST')291@allow_only('GET', 'POST')
298def pad_removal(request):292def pad_removal(request):
299 if request.method == 'GET':293 if request.method == 'GET':