Opened 8 years ago

Closed 8 years ago

#26909 closed Cleanup/optimization (fixed)

Allow UserAttributeSimilarityValidator to validate against properties

Reported by: Kieren Pitts Owned by: Andrew Nester
Component: contrib.auth Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The UserAttributeSimilarityValidator class (in contrib/auth/password_validation.py) has a hardcoded reference to 'username' in it:

DEFAULT_USER_ATTRIBUTES = ('username', 'first_name', 'last_name', 'email')

These attributes are looped through to check new passwords for similarity with existing information on the user. However, if you use a custom user model then you may not have a 'username' (especially if using 'email' as the username) and this then results in an error on line 147 when resetting passwords (using a password that is similar to, say, the email):

verbose_name = force_text(user._meta.get_field(attribute_name).verbose_name)

In some cases you can use built-in auth forms with a custom user model. These use-cases have some restrictions and are outlined in the docs here:

https://docs.djangoproject.com/en/1.9/topics/auth/customizing/#custom-users-and-the-built-in-auth-forms

However, given the hard-coded attributes in the UserAttributeSimilarityValidator class the below statement from the above page is not correct because the missing 'username' field causes an error and using a property does not work with _meta.get_field:

"PasswordResetForm: Assumes that the user model has a field named email that can be used to identify the user and a boolean field named is_active to prevent password resets for inactive users."

This issue only comes to light if the password is similar to the data in one of the other fields (i.e. the SequenceMatcher check suggests they are similar). For example, if you have a custom user model using 'email' as the username field and a user with a username of 'somespecialname@…' tries to set a password of 'somespecialname'.

It's not clear if the problem is with the code which shouldn't have the hardcoded values (perhaps they could be overridden by a setting instead) or if it's a mistake/omission in the docs.

Apologies if I've missed something obvious here.

Change History (8)

comment:1 by Kieren Pitts, 8 years ago

Summary: UserAttributeSimilarityValidator has hardcoded reference to 'username'UserAttributeSimilarityValidator has hardcoded reference to 'username' causing error in password reset built-in auth form when using a custom model

comment:2 by Tim Graham, 8 years ago

According to the UserAttributeSimilarityValidator documentation, "Attributes that don’t exist are ignored" (code).

Is User.username returning some non-empty value on your custom user model?

comment:3 by Kieren Pitts, 8 years ago

Sorry, in our case the model has a 'username' property. So I guess the issue is that the initial check to see if User.username returns something is not consistent with the result of user._meta.get_field because the latter doesn't work with a @property set on the model. My apologies.

comment:4 by Tim Graham, 8 years ago

In your case, do you want the username property to be taken account for this validator? If so, then feel free to submit a patch to fix the crash. If not, then you should pass some custom user_attributes to the validator and I think we could close this and reopen if someone has the use case of validating similarity to properties.

comment:5 by Andrew Nester, 8 years ago

Owner: changed from nobody to Andrew Nester
Status: newassigned

comment:6 by Tim Graham, 8 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

comment:7 by Tim Graham, 8 years ago

Patch needs improvement: set
Summary: UserAttributeSimilarityValidator has hardcoded reference to 'username' causing error in password reset built-in auth form when using a custom modelAllow UserAttributeSimilarityValidator to validate against properties
Type: BugCleanup/optimization

Left some comments for improvement on the PR.

comment:8 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 4591cf3:

Fixed #26909 -- Allowed UserAttributeSimilarityValidator to validate against model properties.

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