Opened 4 weeks ago

Closed 3 weeks ago

#36703 closed Bug (invalid)

Undocumented change of SetPasswordForm in django 5.1 release notes

Reported by: Laurent Bergeron Owned by:
Component: contrib.auth Version: 5.1
Severity: Normal Keywords: Authentication, Forms
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since version 5.1 of django, django.contrib.auth.forms.SetPasswordForm looks like this:

class SetPasswordForm(SetPasswordMixin, forms.Form):
    """
    A form that lets a user set their password without entering the old
    password
    """

    new_password1, new_password2 = SetPasswordMixin.create_password_fields(
        label1=_("New password"), label2=_("New password confirmation")
    )

    def __init__(self, user, *args, **kwargs):
        self.user = user
        super().__init__(*args, **kwargs)

    def clean(self):
        self.validate_passwords("new_password1", "new_password2")
        self.validate_password_for_user(self.user, "new_password2")
        return super().clean()

    def save(self, commit=True):
        return self.set_password_and_save(self.user, "new_password1", commit=commit)

Before version 5.1 though, it looked like this:

class SetPasswordForm(forms.Form):
    """
    A form that lets a user set their password without entering the old
    password
    """

    error_messages = {
        "password_mismatch": _("The two password fields didn’t match."),
    }
    new_password1 = forms.CharField(
        label=_("New password"),
        widget=forms.PasswordInput(attrs={"autocomplete": "new-password"}),
        strip=False,
        help_text=password_validation.password_validators_help_text_html(),
    )
    new_password2 = forms.CharField(
        label=_("New password confirmation"),
        strip=False,
        widget=forms.PasswordInput(attrs={"autocomplete": "new-password"}),
    )

    def __init__(self, user, *args, **kwargs):
        self.user = user
        super().__init__(*args, **kwargs)

    def clean_new_password2(self):
        password1 = self.cleaned_data.get("new_password1")
        password2 = self.cleaned_data.get("new_password2")
        if password1 and password2 and password1 != password2:
            raise ValidationError(
                self.error_messages["password_mismatch"],
                code="password_mismatch",
            )
        password_validation.validate_password(password2, self.user)
        return password2

    def save(self, commit=True):
        password = self.cleaned_data["new_password1"]
        self.user.set_password(password)
        if commit:
            self.user.save()
        return self.user

I can't see this change described anywhere in the 5.1 release note https://docs.djangoproject.com/en/5.2/releases/5.1/

Do I have a blind spot and the change is in fact described in the patch note ? If it is not, should it be or is it too small of a change to be part of the release notes?

Personally, it caused some of my tests to fail when I upgraded from 4.2 to 5.2. I had some logic to modify the error messages and it broke because of the change.

Change History (4)

comment:1 by Laurent Bergeron, 4 weeks ago

Needs documentation: set

comment:2 by ontowhee, 4 weeks ago

Hello! It looks like the changes were from #34429, and the release note you linked mentions The new AdminUserCreationForm and the existing AdminPasswordChangeForm now support disabling password-based authentication by setting an unusable password on form save.

Personally, it caused some of my tests to fail when I upgraded from 4.2 to 5.2. I had some logic to modify the error messages and it broke because of the change.

Can you provide reproduction steps?

in reply to:  2 ; comment:3 by Laurent Bergeron, 3 weeks ago

Replying to ontowhee:

Hello! It looks like the changes were from #34429, and the release note you linked mentions The new AdminUserCreationForm and the existing AdminPasswordChangeForm now support disabling password-based authentication by setting an unusable password on form save.

Yeah that seems to be it.

Can you provide reproduction steps?

I created a custom password change form which had a clean method that looked like this:

def clean(self):
    if "new_password2" in self.errors:
        new_password2_errors = self.errors["new_password2"].as_data()
        for error in new_password2_errors:
            if error.code in self.error_messages:
                custom_error_message = self.error_messages[error.code]
                error.message = custom_error_message
    return super().clean()

And there is a dictionary in the form class that contains custom error messages (self.error_messages) too.

But then I updated Django and this method couldn't change any error messages because the errors on new_password2 (and other field I believe) are now raised in super().clean(). The method worked before I updated though as the errors were raised before my clean method.

To solve the issue I edited my method so it looks like this:

def clean(self):
    super().clean()
    if "new_password2" in self.errors:
        new_password2_errors = self.errors["new_password2"].as_data()
        for error in new_password2_errors:
            if error.code in self.error_messages:
                custom_error_message = self.error_messages[error.code]
                error.message = custom_error_message
    return self.cleaned_data

Now that super().clean() is executed before my error messages replacement code, the latter works as expected. To be clear I didn't open this issue to seek for help on how to solve this issue. I wanted to make sure this change was detailed in the release notes.

in reply to:  3 comment:4 by Natalia Bidart, 3 weeks ago

Component: Documentationcontrib.auth
Easy pickings: unset
Needs documentation: unset
Resolution: invalid
Status: newclosed
Type: UncategorizedBug

Replying to Laurent Bergeron:

Replying to ontowhee:

Hello! It looks like the changes were from #34429, and the release note you linked mentions The new AdminUserCreationForm and the existing AdminPasswordChangeForm now support disabling password-based authentication by setting an unusable password on form save.

Yeah that seems to be it.

Hello Laurent! Thank you for taking the time to create the report. And thank you Lilian for the triage.

Now that super().clean() is executed before my error messages replacement code, the latter works as expected. To be clear I didn't open this issue to seek for help on how to solve this issue. I wanted to make sure this change was detailed in the release notes.

The release notes don't (and can't) include every behavioral detail of internal changes. In this case, the documented pattern is to call super() first when overriding clean(), as described here: https://docs.djangoproject.com/en/5.2/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other. Since the current behavior aligns with the docs, I think there's nothing to change here, so I'm closing this accordingly.

Note: See TracTickets for help on using tickets.
Back to Top