Opened 3 years ago

Closed 2 years ago

#18616 closed New feature (fixed)

New auth signal: user_login_failed

Reported by: micolous Owned by: nobody
Component: contrib.auth Version: master
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 (17)

comment:1 Changed 3 years ago by micolous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 3 years ago by anonymous

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 Changed 3 years ago by micolous

  • Summary changed from New auth signal: user_login_fail to New 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 Changed 3 years ago by micolous

  • Needs documentation set

comment:5 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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

comment:6 Changed 3 years ago by bradpitcher

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

comment:7 Changed 3 years ago by micolous

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 Changed 3 years ago by bradpitcher

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

comment:9 Changed 3 years ago by bradpitcher

  • Needs documentation unset

comment:10 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready 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 Changed 3 years ago by micolous

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

comment:12 Changed 3 years ago by bradpitcher

  • Cc bradpitcher@… added

comment:13 Changed 3 years ago by micolous

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

comment:14 Changed 3 years ago by ptone

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

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 Changed 3 years ago by ptone

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 Changed 2 years ago by Preston Holmes <preston@…>

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

In 7cc4068c4470876c526830778cbdac2fdfd6dc26:

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

Thanks to Brad Pitcher for documentation

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