Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call Django's authenticate function with the request object #5295

Merged
merged 1 commit into from
Oct 5, 2017

Conversation

jgeerds
Copy link
Contributor

@jgeerds jgeerds commented Jul 25, 2017

Description

As of Django 1.11 the authenticate function accepts a request as an
additional argument. This commit fixes compatibility between newer Django
versions and custom authentication backends which already depend on the request
object.

See also:

Django 1.11 release

authenticate() now passes a request argument to the authenticate() method of
authentication backends. Support for methods that don’t accept request as the
first positional argument will be removed in Django 2.1.
  • tox (with all python versions) succeeds

@tomchristie
Copy link
Member

Great stuff, thanks!
Could you explain if and how this works pre-1.11?
The call to authenticate still includes request=... in that case, will that still work okay, and are there cases in which it might fail (eg. custom auth backends?)

Thanks again, looks good.

@jgeerds
Copy link
Contributor Author

jgeerds commented Jul 26, 2017

Django 1.8.x, 1.9.x and 1.10.x releases use the following function signature:

def authenticate(**credentials):

This implies that it is not possible to use positional keyword arguments with this function:

In [7]: def authenticate(**credentials):
   ...:     print(credentials)
   ...: 

In [8]: authenticate('foo', 'bar')
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-8-b0a7d48cf507> in <module>()
----> 1 authenticate('foo', 'bar')

TypeError: authenticate() takes 0 positional arguments but 2 were given

In [9]: authenticate(username='foo', password='bar')
{'password': 'bar', 'username': 'foo'}

In [10]: 

I think this is also the reason why Django 1.11 introduced request as a keyword argument as well (at least until 2.1 comes out). Passing an extra request parameter works fine with pre-1.11:

In [12]: def authenticate(**credentials):
    ...:     print(credentials)
    ...: 

In [13]: authenticate(request='req', username='foo', password='bar')
{'request': 'req', 'password': 'bar', 'username': 'foo'}

However, from a custom auth backend perspective I think it might fail because of this:

    for backend, backend_path in _get_backends(return_tuples=True):
        try:
            inspect.getcallargs(backend.authenticate, **credentials)
        except TypeError:
            # This backend doesn't accept these credentials as arguments. Try the next one.
continue

I'll create a wrapper function in compat.py to handle old versions

As of Django 1.11 the `authenticate` function accepts a request as an
additional argument. This commit fixes compatibility between newer Django
versions and custom authentication backends which already depend on the request
object.

See also:

[Django 1.11 release](https://docs.djangoproject.com/en/1.11/releases/1.11/)

```
authenticate() now passes a request argument to the authenticate() method of
authentication backends. Support for methods that don’t accept request as the
first positional argument will be removed in Django 2.1.
```
@tomchristie tomchristie added this to the 3.7.0 Release milestone Oct 5, 2017
@tomchristie
Copy link
Member

Milestoning for 3.7 - looks like we ought to have this in now.
Anyone else want to review?

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants