Merge branch 'feat/2fa-audit-and-owner-notifications' into feat/step-up-authentication

Author: FrankApiyo

onadata/apps/api/tasks.py

@@ -137,6 +137,12 @@ def send_account_lockout_email(email, message_txt, subject):
     send_generic_email(email, message_txt, subject)
 
 
+@app.task(base=AutoRetryTask)
+def send_two_factor_changed_email(email, message_txt, subject):
+    """Sends second-factor change notification email."""
+    send_generic_email(email, message_txt, subject)
+
+
 @app.task(base=AutoRetryTask)
 def send_account_deactivation_email(email, message_txt, subject):
     """Sends inactive-account deactivation warning email."""

onadata/apps/api/tests/viewsets/test_totp_concurrency.py

@@ -1,4 +1,3 @@
-# -*- coding: utf-8 -*-
 """Concurrency tests for one-time-code verification.
 
 Threads here stand in for the deployment's uWSGI workers (``--workers 30
@@ -11,19 +10,24 @@
 transaction is never committed, so a second connection would not see the
 fixture at all.
 """
+
 import threading
 
+from django.conf import settings
 from django.contrib.auth.models import User
 from django.db import connection
 from django.test import TransactionTestCase, override_settings
 
+import jwt
 from django_otp.oath import totp
 from django_otp.plugins.otp_static.models import StaticDevice, StaticToken
 from django_otp.plugins.otp_totp.models import TOTPDevice
+from rest_framework.test import APIRequestFactory
 
 from onadata.apps.api.viewsets.totp_viewset import (
     RECOVERY_DEVICE_NAME,
     TOTP_DEVICE_NAME,
+    TOTPViewSet,
     _verify_recovery,
     _verify_totp,
 )
@@ -134,3 +138,72 @@ def worker():
             1,
             f"the code verified {sum(results)} times; one code, one use",
         )
+
+
+@override_settings(ENABLE_TWO_FACTOR=True)
+class EnrolmentConfirmationConcurrencyTestCase(TransactionTestCase):
+    """A pending authenticator is confirmed by exactly one request.
+
+    Worse here than a double-spent code: confirmation mints the recovery set,
+    so every racing worker is handed one and each replaces the last. The user
+    is shown several sets and only the final one still works.
+    """
+
+    def setUp(self):
+        super().setUp()
+        self.user = User.objects.create_user(
+            "enrolmentrace", "enrolmentrace@example.com", "pw-9F3kz"
+        )
+        self.device = TOTPDevice.objects.create(
+            user=self.user, name=TOTP_DEVICE_NAME, confirmed=False
+        )
+
+    def test_one_pending_code_confirms_one_enrolment(self):
+        code = f"{totp(self.device.bin_key, self.device.step, self.device.t0):06d}"
+        config = settings.OPENID_CONNECT_VIEWSET_CONFIG
+        sso = jwt.encode(
+            {"email": self.user.email},
+            config["JWT_SECRET_KEY"],
+            algorithm=config["JWT_ALGORITHM"],
+        )
+        barrier = threading.Barrier(CONCURRENT)
+        responses = []
+        guard = threading.Lock()
+
+        def worker():
+            try:
+                request = APIRequestFactory().post(
+                    "/", data={"code": code}, HTTP_SSO=sso
+                )
+                barrier.wait(timeout=10)
+                response = TOTPViewSet.as_view({"post": "enroll_confirm"})(request)
+                with guard:
+                    responses.append(response)
+            finally:
+                connection.close()
+
+        threads = [threading.Thread(target=worker) for _ in range(CONCURRENT)]
+        for thread in threads:
+            thread.start()
+        for thread in threads:
+            thread.join(timeout=30)
+
+        self.assertEqual(len(responses), CONCURRENT, "a worker did not finish")
+        # The losers find no pending row when the lock releases, rather than
+        # going on to spend the code: a double-click is a 409, not a "that
+        # code is not valid" on a code that was perfectly good.
+        self.assertEqual({response.status_code for response in responses}, {200, 409})
+        won = [response for response in responses if response.status_code == 200]
+        self.assertEqual(
+            len(won), 1, f"{len(won)} racing confirmations were each told they won"
+        )
+        # The half that bites: the set the winner was shown is the set that
+        # actually survived, so the codes on the user's screen are the live ones.
+        self.assertEqual(
+            set(won[0].data["codes"]),
+            set(
+                StaticToken.objects.filter(device__user=self.user).values_list(
+                    "token", flat=True
+                )
+            ),
+        )

onadata/apps/api/tests/viewsets/test_totp_viewset.py

@@ -11,6 +11,8 @@
 
 from django.conf import settings
 from django.contrib.auth.models import User
+from django.core import mail
+from django.core.cache import cache
 from django.test import RequestFactory, override_settings
 from django.views.debug import SafeExceptionReporterFilter
 
@@ -22,10 +24,12 @@
 
 from onadata.apps.api.tests.viewsets.test_abstract_viewset import TestAbstractViewSet
 from onadata.apps.api.viewsets.totp_viewset import (
+    RECOVERY_CODE_ALPHABET,
     RECOVERY_CODE_COUNT,
     RECOVERY_DEVICE_NAME,
     TOTP_DEVICE_NAME,
     TOTPViewSet,
+    _verify_recovery,
 )
 
 #: Any test spending two codes must move between windows: django-otp records
@@ -124,9 +128,7 @@ def _enroll(self):
         Always passes the password so the helper works either way: it is
         ignored unless the deployment asks for one, and required when it does.
         """
-        started = self._post_session(
-            "enroll_start", {"password": self.login_password}
-        )
+        started = self._post_session("enroll_start", {"password": self.login_password})
         self.assertEqual(started.status_code, 201)
         device = TOTPDevice.objects.get(
             user=self.user, name=TOTP_DEVICE_NAME, confirmed=False
@@ -393,6 +395,50 @@ def test_generating_recovery_codes_returns_them_once(self):
             {"generated": True, "remaining": RECOVERY_CODE_COUNT},
         )
 
+    def test_recovery_codes_carry_at_least_64_bits_of_randomness(self):
+        """Base32 spends 5 bits a character, so 64 bits needs 13 of them.
+
+        Rate limiting is what actually makes these unguessable -- see the
+        throttling test below -- but the entropy is the half that holds if
+        the throttle is ever turned off.
+        """
+        device = self._enroll()
+        with next_totp_window():
+            response = self._post_with_api_key(
+                "recovery_generate", {"code": current_code(device.key)}
+            )
+        self.assertEqual(response.status_code, 201)
+
+        for code in response.data["codes"]:
+            self.assertGreaterEqual(len(code), 13, code)
+            self.assertTrue(set(code) <= set(RECOVERY_CODE_ALPHABET), code)
+
+    def test_wrong_recovery_codes_are_throttled(self):
+        """The entropy above is sized against a throttled attacker.
+
+        django-otp backs off by ``factor * 2 ** (failures - 1)`` seconds and
+        the factor is a setting, so a deployment can switch this off. Asserted
+        here because turning it off changes what the code length has to be.
+        """
+        self._enroll()
+        with next_totp_window():
+            self._post_with_api_key(
+                "recovery_generate",
+                {"code": current_code(default_device(self.user).key)},
+            )
+        device = StaticDevice.objects.get(user=self.user, name=RECOVERY_DEVICE_NAME)
+        self.assertTrue(device.throttling_enabled, "backoff is off; codes stand alone")
+
+        # Five misses rather than one: the delay doubles each time, so this
+        # leaves ~16 seconds rather than racing a 1-second window.
+        for miss in range(5):
+            self.assertFalse(_verify_recovery(self.user, f"not-a-code{miss}"))
+
+        # A real code, refused only because the misses started the backoff.
+        spendable = device.token_set.first().token
+        self.assertFalse(_verify_recovery(self.user, spendable))
+        self.assertTrue(device.token_set.filter(token=spendable).exists())
+
     def test_regenerating_replaces_rather_than_appends(self):
         """Spent codes left behind would make the remaining count a lie."""
         device = self._enroll()
@@ -800,9 +846,7 @@ def test_first_enrolment_refuses_without_the_account_password(self):
         )
 
     def test_first_enrolment_accepts_the_account_password(self):
-        response = self._post_session(
-            "enroll_start", {"password": self.login_password}
-        )
+        response = self._post_session("enroll_start", {"password": self.login_password})
 
         self.assertEqual(response.status_code, 201)
         self.assertIn("otpauthUri", response.data)
@@ -813,6 +857,43 @@ def test_first_enrolment_refuses_a_wrong_password(self):
         self.assertEqual(response.status_code, 403)
         self.assertFalse(TOTPDevice.objects.filter(user=self.user).exists())
 
+    def test_enrolment_password_failures_count_towards_the_login_lockout(self):
+        """Guesses here spend the same allowance as guesses at the login form.
+
+        Both doors open on one password, so a budget of its own here would be
+        a way around the lockout rather than a second lock.
+        """
+        user = User.objects.create_user(
+            "enrolmentlockout", "enrolmentlockout@example.com", "correct-password"
+        )
+        # The counter lives in the cache, which the test runner does not roll
+        # back, so it would otherwise outlast this test.
+        self.addCleanup(cache.clear)
+        view = TOTPViewSet.as_view({"post": "enroll_start"})
+
+        def start_enrolment(password):
+            return view(
+                self.factory.post("/", data={"password": password}, **self._sso(user))
+            )
+
+        # The probe at the end says nothing unless this is accepted first.
+        self.assertEqual(start_enrolment("correct-password").status_code, 201)
+        TOTPDevice.objects.filter(user=user).delete()
+
+        with patch(
+            "onadata.libs.authentication.send_account_lockout_email.apply_async"
+        ):
+            for attempt in range(settings.MAX_LOGIN_ATTEMPTS):
+                self.assertEqual(
+                    start_enrolment("wrong-password").status_code, 403, attempt
+                )
+
+            refused = start_enrolment("correct-password")
+
+        self.assertEqual(refused.status_code, 403)
+        self.assertEqual(refused.data["reason"], "locked_out")
+        self.assertFalse(TOTPDevice.objects.filter(user=user).exists())
+
     def test_an_account_with_no_password_cannot_enrol(self):
         """Having no password is not a way past the password.
 
@@ -859,9 +940,7 @@ def test_submitted_secrets_are_scrubbed_from_error_reports(self):
         with DEBUG off because the reporter's filter is inert when it is on.
         """
         secret = "sup3r-s3cret-pw"  # nosec B105 - test fixture, not a credential
-        request = RequestFactory().post(
-            "/", data={"password": secret}, **self._sso()
-        )
+        request = RequestFactory().post("/", data={"password": secret}, **self._sso())
 
         TOTPViewSet.as_view({"post": "enroll_start"})(request)
 
@@ -942,9 +1021,7 @@ def test_viewing_returns_the_codes_that_still_work(self):
             )
 
         self.assertEqual(viewed.status_code, 200)
-        self.assertEqual(
-            sorted(viewed.data["codes"]), sorted(generated.data["codes"])
-        )
+        self.assertEqual(sorted(viewed.data["codes"]), sorted(generated.data["codes"]))
         self.assertEqual(
             self._get_status().data["recoveryCodes"],
             {"generated": True, "remaining": RECOVERY_CODE_COUNT},
@@ -1000,6 +1077,50 @@ def test_viewing_a_set_that_was_never_generated_is_a_404(self):
 
         self.assertEqual(response.status_code, 404)
 
+    def test_enrolment_is_written_to_the_security_audit_log(self):
+        with self.assertLogs("audit_logger", level="DEBUG") as captured:
+            self._enroll()
+
+        actions = {
+            getattr(record, "formhub_action", None) for record in captured.records
+        }
+        self.assertIn("two-factor-enrolled", actions)
+
+    def test_failed_verification_is_written_to_the_security_audit_log(self):
+        self._enroll()
+
+        with self.assertLogs("audit_logger", level="DEBUG") as captured:
+            response = self._verify({"code": "000000", "method": "totp"})
+
+        self.assertEqual(response.status_code, 403)
+        actions = {
+            getattr(record, "formhub_action", None) for record in captured.records
+        }
+        self.assertIn("two-factor-verification-failed", actions)
+
+    def test_enrolment_notifies_the_account_owner(self):
+        mail.outbox = []
+
+        self._enroll()
+
+        self.assertTrue(
+            any(self.user.email in message.to for message in mail.outbox),
+            "enrolling a factor did not notify the account owner",
+        )
+
+    def test_disabling_two_factor_notifies_the_account_owner(self):
+        device = self._enroll()
+        mail.outbox = []
+        with next_totp_window():
+            response = self._post_with_api_key(
+                "disable", {"code": current_code(device.key)}
+            )
+
+        self.assertEqual(response.status_code, 200)
+        self.assertTrue(
+            any(self.user.email in message.to for message in mail.outbox),
+            "disabling a factor did not notify the account owner",
+        )
 
 
 @override_settings(ENABLE_TWO_FACTOR=False)