Make request available in authentication backends

[Carl Meyer]

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.

[Simon Charette]

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.

[Aleksej Manaev]

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
                )

[Tim Graham]

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.

[Aleksej Manaev]

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

[Tim Graham]

​PR with comments for improvement. Please uncheck "Patch needs improvement" after the pull request is updated.

[Tim Graham <timograham@…>]

In 4b9330cc:

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

[Marti Raudsepp]

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.

[Tim Graham]

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.

[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.

[Tim Graham]

Thanks, please check this ​PR.

[GitHub <noreply@…>]

In c31e7ab:

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

[Tim Graham <timograham@…>]

In 53f5dc10:

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

Backport of c31e7ab5a4b062225bc4f6b5cae065325dd30f1f from master

[Tim Graham <timograham@…>]

In 5e31be1b:

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

Per deprecation timeline.