From 405e6f1445c23fd48628a794ae06bd44f34e04b3 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 5 Oct 2026 18:39:33 +0300 Subject: [PATCH] fix(admin): a new user's password reaches the database only hashed The admin page that creates a user saved the form first, which wrote the user with the raw password in the password column, and hashed it with a second save afterwards. The raw password was in the database, and in its journal and any copy taken meanwhile, and stayed there if anything failed between the two writes. - UserCreateForm hashes the password before the first save; groups and permissions are saved as before. - The form applies AUTH_PASSWORD_VALIDATORS, after the user fields are set (so the similarity check sees the new username), as the password change forms already did. The settings template configures no validators, so nothing changes unless an installation sets them. The tests record the SQL parameters of the request: the previous code wrote the raw password in the INSERT; it also accepted passwords that configured validators reject. --- admin/forms.py | 23 ++++++++++++++ admin/test_user_create.py | 65 +++++++++++++++++++++++++++++++++++++++ admin/views.py | 3 -- 3 files changed, 88 insertions(+), 3 deletions(-) create mode 100644 admin/test_user_create.py diff --git a/admin/forms.py b/admin/forms.py index 6d44b4c0..4ebf689c 100644 --- a/admin/forms.py +++ b/admin/forms.py @@ -1,6 +1,8 @@ from django import forms +from django.contrib.auth import password_validation from django.contrib.auth.forms import ReadOnlyPasswordHashField from django.contrib.auth.models import Group, User +from django.core.exceptions import ValidationError from django.urls import reverse_lazy from django.utils.text import format_lazy from django.utils.translation import gettext_lazy as _ @@ -102,6 +104,27 @@ class UserCreateForm(UserForm): content_type__app_label="accounts", codename="change_password" ) + def _post_clean(self): + # The validators in AUTH_PASSWORD_VALIDATORS, as on the password change + # forms; after the instance has the form's values (username, email). + super()._post_clean() + password = self.cleaned_data.get("password") + if password: + try: + password_validation.validate_password(password, self.instance) + except ValidationError as error: + self.add_error("password", error) + + def save(self, commit=True): + # hashed before the user is first written: the raw password never + # reaches the database + user = super().save(commit=False) + user.set_password(self.cleaned_data["password"]) + if commit: + user.save() + self.save_m2m() + return user + class Meta: model = User fields = [ diff --git a/admin/test_user_create.py b/admin/test_user_create.py new file mode 100644 index 00000000..cdb0b5c7 --- /dev/null +++ b/admin/test_user_create.py @@ -0,0 +1,65 @@ +"""Creating a user in the admin pages: the password reaches the database only +hashed, and the configured password validators apply as on the change forms.""" + +from django.contrib.auth.models import Group, Permission, User +from django.db import connection +from django.test import TestCase, override_settings +from django.urls import reverse + +RAW = "Raw-Passw0rd-in-the-form" +LIMITS = {"max_instances": 1, "max_cpus": 1, "max_memory": 1024, "max_disk_size": 4} + + +class UserCreateTests(TestCase): + def setUp(self): + self.client.force_login(User.objects.create_superuser("uc-admin", "uc@example.com", "pw")) + self.change_password = Permission.objects.get(content_type__app_label="accounts", codename="change_password") + + def create(self, **data): + return self.client.post(reverse("admin:user_create"), {"username": "newbie", "password": RAW, **LIMITS, **data}) + + def test_the_raw_password_is_never_written(self): + written = [] + + def record(execute, sql, params, many, context): + if RAW in repr(params): + written.append(sql) + return execute(sql, params, many, context) + + with connection.execute_wrapper(record): + response = self.create() + self.assertRedirects(response, reverse("admin:user_list")) + self.assertEqual(written, []) + self.assertTrue(User.objects.get(username="newbie").check_password(RAW)) + + def test_groups_and_permissions_are_saved(self): + group = Group.objects.create(name="uc-group") + self.create(groups=[group.pk], user_permissions=[self.change_password.pk]) + user = User.objects.get(username="newbie") + self.assertEqual(list(user.groups.all()), [group]) + self.assertEqual(list(user.user_permissions.all()), [self.change_password]) + + def test_an_unticked_permission_stays_off(self): + self.create() + self.assertFalse(User.objects.get(username="newbie").user_permissions.exists()) + + @override_settings( + AUTH_PASSWORD_VALIDATORS=[ + {"NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", "OPTIONS": {"min_length": 30}} + ] + ) + def test_configured_password_validators_apply(self): + response = self.create() + self.assertEqual(response.status_code, 200) + self.assertContains(response, "at least 30 characters") + self.assertFalse(User.objects.filter(username="newbie").exists()) + + @override_settings( + AUTH_PASSWORD_VALIDATORS=[{"NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator"}] + ) + def test_the_password_is_checked_against_the_new_username(self): + response = self.client.post( + reverse("admin:user_create"), {"username": "rumpelstiltskin", "password": "rumpelstiltskin1", **LIMITS} + ) + self.assertEqual(response.status_code, 200) + self.assertContains(response, "too similar to the username") diff --git a/admin/views.py b/admin/views.py index 03d7433f..11bb27be 100644 --- a/admin/views.py +++ b/admin/views.py @@ -95,9 +95,6 @@ def user_create(request): attributes_form = forms.UserAttributesForm(request.POST or None) if user_form.is_valid() and attributes_form.is_valid(): user = user_form.save() - password = user_form.cleaned_data["password"] - user.set_password(password) - user.save() attributes = attributes_form.save(commit=False) attributes.user = user attributes.save()