From a6579938e3cea3deb15d602c48f668b57cbd6a8e Mon Sep 17 00:00:00 2001 From: Sidney Richards Date: Wed, 16 Oct 2024 16:59:31 +0200 Subject: [PATCH] [#2816] Split user/password auth from token verification Previously, the UserModelEmailBackend was overloaded to support both username/password authentication, as well as TOTP validation. It did this, in part, by verifying the 2FA flag on the site configuration. This was both confusing (an authentication backend should do one thing only, multiple logics means multiple backends), as well as mixing concerns (the _view_ should decide which arguments to pass to to authentication backend based on the site configuration, authentication backends should only authenticate). This commit separates both concerns into independent backends, and adds some tests to ensure that they are properly invoked. --- src/open_inwoner/accounts/backends.py | 21 ++-- src/open_inwoner/accounts/tests/test_auth.py | 36 ++++++- .../accounts/tests/test_backends.py | 97 +++++++++++++++++++ src/open_inwoner/conf/base.py | 1 + 4 files changed, 147 insertions(+), 8 deletions(-) diff --git a/src/open_inwoner/accounts/backends.py b/src/open_inwoner/accounts/backends.py index bf55a60c82..e60b115f57 100644 --- a/src/open_inwoner/accounts/backends.py +++ b/src/open_inwoner/accounts/backends.py @@ -2,7 +2,7 @@ from django.conf import settings from django.contrib.auth import get_user_model -from django.contrib.auth.backends import ModelBackend +from django.contrib.auth.backends import BaseBackend, ModelBackend from django.contrib.auth.hashers import check_password from django.urls import reverse, reverse_lazy @@ -21,14 +21,14 @@ class UserModelEmailBackend(ModelBackend): """ Authentication backend for login with email address. + + Based on the default ModelBackend, but with support for case insensitive + email lookups, scoped to the correct login type. """ - def authenticate( - self, request, username=None, password=None, user=None, token=None, **kwargs - ): - config = SiteConfiguration.get_solo() + def authenticate(self, request, username=None, password=None): User = get_user_model() - if username and password and not config.login_2fa_sms: + if username and password: try: user = User.objects.get( email__iexact=username, @@ -51,8 +51,15 @@ def authenticate( User().set_password(password) return None + +class Verify2FATokenBackend(BaseBackend): + """ + Verify a TOTP token for a user. + """ + + def authenticate(self, request, *, user=None, token=None): # 2FA with sms verification - if config.login_2fa_sms and user and token: + if user and token: accepted, drift = accept_totp( key=user.seed, response=token, diff --git a/src/open_inwoner/accounts/tests/test_auth.py b/src/open_inwoner/accounts/tests/test_auth.py index c1ce22d7bb..b97a7f7d01 100644 --- a/src/open_inwoner/accounts/tests/test_auth.py +++ b/src/open_inwoner/accounts/tests/test_auth.py @@ -1,15 +1,17 @@ -from unittest.mock import patch +from unittest.mock import ANY, patch from urllib.parse import urlencode from django.contrib.auth.signals import user_logged_in from django.contrib.sites.models import Site from django.core import mail +from django.core.signing import TimestampSigner from django.test import RequestFactory, TestCase, override_settings from django.urls import reverse, reverse_lazy from django.utils.translation import gettext as _ import requests_mock from django_webtest import WebTest +from freezegun import freeze_time from furl import furl from pyquery import PyQuery as PQ @@ -1715,6 +1717,38 @@ def test_login(self): # Verify that the user has been authenticated self.assertIn("_auth_user_id", self.app.session) + @patch("open_inwoner.accounts.gateways.gateway.send") + @freeze_time("2023-05-22 12:05:01") + def test_regular_login_with_valid_credentials_triggers_the_2fa_flow( + self, mock_gateway_send + ): + config = SiteConfiguration.get_solo() + config.login_2fa_sms = True + config.save() + + response = self.app.get(reverse("login")) + mock_gateway_send.assert_not_called() + + login_form = response.forms["login-form"] + login_form["username"] = self.user.email + login_form["password"] = "test" + login_form_response = login_form.submit() + login_form_response.follow() + + mock_gateway_send.assert_called_once_with(to=self.user.phonenumber, token=ANY) + + params = { + "next": "", + "user": TimestampSigner().sign(self.user.pk), + } + verify_token_url = furl(reverse("verify_token")).add(params).url + self.assertRedirects(login_form_response, verify_token_url) + self.assertNotIn( + "_auth_user_id", + self.app.session, + msg="Valid credentials alone should not authenticate a user, second factor required", + ) + def test_login_page_shows_correct_digid_login_url(self): config = OpenIDConnectDigiDConfig.get_solo() diff --git a/src/open_inwoner/accounts/tests/test_backends.py b/src/open_inwoner/accounts/tests/test_backends.py index 595702ef9d..c4e020c2a0 100644 --- a/src/open_inwoner/accounts/tests/test_backends.py +++ b/src/open_inwoner/accounts/tests/test_backends.py @@ -1,9 +1,15 @@ +import datetime from unittest.mock import patch +from django.conf import settings from django.contrib import auth from django.test import RequestFactory, TestCase, override_settings from django.urls import reverse +from freezegun import freeze_time +from oath import totp + +from open_inwoner.accounts.choices import LoginTypeChoices from open_inwoner.accounts.tests.factories import UserFactory @@ -69,3 +75,94 @@ def test_admin_oidc_use_correct_backend( result = auth.authenticate(request) self.assertEqual(result, self.user) + + +@override_settings( + AUTHENTICATION_BACKENDS=[ + "open_inwoner.accounts.backends.UserModelEmailBackend", + ] +) +class UserModelEmailBackendTestCase(TestCase): + @classmethod + def setUpTestData(cls): + super().setUpTestData() + + cls.user = UserFactory( + login_type=LoginTypeChoices.default, password="keepitsecret" + ) + + def test_correct_username_password_return_user(self): + request = RequestFactory().post(reverse("login")) + + result = auth.authenticate( + request, username=self.user.email, password="keepitsecret" + ) + self.assertEqual(result, self.user) + + def test_incorrect_username_password_return_none(self): + request = RequestFactory().post(reverse("login")) + + result = auth.authenticate( + request, username=self.user.email, password="incorrect" + ) + self.assertEqual(result, None) + + def test_missing_username_and_or_password_returns_none(self): + for username in (self.user.email, "", None): + for password in ("keepitsecret", "", None): + if username and password: + # This is the successful case, exclude it, but ensure we + # also have permutations with one valid/one invalid value. + continue + + with self.subTest(f"{username=} {password=}"): + request = RequestFactory().post(reverse("login")) + result = auth.authenticate( + request, username=username, password=password + ) + self.assertEqual(result, None) + + +@override_settings( + AUTHENTICATION_BACKENDS=[ + "open_inwoner.accounts.backends.Verify2FATokenBackend", + ] +) +class Verify2FATokenBackendTestCase(TestCase): + @classmethod + def setUpTestData(cls): + super().setUpTestData() + + cls.user = UserFactory(login_type=LoginTypeChoices.default) + cls.expires_in = getattr(settings, "ACCOUNTS_USER_TOKEN_EXPIRE_TIME", 300) + cls.make_token = lambda: totp(cls.user.seed, period=cls.expires_in) + + @freeze_time("2023-05-22 12:05:01") + def test_valid_token_and_user_returns_user(self): + request = RequestFactory().get(reverse("verify_token")) + + result = auth.authenticate(request, user=self.user, token=self.make_token()) + self.assertEqual(result, self.user) + + def test_expired_token_and_valid_user_returns_none(self): + request = RequestFactory().get(reverse("verify_token")) + + with freeze_time("2023-05-22 12:05:01") as ft: + token = self.make_token() + + ft.tick(delta=datetime.timedelta(seconds=self.expires_in * 2)) + result = auth.authenticate(request, user=self.user, token=token) + self.assertEqual(result, None) + + def test_missing_user_and_or_token_returns_none(self): + for user in (self.user.email, "", None): + for token in (self.make_token(), "", None): + if user and token: + # This is the successful case, exclude it, but ensure we + # also have permutations with one valid/one invalid value. + continue + + with self.subTest(f"{user=} {token=}"): + request = RequestFactory().get(reverse("verify_token")) + result = auth.authenticate(request, user=user, token=token) + self.assertEqual(result, None) diff --git a/src/open_inwoner/conf/base.py b/src/open_inwoner/conf/base.py index 50c455841c..178a279219 100644 --- a/src/open_inwoner/conf/base.py +++ b/src/open_inwoner/conf/base.py @@ -496,6 +496,7 @@ AUTHENTICATION_BACKENDS = [ "open_inwoner.accounts.backends.CustomAxesBackend", "open_inwoner.accounts.backends.UserModelEmailBackend", + "open_inwoner.accounts.backends.Verify2FATokenBackend", "digid_eherkenning.backends.DigiDBackend", "eherkenning.backends.eHerkenningBackend", "digid_eherkenning_oidc_generics.backends.OIDCAuthenticationDigiDBackend",