Opened 7 years ago
Closed 7 years ago
#28718 closed Bug (fixed)
Password reset shouldn't depend on the current password being hashed with a supported hasher
Reported by: | Josh Harwood | Owned by: | Tim Graham |
---|---|---|---|
Component: | contrib.auth | Version: | 1.11 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Currently the built in django password reset system requires that you have an active account and that your password can be compared to by an enabled hasher.
I think that this is in error, as you are about to reset the password to something new (hence resetting it) and the standard process of password resetting requires an email confirmation. I can see no way in which this is able to be abused by a malicious 3rd party. If I'm mistaken here then feel free to correct me.
I propose that the system is changed to just require that the user is active and that their password is not marked disabled as per the UNUSABLE_PASSWORD_PREFIX.
Change History (10)
follow-up: 2 comment:1 by , 7 years ago
Type: | Bug → Cleanup/optimization |
---|
comment:2 by , 7 years ago
Replying to Tim Graham:
Where's that restriction implemented?
This line with "has_usable_password": https://github.com/django/django/blob/master/django/contrib/auth/forms.py#L264
comment:3 by , 7 years ago
Summary: | Resetting your password shouldn't depend on your current password being hashed with a supported hasher → Password reset shouldn't depend on the current password being hashed with a supported hasher |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Cleanup/optimization → Bug |
It looks like this is a regression in aeb1389442d0f9669edf6660b747fd10693b63a7. The change in django/contrib/auth/forms.py
line 234 added the undesired check that the password uses a valid hasher.
comment:4 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:6 by , 7 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Not going to be able to finish this. Just needs unit tests. See pull request for more details.
comment:7 by , 7 years ago
Owner: | changed from | to
---|
comment:8 by , 7 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
I think the change to has_usable_password()
in 703c266682be39f7153498ad0d8031231f12ee79 was unintentional and that adding a User.has_disabled_password()
method as originally proposed here will only add more confusion. I've proposed an alternate PR.
Where's that restriction implemented?