From 0b5eb990ed584fab3c2db36ee7b199d5602d51b9 Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 25 Oct 2026 23:20:28 +0300 Subject: [PATCH] fix(admin): a forced password change cannot trap a user or be skipped with a token - The administrator's password form took "password-based authentication: Disabled" with "must change it at the next login": the change form asks for the old password, which the user then never had, so they could not get past it. That combination is refused, and an administrator cannot disable their own password. - A user who must change the password could still use the API with their tokens. A token is now refused until they change it, as the pages are. --- accounts/auth.py | 7 +++++++ accounts/tests/test_api_tokens.py | 9 ++++++++- admin/forms.py | 7 +++++++ admin/tests/test_user_pages.py | 16 ++++++++++++++++ admin/views.py | 6 +++++- 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/accounts/auth.py b/accounts/auth.py index 0e17d23c..e0bf08d3 100644 --- a/accounts/auth.py +++ b/accounts/auth.py @@ -3,8 +3,12 @@ from datetime import timedelta from django.utils import timezone +from django.utils.translation import gettext_lazy as _ from drf_spectacular.contrib.knox_auth_token import KnoxTokenScheme from knox.auth import TokenAuthentication as KnoxTokenAuthentication +from rest_framework import exceptions + +from accounts.models import UserAttributes # a token's last use is written at most once a minute, not on every request LAST_USED_EVERY = timedelta(minutes=1) @@ -13,6 +17,9 @@ LAST_USED_EVERY = timedelta(minutes=1) class TokenAuthentication(KnoxTokenAuthentication): def validate_user(self, auth_token): user, token = super().validate_user(auth_token) + # as the pages do (ForcePasswordChangeMiddleware), until the user changes the password + if UserAttributes.objects.filter(user=user, must_change_password=True).exists(): + raise exceptions.AuthenticationFailed(_("Change your password before using the API.")) now = timezone.now() if token.last_used is None or now - token.last_used >= LAST_USED_EVERY: type(token).objects.filter(pk=token.pk).update(last_used=now) diff --git a/accounts/tests/test_api_tokens.py b/accounts/tests/test_api_tokens.py index 8104ae24..0c6140d8 100644 --- a/accounts/tests/test_api_tokens.py +++ b/accounts/tests/test_api_tokens.py @@ -5,7 +5,7 @@ import re from datetime import timedelta from unittest.mock import patch -from accounts.models import ApiToken +from accounts.models import ApiToken, UserAttributes from django.contrib.auth import get_user_model from django.db import connection from django.test import TestCase @@ -116,6 +116,13 @@ class ApiTokenTests(TestCase): self.client.force_login(self.user) self.assertIn("Last used", self.client.get(reverse("accounts:profile")).content.decode()) + def test_a_token_stops_while_its_user_must_change_the_password(self): + token = self.make() + UserAttributes.objects.update_or_create(user=self.user, defaults={"must_change_password": True}) + self.assertEqual(self.api(token).status_code, 401) + UserAttributes.objects.filter(user=self.user).update(must_change_password=False) + self.assertEqual(self.api(token).status_code, 200) + def test_a_user_has_at_most_ten_tokens(self): for number in range(10): self.make(name=f"t{number}") diff --git a/admin/forms.py b/admin/forms.py index 8784fdc0..0f9f357e 100644 --- a/admin/forms.py +++ b/admin/forms.py @@ -133,3 +133,10 @@ class AdminPasswordForm(AdminPasswordChangeForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields["must_change_password"] = self.fields.pop("must_change_password") # last, after Django's own + + def clean(self): + cleaned = super().clean() + # the change form asks for the old password: a user without one could never pass it + if cleaned.get("must_change_password") and not cleaned.get("set_usable_password", True): + raise ValidationError(_("A user without a password cannot be made to change it")) + return cleaned diff --git a/admin/tests/test_user_pages.py b/admin/tests/test_user_pages.py index 3220c2f5..a4b3b62f 100644 --- a/admin/tests/test_user_pages.py +++ b/admin/tests/test_user_pages.py @@ -79,3 +79,19 @@ class UserPagesTests(TestCase): self.client.force_login(self.admin) self.client.post(url, {"password1": "New-pw-4567y", "password2": "New-pw-4567y"}) self.assertFalse(UserAttributes.objects.get(user=self.alice).must_change_password) + + def test_a_user_without_a_password_cannot_be_made_to_change_it(self): + # the change form asks for the old password: one they never had + url = reverse("admin:user_update_password", args=[self.alice.pk]) + response = self.client.post(url, {"usable_password": "false", "must_change_password": "on"}) + self.assertContains(response, "A user without a password cannot be made to change it") + self.alice.refresh_from_db() + self.assertTrue(self.alice.has_usable_password()) + + def test_an_administrator_keeps_their_own_password(self): + url = reverse("admin:user_update_password", args=[self.admin.pk]) + response = self.client.post(url, {"usable_password": "false"}) + self.assertContains(response, "You cannot block, delete or demote your own account") + self.admin.refresh_from_db() + self.assertTrue(self.admin.has_usable_password()) + diff --git a/admin/views.py b/admin/views.py index 76695c6a..40f02d95 100644 --- a/admin/views.py +++ b/admin/views.py @@ -187,7 +187,11 @@ def user_update_password(request, pk): user = get_object_or_404(User, pk=pk) if request.method == "POST": form = forms.AdminPasswordForm(user, request.POST) - if form.is_valid(): + valid = form.is_valid() + if valid and user == request.user and not form.cleaned_data["set_usable_password"]: + form.add_error(None, OWN_ACCOUNT) + valid = False + if valid: with audit(request, None, f"User {user.username}", "set password"), transaction.atomic(): user = form.save() UserAttributes.objects.update_or_create(