Inconsistent Behavior in Auth with UserAttributeSimilarityValidator
54 views
Skip to first unread message
Andrew Pinkham
Mar 20, 2017, 10:29:59 AM3/20/17
to django-d...@googlegroups.com
Hi,
I've found some inconsistent behavior in how password validation via the `UserAttributeSimilarityValidator` is handled by `contrib/auth`. I did not find any mention of this in Trac or on the mailing list, but apologies if this has already been discussed.
AFAICT: When creating a user, the password is only checked in similarity to the `username` field. When changing a password, the password is checked in similarity to `username`, `first_name`, `last_name`, and `email`.
This seems undesirable - it is possible to set a password at creation time that might be rejected if set during a password change.
In short:
When `SetPasswordForm` calls password validation, it has all of the fields on the `User` instance, but when `UserCreationForm` does so, it manually adds the one field being checked.
In depth:
In `django/contrib/auth/forms.py`, the `SetPasswordForm` and `UserCreationForm` both call `password_validation.validate_password()` in the `clean_password2()` method. In both instances, the forms pass `password2` and `self.instance`. In the `UserCreationForm`, the `ModelForm` hasn't yet created the `User` instance. `UserCreationForm` manually adds `username` to the empty `User` instance on line 105 to allow for user attribute validation.
Rather than check the validity of the passwords in the `password2` clean method, it seems like it would be better to wait until the `User` instance has been created. We can use the `ModelForm` `_post_clean()` hook to achieve this.
def _post_clean(self):
super()._post_clean() # creates self.instance
password = self.cleaned_data.get("password1")
if password:
try:
password_validation.validate_password(password, self.instance)
except ValidationError as error:
self.add_error("password1", error)
This keeps the two forms' behaviors consistent. Note that I've opted to use `password1` instead of `password2` because it feels more logical to show the problem above both passwords, rather than between the two fields.
Is there a reason *not* to make this change? Are there specific reasons for why we are only checking the `username` field during creation?
Feedback appreciated. If others approve of this, I will open a PR.
Andrew
http://jambonsw.com
http://django-unleashed.com
I've found some inconsistent behavior in how password validation via the `UserAttributeSimilarityValidator` is handled by `contrib/auth`. I did not find any mention of this in Trac or on the mailing list, but apologies if this has already been discussed.
AFAICT: When creating a user, the password is only checked in similarity to the `username` field. When changing a password, the password is checked in similarity to `username`, `first_name`, `last_name`, and `email`.
This seems undesirable - it is possible to set a password at creation time that might be rejected if set during a password change.
In short:
When `SetPasswordForm` calls password validation, it has all of the fields on the `User` instance, but when `UserCreationForm` does so, it manually adds the one field being checked.
In depth:
In `django/contrib/auth/forms.py`, the `SetPasswordForm` and `UserCreationForm` both call `password_validation.validate_password()` in the `clean_password2()` method. In both instances, the forms pass `password2` and `self.instance`. In the `UserCreationForm`, the `ModelForm` hasn't yet created the `User` instance. `UserCreationForm` manually adds `username` to the empty `User` instance on line 105 to allow for user attribute validation.
Rather than check the validity of the passwords in the `password2` clean method, it seems like it would be better to wait until the `User` instance has been created. We can use the `ModelForm` `_post_clean()` hook to achieve this.
def _post_clean(self):
super()._post_clean() # creates self.instance
password = self.cleaned_data.get("password1")
if password:
try:
password_validation.validate_password(password, self.instance)
except ValidationError as error:
self.add_error("password1", error)
This keeps the two forms' behaviors consistent. Note that I've opted to use `password1` instead of `password2` because it feels more logical to show the problem above both passwords, rather than between the two fields.
Is there a reason *not* to make this change? Are there specific reasons for why we are only checking the `username` field during creation?
Feedback appreciated. If others approve of this, I will open a PR.
Andrew
http://jambonsw.com
http://django-unleashed.com
Reply all
Reply to author
Forward
0 new messages