Code

Opened 2 years ago

Closed 18 months ago

#17944 closed Bug (fixed)

Unable to access to User record in the admin if the user has a unmanageable password

Reported by: saxix Owned by: claudep
Component: contrib.auth Version: 1.4-rc-2
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If the user has a unmanageable password (ie no hashers found) ReadOnlyPasswordHashWidget reraise ValueError up to the UI, this prevents to access to the user record if the form contains ReadOnlyPasswordHashWidget ( admin ), this happen also for blank password. If the system use a custom backend with external authentication there is no reason for django to known the password algorithm, but should be still possible to access to the user record.
The patch simply surround the involved lined into the ReadOnlyPasswordHashWidget.render() and display a 'Unknown password hashing algorithm' message instead.
Not sure if the message could be improved/changed.

Attachments (3)

ReadOnlyPasswordHashWidget.patch (1.3 KB) - added by saxix 2 years ago.
ReadOnlyPasswordHashWidget_0.1.patch (3.9 KB) - added by saxix 2 years ago.
17944.3.patch (7.6 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (17)

Changed 2 years ago by saxix

comment:1 Changed 2 years ago by saxix

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 2 years ago by carljm

  • Needs tests set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

This is definitely a release-blocking regression, thanks for the report.

Patch looks good to me except that the "except" clause captures the exception when it has no use for it (and uses the old syntax to do so); the ",e" should just be removed.

And it needs a test.

This violates the string freeze, but I don't see any good alternative. Having this fixed with an untranslated message in 1.4 is better than not having it fixed.

Changed 2 years ago by saxix

comment:3 Changed 2 years ago by saxix

  • Needs tests unset

Added ReadOnlyPasswordHashWidget_0.1.patch

  • added tests both for empty and unparseable password
  • removed exception capturing

comment:4 Changed 2 years ago by saxix

  • Owner changed from nobody to saxix

comment:5 Changed 2 years ago by saxix

  • Version changed from 1.4-beta-1 to 1.4-rc-2

comment:6 Changed 2 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

I've updated the patch slightly to make the try/except clause as specific as possible and add one more test.

I agree with Carl that an untranslated string is less of a problem than an uncaught exception.

(There's a little bit of noise in the patch because my editor strips trailing spaces, sorry.)

Changed 2 years ago by aaugustin

comment:7 Changed 2 years ago by aaugustin

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

In [17775]:

Fixed #17944 -- Prevented an error in the user change page of the admin when the content of the password field doesn't match the expected format. Thanks saxix for the report and initial patch.

comment:8 Changed 2 years ago by jezdez

In [17779]:

Reverted the introduction of a translation string in r17775 as it happened after string freeze. Refs #17944.

comment:9 Changed 2 years ago by aaugustin

  • Has patch unset
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to Accepted

I'm re-opening the bug so we remember to re-introduce the ugettext call after 1.4.

comment:10 Changed 2 years ago by aaugustin

  • Owner changed from saxix to aaugustin
  • Status changed from reopened to new

comment:11 Changed 2 years ago by aaugustin

  • Owner changed from aaugustin to nobody
  • Severity changed from Release blocker to Normal

comment:12 Changed 18 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Severity changed from Normal to Release blocker

Oops, let's not forget this before 1.5.

comment:13 Changed 18 months ago by claudep

  • Owner changed from aaugustin to claudep

comment:14 Changed 18 months ago by claudep

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

In fact, already fixed in [703c266682be39f7153498ad0d8031231f12ee79].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.