From 0ebed5fa95f53b87383901bbd9341ef3c974344f Mon Sep 17 00:00:00 2001 From: Natalia <124304+nessita@users.noreply.github.com> Date: Thu, 15 Aug 2024 10:27:24 -0300 Subject: [PATCH] Fixed #35678 -- Removed "usable_password" field from BaseUserCreationForm. Refs #34429: Following the implementation allowing the setting of unusable passwords via the admin site, the `BaseUserCreationForm` and `UserCreationForm` were extended to include a new field for choosing whether password-based authentication for the new user should be enabled or disabled at creation time. Given that these forms are designed to be extended when implementing custom user models, this branch ensures that this new field is moved to a new, admin-dedicated, user creation form `AdminUserCreationForm`. Regression in e626716c28b6286f8cf0f8174077f3d2244f3eb3. Thanks Simon Willison for the report, Fabian Braun and Sarah Boyce for the review. --- django/contrib/auth/admin.py | 4 +- django/contrib/auth/forms.py | 100 +++++++++++++++++++----------- docs/releases/5.1.1.txt | 6 ++ docs/releases/5.1.txt | 10 +-- docs/topics/auth/default.txt | 32 ++++++---- tests/auth_tests/test_forms.py | 110 +++++++++++++++++++++++---------- 6 files changed, 174 insertions(+), 88 deletions(-) diff --git a/django/contrib/auth/admin.py b/django/contrib/auth/admin.py index 8e1d63ef071..e977d3ded54 100644 --- a/django/contrib/auth/admin.py +++ b/django/contrib/auth/admin.py @@ -5,8 +5,8 @@ from django.contrib.admin.utils import unquote from django.contrib.auth import update_session_auth_hash from django.contrib.auth.forms import ( AdminPasswordChangeForm, + AdminUserCreationForm, UserChangeForm, - UserCreationForm, ) from django.contrib.auth.models import Group, User from django.core.exceptions import PermissionDenied @@ -71,7 +71,7 @@ class UserAdmin(admin.ModelAdmin): ), ) form = UserChangeForm - add_form = UserCreationForm + add_form = AdminUserCreationForm change_password_form = AdminPasswordChangeForm list_display = ("username", "email", "first_name", "last_name", "is_staff") list_filter = ("is_staff", "is_superuser", "is_active", "groups") diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py index 31e96ff91ce..a11668944a8 100644 --- a/django/contrib/auth/forms.py +++ b/django/contrib/auth/forms.py @@ -96,18 +96,11 @@ class UsernameField(forms.CharField): class SetPasswordMixin: """ Form mixin that validates and sets a password for a user. - - This mixin also support setting an unusable password for a user. """ error_messages = { "password_mismatch": _("The two password fields didn’t match."), } - usable_password_help_text = _( - "Whether the user will be able to authenticate using a password or not. " - "If disabled, they may still be able to authenticate using other backends, " - "such as Single Sign-On or LDAP." - ) @staticmethod def create_password_fields(label1=_("Password"), label2=_("Password confirmation")): @@ -127,33 +120,14 @@ class SetPasswordMixin: ) return password1, password2 - @staticmethod - def create_usable_password_field(help_text=usable_password_help_text): - return forms.ChoiceField( - label=_("Password-based authentication"), - required=False, - initial="true", - choices={"true": _("Enabled"), "false": _("Disabled")}, - widget=forms.RadioSelect(attrs={"class": "radiolist inline"}), - help_text=help_text, - ) - def validate_passwords( self, password1_field_name="password1", password2_field_name="password2", - usable_password_field_name="usable_password", ): - usable_password = ( - self.cleaned_data.pop(usable_password_field_name, None) != "false" - ) - self.cleaned_data["set_usable_password"] = usable_password password1 = self.cleaned_data.get(password1_field_name) password2 = self.cleaned_data.get(password2_field_name) - if not usable_password: - return self.cleaned_data - if not password1 and password1_field_name not in self.errors: error = ValidationError( self.fields[password1_field_name].error_messages["required"], @@ -177,30 +151,81 @@ class SetPasswordMixin: def validate_password_for_user(self, user, password_field_name="password2"): password = self.cleaned_data.get(password_field_name) - if password and self.cleaned_data["set_usable_password"]: + if password: try: password_validation.validate_password(password, user) except ValidationError as error: self.add_error(password_field_name, error) def set_password_and_save(self, user, password_field_name="password1", commit=True): - if self.cleaned_data["set_usable_password"]: - user.set_password(self.cleaned_data[password_field_name]) - else: - user.set_unusable_password() + user.set_password(self.cleaned_data[password_field_name]) if commit: user.save() return user +class SetUnusablePasswordMixin: + """ + Form mixin that allows setting an unusable password for a user. + + This mixin should be used in combination with `SetPasswordMixin`. + """ + + usable_password_help_text = _( + "Whether the user will be able to authenticate using a password or not. " + "If disabled, they may still be able to authenticate using other backends, " + "such as Single Sign-On or LDAP." + ) + + @staticmethod + def create_usable_password_field(help_text=usable_password_help_text): + return forms.ChoiceField( + label=_("Password-based authentication"), + required=False, + initial="true", + choices={"true": _("Enabled"), "false": _("Disabled")}, + widget=forms.RadioSelect(attrs={"class": "radiolist inline"}), + help_text=help_text, + ) + + def validate_passwords( + self, + *args, + usable_password_field_name="usable_password", + **kwargs, + ): + usable_password = ( + self.cleaned_data.pop(usable_password_field_name, None) != "false" + ) + self.cleaned_data["set_usable_password"] = usable_password + + if usable_password: + super().validate_passwords(*args, **kwargs) + + def validate_password_for_user(self, user, **kwargs): + if self.cleaned_data["set_usable_password"]: + super().validate_password_for_user(user, **kwargs) + + def set_password_and_save(self, user, commit=True, **kwargs): + if self.cleaned_data["set_usable_password"]: + user = super().set_password_and_save(user, **kwargs, commit=commit) + else: + user.set_unusable_password() + if commit: + user.save() + return user + + class BaseUserCreationForm(SetPasswordMixin, forms.ModelForm): """ A form that creates a user, with no privileges, from the given username and password. + + This is the documented base class for customizing the user creation form. + It should be kept mostly unchanged to ensure consistency and compatibility. """ password1, password2 = SetPasswordMixin.create_password_fields() - usable_password = SetPasswordMixin.create_usable_password_field() class Meta: model = User @@ -520,13 +545,13 @@ class PasswordChangeForm(SetPasswordForm): return old_password -class AdminPasswordChangeForm(SetPasswordMixin, forms.Form): +class AdminPasswordChangeForm(SetUnusablePasswordMixin, SetPasswordMixin, forms.Form): """ A form used to change the password of a user in the admin interface. """ required_css_class = "required" - usable_password_help_text = SetPasswordMixin.usable_password_help_text + ( + usable_password_help_text = SetUnusablePasswordMixin.usable_password_help_text + ( '" ) @@ -538,7 +563,7 @@ class AdminPasswordChangeForm(SetPasswordMixin, forms.Form): self.fields["password1"].widget.attrs["autofocus"] = True if self.user.has_usable_password(): self.fields["usable_password"] = ( - SetPasswordMixin.create_usable_password_field( + SetUnusablePasswordMixin.create_usable_password_field( self.usable_password_help_text ) ) @@ -558,3 +583,8 @@ class AdminPasswordChangeForm(SetPasswordMixin, forms.Form): if "set_usable_password" in data or "password1" in data and "password2" in data: return ["password"] return [] + + +class AdminUserCreationForm(SetUnusablePasswordMixin, UserCreationForm): + + usable_password = SetUnusablePasswordMixin.create_usable_password_field() diff --git a/docs/releases/5.1.1.txt b/docs/releases/5.1.1.txt index 25c0b4c297c..417584b58e7 100644 --- a/docs/releases/5.1.1.txt +++ b/docs/releases/5.1.1.txt @@ -12,3 +12,9 @@ Bugfixes * Fixed a regression in Django 5.1 that caused a crash of ``Window()`` when passing an empty sequence to the ``order_by`` parameter, and a crash of ``Prefetch()`` for a sliced queryset without ordering (:ticket:`35665`). + +* Fixed a regression in Django 5.1 where a new ``usable_password`` field was + included in :class:`~django.contrib.auth.forms.BaseUserCreationForm` (and + children). A new :class:`~django.contrib.auth.forms.AdminUserCreationForm` + including this field was added, isolating the feature to the admin where it + was intended (:ticket:`35678`). diff --git a/docs/releases/5.1.txt b/docs/releases/5.1.txt index f47fa8bd3ff..bc868fddda6 100644 --- a/docs/releases/5.1.txt +++ b/docs/releases/5.1.txt @@ -118,11 +118,11 @@ Minor features * The default ``parallelism`` of the ``ScryptPasswordHasher`` is increased from 1 to 5, to follow OWASP recommendations. -* :class:`~django.contrib.auth.forms.BaseUserCreationForm` and - :class:`~django.contrib.auth.forms.AdminPasswordChangeForm` now support - disabling password-based authentication by setting an unusable password on - form save. This is now available in the admin when visiting the user creation - and password change pages. +* The new :class:`~django.contrib.auth.forms.AdminUserCreationForm` and + the existing :class:`~django.contrib.auth.forms.AdminPasswordChangeForm` now + support disabling password-based authentication by setting an unusable + password on form save. This is now available in the admin when visiting the + user creation and password change pages. * :func:`~.django.contrib.auth.decorators.login_required`, :func:`~.django.contrib.auth.decorators.permission_required`, and diff --git a/docs/topics/auth/default.txt b/docs/topics/auth/default.txt index b0599e4be2e..a81de8f2cc4 100644 --- a/docs/topics/auth/default.txt +++ b/docs/topics/auth/default.txt @@ -1645,6 +1645,23 @@ provides several built-in forms located in :mod:`django.contrib.auth.forms`: Option to disable (or reenable) password-based authentication was added. +.. class:: AdminUserCreationForm + + .. versionadded:: 5.1.1 + + A form used in the admin interface to create a new user. Inherits from + :class:`UserCreationForm`. + + It includes an additional ``usable_password`` field, enabled by default. If + ``usable_password`` is enabled, it verifies that ``password1`` and + ``password2`` are non empty and match, validates the password using + :func:`~django.contrib.auth.password_validation.validate_password`, and + sets the user's password using + :meth:`~django.contrib.auth.models.User.set_password()`. + If ``usable_password`` is disabled, no password validation is done, and + password-based authentication is disabled for the user by calling + :meth:`~django.contrib.auth.models.User.set_unusable_password()`. + .. class:: AuthenticationForm A form for logging a user in. @@ -1735,21 +1752,12 @@ provides several built-in forms located in :mod:`django.contrib.auth.forms`: A :class:`~django.forms.ModelForm` for creating a new user. This is the recommended base class if you need to customize the user creation form. - It has four fields: ``username`` (from the user model), ``password1``, - ``password2``, and ``usable_password`` (the latter is enabled by default). - If ``usable_password`` is enabled, it verifies that ``password1`` and - ``password2`` are non empty and match, validates the password using + It has three fields: ``username`` (from the user model), ``password1``, + and ``password2``. It verifies that ``password1`` and ``password2`` match, + validates the password using :func:`~django.contrib.auth.password_validation.validate_password`, and sets the user's password using :meth:`~django.contrib.auth.models.User.set_password()`. - If ``usable_password`` is disabled, no password validation is done, and - password-based authentication is disabled for the user by calling - :meth:`~django.contrib.auth.models.User.set_unusable_password()`. - - .. versionchanged:: 5.1 - - Option to create users with disabled password-based authentication was - added. .. class:: UserCreationForm diff --git a/tests/auth_tests/test_forms.py b/tests/auth_tests/test_forms.py index 22b0aa67188..13f86b6b770 100644 --- a/tests/auth_tests/test_forms.py +++ b/tests/auth_tests/test_forms.py @@ -5,6 +5,7 @@ from unittest import mock from django.contrib.auth.forms import ( AdminPasswordChangeForm, + AdminUserCreationForm, AuthenticationForm, BaseUserCreationForm, PasswordChangeForm, @@ -79,6 +80,12 @@ class BaseUserCreationFormTest(TestDataMixin, TestCase): form_class = BaseUserCreationForm + def test_form_fields(self): + form = self.form_class() + self.assertEqual( + list(form.fields.keys()), ["username", "password1", "password2"] + ) + def test_user_already_exists(self): data = { "username": "testclient", @@ -239,16 +246,6 @@ class BaseUserCreationFormTest(TestDataMixin, TestCase): form["password2"].errors, ) - # passwords are not validated if `usable_password` is unset - data = { - "username": "othertestclient", - "password1": "othertestclient", - "password2": "othertestclient", - "usable_password": "false", - } - form = BaseUserCreationForm(data) - self.assertIs(form.is_valid(), True, form.errors) - def test_password_whitespace_not_stripped(self): data = { "username": "testuser", @@ -330,19 +327,6 @@ class BaseUserCreationFormTest(TestDataMixin, TestCase): ["The password is too similar to the first name."], ) - # passwords are not validated if `usable_password` is unset - form = CustomUserCreationForm( - { - "username": "testuser", - "password1": "testpassword", - "password2": "testpassword", - "first_name": "testpassword", - "last_name": "lastname", - "usable_password": "false", - } - ) - self.assertIs(form.is_valid(), True, form.errors) - def test_username_field_autocapitalize_none(self): form = self.form_class() self.assertEqual( @@ -362,17 +346,6 @@ class BaseUserCreationFormTest(TestDataMixin, TestCase): form.fields[field_name].widget.attrs["autocomplete"], autocomplete ) - def test_unusable_password(self): - data = { - "username": "new-user-which-does-not-exist", - "usable_password": "false", - } - form = BaseUserCreationForm(data) - self.assertIs(form.is_valid(), True, form.errors) - u = form.save() - self.assertEqual(u.username, data["username"]) - self.assertFalse(u.has_usable_password()) - class CustomUserCreationFormTest(TestDataMixin, TestCase): @@ -1602,3 +1575,72 @@ class AdminPasswordChangeFormTest(TestDataMixin, TestCase): self.assertIs(form.is_valid(), True) # Valid despite password empty/mismatch. user = form.save(commit=True) self.assertIs(user.has_usable_password(), False) + + +class AdminUserCreationFormTest(BaseUserCreationFormTest): + + form_class = AdminUserCreationForm + + def test_form_fields(self): + form = self.form_class() + self.assertEqual( + list(form.fields.keys()), + ["username", "password1", "password2", "usable_password"], + ) + + @override_settings( + AUTH_PASSWORD_VALIDATORS=[ + { + "NAME": ( + "django.contrib.auth.password_validation." + "UserAttributeSimilarityValidator" + ) + }, + { + "NAME": ( + "django.contrib.auth.password_validation.MinimumLengthValidator" + ), + "OPTIONS": { + "min_length": 12, + }, + }, + ] + ) + def test_no_password_validation_if_unusable_password_set(self): + data = { + "username": "otherclient", + "password1": "otherclient", + "password2": "otherclient", + "usable_password": "false", + } + form = self.form_class(data) + # Passwords are not validated if `usable_password` is unset. + self.assertIs(form.is_valid(), True, form.errors) + + class CustomUserCreationForm(self.form_class): + class Meta(self.form_class.Meta): + model = User + fields = ("username", "email", "first_name", "last_name") + + form = CustomUserCreationForm( + { + "username": "testuser", + "password1": "testpassword", + "password2": "testpassword", + "first_name": "testpassword", + "last_name": "lastname", + "usable_password": "false", + } + ) + self.assertIs(form.is_valid(), True, form.errors) + + def test_unusable_password(self): + data = { + "username": "new-user-which-does-not-exist", + "usable_password": "false", + } + form = self.form_class(data) + self.assertIs(form.is_valid(), True, form.errors) + u = form.save() + self.assertEqual(u.username, data["username"]) + self.assertFalse(u.has_usable_password())