Opened 12 years ago

Closed 11 years ago

Last modified 8 years ago

#18616 closed New feature (fixed)

New auth signal: user_login_failed

Reported by: Michael Owned by: nobody
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: bradpitcher@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've implemented a new signal in Django called user_login_fail, in django.contrib.auth.

It is fired whenever an unsuccessful use of django.contrib.auth.authenticate() occurs, so will apply to all unsuccessful login attempts.

An example of use:

from django.contrib.auth.signals import user_login_fail

def login_attempt_failure_handler(sender, **kwargs):
	print "login attempt failure from %s: %r" % (sender, kwargs)

user_login_failure.connect(login_attempt_failure_handler)

This would then print on the console while running the development server:

login attempt failure from django.contrib.auth: {'credentials': {'username': u'michael', 'password': u'notmypassword'}, 'signal': <django.dispatch.dispatcher.Signal object at 0x1ba58d0>}

I'm aware at the moment that this passes back the password, however the authenticate method takes in kwargs for it's authentication, and could include some extra information needed when notifying the administrator (such as a login realm).

The other issue is that this has no idea about what is the sender or the request associated with this signal. I'm unsure of whether this could be fixed in the signal (and present some better information), or just delegate this responsibility to the use of the traceback module.

Patch / pull request is incoming.

Change History (19)

comment:1 by Michael, 12 years ago

Pull request with patch: https://github.com/django/django/pull/201

Tests pass with SQLite. I've added an additional test to make sure this signal is fired correctly.

comment:2 by anonymous, 12 years ago

Great idea. I manually tested this to work for me too, with the minor exception that the last line of code should be

user_login_fail.connect(login_attempt_failure_handler)

comment:3 by Michael, 12 years ago

Summary: New auth signal: user_login_failNew auth signal: user_login_failed

Per the Github pull request comment by Brad Pitcher, I've changed the name of the signal to user_login_failed to match the verb tense of the other signals.

Updated the code and the tests, and checks out.

comment:4 by Michael, 12 years ago

Needs documentation: set

comment:5 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

Yes, I've needed this myself, mostly for logging or security.

comment:6 by Brad Pitcher, 12 years ago

I wrote some documentation for this in another pull request:
https://github.com/django/django/pull/204

comment:7 by Michael, 12 years ago

Brad, the documentation looks good, however the use of the sender parameter isn't correct there (it appears copy-pasted from the logout signal?)

The sender parameter sends the name of the auth module. Because the login failure isn't tied directly to a request (authenticate doesn't provide such a parameter), it doesn't know about where it came from.

This comes back to my note in the OP that I'm unsure if this should be something fixed in the signal or if the traceback module is a better option. Or perhaps the signal should just set the sender to None.

comment:8 by Brad Pitcher, 12 years ago

Thanks for the catch. I updated the documentation for the sender parameter in https://github.com/brad/django/tree/ticket_18616_docs

comment:9 by Brad Pitcher, 12 years ago

Needs documentation: unset

comment:10 by Jannis Leidel, 12 years ago

Triage Stage: AcceptedReady for checkin

Seems like https://github.com/django/django/pull/201 is the latest code. Please make sure to always point to the correct pull request here!

comment:11 by Michael, 12 years ago

I've updated my pull request now (201) with Brad's documentation fix mentioned.

comment:12 by Brad Pitcher, 12 years ago

Cc: bradpitcher@… added

comment:13 by Michael, 12 years ago

I've now merged django/master into my pull request.

comment:14 by Preston Holmes, 11 years ago

I've reviewed, merged locally, verified docs, tests and ready to land, but have asked Paul to weigh in on the idea of sending credentials in the signal. While a project author should be able to fully control what listeners register for the signal, I'd feel better with Paul's quick opinion on it.

comment:15 by Paul McMillan, 11 years ago

I'd strongly prefer that we didn't send the password in a signal. I realize that this could be (ab)used for things like "you just tried to log in with your mother's maiden name, and we've switched to requiring the name of your first dentist!", or it could be used slightly more legitimately to chain into some other kind of system that acts kinda like a backend but not really. Those use cases should really be their own auth backend. I think this is primarily useful for logging (and acting on) failed login attempts. In that case, the actual password used probably shouldn't be passed along.

As the original poster said, since the credentials are a dict we don't know in advance which field is the password (or otherwise sensitive). Can we re-use the filtering system we already have in place to remove passwords from tracebacks?

comment:16 by Preston Holmes, 11 years ago

It wouldn't take much to use a filtering system like that used for debug views.

django.debug.views.decorators & django.views.debug contain the pattern

If we were to do that though, it would seem that the backend should specify what the "sensitive credentials" are - but seems a bit overkill for this signal, and pretty meaningless as a general backend attribute without some clear definition of what contexts a credential would be "sensitive".

I think the best middle ground would be to use a regex along the lines of:

https://github.com/django/django/blob/54c81a1c936f3682e3405d6737958fdffa39bed9/django/views/debug.py#L20

and strip any key from the dictionary that matches.

This along with a note in the docs seems like it should cover most foot shooting situations.

comment:17 by Preston Holmes <preston@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 7cc4068c4470876c526830778cbdac2fdfd6dc26:

Fixed #18616 -- added user_login_fail signal to contrib.auth

Thanks to Brad Pitcher for documentation

comment:18 by Cristiano Coelho, 8 years ago

Sorry for writing to such an old ticket but I couldn't find anything else related to this.
Is there any option for a signal to be triggered as well when an account that's not an admin attempts to access the admin page? As right now it 'almost' looks like a failed log in attempt, but it isn't.

comment:19 by Tim Graham, 8 years ago

Tickets aren't an appropriate place to ask usage questions. Please use support channels instead.

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