Opened 7 years ago

Closed 5 years ago

Last modified 2 years ago

#8342 closed (fixed)

The admin wrongly assumes you can't login with your email

Reported by: julien Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords:
Cc: ramusus@…, daniel@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

On one of my sites, I have a custom authentication backend which allows you to login with your email, and just your email (the username attribute is basically ignored in the process).
It works fine, you can login both on the front-end site and on the admin site with your email.

However, if you mistype the email address or try to login with an email that doesn't exist, in the admin, then you get the message "Usernames cannot contain the '@' character.".

I don't know what's the best approach to fix this. I report now and will try to think of a patch.

This is something that I think is important to fix before 1.0, as that error message is quite misleading.

Attachments (1)

8342.login.admin.error.diff (3.5 KB) - added by julien 7 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 7 years ago by ubernostrum

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #7591, which was marked wontfix by one of Django's lead developers.

comment:2 Changed 7 years ago by julien

Sure.. but this ticket was not to include email authentication in core, like in #7591, but to make customization a bit more flexible. The admin delegates authentication to custom backends but then wrongly assumes it should be a certain way, I maintain that. I believe the right approach would have been to provide some overridable hook to customize the display of error messages. Anyway...

comment:3 follow-up: Changed 7 years ago by julien

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I don't think this is a duplicate of #7591. #7591 asked to include email authentication in core, whereas this ticket is asking for more flexibility in customisation.

The admin willingly delegates authentication to whatever backend you've set up:

user = authenticate(username=username, password=password)

That's great, but then why assume that you can't authenticate with an email? I understand that this is the default behaviour, but it should be made easy to override that behaviour.

If you decide to wontfix this, it would be appreciated if you could give some pointers to other clean ways to achieve this.

comment:4 Changed 7 years ago by julien

  • Has patch set

The attached patch adds a hook for displaying a custom error message when login fails. You can then simply override it to customise it to your heart's content:

class CustomAdminSite(AdminSite):
    def get_login_error_message(self, request, is_authenticated, username, password):
        return 'blah'

admin.site = CustomAdminSite()

Changed 7 years ago by julien

comment:5 Changed 7 years ago by julien

Just another thought on the patch. Would it be best to send a boolean is_authenticated (as in current patch) or to send the actual user. I suspect there might be cases where you'd like to get some information from the user to display a custom message:

class CustomAdminSite(AdminSite):
    def get_login_error_message(self, request, user, username, password):
        if user is not None:
            return 'blah: %s' % (user.whatever)
        else:
            return 'blah'

comment:6 in reply to: ↑ 3 Changed 7 years ago by ubernostrum

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

Replying to julien:

That's great, but then why assume that you can't authenticate with an email? I understand that this is the default behaviour, but it should be made easy to override that behaviour.

It is easy to override that behavior. Just override AdminSite.login(), which you'll probably want to do anyway if you're using a non-default authentication scheme.

comment:7 Changed 7 years ago by julien

Fair enough for the wontfix. Overriding AdminSite.login() is not what I call a clean way though. That's still 56 lines of code to hack... On a final note, that view does already allow non-default authentication schemes. What it does wrong is the error message it displays when login fails...

comment:8 Changed 7 years ago by julien

I see that #8878 was closed as duplicate.

Hmmm... the more I think about this, the more I really think this should be fixed. The current behaviour is just wrong.
Your suggestion of overriding AdminSite.login() defeats the purpose of custom authentication backends altogether.

The fact is: email authentication is becoming increasingly popular and inevitably a growing number of users are going to get hit by this. Again, I'm not saying email authentication should be in core, I'm saying the admin should not make any assumption about the way you're supposed to login. The admin should make it easy and clean to override the behaviour.

comment:9 Changed 6 years ago by ramusus

  • Cc ramusus@… added

comment:10 Changed 5 years ago by djansoft

  • Cc daniel@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

Is there any way to upvote tickets? If I could I would upvote this one. There are so many websites using the email address as username. The '@' symbol should be valid by default.

What's wrong with julien's code?

comment:11 Changed 5 years ago by ubernostrum

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

Per the docs, when you disagree with a wontfix the thing to do is take it to the django-developers mailing list to try to build consensus for your view, not to just reopen the ticket.

comment:12 Changed 5 years ago by kmtracey

  • milestone changed from 1.0 to 1.3
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

Actually this needs to be fixed, since r12634 has changed the rules for usernames to allow '@'. That message, which is still present in current trunk, is flat-out wrong now, even if you are not doing anything funky to allow login with email address instead of username.

comment:13 Changed 5 years ago by kmtracey

  • Patch needs improvement set

See #13928 and note that same code/message is currently present in at least two places. Both needs to be fixed.

comment:14 Changed 5 years ago by bendavis78

Honestly, I don't understand why there's any validation on the log-in form other than to authenticate the given credentials. That kind of validation should only be done when creating or modifying a user, not when logging in. In fact, best security practices dictate that the only error message you should show when logging in is "the username and/or password is incorrect".

comment:15 Changed 5 years ago by jezdez

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

(In [14769]) Fixed #8342 -- Removed code from the admin that assumed that you can't login with an email address (nixed by r12634). Also refactored login code slightly to be DRY by using more of auth app's forms and views.

comment:16 Changed 5 years ago by jezdez

(In [14773]) [1.2.X] Fixed #8342 -- Removed code from the admin that assumed that you can't login with an email address (nixed by r12634).

Backport from trunk (r14769).

comment:17 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:18 Changed 2 years ago by Tim Graham <timograham@…>

In 61ecb5f48a4732f1471f858c64904ec1c4763925:

Fixed #20855 -- Added documentation of current_app and extra_context params to django.contrib.auth views

refs #5298 and refs #8342

comment:19 Changed 2 years ago by Tim Graham <timograham@…>

In 77293f9354774e2631087935f1550c674093c433:

[1.6.x] Fixed #20855 -- Added documentation of current_app and extra_context params to django.contrib.auth views

refs #5298 and refs #8342

Backport of 61ecb5f48a from master

comment:20 Changed 2 years ago by Tim Graham <timograham@…>

In 145e0a14f0c098d7bbf2bfb593f7b6cfe1b02d72:

[1.5.x] Fixed #20855 -- Added documentation of current_app and extra_context params to django.contrib.auth views

refs #5298 and refs #8342

Backport of 61ecb5f48a from master.

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