Merge ~roadmr/canonical-identity-provider:2fa-update-last-nag-paper-only into canonical-identity-provider:master

[Daniel Manrique]

Update last_nag only for codes from paper device.

This is described in the spec as the desired behavior, but was incorrectly implemented, updating last_nag for *any* valid 2fa authentication.

With this we'll keep nagging the user on every 2fa request until they do effectively enter a code from a backup device, which resets the counter to 6 weeks from now for the next nag.

How to QA:

- set the account's last_nag to either None or a date 8 weeks in the past.
- Ensure account has a real device and no paper devices.
- Log in, there should be no nag on the 2fa screen, enter a real code.
- (Maybe ensure the account last_nag was not updated at this point)
- Add a paper device
- Log out
- Log in
- a nag should appear in the 2fa screen
- Enter a code from the real device
- Log out
- Log in
- The nag should again appear (since user has never used their backup device as far as we know)
- Enter code from backup device
- log out
- log in
- nag is now gone
- account last_nag should now be 6 weeks in the future for the next nag.

[Maximiliano Bertacchini (maxiberta)]

LGTM, thanks!

review: Approve

diff --git a/src/identityprovider/models/twofactor.py b/src/identityprovider/models/twofactor.py
index a89bf07..c93f23e 100644
--- a/src/identityprovider/models/twofactor.py
+++ b/src/identityprovider/models/twofactor.py
@@ -39,6 +39,11 @@ def authenticate(account, otp):
         if settings.READ_ONLY_MODE and not device.is_totp():
             continue
         if device.authenticate(otp):
+            # Update account's last_nag only if used device
+            # was paper-based
+            if device.device_type == 'paper':
+                account.backup_device_last_nag = now()
+                account.save()
             return True
     # avoid circular import (identityprovider.login import
     # identityprovider.models which import from .twofactor import *
diff --git a/src/webui/tests/test_views_ui.py b/src/webui/tests/test_views_ui.py
index 93d4e43..a9a5742 100644
--- a/src/webui/tests/test_views_ui.py
+++ b/src/webui/tests/test_views_ui.py
@@ -2181,6 +2181,51 @@ class TwoFactorNagTestCase(BaseTwoFactorLoginTestCase):
         device.refresh_from_db()
         self.assertEqual(device.last_use, frozen)
 
+    def test_check_view_success_non_paper_does_not_update_last_nag(self):
+        # Test name is incomplete! Successful 2fa on a non-paper device
+        # does NOT update last_nag but does update the device's last use date.
+        #
+        # If the user has been nagged before and needs to be nagged again
+        # (default)
+        last_nagged = self.account.backup_device_last_nag
+        # And they have one paper and one real device
+        test_key = 'A' * 36
+        real_device = self.factory.make_device(
+            account=self.account,
+            device_type='google', name='Googlerino', key=test_key)
+        self.make_paper_backup_device()
+        # when they log in
+        self.client.login(username=self.email, password=self.password)
+
+        # mock accept_hotp with a sneaky method that only accepts the
+        # device we're interested in testing for last_use updates.
+        def mock_accept_hotp(*args, **kwargs):
+            if args[0] == test_key:
+                return (True, 1)
+            else:
+                return (False, 0)
+
+        self.patch('identityprovider.models.twofactor.accept_hotp',
+                   side_effect=mock_accept_hotp)
+        frozen = now()
+        self.patch('identityprovider.models.twofactor.now',
+                   return_value=frozen)
+        # and post to the 2fa form with the code from the device under test
+        # (see mockery)
+        self.do_twofactor(next_url=reverse('account-edit'),
+                          oath_token='123456')
+        # Counter was increased on real device, meaning auth with it was
+        # successful
+        real_device.refresh_from_db()
+        self.assertEqual(1, real_device.counter)
+        # last_nag is not updated because the device used to log in was not
+        # paper-based.
+        self.account.refresh_from_db()
+        self.assertEqual(self.account.backup_device_last_nag, last_nagged)
+        # (last_use on success is thoroughly tested in
+        # test_check_view_success_updates_actual_used_device_from_many
+        # so we don't retest that here
+
     def test_check_view_fail_doesnt_update_last_nag_or_device_last_use(self):
         # If the user has been nagged before and needs to be nagged again
         # (default - does not affect this test)
@@ -2318,7 +2363,9 @@ class TwoFactorNagTestCase(BaseTwoFactorLoginTestCase):
         # And they have a few candidate devices (one paper, one real, and the
         # one we're going to use to test against)
         other_paper_device = self.make_paper_backup_device()
-        real_device = self.factory.make_device()
+        real_device = self.factory.make_device(
+            account=self.account,
+            device_type='google', name='Googlerino')
         test_key = 'A' * 36
         device = self.make_paper_backup_device(key=test_key)
         # when they log in
diff --git a/src/webui/views/ui.py b/src/webui/views/ui.py
index 7ad463d..50ae47f 100644
--- a/src/webui/views/ui.py
+++ b/src/webui/views/ui.py
@@ -358,8 +358,6 @@ class TwoFactorView(LoginBaseView):
         try:
             twofactor.authenticate(account, form.cleaned_data['oath_token'])
             twofactor.login(request)
-            account.backup_device_last_nag = now()
-            account.save()
             stats.increment('flows.login', key='success', rpconfig=rpconfig)
             stats.increment('flows.2fa', key='success', rpconfig=rpconfig)
         except AuthenticationError as e: