Merge lp:~maxiberta/canonical-identity-provider/2fa-readonly into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Read-only mode 2FA: allow TOTP devices only, disable sync.

2FA is currently bypassed in readonly mode, lowering the security requirements of accounts with 2FA devices. This branch should improve the situation by allowing TOTP devices in readonly mode, while counter based devices (HOTP and text codes) are not allowed to authenticate. A warning message is shown next to the 2FA input when in readonly mode, and non-TOTPs will just be rejected as invalid.

Counter based device sync is disabled in readonly mode, as well.

Screenshots:

Readwrite (unchanged): https://private-fileshare.canonical.com/~maxiberta/Screenshot_20191022_164412.png

Read-only: https://private-fileshare.canonical.com/~maxiberta/Screenshot_20191022_163904.png (note the missing "sync your device" links)

Note this depends on a prerequisite branch with general readonly improvements.

[Daniel Manrique (roadmr)]

LGTM!

review: Approve

=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py	2019-06-18 20:13:31 +0000
+++ src/identityprovider/forms.py	2019-10-23 14:40:43 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import logging
@@ -114,7 +114,11 @@
     def __init__(self, *args, **kwargs):
         user = kwargs.pop('user', None)
         super(TwoFactorForm, self).__init__(*args, **kwargs)
-        self.sync_2fa_available = user and user.has_twofactor_hotp_devices()
+        self.sync_2fa_available = (
+            user
+            and user.has_twofactor_hotp_devices()
+            and not settings.READ_ONLY_MODE
+        )
 
 
 class TwoFactorSyncForm(forms.Form):

=== modified file 'src/identityprovider/models/twofactor.py'
--- src/identityprovider/models/twofactor.py	2019-02-22 15:35:45 +0000
+++ src/identityprovider/models/twofactor.py	2019-10-23 14:40:43 +0000
@@ -36,6 +36,8 @@
 def authenticate(account, otp):
     """Authenticates oath_token against the devices for an account."""
     for device in account.devices.order_by('-counter'):
+        if settings.READ_ONLY_MODE and not device.is_totp():
+            continue
         if device.authenticate(otp):
             return True
     # avoid circular import (identityprovider.login import
@@ -46,9 +48,10 @@
 
 def sync(account, otps):
     """Tries to sync one of the devices with the provided otps."""
-    for device in account.devices.order_by('-counter'):
-        if not device.is_totp() and device.sync(otps):
-            return True
+    if not settings.READ_ONLY_MODE:
+        for device in account.devices.order_by('-counter'):
+            if not device.is_totp() and device.sync(otps):
+                return True
     # avoid circular import (identityprovider.login import
     # identityprovider.models which import from .twofactor import *
     from identityprovider.login import AuthenticationError
@@ -56,10 +59,7 @@
 
 
 def is_twofactor_enabled(request):
-    if settings.READ_ONLY_MODE:
-        return False
-    else:
-        return gargoyle.is_active('TWOFACTOR', request)
+    return gargoyle.is_active('TWOFACTOR', request)
 
 
 def login(request):

=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2019-10-14 20:57:04 +0000
+++ src/identityprovider/tests/test_forms.py	2019-10-23 14:40:43 +0000
@@ -191,11 +191,11 @@
         form = self.get_form()
         self.assertFalse(form.sync_2fa_available)
 
-    def test_2fa_sync_unavailable_with_no_htop(self):
+    def test_2fa_sync_unavailable_with_no_hotp(self):
         form = self.get_form(user=self.account)
         self.assertFalse(form.sync_2fa_available)
 
-    def test_2fa_sync_available_with_htop(self):
+    def test_2fa_sync_available_with_hotp(self):
         self.factory.make_device(account=self.account, oath_mode='hotp')
         form = self.get_form(user=self.account)
         self.assertTrue(form.sync_2fa_available)
@@ -205,6 +205,12 @@
         form = self.get_form(user=self.account)
         self.assertFalse(form.sync_2fa_available)
 
+    def test_2fa_sync_unavailable_with_hotp_when_readonly(self):
+        self.factory.make_device(account=self.account, oath_mode='hotp')
+        with self.settings(READ_ONLY_MODE=True):
+            form = self.get_form(user=self.account)
+        self.assertFalse(form.sync_2fa_available)
+
 
 class TwoFactorLoginFormTestCase(LoginFormTestCase, TwoFactorFormTestCase):
 

=== modified file 'src/identityprovider/tests/test_models_twofactor.py'
--- src/identityprovider/tests/test_models_twofactor.py	2018-06-21 15:08:26 +0000
+++ src/identityprovider/tests/test_models_twofactor.py	2019-10-23 14:40:43 +0000
@@ -9,6 +9,7 @@
 from identityprovider.models.twofactor import (
     TWOFACTOR_LOGIN,
     AuthenticationDevice,
+    authenticate,
     is_authenticated,
     is_twofactor_enabled,
     is_upgraded,
@@ -151,7 +152,7 @@
 
     def test_is_twofactor_enabled_in_readonly_mode(self):
         with self.settings(READ_ONLY_MODE=True):
-            self.assertFalse(is_twofactor_enabled(self.request))
+            self.assertTrue(is_twofactor_enabled(self.request))
 
     @patch('identityprovider.models.twofactor.gargoyle.is_active')
     def test_is_twofactor_enabled_in_readwrite_mode(self, mock_is_active):
@@ -324,3 +325,71 @@
         devices[0].sync.assert_not_called(otps)
         devices[1].sync.assert_not_called(otps)
         devices[2].sync.assert_not_called(otps)
+
+    def test_sync_disabled_when_readonly(self):
+        account = Mock()
+        devices = [Mock(), Mock()]
+        otps = ['otp1', 'otp2']
+        account.devices.order_by.return_value = devices
+        devices[0].is_totp.return_value = False
+        devices[0].sync.return_value = False
+        devices[1].is_totp.return_value = False
+        devices[1].sync.return_value = True
+
+        with self.settings(READ_ONLY_MODE=True):
+            self.assertRaises(AuthenticationError, sync, account, otps)
+
+    def test_authenticate_stops_on_first_device_that_succeeds(self):
+        account = Mock()
+        devices = [Mock(), Mock(), Mock()]
+        account.devices.order_by.return_value = devices
+        devices[0].is_totp.return_value = False
+        devices[0].authenticate.return_value = False
+        devices[1].is_totp.return_value = True
+        devices[1].authenticate.return_value = True
+
+        self.assertTrue(authenticate(account, 'otp'))
+
+        devices[0].authenticate.assert_called_once_with('otp')
+        devices[1].authenticate.assert_called_once_with('otp')
+        self.assertFalse(devices[2].called)
+
+    def test_authenticate_raises_error_if_none_of_the_devices_succeeds(self):
+        account = Mock()
+        devices = [Mock(), Mock(), Mock()]
+        account.devices.order_by.return_value = devices
+        devices[0].authenticate.return_value = False
+        devices[0].is_totp.return_value = False
+        devices[1].authenticate.return_value = False
+        devices[1].is_totp.return_value = False
+        devices[2].authenticate.return_value = False
+        devices[2].is_totp.return_value = False
+
+        self.assertRaises(AuthenticationError, authenticate, account, 'otp')
+
+        devices[0].authenticate.assert_called_once_with('otp')
+        devices[1].authenticate.assert_called_once_with('otp')
+        devices[2].authenticate.assert_called_once_with('otp')
+
+    def test_authenticate_totp_enabled_when_readonly(self):
+        account = Mock()
+        devices = [Mock(), Mock()]
+        account.devices.order_by.return_value = devices
+        devices[0].is_totp.return_value = True
+        devices[0].authenticate.return_value = True
+
+        with self.settings(READ_ONLY_MODE=True):
+            self.assertTrue(authenticate(account, 'otp'))
+
+    def test_authenticate_non_totp_disabled_when_readonly(self):
+        account = Mock()
+        devices = [Mock(), Mock()]
+        otp = 'otp'
+        account.devices.order_by.return_value = devices
+        devices[0].is_totp.return_value = False
+        devices[0].authenticate.return_value = True
+        devices[1].is_totp.return_value = True
+        devices[1].authenticate.return_value = False
+
+        with self.settings(READ_ONLY_MODE=True):
+            self.assertRaises(AuthenticationError, authenticate, account, otp)

=== modified file 'src/webui/templates/registration/twofactor.html'
--- src/webui/templates/registration/twofactor.html	2016-12-09 10:16:50 +0000
+++ src/webui/templates/registration/twofactor.html	2019-10-23 14:40:43 +0000
@@ -52,6 +52,10 @@
                 <p id="oathtoken" class="input-row{% if form.oath_token.errors or form.non_field_errors %} haserrors{% endif %}">
 
                     <label for="id_oath_token">{% trans "Type your verification code:" %}</label>
+                    {% if readonly %}
+                    <span class="error">{% trans "Counter-based devices are disabled while the system is in read-only mode." %}</span>
+                    <span class="error">{% trans "Please use a time-based device, or try again later." %}</span>
+                    {% endif %}
 
                     {{ form.oath_token }}
 

=== modified file 'src/webui/tests/test_views_registration.py'
--- src/webui/tests/test_views_registration.py	2019-10-22 14:03:48 +0000
+++ src/webui/tests/test_views_registration.py	2019-10-23 14:40:43 +0000
@@ -797,3 +797,26 @@
 
         # success
         self.assertRedirects(response, reverse('account-index'))
+
+    def test_get_disabled_when_readonly(self):
+        self.factory.make_account(email=self.email)
+        self.do_login()
+
+        with readonly_enabled():
+            response = self.client.get(self.url)
+
+        self.assertEqual(response.status_code, 403)
+        self.assertTemplateUsed(response, 'readonly.html')
+
+    def test_post_disabled_when_readonly(self):
+        account = self.factory.make_account(email=self.email)
+        device = self.factory.make_device(account)
+        data = self.make_data(device, first_counter=10)
+        self.do_login()
+
+        with readonly_enabled():
+            response = self.client.post(self.url, data=data)
+
+        self.assertEqual(response.status_code, 403)
+        self.assertTemplateUsed(response, 'readonly.html')
+        self.assertEqual(device.counter, 0)

=== modified file 'src/webui/tests/test_views_ui.py'
--- src/webui/tests/test_views_ui.py	2019-10-22 14:03:48 +0000
+++ src/webui/tests/test_views_ui.py	2019-10-23 14:40:43 +0000
@@ -1529,13 +1529,18 @@
             'webui.views.ui.twofactor.user_requires_twofactor_auth',
             return_value=True)
 
-    def test_twofactor_login_disabled_when_readonly(self):
+    def test_twofactor_login_shows_warning_when_readonly(self):
         self.mock_site.return_value = False
+        self.factory.make_device(account=self.account)
 
         with switches(TWOFACTOR=True), readonly_enabled():
             self.do_login()
             response = self.client.get(self.url)
-        self.assertEqual(response.status_code, 404)
+
+        self.assertContains(
+            response,
+            'Counter-based devices are disabled '
+            'while the system is in read-only mode.')
 
     def test_when_site_does_not_require_twofactor_no_oath_field(self):
         self.mock_site.return_value = False
@@ -1718,7 +1723,7 @@
 
         self.assertContains(response, reverse('sync_2fa'))
 
-    def test_shows_sync_link_when_otp_is_invalid_explicit_and_have_htop(self):
+    def test_shows_sync_link_when_otp_is_invalid_explicit_and_have_hotp(self):
         self.factory.make_device(account=self.account, oath_mode='hotp')
         self.client.login(username=self.email, password=self.password)
 
@@ -1726,7 +1731,7 @@
 
         self.assertContains(response, reverse('sync_2fa'))
 
-    def test_hide_sync_link_when_otp_is_invalid_explicit_and_no_htop(self):
+    def test_hide_sync_link_when_otp_is_invalid_explicit_and_no_hotp(self):
         self.client.login(username=self.email, password=self.password)
 
         response = self.do_twofactor(oath_token='123456')

=== modified file 'src/webui/views/registration.py'
--- src/webui/views/registration.py	2019-01-09 18:37:18 +0000
+++ src/webui/views/registration.py	2019-10-23 14:40:43 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010, 2012 Canonical Ltd.  This software is licensed under
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under
 # the GNU Affero General Public License version 3 (see the file
 # LICENSE).
 
@@ -310,6 +310,7 @@
             return self._render_form(request, form, request.POST)
 
     @method_decorator(dont_cache)
+    @method_decorator(check_readonly)
     @method_decorator(limitlogin())
     # for this page, we need to be logged in but NOT twofactored
     @method_decorator(login_required)