Opened 5 years ago

Closed 5 years ago

Last modified 5 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@…> 5 years ago.
16837.general_error_message.2.diff (4.2 KB) - added by Wim Feijen <wim@…> 5 years ago.
Better error message: updated tests as well.
16837.general_error_message.patch3.diff (4.2 KB) - added by Wim Feijen <wim@…> 5 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)

Changed 5 years ago by Wim Feijen <wim@…>

comment:1 Changed 5 years ago by Wim Feijen <wim@…>

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

comment:2 Changed 5 years ago by Wim Feijen <wim@…>

Tests fail - need update

comment:3 Changed 5 years ago by Wim Feijen <wim@…>

Patch needs improvement: set

comment:4 Changed 5 years ago by Julien Phalip

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

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

Changed 5 years ago by Wim Feijen <wim@…>

Better error message: updated tests as well.

comment:6 Changed 5 years ago by Wim Feijen <wim@…>

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 Changed 5 years ago by Julien Phalip

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 ;)

Changed 5 years ago by Wim Feijen <wim@…>

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

comment:8 Changed 5 years ago by Wim Feijen <wim@…>

Patch needs improvement: unset

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

comment:9 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

Great, thanks for your patience ;)

comment:10 Changed 5 years ago by Wim Feijen <wim@…>

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

comment:11 Changed 5 years ago by Wim Feijen <wim@…>

Owner: changed from nobody to Wim Feijen

comment:12 Changed 5 years ago by Paul McMillan

Resolution: fixed
Status: newclosed

In [16872]:

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

comment:13 Changed 5 years ago by Paul McMillan

In [16878]:

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

comment:14 Changed 5 years ago by Jannis Leidel

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 Changed 5 years ago by Paul McMillan

In [16891]:

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

comment:16 Changed 5 years ago by Paul McMillan

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