From 9b0e8c1e83db491c5ae16492ade226d1f9aacfa5 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 | 22 ++-- src/open_inwoner/accounts/tests/test_auth.py | 36 +++++- .../accounts/tests/test_backends.py | 112 +++++++++++++++++- src/open_inwoner/conf/base.py | 3 +- src/open_inwoner/conf/ci.py | 1 + 5 files changed, 162 insertions(+), 12 deletions(-) diff --git a/src/open_inwoner/accounts/backends.py b/src/open_inwoner/accounts/backends.py index c1bb8e2fc6..8ca9c98a36 100644 --- a/src/open_inwoner/accounts/backends.py +++ b/src/open_inwoner/accounts/backends.py @@ -3,12 +3,11 @@ 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 from axes.backends import AxesBackend -from digid_eherkenning.oidc.backends import BaseBackend from mozilla_django_oidc_db.backends import OIDCAuthenticationBackend from mozilla_django_oidc_db.config import dynamic_setting from oath import accept_totp @@ -25,14 +24,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, @@ -55,8 +54,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 50a2acd422..1f4bd81da7 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 @@ -1709,6 +1711,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 = OpenIDDigiDConfig.get_solo() diff --git a/src/open_inwoner/accounts/tests/test_backends.py b/src/open_inwoner/accounts/tests/test_backends.py index e72fab9660..7881c2af8c 100644 --- a/src/open_inwoner/accounts/tests/test_backends.py +++ b/src/open_inwoner/accounts/tests/test_backends.py @@ -1,13 +1,18 @@ +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 furl import furl from mozilla_django_oidc_db.config import store_config +from oath import totp -from .factories import UserFactory +from open_inwoner.accounts.choices import LoginTypeChoices +from open_inwoner.accounts.tests.factories import UserFactory class OIDCBackendTestCase(TestCase): @@ -90,3 +95,108 @@ def test_admin_oidc_selects_correct_backend( self.assertEqual( result.backend, "open_inwoner.accounts.backends.CustomOIDCBackend" ) + + +@override_settings( + AUTHENTICATION_BACKENDS=[ + "open_inwoner.accounts.backends.UserModelEmailBackend", + ] +) +class UserModelEmailBackendTestCase(TestCase): + @classmethod + def setUpTestData(cls): + super().setUpTestData() + + cls.password = "keepitsecert" + cls.user = UserFactory( + login_type=LoginTypeChoices.default, password=cls.password + ) + for login_type in LoginTypeChoices: + UserFactory(login_type=login_type) + + def test_duplicate_emails_on_case_results_in_no_match(self): + request = RequestFactory().post(reverse("login")) + + UserFactory(email=self.user.email.upper(), login_type=self.user.login_type) + result = auth.authenticate( + request, username=self.user.email, password=self.password + ) + self.assertEqual(result, None) + + def test_correct_username_password_return_user(self): + request = RequestFactory().post(reverse("login")) + + result = auth.authenticate( + request, username=self.user.email, password=self.password + ) + self.assertEqual(result, self.user) + + def test_incorrect_username_password_return_none(self): + request = RequestFactory().post(reverse("login")) + + for username, password in ( + (self.user.email, "incorrect"), + ("incorrect", self.password), + ): + result = auth.authenticate(request, username=username, password=password) + self.assertEqual(result, None) + + def test_missing_username_and_or_password_returns_none(self): + for username in (self.user.email, "", None): + for password in (self.password, "", 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 21cf5f9513..1743c2db52 100644 --- a/src/open_inwoner/conf/base.py +++ b/src/open_inwoner/conf/base.py @@ -492,11 +492,10 @@ ] -# Allow logging in with email+password AUTHENTICATION_BACKENDS = [ "open_inwoner.accounts.backends.CustomAxesBackend", "open_inwoner.accounts.backends.UserModelEmailBackend", - "django.contrib.auth.backends.ModelBackend", + "open_inwoner.accounts.backends.Verify2FATokenBackend", "digid_eherkenning.backends.DigiDBackend", "eherkenning.backends.eHerkenningBackend", "open_inwoner.accounts.backends.DigiDEHerkenningOIDCBackend", diff --git a/src/open_inwoner/conf/ci.py b/src/open_inwoner/conf/ci.py index 13254f7c65..fd3a61312e 100644 --- a/src/open_inwoner/conf/ci.py +++ b/src/open_inwoner/conf/ci.py @@ -32,6 +32,7 @@ AUTHENTICATION_BACKENDS = [ "open_inwoner.accounts.backends.CustomAxesBackend", "open_inwoner.accounts.backends.UserModelEmailBackend", + "open_inwoner.accounts.backends.Verify2FATokenBackend", "django.contrib.auth.backends.ModelBackend", # mock login like dev.py "digid_eherkenning.mock.backends.DigiDBackend",