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 , 4 weeks ago
| Needs documentation: | set |
|---|
follow-up: 3 comment:2 by , 4 weeks ago
follow-up: 4 comment:3 by , 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.
comment:4 by , 3 weeks ago
| Component: | Documentation → contrib.auth |
|---|---|
| Easy pickings: | unset |
| Needs documentation: | unset |
| Resolution: | → invalid |
| Status: | new → closed |
| Type: | Uncategorized → Bug |
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.
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.Can you provide reproduction steps?