[Django] #23155: Add request attr to user_login_failed signal
Django
------------------------------+----------------------
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.7-rc-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+----------------------
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23155>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by brian):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:1>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by brian):
* cc: brian@… (added)
Comment:
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().
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:2>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:3>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Simply passing `request` to `authenticate` is backwards incompatible:
every third-party authentication backend will be silently skipped because
the arguments don't match.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:4>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomc):
* version: 1.7-rc-2 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:5>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:6>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by brian):
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)
[...]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:7>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by brian):
Also `**credentials`` is used to generate the user_login_failed signal.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:8>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gavinwahl):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:9>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:10>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gavinwahl):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:11>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gavinwahl):
* has_patch: 0 => 1
Comment:
<https://github.com/django/django/pull/6158>
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:12>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:13>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by MoritzS):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:14>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:15>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by MoritzS):
* cc: moritz.sichert@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:16>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by gavinwahl):
I'm unsure how to continue. What is the desired change?
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:17>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by gavinwahl):
* cc: gavinwahl+gh@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:18>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
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()`:
{{{ #!python
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
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:19>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I think #25187 could be used as that separate ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:20>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f0f3de3c96694fd3602541fa6074930667509ac8" f0f3de3c]:
{{{
#!CommitTicketReference repository=""
revision="f0f3de3c96694fd3602541fa6074930667509ac8"
Fixed #23155 -- Added request argument to user_login_failed signal.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:21>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by AleksejManaev):
* cc: AleksejManaev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:22>
Django
Reporter: anonymous | Owner: nobody
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by mlevental):
* cc: m.levental@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23155#comment:23>