Inconsistent Behavior in Auth with UserAttributeSimilarityValidator

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Inconsistent Behavior in Auth with UserAttributeSimilarityValidator

Andrew Pinkham
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

--
You received this message because you are subscribed to the Google Groups "Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/878428A0-4F59-403A-A611-DD9208A605C9%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.
Loading...