Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#16837 closed Bug (fixed)

updated error message when logging in into the admin fails because is_staff is False

Reported by: Wim Feijen <wim@…> Owned by: Wim Feijen
Component: contrib.admin Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When a user tries to login on the admin, with correct username &
password, but is_staff is set to False, the error message is
misleadingly wrong:
"Please enter a correct username and password. Note that both fields
are case-sensitive."

After discussion on django-developers:
http://groups.google.com/group/django-developers/browse_thread/thread/c070dcd878a75a2b

a solution was proposed to have a general message in all cases, so potential attackers cannot distinguish between the case where username&password are right and is_staff = False versus the case where username&password don't fit.

The message is:

"Username and password incorrect or access to this page is restricted".

as proposed by Adam Jenkins, with an added "is".

Although the global variable ERROR_MESSAGE does not seem to be used anywhere else in django, I'll keep it as it is for now.

Gentlemen and ladies, now we need translations.

Wim

Attachments (3)

16837.general_error_message.diff (631 bytes ) - added by Wim Feijen <wim@…> 13 years ago.
16837.general_error_message.2.diff (4.2 KB ) - added by Wim Feijen <wim@…> 13 years ago.
Better error message: updated tests as well.
16837.general_error_message.patch3.diff (4.2 KB ) - added by Wim Feijen <wim@…> 13 years ago.
Sets ERROR_MESSAGE in tests in stead of importing, in order to prevent double rainbows ;)

Download all attachments as: .zip

Change History (19)

by Wim Feijen <wim@…>, 13 years ago

comment:1 by Wim Feijen <wim@…>, 13 years ago

Has patch: set

comment:2 by Wim Feijen <wim@…>, 13 years ago

Tests fail - need update

comment:3 by Wim Feijen <wim@…>, 13 years ago

Patch needs improvement: set

comment:4 by Julien Phalip, 13 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

I think this is in principle a good compromise. The suggested patch feels a bit dry though, and could be improved. We don't want to lose the "Note that both fields are case-sensitive." tip either. Please also add some tests.

comment:5 by Jochem, 13 years ago

What about... "Please enter the correct username and password for a staff account."?

by Wim Feijen <wim@…>, 13 years ago

Better error message: updated tests as well.

comment:6 by Wim Feijen <wim@…>, 13 years ago

Hi Julien, and Jocmeh. Thanks for your suggestions. I have updated the error message to:
"Please enter the correct username and password "
"for a staff account. Note that both fields are case-sensitive."

A patch (with working tests) is attached.

Thanks, Wim

comment:7 by Julien Phalip, 13 years ago

Needs tests: unset

Thank you Wim, the message looks good. However, the tests aren't proper. If ERROR_MESSAGE accidentally gets modified to, for example, "I love double rainbows", the tests will still pass :D
The message may have serious security implications so it's important to explicitly test it.

Also, one note on the process. When you provide a patch that *you* think is right, please feel free to remove the "Needs tests" and "Patch needs improvement" flags, so that the ticket can more easily be found by people looking for patches to review. In this case, I'm removing "Needs tests" because there are some, but leaving "Patch needs improvement" because it does ;)

by Wim Feijen <wim@…>, 13 years ago

Sets ERROR_MESSAGE in tests in stead of importing, in order to prevent double rainbows ;)

comment:8 by Wim Feijen <wim@…>, 13 years ago

Patch needs improvement: unset

Julien, thanks for looking into the patch so quickly. Also thanks for the feedback for helping me improve. :)

comment:9 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

Great, thanks for your patience ;)

comment:10 by Wim Feijen <wim@…>, 13 years ago

Summary: when logging in into the adminupdated error message when logging in into the admin fails because is_staff is False

comment:11 by Wim Feijen <wim@…>, 13 years ago

Owner: changed from nobody to Wim Feijen

comment:12 by Paul McMillan, 12 years ago

Resolution: fixed
Status: newclosed

In [16872]:

Fixed #16837 -- Improved error message for admin login.

comment:13 by Paul McMillan, 12 years ago

In [16878]:

[1.3.X] Fixed #16837 -- Improved error messages for admin login. Thanks Wim Feijen for the patch.

comment:14 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: closedreopened

It's common policy to not backport changes to a release branch which change a translation string. This needs to be reverted in the 1.3.X branch.

comment:15 by Paul McMillan, 12 years ago

In [16891]:

[1.3.X] Reverting r16878 (improved admin error message) per advice from jezdez. refs #16837

comment:16 by Paul McMillan, 12 years ago

Resolution: fixed
Status: reopenedclosed

Closing after reverting the change in the 1.3.X branch.

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