[Django] #25187: Make request available in authentication backends
Django
----------------------------------------+------------------------
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
In some cases it is important for an authentication backend to have access
to additional data from the request; for instance, the request host (if
implementing host-based multi-tenancy, where each user should be
restricted to login only on their assigned host/site).
Currently, it's quite difficult to do this. The two options are a) making
the request thread-local, which is temptingly easy but Django otherwise
has resisted, or b) replacing/copying quite large chunks of code: all of
`AuthenticationForm.clean()`, `AuthenticationMiddleware`,
`auth.middleware.get_user` and `auth.get_user`.
My favorite option to resolve this takes advantage of the fact that
authentication backends are re-instantiated for every use (by
`load_backend`), and so the request could be passed into them as an
instantiation argument, making it always available as `self.request`. This
avoids needing to change the signatures of any methods on a backend,
keeping `self.request` unobtrusive for backends which don't need it. It
does mean that we'd need a deprecation path towards requiring custom
backends to accept this new initialization parameter; or else we'd need a
class attribute flag like `accepts_request` to explicitly mark backends
which want the request.
The other necessary change would be to pass the request from
`AuthenticationForm` into `contrib.auth.authenticate`, so it can pass it
on to the backend instantiation. We've backed ourselves into a bit of a
corner here with the `**credentials` signature of `authenticate`; we can't
add even an optional `request` argument without potentially breaking
existing backends that pass `request` as an actual credential. One option
would be a new `authenticate_with_request` function which would be used by
`AuthenticationForm`, but leave the existing `authenticate` signature as-
is for people who are using it directly.
Very open to alternative suggestions for how to resolve the basic issue
here.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
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 charettes):
* version: 1.8 => master
* stage: Unreviewed => Accepted
Comment:
Replying to [ticket:25187 carljm]:
> In some cases it is important for an authentication backend to have
access to additional data from the request; for instance, the request host
(if implementing host-based multi-tenancy, where each user should be
restricted to login only on their assigned host/site).
This definitely a common requirement for [https://github.com/charettes
/django-tenancy/blob/master/tenancy/auth/backends.py#L22-L32 multi-
tenancy] solutions based on Django whom actually rely on
[https://github.com/bernardopires/django-tenant-
schemas/blob/9d85507417dbe4031992af92c9233bedc4a5a8bf/tenant_schemas/middleware.py#L29
thread-local] to work around this limitation.
Since authentication is almost always bound to a request it makes sense to
add this feature even if most application don't deal with multi-tenancy.
This should also make per-backend authentication throttling easier to
implement.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:1>
Django
Reporter: carljm | 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 charettes):
* cc: charettes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:2>
Django
Reporter: carljm | Owner: johngian
Type: New feature | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by johngian):
* owner: nobody => johngian
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:3>
Django
Reporter: carljm | Owner:
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 johngian):
* owner: johngian =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:4>
Django
Reporter: carljm | Owner:
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
Comment (by AleksejManaev):
What do you think about my solution based on
[https://code.djangoproject.com/ticket/23155#comment:19 this] proposal?
{{{ #!python
def authenticate(request=None, **credentials):
"""
If the given credentials are valid, return a User object.
"""
for backend, backend_path in _get_backends(return_tuples=True):
try:
inspect.getcallargs(backend.authenticate, request,
**credentials)
user = backend.authenticate(request, **credentials)
except PermissionDenied:
# This backend says to stop in our tracks - this user
should not be allowed in at all.
break
except TypeError:
try:
inspect.getcallargs(backend.authenticate, **credentials)
user = backend.authenticate(**credentials)
except PermissionDenied:
# This backend says to stop in our tracks - this user
should not be allowed in at all.
break
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/25187#comment:5>
Django
Reporter: carljm | Owner:
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
Comment (by timgraham):
It's not obvious to me why the code duplication is necessary compared to
the simpler version in my comment on the other ticket but if it's needed
and you have some tests to show why, then please send a pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:6>
Django
Reporter: carljm | Owner:
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
Comment (by AleksejManaev):
The code duplication is necessary for distinguishing between a call with a
request parameter and without one.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:7>
Django
Reporter: carljm | Owner:
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 AleksejManaev):
* cc: AleksejManaev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:8>
Django
Reporter: carljm | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/6903 PR] with comments for
improvement. Please uncheck "Patch needs improvement" after the pull
request is updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:9>
Django
Reporter: carljm | Owner: Tim
| Graham <timograham@…>
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
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"4b9330ccc04575f9e5126529ec355a450d12e77c" 4b9330cc]:
{{{
#!CommitTicketReference repository=""
revision="4b9330ccc04575f9e5126529ec355a450d12e77c"
Fixed #25187 -- Made request available in authentication backends.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:10>
Django
Reporter: carljm | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by mlevental):
* cc: m.levental@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:11>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Marti Raudsepp):
Testing on 1.11b1, this change breaks code like django-cas-ng that
provided their own view to pass the `request` argument:
https://github.com/mingchen/django-cas-
ng/blob/master/django_cas_ng/views.py#L59-L61
If this can't be fixed in time for 1.11 then the release notes should
document clearly that `request` needs to be the first positional argument,
not a keyword argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:12>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Marti Raudsepp):
* cc: marti@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:13>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
It looks like `CASBackend.authenticate()` doesn't follow
[https://docs.djangoproject.com/en/stable/topics/auth/customizing
/#writing-an-authentication-backend the advice in the documentation], "The
`authenticate` method takes credentials as keyword arguments." Instead,
it's taking positional arguments.
I'm not against trying to maintain backwards compatibility for this case
but I'm not sure that it's a high priority. Are you willing to try to
write a patch? I'll attach a test that I started.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:14>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* Attachment "25187-positional-arg-test.diff" added.
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Marti Raudsepp):
> It looks like CASBackend.authenticate() doesn't follow the advice in
the documentation, "The authenticate method takes credentials as keyword
arguments." Instead, it's taking positional arguments.
After further thought about this, I think you're mistaken.
`CASBackend.authenticate()` allows passing arguments as positional as well
as keyword. Even if it were changed to `authenticate(self, ticket=None,
service=None, request=None)` the change would break CASBackend. If it were
changed to take keyword-only arguments, after the change Django would
still fail to pass it the `request` argument that previously worked.
The actual problem here is that this change breaks the contract. The
documentation quote you provided suggests that arguments are always passed
as keyword arguments, but now it requires `request` to be the first
positional argument.
If Django code was changed to `inspect.getcallargs(backend.authenticate,
request=request, **credentials)`, you would retain the previous contract
of always passing keyword arguments and it would fix this issue as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:15>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Thanks, please check this [https://github.com/django/django/pull/8103 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:16>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"c31e7ab5a4b062225bc4f6b5cae065325dd30f1f" c31e7ab]:
{{{
#!CommitTicketReference repository=""
revision="c31e7ab5a4b062225bc4f6b5cae065325dd30f1f"
Refs #25187 -- Fixed AuthBackend.authenticate() compatibility for
signatures that accept a request kwarg.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:17>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"53f5dc10cdc0c1fbeb48b2d98fbdd26c1996ec59" 53f5dc10]:
{{{
#!CommitTicketReference repository=""
revision="53f5dc10cdc0c1fbeb48b2d98fbdd26c1996ec59"
[1.11.x] Refs #25187 -- Fixed AuthBackend.authenticate() compatibility for
signatures that accept a request kwarg.
Backport of c31e7ab5a4b062225bc4f6b5cae065325dd30f1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:18>
Django
Reporter: Carl Meyer | Owner: Tim
| Graham <timograham@…>
Type: New feature | Status: closed
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5e31be1b96f60232e1e04140c5f0e33d8c2319f1" 5e31be1b]:
{{{
#!CommitTicketReference repository=""
revision="5e31be1b96f60232e1e04140c5f0e33d8c2319f1"
Refs #25187 -- Required the authenticate() method of authentication
backends to have request as the first positional argument.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25187#comment:19>