When validating TOTP in TOTPDeviceForm, allow a drift in clocks of +/-1 instead of just -1 (#458)

When validating the TOTP in the TOTPDeviceForm, loop over
offsets of [-tolerance, tolerance] inclusive instead of
[-tolerance, tolerance).
Also added a unit test that checks offets of -1, 0 and 1
are valid and -2 and +2 are not.

Author: mbakereth

CHANGELOG.md

@@ -7,6 +7,7 @@
 ### Changed
 - Suppressed default_app_config warning on Django 3.2+
 - qrcode dependency limit upped to 7.99 and django-phonenumber-field to 7
+- When validating a TOTP after scanning the QR code, allow a time drift of +/-1 instead of just -1
 
 ## 1.13.1
 

tests/test_totpdeviceform.py

@@ -0,0 +1,67 @@
+from binascii import unhexlify
+from unittest.mock import patch
+
+import django_otp.oath
+from django.test import TestCase
+
+from two_factor.forms import TOTPDeviceForm
+
+# Use this as the Unix time for all TOTPs.  It is chosen arbitrarily
+# as 3 Jan 2022
+TEST_TIME = 1641194517
+
+
+@patch('django_otp.oath.time', return_value=TEST_TIME)
+class TOTPDeviceFormTest(TestCase):
+    """
+    This class tests how the TOTPDeviceForm validator handles drift between its clock
+    and the TOTP device.
+
+    If there is a drift in the range [-tolerance, +tolerance], (tolerance is hardcoded
+    to 1), then the TOTP should be accepted.  Outside this range, it should be rejected.
+    """
+
+    key = '12345678901234567890'
+
+    def setUp(self):
+        super().setUp()
+        self.bin_key = unhexlify(TOTPDeviceFormTest.key.encode())
+        self.empty_form = TOTPDeviceForm(TOTPDeviceFormTest.key, None)
+
+    def totp_with_offset(self, offset):
+        return django_otp.oath.totp(self.bin_key, self.empty_form.step,
+            self.empty_form.t0, self.empty_form.digits, self.empty_form.drift + offset)
+
+    def test_offset_0(self, mock_test):
+        device_totp = self.totp_with_offset(0)
+        form = TOTPDeviceForm(TOTPDeviceFormTest.key, None,
+            data={'token': device_totp})
+        self.assertTrue(form.is_valid())
+
+    def test_offset_minus1(self, mock_test):
+        device_totp = self.totp_with_offset(-1)
+        form = TOTPDeviceForm(TOTPDeviceFormTest.key, None,
+            data={'token': device_totp})
+        self.assertTrue(form.is_valid())
+
+    def test_offset_plus1(self, mock_test):
+        device_totp = self.totp_with_offset(1)
+        form = TOTPDeviceForm(TOTPDeviceFormTest.key, None,
+            data={'token': device_totp})
+        self.assertTrue(form.is_valid())
+
+    def test_offset_minus2(self, mock_test):
+        device_totp = self.totp_with_offset(-2)
+        form = TOTPDeviceForm(TOTPDeviceFormTest.key, None,
+            data={'token': device_totp})
+        self.assertFalse(form.is_valid())
+        self.assertEqual(form.errors['token'][0],
+            TOTPDeviceForm.error_messages['invalid_token'])
+
+    def test_offset_plus2(self, mock_test):
+        device_totp = self.totp_with_offset(2)
+        form = TOTPDeviceForm(TOTPDeviceFormTest.key, None,
+            data={'token': device_totp})
+        self.assertFalse(form.is_valid())
+        self.assertEqual(form.errors['token'][0],
+            TOTPDeviceForm.error_messages['invalid_token'])

two_factor/forms.py

@@ -125,7 +125,7 @@ def clean_token(self):
         if 'valid_t0' in self.metadata:
             t0s.append(int(time()) - self.metadata['valid_t0'])
         for t0 in t0s:
-            for offset in range(-self.tolerance, self.tolerance):
+            for offset in range(-self.tolerance, self.tolerance + 1):
                 if totp(key, self.step, t0, self.digits, self.drift + offset) == token:
                     self.drift = offset
                     self.metadata['valid_t0'] = int(time()) - t0