Opened 5 years ago

Closed 4 years ago

#14674 closed Bug (fixed)

ResetPasswordForm doesn't consider unusable_password setting

Reported by: summerisgone Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords: unusable_password, authorization, reset password, sprintnov13
Cc: lrekucki 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 summerisgone 5 years ago.
Excluded users with unusable password from ResetPasswordForm
14764.diff (2.1 KB) - added by thejaswi_puthraya 5 years ago.
Test and patch
14764_2.diff (2.8 KB) - added by thejaswi_puthraya 5 years ago.
updated the code, add docs and tests
ticket14674.diff (4.5 KB) - added by lrekucki 5 years ago.

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by summerisgone

Excluded users with unusable password from ResetPasswordForm

comment:1 Changed 5 years ago by summerisgone

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

comment:2 Changed 5 years ago by nessita

  • Owner changed from nobody to nessita

comment:3 Changed 5 years ago by nessita

  • Component changed from Uncategorized to Authentication
  • Needs tests set
  • Owner changed from nessita to nobody
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The patch needs to also provide unit tests.

Changed 5 years ago by thejaswi_puthraya

Test and patch

comment:4 Changed 5 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 5 years ago by thejaswi_puthraya

  • Needs tests unset

comment:6 Changed 5 years ago by thejaswi_puthraya

  • Keywords sprintnov13 added

Changed 5 years ago by thejaswi_puthraya

updated the code, add docs and tests

comment:7 Changed 5 years ago by lrekucki

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 5 years ago by lrekucki

  • Cc lrekucki added

comment:9 Changed 5 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 5 years ago by lrekucki

comment:10 follow-up: Changed 5 years ago by lrekucki

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 5 years ago by ramiro

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 4 years ago by jaddison

  • milestone 1.3 deleted
  • Severity set to Normal
  • Type set to Bug

comment:13 Changed 4 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

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