Opened 6 years ago

Closed 5 years ago

#14674 closed Bug (fixed)

ResetPasswordForm doesn't consider unusable_password setting

Reported by: Ivan Gromov Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords: unusable_password, authorization, reset password, sprintnov13
Cc: Łukasz Rekucki Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

For example, users, who been logged in by LDAP, can change their password with django.contrib.auth.forms.PasswordResetForm and become independent from LDAP server.

Attachments (4)

14674.patch (1.9 KB) - added by Ivan Gromov 6 years ago.
Excluded users with unusable password from ResetPasswordForm
14764.diff (2.1 KB) - added by Thejaswi Puthraya 6 years ago.
Test and patch
14764_2.diff (2.8 KB) - added by Thejaswi Puthraya 6 years ago.
updated the code, add docs and tests
ticket14674.diff (4.5 KB) - added by Łukasz Rekucki 6 years ago.

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by Ivan Gromov

Attachment: 14674.patch added

Excluded users with unusable password from ResetPasswordForm

comment:1 Changed 6 years ago by Ivan Gromov

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 6 years ago by Natalia Bidart

Owner: changed from nobody to Natalia Bidart

comment:3 Changed 6 years ago by Natalia Bidart

Component: UncategorizedAuthentication
Needs tests: set
Owner: changed from Natalia Bidart to nobody
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The patch needs to also provide unit tests.

Changed 6 years ago by Thejaswi Puthraya

Attachment: 14764.diff added

Test and patch

comment:4 Changed 6 years ago by Thejaswi Puthraya

Patch is more or less the same as nessita's with the exception that it shows an error that the password cannot be changed for this account. Adds a test as an addition.

Doesn't have the docs of nessita's patch.

comment:5 Changed 6 years ago by Thejaswi Puthraya

Needs tests: unset

comment:6 Changed 6 years ago by Thejaswi Puthraya

Keywords: sprintnov13 added

Changed 6 years ago by Thejaswi Puthraya

Attachment: 14764_2.diff added

updated the code, add docs and tests

comment:7 Changed 6 years ago by Łukasz Rekucki

The current patch changes a single query that populates self.user_cache into to 2 queries that don't. I would suggest something in lines of:

if len(self.users_cache) == 0:
    raise forms.ValidationError(_("That e-mail address doesn't have an associated user account. Are you sure you've registered?"))
if any((user.password == UNUSABLE_PASSWORD) for user in self.users_cache):
    raise forms.ValidationError(_("The user account associated with this email address cannot reset it's password.")) 

comment:8 Changed 6 years ago by Łukasz Rekucki

Cc: Łukasz Rekucki added

comment:9 Changed 6 years ago by Thejaswi Puthraya

lrekucki,
I like your patch but the 'any' function is part of python 2.5 and later.

I am not sure if 2 'count' queries or evaluating the queryset is recommended.

Changed 6 years ago by Łukasz Rekucki

Attachment: ticket14674.diff added

comment:10 Changed 6 years ago by Łukasz Rekucki

Here's a patch. Lack of "any" isn't a problem, 'cause you can just turn in into 3 line loop... or add it to django.utils.itercompat like I did (we have all(), so I don't see a reason why we shouldn't have any()). Anyway, after giving this more thought, this can cause trouble if someone used the PasswordResetForm as part of his registration process. I know at least one application that would be affected, but this may just be a misuse of this form.

comment:11 in reply to:  10 Changed 6 years ago by Ramiro Morales

Replying to lrekucki:

Lack of "any" isn't a problem, 'cause you can just turn in into 3 line loop... or add it to django.utils.itercompat like I did

FYI, I already added any() in r14563/r14565 with a simple and uncontroversial modification, feel free to pursue proposal of the bigger changes mods to itercompat you had implemented in your patch.

comment:12 Changed 5 years ago by James Addison

milestone: 1.3
Severity: Normal
Type: Bug

comment:13 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [16455]:

Fixed #14674 -- Prevent user accounts with an unusable password from resetting passwords. Thanks, summerisgone, thejaswi_puthraya and lrekucki.

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