#25187 closed New feature (fixed)
Make request available in authentication backends
Reported by: | Carl Meyer | Owned by: | |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette, Aleksej Manaev, m.levental@…, marti@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
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.
Attachments (1)
Change History (20)
comment:1 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 1.8 → master |
comment:2 by , 9 years ago
Cc: | added |
---|
comment:3 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 9 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:5 by , 8 years ago
What do you think about my solution based on this proposal?
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 )
comment:6 by , 8 years ago
It's not obvious to me why the code duplication is necessary compared to the simpler version in ticket:23155#comment:19, but if it's needed and you have some tests to show why, then please send a pull request.
comment:7 by , 8 years ago
The code duplication is necessary for distinguishing between a call with a request parameter and without one.
comment:8 by , 8 years ago
Cc: | added |
---|
comment:9 by , 8 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
PR with comments for improvement. Please uncheck "Patch needs improvement" after the pull request is updated.
comment:11 by , 8 years ago
Cc: | added |
---|
comment:12 by , 8 years ago
Testing on 1.11b1, this change breaks code like django-cas-ng that provided their own view to pass request
as a keyword argument:
https://github.com/mingchen/django-cas-ng/blob/master/django_cas_ng/backends.py#L19 (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.
comment:13 by , 8 years ago
Cc: | added |
---|
comment:14 by , 8 years ago
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.
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.
by , 8 years ago
Attachment: | 25187-positional-arg-test.diff added |
---|
comment:15 by , 8 years ago
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.
Replying to carljm:
This definitely a common requirement for multi-tenancy solutions based on Django whom actually rely on 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.