Opened 16 years ago

Closed 14 years ago

Last modified 11 years ago

#8342 closed (fixed)

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

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

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 Phalip 16 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 by James Bennett, 16 years ago

Resolution: duplicate
Status: newclosed

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

comment:2 by Julien Phalip, 16 years ago

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 by Julien Phalip, 16 years ago

Resolution: duplicate
Status: closedreopened

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 by Julien Phalip, 16 years ago

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

by Julien Phalip, 16 years ago

Attachment: 8342.login.admin.error.diff added

comment:5 by Julien Phalip, 16 years ago

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'

in reply to:  3 comment:6 by James Bennett, 16 years ago

Resolution: wontfix
Status: reopenedclosed

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 by Julien Phalip, 16 years ago

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 by Julien Phalip, 16 years ago

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 by ramusus, 15 years ago

Cc: ramusus@… added

comment:10 by Daniel Gonzalez Gasull, 14 years ago

Cc: daniel@… added
Resolution: wontfix
Status: closedreopened

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 by James Bennett, 14 years ago

Resolution: wontfix
Status: reopenedclosed

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 by Karen Tracey, 14 years ago

milestone: 1.01.3
Resolution: wontfix
Status: closedreopened
Triage Stage: UnreviewedAccepted

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 by Karen Tracey, 14 years ago

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 by Ben Davis, 14 years ago

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 by Jannis Leidel, 14 years ago

Resolution: fixed
Status: reopenedclosed

(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 by Jannis Leidel, 14 years ago

(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 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:18 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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