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)
Change History (17)
Changed 5 years ago by
| Attachment: | ReadOnlyPasswordHashWidget.patch added |
|---|
comment:1 Changed 5 years ago by
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
| Patch needs improvement: | unset |
| Type: | Uncategorized → Bug |
comment:2 Changed 5 years ago by
| Needs tests: | set |
|---|---|
| Severity: | Normal → Release blocker |
| Triage Stage: | Unreviewed → Accepted |
Changed 5 years ago by
| Attachment: | ReadOnlyPasswordHashWidget_0.1.patch added |
|---|
comment:3 Changed 5 years ago by
| 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
| Owner: | changed from nobody to Stefano Apostolico |
|---|
comment:5 Changed 5 years ago by
| Version: | 1.4-beta-1 → 1.4-rc-2 |
|---|
comment:6 Changed 5 years ago by
| Triage Stage: | Accepted → 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 5 years ago by
| Attachment: | 17944.3.patch added |
|---|
comment:9 Changed 5 years ago by
| Has patch: | unset |
|---|---|
| Resolution: | fixed |
| Status: | closed → reopened |
| Triage Stage: | Ready for checkin → Accepted |
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
| Owner: | changed from Stefano Apostolico to Aymeric Augustin |
|---|---|
| Status: | reopened → new |
comment:11 Changed 5 years ago by
| Owner: | changed from Aymeric Augustin to nobody |
|---|---|
| Severity: | Release blocker → Normal |
comment:12 Changed 4 years ago by
| Owner: | changed from nobody to Aymeric Augustin |
|---|---|
| Severity: | Normal → Release blocker |
Oops, let's not forget this before 1.5.
comment:13 Changed 4 years ago by
| Owner: | changed from Aymeric Augustin to Claude Paroz |
|---|
comment:14 Changed 4 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
In fact, already fixed in [703c266682be39f7153498ad0d8031231f12ee79].
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.