Opened 5 years ago

Closed 4 years ago

#17944 closed Bug (fixed)

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

Reported by: Stefano Apostolico Owned by: Claude Paroz
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 Stefano Apostolico 5 years ago.
ReadOnlyPasswordHashWidget_0.1.patch (3.9 KB) - added by Stefano Apostolico 5 years ago.
17944.3.patch (7.6 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by Stefano Apostolico

comment:1 Changed 5 years ago by Stefano Apostolico

Type: UncategorizedBug

comment:2 Changed 5 years ago by Carl Meyer

Needs tests: set
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

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

comment:3 Changed 5 years ago by Stefano Apostolico

Needs tests: unset

Added ReadOnlyPasswordHashWidget_0.1.patch

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

comment:4 Changed 5 years ago by Stefano Apostolico

Owner: changed from nobody to Stefano Apostolico

comment:5 Changed 5 years ago by Stefano Apostolico

Version: 1.4-beta-11.4-rc-2

comment:6 Changed 5 years ago by Aymeric Augustin

Triage Stage: AcceptedReady 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 5 years ago by Aymeric Augustin

Attachment: 17944.3.patch added

comment:7 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

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

In [17779]:

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

comment:9 Changed 5 years ago by Aymeric Augustin

Has patch: unset
Resolution: fixed
Status: closedreopened
Triage Stage: Ready for checkinAccepted

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

comment:10 Changed 5 years ago by Aymeric Augustin

Owner: changed from Stefano Apostolico to Aymeric Augustin
Status: reopenednew

comment:11 Changed 5 years ago by Aymeric Augustin

Owner: changed from Aymeric Augustin to nobody
Severity: Release blockerNormal

comment:12 Changed 4 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Severity: NormalRelease blocker

Oops, let's not forget this before 1.5.

comment:13 Changed 4 years ago by Claude Paroz

Owner: changed from Aymeric Augustin to Claude Paroz

comment:14 Changed 4 years ago by Claude Paroz

Resolution: fixed
Status: newclosed

In fact, already fixed in [703c266682be39f7153498ad0d8031231f12ee79].

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