Opened 4 years ago

Closed 4 years ago

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

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

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

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

Tests fail - need update

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

  • Patch needs improvement set

comment:4 Changed 4 years ago by julien

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by jocmeh

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

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

Better error message: updated tests as well.

comment:6 Changed 4 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 4 years ago by julien

  • 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 4 years ago by Wim Feijen <wim@…>

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

comment:8 Changed 4 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 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Great, thanks for your patience ;)

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

  • Summary changed from when logging in into the admin to updated error message when logging in into the admin fails because is_staff is False

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

  • Owner changed from nobody to wimfeijen

comment:12 Changed 4 years ago by PaulM

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

In [16872]:

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

comment:13 Changed 4 years ago by PaulM

In [16878]:

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

comment:14 Changed 4 years ago by jezdez

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 4 years ago by PaulM

In [16891]:

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

comment:16 Changed 4 years ago by PaulM

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

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

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