Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#25187 closed New feature (fixed)

Make request available in authentication backends

Reported by: Carl Meyer Owned by: Tim Graham <timograham@…>
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)

25187-positional-arg-test.diff (1.7 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (20)

in reply to:  description comment:1 by Simon Charette, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.8master

Replying to 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 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.

comment:2 by Simon Charette, 9 years ago

Cc: Simon Charette added

comment:3 by John Giannelos, 9 years ago

Owner: changed from nobody to John Giannelos
Status: newassigned

comment:4 by John Giannelos, 9 years ago

Owner: John Giannelos removed
Status: assignednew

comment:5 by Aleksej Manaev, 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 Tim Graham, 8 years ago

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.

Version 0, edited 8 years ago by Tim Graham (next)

comment:7 by Aleksej Manaev, 8 years ago

The code duplication is necessary for distinguishing between a call with a request parameter and without one.

comment:8 by Aleksej Manaev, 8 years ago

Cc: Aleksej Manaev added

comment:9 by Tim Graham, 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:10 by Tim Graham <timograham@…>, 8 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 4b9330cc:

Fixed #25187 -- Made request available in authentication backends.

comment:11 by mlevental, 8 years ago

Cc: m.levental@… added

comment:12 by Marti Raudsepp, 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.

Last edited 8 years ago by Marti Raudsepp (previous) (diff)

comment:13 by Marti Raudsepp, 8 years ago

Cc: marti@… added

comment:14 by Tim Graham, 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 Tim Graham, 8 years ago

comment:15 by Marti Raudsepp, 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.

comment:16 by Tim Graham, 8 years ago

Thanks, please check this PR.

comment:17 by GitHub <noreply@…>, 8 years ago

In c31e7ab:

Refs #25187 -- Fixed AuthBackend.authenticate() compatibility for signatures that accept a request kwarg.

comment:18 by Tim Graham <timograham@…>, 8 years ago

In 53f5dc10:

[1.11.x] Refs #25187 -- Fixed AuthBackend.authenticate() compatibility for signatures that accept a request kwarg.

Backport of c31e7ab5a4b062225bc4f6b5cae065325dd30f1f from master

comment:19 by Tim Graham <timograham@…>, 7 years ago

In 5e31be1b:

Refs #25187 -- Required the authenticate() method of authentication backends to have request as the first positional argument.

Per deprecation timeline.

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