Opened 13 years ago

Closed 13 years ago

#14674 closed Bug (fixed)

ResetPasswordForm doesn't consider unusable_password setting

Reported by: Ivan Gromov Owned by: nobody
Component: contrib.auth Version: dev
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: no UI/UX: no

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 13 years ago.
Excluded users with unusable password from ResetPasswordForm
14764.diff (2.1 KB ) - added by Thejaswi Puthraya 13 years ago.
Test and patch
14764_2.diff (2.8 KB ) - added by Thejaswi Puthraya 13 years ago.
updated the code, add docs and tests
ticket14674.diff (4.5 KB ) - added by Łukasz Rekucki 13 years ago.

Download all attachments as: .zip

Change History (17)

by Ivan Gromov, 13 years ago

Attachment: 14674.patch added

Excluded users with unusable password from ResetPasswordForm

comment:1 by Ivan Gromov, 13 years ago

Has patch: set

comment:2 by Natalia Bidart, 13 years ago

Owner: changed from nobody to Natalia Bidart

comment:3 by Natalia Bidart, 13 years ago

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.

by Thejaswi Puthraya, 13 years ago

Attachment: 14764.diff added

Test and patch

comment:4 by Thejaswi Puthraya, 13 years ago

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 by Thejaswi Puthraya, 13 years ago

Needs tests: unset

comment:6 by Thejaswi Puthraya, 13 years ago

Keywords: sprintnov13 added

by Thejaswi Puthraya, 13 years ago

Attachment: 14764_2.diff added

updated the code, add docs and tests

comment:7 by Łukasz Rekucki, 13 years ago

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 by Łukasz Rekucki, 13 years ago

Cc: Łukasz Rekucki added

comment:9 by Thejaswi Puthraya, 13 years ago

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.

by Łukasz Rekucki, 13 years ago

Attachment: ticket14674.diff added

comment:10 by Łukasz Rekucki, 13 years ago

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.

in reply to:  10 comment:11 by Ramiro Morales, 13 years ago

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 by James Addison, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:13 by Jannis Leidel, 13 years ago

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