Opened 2 years ago

Closed 2 weeks ago

Last modified 13 days ago

#23155 closed New feature (fixed)

Add request attr to user_login_failed signal

Reported by: anonymous Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: brian@…, moritz.sichert@…, gavinwahl+gh@…, Aleksej Manaev, m.levental@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

It would be convenient to request instance in the signal user_login_failed.
This will get the users data and make limitations, such as IP address.
What do you think?

Change History (23)

comment:1 Changed 2 years ago by Brian May

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

The user_logged_in and user_logged_out signals both have the request parameter, so having request for user_login_failed would make sense IMHO.

comment:2 Changed 2 years ago by Brian May

Cc: brian@… added

Implementing this looks not-trivial.

The authenticate() method at django.contrib.auth takes username and password parameters, but does not take a request parameter.

https://docs.djangoproject.com/en/dev/topics/auth/default/#django.contrib.auth.authenticate

This function sends the user_login_failed signal. Fixing this may require changing the API of authenticate().

comment:3 Changed 2 years ago by anonymous

Adding optional request attr to authenticate() function and according to Authentication backends would give more options and flexibility.
I believe that the framework should be improved and made ​​more flexible.

comment:4 Changed 2 years ago by Aymeric Augustin

Simply passing request to authenticate is backwards incompatible: every third-party authentication backend will be silently skipped because the arguments don't match.

comment:5 Changed 2 years ago by tomc

Version: 1.7-rc-2master

comment:6 Changed 2 years ago by Tim Graham

Aymeric, I think you mixed django.contrib.auth.authenticate() and the authenticate() method of each backend. We wouldn't have to pass request to the auth backends because the signal is sent in the former so it might be feasible.

comment:7 Changed 2 years ago by Brian May

The caller of django.contrib.auth.authenticate() would have to be updated to take this new parameter. That could be code outside Django.

We could make it an optional parameter, however then you don't actually get the benefits unless it is called correctly. New code that uses the new request parameter will get confused if used with older Django versions as the request parameter will become part of **credentials, and this will get passed through to every backend, like it or not.

The current code is:

  def authenticate(**credentials):                                                 
      for backend in get_backends():                                               
          try:                                                                     
              user = backend.authenticate(**credentials)                           
          [...]

comment:8 Changed 2 years ago by Brian May

Also **credentials is used to generate the user_login_failed signal.

Last edited 2 years ago by Brian May (previous) (diff)

comment:9 Changed 8 months ago by Gavin Wahl

One way I thought of to fix this is to add an optional _request kwarg to django.contrib.auth.authenticate. If provided, it would be passed into the call to user_login_failed. This would be backwards incompatible if anyone were using an authentication backend that used _request, which seems unlikely. Would a patch implementing this approach be accepted?

As it is user_login_failed isn't useful for what seems like its primary purpose -- logging suspected attacks, which would need the IP address to be useful at all.

comment:10 Changed 7 months ago by Tim Graham

I don't see any obvious problem with that. The underscore seems unnecessary to me unless you know of a case where there would be a collision.

comment:11 Changed 7 months ago by Gavin Wahl

It seems reasonable that someone has an auth backend that does already take the whole request as a credential, and I wouldn't want to break that. I'll send a pull request soon.

comment:13 Changed 7 months ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:14 Changed 7 months ago by Moritz Sichert

I think the underscore is not really necessary.

It might be reasonable that some people already pass the request to authenticate() but I don't think it's likely that anyone would use the request keyword for anything else than the actual request.

Another disadvantage for using the underscore is that if you have a backend that expects a request you'd have to write something like authenticate(request=request, _request=request).

I propose the request argument to be handled just like any other keyword argument, i.e. passing it along to the backend.

This has following advantages:

  • the authenticate(request=request, _request=request) syntax is not needed
  • there won't be any backwards compatibilities regarding authentication backends because if you passed the request to authenticate() before, it will still be passed to the backend, and if you didn't, the backend won't get any unexpected arguments
  • it is still possible to provide a backwards compatibility path, if needed, by first trying to call the backend with the request and if that fails because of a TypeError try again without it

What do you think?

comment:15 Changed 7 months ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

A deprecation path toward making request a required argument of backend.authenticate() makes sense to me and does seem cleaner in the long run. I guess that should be a separate ticket that precedes this one though.

comment:16 Changed 7 months ago by Moritz Sichert

Cc: moritz.sichert@… added

comment:17 Changed 6 months ago by Gavin Wahl

I'm unsure how to continue. What is the desired change?

comment:18 Changed 6 months ago by Gavin Wahl

Cc: gavinwahl+gh@… added

comment:19 Changed 6 months ago by Tim Graham

What I meant about a separate ticket for a deprecation to make request a required argument for authentication backends is something like this in authenticate():

try:
    inspect.getcallargs(backend.authenticate, request, **credentials)
except TypeError:
    try:
        inspect.getcallargs(backend.authenticate, **credentials)
    except TypeError:
        # This backend doesn't accept these credentials as arguments. Try the next one.
        continue
    else:
        warnings.warn(
            "Update authentication backend %s to accept a "
            "positional `request` argument." % backend
            RemovedInDjango20Warning
        )

comment:20 Changed 4 months ago by Tim Graham

I think #25187 could be used as that separate ticket.

comment:21 Changed 2 weeks ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In f0f3de3c:

Fixed #23155 -- Added request argument to user_login_failed signal.

comment:22 Changed 13 days ago by Aleksej Manaev

Cc: Aleksej Manaev added

comment:23 Changed 13 days ago by mlevental

Cc: m.levental@… added
Note: See TracTickets for help on using tickets.
Back to Top