Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#2816] Split user/password auth from token verification #1449

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions src/open_inwoner/accounts/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
swrichards marked this conversation as resolved.
Show resolved Hide resolved
user = User.objects.get(
email__iexact=username,
Expand All @@ -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:
swrichards marked this conversation as resolved.
Show resolved Hide resolved
accepted, drift = accept_totp(
key=user.seed,
response=token,
Expand Down
36 changes: 35 additions & 1 deletion src/open_inwoner/accounts/tests/test_auth.py
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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()

Expand Down
112 changes: 111 additions & 1 deletion src/open_inwoner/accounts/tests/test_backends.py
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -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"
swrichards marked this conversation as resolved.
Show resolved Hide resolved
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cls.make_token = lambda: totp(cls.user.seed, period=cls.expires_in)
cls.token = lambda: totp(cls.user.seed, period=cls.expires_in)

The value for this will be the same for all tests, so it's a bit misleading to call it make_token (which to me suggests that a new token is made every time this is called)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The totp function does use the current timestamp, and not all tests freeze the time (or in the same way), so I thought it'd still be easier to reason about if it's a callable. But I will add a freeze_time to the whole class, because time-based tests should be deterministic.


@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)
3 changes: 1 addition & 2 deletions src/open_inwoner/conf/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/open_inwoner/conf/ci.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading