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

[Simon Davy]

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.

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.

[David Owen (dsowen)]

#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

[David Owen (dsowen)]

Looks great.

review: Approve

=== 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 @@
     return _dont_cache_decorator
 
 
-def sso_login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME, login_url=None):
+def sso_login_required(function=None, redirect_field_name=REDIRECT_FIELD_NAME,
+        login_url=None, require_twofactor=False):
     """
     Decorator the wraps up django's login_required, and also checks for 2f
     """
     def decorator(view_func):
         @wraps(view_func, assigned=available_attrs(view_func))
         def _wrapped_view(request, *args, **kwargs):
-            if (twofactor.user_requires_twofactor_auth(request, request.user) and
-                    not twofactor.is_authenticated(request)):
+            required = (require_twofactor or
+                twofactor.user_requires_twofactor_auth(request, request.user))
+            if required and not twofactor.is_authenticated(request):
                 # only valid reverse arg is token
                 reverse_args = {}
                 if 'token' in kwargs and kwargs['token'] != None:
@@ -67,7 +69,7 @@
                         redirect_field_name)
             return view_func(request, *args, **kwargs)
         return django_login_required(
-                _wrapped_view, 
+                _wrapped_view,
                 redirect_field_name,
                 login_url)
 

=== 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 @@
 
     @patch('identityprovider.decorators.twofactor.is_authenticated')
     @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
-    def test_allowed_when_2f_required_and_2f_authed(self, mock_user, mock_auth):
-        mock_user.return_value = True
-        mock_auth.return_value = True
-        view = sso_login_required(self.fake_view, login_url='/login')
-        response = view(self.mock_request(True))
-        self.assertEqual(response, 'SUCCESS')
-
-    @patch('identityprovider.decorators.twofactor.is_authenticated')
-    @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
-    def test_redirected_when_2f_required_and_not_2f_authed(self, mock_user, mock_auth):
-        mock_user.return_value = True
-        mock_auth.return_value = False
-        view = sso_login_required(self.fake_view, login_url='/login')
+    def test_allowed_when_2f_required_by_user_and_2f_authed(self, mock_user, mock_auth):
+        mock_user.return_value = True
+        mock_auth.return_value = True
+        view = sso_login_required(self.fake_view, login_url='/login')
+        response = view(self.mock_request(True))
+        self.assertEqual(response, 'SUCCESS')
+
+    @patch('identityprovider.decorators.twofactor.is_authenticated')
+    @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
+    def test_allowed_when_2f_required_by_keyword_and_2f_authed(self, mock_user, mock_auth):
+        mock_user.return_value = False
+        mock_auth.return_value = True
+        deco = sso_login_required(require_twofactor=True, login_url='/login')
+        view = deco(self.fake_view)
+        response = view(self.mock_request(True))
+        self.assertEqual(response, 'SUCCESS')
+
+    @patch('identityprovider.decorators.twofactor.is_authenticated')
+    @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
+    def test_redirected_when_2f_required_by_user_and_not_2f_authed(self, mock_user, mock_auth):
+        mock_user.return_value = True
+        mock_auth.return_value = False
+        view = sso_login_required(self.fake_view, login_url='/login')
+        response = view(self.mock_request(True, '/target'))
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], '/two_factor_auth?next=/target')
+
+    @patch('identityprovider.decorators.twofactor.is_authenticated')
+    @patch('identityprovider.decorators.twofactor.user_requires_twofactor_auth')
+    def test_redirected_when_2f_required_by_keyword_and_not_2f_authed(self, mock_user, mock_auth):
+        mock_user.return_value = False
+        mock_auth.return_value = False
+        deco = sso_login_required(require_twofactor=True, login_url='/login')
+        view = deco(self.fake_view)
         response = view(self.mock_request(True, '/target'))
         self.assertEqual(response.status_code, 302)
         self.assertEqual(response['Location'], '/two_factor_auth?next=/target')

=== 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 @@
 from django.test import TestCase
 from lxml.html import fromstring
 from pyquery import PyQuery
+import mock
 
 from identityprovider.models import AuthenticationDevice
 from identityprovider.views.device_urldata import DEVICE_LIST
@@ -32,6 +33,14 @@
             password='test')
 
         self.rename_view = reverse('device-rename', args=[self.device.id])
+        self.patch = mock.patch(
+                'identityprovider.models.twofactor.is_authenticated')
+        self.mock_auth = self.patch.__enter__()
+        self.mock_auth.return_value = True
+
+    def tearDown(self):
+        self.patch.__exit__(None, None, None)
+
 
     def test_device_rename_get(self):
         response = self.client.get(self.rename_view)
@@ -66,3 +75,9 @@
         # post as new user
         response = self.client.post(self.rename_view, {'name': 'bar'})
         self.assertEqual(response.status_code, 404)
+
+    def test_device_rename_post_not_authed(self):
+        self.mock_auth.return_value = False
+        response = self.client.post(self.rename_view, {'name': 'bar'})
+        self.assertEqual(response.status_code, 302)
+        self.assertIn(reverse('twofactor'), response['Location'])

=== 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 @@
 
 
 # A decorator
-require_twofactor = switch_is_active('TWOFACTOR')
+require_twofactor_enabled = switch_is_active('TWOFACTOR')
 
 
 device_types = {
@@ -74,7 +74,7 @@
 
 
 @sso_login_required
-@require_twofactor
+@require_twofactor_enabled
 @allow_only('GET', 'POST')
 def device_list(request):
     if request.method == 'POST':
@@ -113,7 +113,7 @@
 
 
 @sso_login_required
-@require_twofactor
+@require_twofactor_enabled
 @allow_only('GET', 'POST')
 def device_addition(request):
 
@@ -211,7 +211,7 @@
 
 
 @sso_login_required
-@require_twofactor
+@require_twofactor_enabled
 @allow_only('GET', 'POST')
 def device_verification(request, device_id):
     # TODO: Verify that the logged-in user actually owns the device
@@ -229,16 +229,10 @@
     return get_object_or_404(AuthenticationDevice, id=device_id, account=user)
 
 
-@sso_login_required
-@require_twofactor
+@sso_login_required(require_twofactor=True)
+@require_twofactor_enabled
 @allow_only('GET', 'POST')
 def device_removal(request, device_id):
-    if not twofactor.is_authenticated(request):
-        return redirect_to_login(
-            request.get_full_path(),
-            reverse('twofactor')
-        )
-
     device = _get_device_or_404(device_id, request.user)
 
     if request.method != 'POST':
@@ -289,11 +283,11 @@
                     device_list_path=reverse(DEVICE_LIST),
                     form=form))
 
-device_rename = sso_login_required(DeviceRenameView.as_view())
+device_rename = sso_login_required(require_twofactor=True)(DeviceRenameView.as_view())
 
 
 @sso_login_required
-@require_twofactor
+@require_twofactor_enabled
 @allow_only('GET', 'POST')
 def pad_removal(request):
     if request.method == 'GET':