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

Proposed by Maximiliano Bertacchini on 2019-10-22
Status: Merged
Approved by: Maximiliano Bertacchini on 2019-10-23
Approved revision: 1708
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~maxiberta/canonical-identity-provider/2fa-readonly
Merge into: lp:canonical-identity-provider/release
Prerequisite: lp:~maxiberta/canonical-identity-provider/readonly-improvements
Diff against target: 292 lines (+129/-17)
8 files modified
src/identityprovider/forms.py (+6/-2)
src/identityprovider/models/twofactor.py (+7/-7)
src/identityprovider/tests/test_forms.py (+8/-2)
src/identityprovider/tests/test_models_twofactor.py (+70/-1)
src/webui/templates/registration/twofactor.html (+4/-0)
src/webui/tests/test_views_registration.py (+23/-0)
src/webui/tests/test_views_ui.py (+9/-4)
src/webui/views/registration.py (+2/-1)
To merge this branch: bzr merge lp:~maxiberta/canonical-identity-provider/2fa-readonly
Reviewer Review Type Date Requested Status
Daniel Manrique 2019-10-22 Approve on 2019-10-23
Review via email: mp+374530@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

LGTM!

review: Approve
1708. By Maximiliano Bertacchini on 2019-10-23

Update 2FA readonly warning message.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/identityprovider/forms.py'
2--- src/identityprovider/forms.py 2019-06-18 20:13:31 +0000
3+++ src/identityprovider/forms.py 2019-10-23 14:40:43 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2010 Canonical Ltd. This software is licensed under the
6+# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 import logging
10@@ -114,7 +114,11 @@
11 def __init__(self, *args, **kwargs):
12 user = kwargs.pop('user', None)
13 super(TwoFactorForm, self).__init__(*args, **kwargs)
14- self.sync_2fa_available = user and user.has_twofactor_hotp_devices()
15+ self.sync_2fa_available = (
16+ user
17+ and user.has_twofactor_hotp_devices()
18+ and not settings.READ_ONLY_MODE
19+ )
20
21
22 class TwoFactorSyncForm(forms.Form):
23
24=== modified file 'src/identityprovider/models/twofactor.py'
25--- src/identityprovider/models/twofactor.py 2019-02-22 15:35:45 +0000
26+++ src/identityprovider/models/twofactor.py 2019-10-23 14:40:43 +0000
27@@ -36,6 +36,8 @@
28 def authenticate(account, otp):
29 """Authenticates oath_token against the devices for an account."""
30 for device in account.devices.order_by('-counter'):
31+ if settings.READ_ONLY_MODE and not device.is_totp():
32+ continue
33 if device.authenticate(otp):
34 return True
35 # avoid circular import (identityprovider.login import
36@@ -46,9 +48,10 @@
37
38 def sync(account, otps):
39 """Tries to sync one of the devices with the provided otps."""
40- for device in account.devices.order_by('-counter'):
41- if not device.is_totp() and device.sync(otps):
42- return True
43+ if not settings.READ_ONLY_MODE:
44+ for device in account.devices.order_by('-counter'):
45+ if not device.is_totp() and device.sync(otps):
46+ return True
47 # avoid circular import (identityprovider.login import
48 # identityprovider.models which import from .twofactor import *
49 from identityprovider.login import AuthenticationError
50@@ -56,10 +59,7 @@
51
52
53 def is_twofactor_enabled(request):
54- if settings.READ_ONLY_MODE:
55- return False
56- else:
57- return gargoyle.is_active('TWOFACTOR', request)
58+ return gargoyle.is_active('TWOFACTOR', request)
59
60
61 def login(request):
62
63=== modified file 'src/identityprovider/tests/test_forms.py'
64--- src/identityprovider/tests/test_forms.py 2019-10-14 20:57:04 +0000
65+++ src/identityprovider/tests/test_forms.py 2019-10-23 14:40:43 +0000
66@@ -191,11 +191,11 @@
67 form = self.get_form()
68 self.assertFalse(form.sync_2fa_available)
69
70- def test_2fa_sync_unavailable_with_no_htop(self):
71+ def test_2fa_sync_unavailable_with_no_hotp(self):
72 form = self.get_form(user=self.account)
73 self.assertFalse(form.sync_2fa_available)
74
75- def test_2fa_sync_available_with_htop(self):
76+ def test_2fa_sync_available_with_hotp(self):
77 self.factory.make_device(account=self.account, oath_mode='hotp')
78 form = self.get_form(user=self.account)
79 self.assertTrue(form.sync_2fa_available)
80@@ -205,6 +205,12 @@
81 form = self.get_form(user=self.account)
82 self.assertFalse(form.sync_2fa_available)
83
84+ def test_2fa_sync_unavailable_with_hotp_when_readonly(self):
85+ self.factory.make_device(account=self.account, oath_mode='hotp')
86+ with self.settings(READ_ONLY_MODE=True):
87+ form = self.get_form(user=self.account)
88+ self.assertFalse(form.sync_2fa_available)
89+
90
91 class TwoFactorLoginFormTestCase(LoginFormTestCase, TwoFactorFormTestCase):
92
93
94=== modified file 'src/identityprovider/tests/test_models_twofactor.py'
95--- src/identityprovider/tests/test_models_twofactor.py 2018-06-21 15:08:26 +0000
96+++ src/identityprovider/tests/test_models_twofactor.py 2019-10-23 14:40:43 +0000
97@@ -9,6 +9,7 @@
98 from identityprovider.models.twofactor import (
99 TWOFACTOR_LOGIN,
100 AuthenticationDevice,
101+ authenticate,
102 is_authenticated,
103 is_twofactor_enabled,
104 is_upgraded,
105@@ -151,7 +152,7 @@
106
107 def test_is_twofactor_enabled_in_readonly_mode(self):
108 with self.settings(READ_ONLY_MODE=True):
109- self.assertFalse(is_twofactor_enabled(self.request))
110+ self.assertTrue(is_twofactor_enabled(self.request))
111
112 @patch('identityprovider.models.twofactor.gargoyle.is_active')
113 def test_is_twofactor_enabled_in_readwrite_mode(self, mock_is_active):
114@@ -324,3 +325,71 @@
115 devices[0].sync.assert_not_called(otps)
116 devices[1].sync.assert_not_called(otps)
117 devices[2].sync.assert_not_called(otps)
118+
119+ def test_sync_disabled_when_readonly(self):
120+ account = Mock()
121+ devices = [Mock(), Mock()]
122+ otps = ['otp1', 'otp2']
123+ account.devices.order_by.return_value = devices
124+ devices[0].is_totp.return_value = False
125+ devices[0].sync.return_value = False
126+ devices[1].is_totp.return_value = False
127+ devices[1].sync.return_value = True
128+
129+ with self.settings(READ_ONLY_MODE=True):
130+ self.assertRaises(AuthenticationError, sync, account, otps)
131+
132+ def test_authenticate_stops_on_first_device_that_succeeds(self):
133+ account = Mock()
134+ devices = [Mock(), Mock(), Mock()]
135+ account.devices.order_by.return_value = devices
136+ devices[0].is_totp.return_value = False
137+ devices[0].authenticate.return_value = False
138+ devices[1].is_totp.return_value = True
139+ devices[1].authenticate.return_value = True
140+
141+ self.assertTrue(authenticate(account, 'otp'))
142+
143+ devices[0].authenticate.assert_called_once_with('otp')
144+ devices[1].authenticate.assert_called_once_with('otp')
145+ self.assertFalse(devices[2].called)
146+
147+ def test_authenticate_raises_error_if_none_of_the_devices_succeeds(self):
148+ account = Mock()
149+ devices = [Mock(), Mock(), Mock()]
150+ account.devices.order_by.return_value = devices
151+ devices[0].authenticate.return_value = False
152+ devices[0].is_totp.return_value = False
153+ devices[1].authenticate.return_value = False
154+ devices[1].is_totp.return_value = False
155+ devices[2].authenticate.return_value = False
156+ devices[2].is_totp.return_value = False
157+
158+ self.assertRaises(AuthenticationError, authenticate, account, 'otp')
159+
160+ devices[0].authenticate.assert_called_once_with('otp')
161+ devices[1].authenticate.assert_called_once_with('otp')
162+ devices[2].authenticate.assert_called_once_with('otp')
163+
164+ def test_authenticate_totp_enabled_when_readonly(self):
165+ account = Mock()
166+ devices = [Mock(), Mock()]
167+ account.devices.order_by.return_value = devices
168+ devices[0].is_totp.return_value = True
169+ devices[0].authenticate.return_value = True
170+
171+ with self.settings(READ_ONLY_MODE=True):
172+ self.assertTrue(authenticate(account, 'otp'))
173+
174+ def test_authenticate_non_totp_disabled_when_readonly(self):
175+ account = Mock()
176+ devices = [Mock(), Mock()]
177+ otp = 'otp'
178+ account.devices.order_by.return_value = devices
179+ devices[0].is_totp.return_value = False
180+ devices[0].authenticate.return_value = True
181+ devices[1].is_totp.return_value = True
182+ devices[1].authenticate.return_value = False
183+
184+ with self.settings(READ_ONLY_MODE=True):
185+ self.assertRaises(AuthenticationError, authenticate, account, otp)
186
187=== modified file 'src/webui/templates/registration/twofactor.html'
188--- src/webui/templates/registration/twofactor.html 2016-12-09 10:16:50 +0000
189+++ src/webui/templates/registration/twofactor.html 2019-10-23 14:40:43 +0000
190@@ -52,6 +52,10 @@
191 <p id="oathtoken" class="input-row{% if form.oath_token.errors or form.non_field_errors %} haserrors{% endif %}">
192
193 <label for="id_oath_token">{% trans "Type your verification code:" %}</label>
194+ {% if readonly %}
195+ <span class="error">{% trans "Counter-based devices are disabled while the system is in read-only mode." %}</span>
196+ <span class="error">{% trans "Please use a time-based device, or try again later." %}</span>
197+ {% endif %}
198
199 {{ form.oath_token }}
200
201
202=== modified file 'src/webui/tests/test_views_registration.py'
203--- src/webui/tests/test_views_registration.py 2019-10-22 14:03:48 +0000
204+++ src/webui/tests/test_views_registration.py 2019-10-23 14:40:43 +0000
205@@ -797,3 +797,26 @@
206
207 # success
208 self.assertRedirects(response, reverse('account-index'))
209+
210+ def test_get_disabled_when_readonly(self):
211+ self.factory.make_account(email=self.email)
212+ self.do_login()
213+
214+ with readonly_enabled():
215+ response = self.client.get(self.url)
216+
217+ self.assertEqual(response.status_code, 403)
218+ self.assertTemplateUsed(response, 'readonly.html')
219+
220+ def test_post_disabled_when_readonly(self):
221+ account = self.factory.make_account(email=self.email)
222+ device = self.factory.make_device(account)
223+ data = self.make_data(device, first_counter=10)
224+ self.do_login()
225+
226+ with readonly_enabled():
227+ response = self.client.post(self.url, data=data)
228+
229+ self.assertEqual(response.status_code, 403)
230+ self.assertTemplateUsed(response, 'readonly.html')
231+ self.assertEqual(device.counter, 0)
232
233=== modified file 'src/webui/tests/test_views_ui.py'
234--- src/webui/tests/test_views_ui.py 2019-10-22 14:03:48 +0000
235+++ src/webui/tests/test_views_ui.py 2019-10-23 14:40:43 +0000
236@@ -1529,13 +1529,18 @@
237 'webui.views.ui.twofactor.user_requires_twofactor_auth',
238 return_value=True)
239
240- def test_twofactor_login_disabled_when_readonly(self):
241+ def test_twofactor_login_shows_warning_when_readonly(self):
242 self.mock_site.return_value = False
243+ self.factory.make_device(account=self.account)
244
245 with switches(TWOFACTOR=True), readonly_enabled():
246 self.do_login()
247 response = self.client.get(self.url)
248- self.assertEqual(response.status_code, 404)
249+
250+ self.assertContains(
251+ response,
252+ 'Counter-based devices are disabled '
253+ 'while the system is in read-only mode.')
254
255 def test_when_site_does_not_require_twofactor_no_oath_field(self):
256 self.mock_site.return_value = False
257@@ -1718,7 +1723,7 @@
258
259 self.assertContains(response, reverse('sync_2fa'))
260
261- def test_shows_sync_link_when_otp_is_invalid_explicit_and_have_htop(self):
262+ def test_shows_sync_link_when_otp_is_invalid_explicit_and_have_hotp(self):
263 self.factory.make_device(account=self.account, oath_mode='hotp')
264 self.client.login(username=self.email, password=self.password)
265
266@@ -1726,7 +1731,7 @@
267
268 self.assertContains(response, reverse('sync_2fa'))
269
270- def test_hide_sync_link_when_otp_is_invalid_explicit_and_no_htop(self):
271+ def test_hide_sync_link_when_otp_is_invalid_explicit_and_no_hotp(self):
272 self.client.login(username=self.email, password=self.password)
273
274 response = self.do_twofactor(oath_token='123456')
275
276=== modified file 'src/webui/views/registration.py'
277--- src/webui/views/registration.py 2019-01-09 18:37:18 +0000
278+++ src/webui/views/registration.py 2019-10-23 14:40:43 +0000
279@@ -1,4 +1,4 @@
280-# Copyright 2010, 2012 Canonical Ltd. This software is licensed under
281+# Copyright 2010-2019 Canonical Ltd. This software is licensed under
282 # the GNU Affero General Public License version 3 (see the file
283 # LICENSE).
284
285@@ -310,6 +310,7 @@
286 return self._render_form(request, form, request.POST)
287
288 @method_decorator(dont_cache)
289+ @method_decorator(check_readonly)
290 @method_decorator(limitlogin())
291 # for this page, we need to be logged in but NOT twofactored
292 @method_decorator(login_required)