pytest-django icon indicating copy to clipboard operation
pytest-django copied to clipboard

admin_client / admin_user fixtures are dependent on AUTHENTICATION_BACKEND order

Open TauPan opened this issue 3 years ago • 4 comments

I just upgraded to pytest-django 4.1.0 from 3.10.0 and a lot of my tests broke which rely on admin_client / admin_user.

It turns out that the code in https://github.com/pytest-dev/pytest-django/commit/79b7754669660543b593bdad471e73a9dabc04ed uses the first backend that has get_user, which may return None.

In my case I have base settings in a file settings_global.py and recommend to import that from the settings.py in production via:

from .settings_global import *  # noqa

and modify settings after that line.

settings_global.py contains:

AUTHENTICATION_BACKENDS = (
    'django.contrib.auth.backends.RemoteUserBackend',
    'guardian.backends.ObjectPermissionBackend',
)

Using RemoteUserBackend is recommended for production (using kerberos, AD, x509 client certs or whatever the apache authentication mechanism is deemed appropriate for the production site).

So for settings_test.py I have:

AUTHENTICATION_BACKENDS += (  # noqa
    'django.contrib.auth.backends.ModelBackend',  # this is default'
)

(Of course it's explicitly discouraged to use ModelBackend for production.

This results in RemoteUserBackend being used first for tests, which are supposed to use ModelBackend.

The test failures are caused by user being None, which the views automatically convert to the Guardian AnonymousUser instance, which of course has no permissions.

Of course the simple fix is:

AUTHENTICATION_BACKENDS = (  # noqa
    'django.contrib.auth.backends.ModelBackend',  # this is default'
) + AUTHENTICATION_BACKENDS  # noqa

So ModelBackend is used first for tests.

I'm not sure if that's intended, as the tests worked with 3.10.0.

At least I'd suggest that this should be documented.

TauPan avatar Jan 04 '21 10:01 TauPan

I forgot to mention that the apiclient from django-rest-framework also has a method force_authenticate which doesn't behave like that, so it might be worth looking at their implementation.

TauPan avatar Jan 04 '21 10:01 TauPan

So IIUC, login calls the regular authenticate function which iterates over all AUTHENTICATION_BACKENDS until one returns a user or PermissionDenied, and in your case it would reach the ModelBackend at the end and succeed. Also login fails silently (returns False which we don't check).

But force_login (without an explicit backend argument) just uses the first AUTHENTICATION_BACKEND for some reason. But I don't quite follow how it actually fails. From what I can see the request.user would still be the admin user. I don't quite follow the Guardian AnonymousUser user bit, can you explain further?

Of course the simple fix is:

Seems to me like it's the right thing to do, regardless of the pytest-django change - saves the tests the two unsuccessful backend iterations.

I forgot to mention that the apiclient from django-rest-framework also has a method force_authenticate which doesn't behave like that, so it might be worth looking at their implementation.

They seemed to have achieved it through a (IMO) ugly hack - they set a private field on the request and the actual non-test code special cases it -- eek.

bluetech avatar Jan 04 '21 21:01 bluetech

Sorry for the late reply.

So IIUC, login calls the regular authenticate function which iterates over all AUTHENTICATION_BACKENDS until one returns a user or PermissionDenied, and in your case it would reach the ModelBackend at the end and succeed. Also login fails silently (returns False which we don't check).

I didn't really look into that.

But force_login (without an explicit backend argument) just uses the first AUTHENTICATION_BACKEND for some reason. But I don't quite follow how it actually fails.

Well, this is the code from django 2.2.17:

    def force_login(self, user, backend=None):
        def get_backend():
            from django.contrib.auth import load_backend
            for backend_path in settings.AUTHENTICATION_BACKENDS:
                backend = load_backend(backend_path)
                if hasattr(backend, 'get_user'):
                    return backend_path
        if backend is None:
            backend = get_backend()
        user.backend = backend
        self._login(user, backend)

The inner function get_backend will return the first backend that has get_user and use that. In the case of my broken configuration that would be RemoteUserBackend. That is the only backend being tried by login in that case.

The result is None.

I don't know how admin_client was logged in before.

From what I can see the request.user would still be the admin user. I don't quite follow the Guardian AnonymousUser user bit, can you explain further?

See the two blue boxes in https://django-guardian.readthedocs.io/en/stable/configuration.html#configuration but I really don't think this is relevant, since the result from django's force_login is None unexpectedly.

Of course the simple fix is:

Seems to me like it's the right thing to do, regardless of the pytest-django change - saves the tests the two unsuccessful backend iterations.

Probably yes. Note that there are no backend iterations. The first backend is tried only.

I forgot to mention that the apiclient from django-rest-framework also has a method force_authenticate which doesn't behave like that, so it might be worth looking at their implementation.

They seemed to have achieved it through a (IMO) ugly hack - they set a private field on the request and the actual non-test code special cases it -- eek.

I'd agree that special casing the non test code is indeed calling for trouble. On the other hand I don't mind slapping attributes on objects, but probably I've learned that bad habit from DRF (they do it in quite a lot of places).

TauPan avatar Jan 12 '21 07:01 TauPan

So I've run into the same issue. It seems that my test project settings like so leads to many 302 login redirects when trying to use the admin_client to access admin URLs:

AUTHENTICATION_BACKENDS = [
    "ariadne_jwt.backends.JSONWebTokenBackend",
    "django.contrib.auth.backends.ModelBackend",
]

Changing the order to be like the following allowed the tests to start passing again:

AUTHENTICATION_BACKENDS = [
    "django.contrib.auth.backends.ModelBackend",
    "ariadne_jwt.backends.JSONWebTokenBackend",
]

The ariadne-jwt backend is here: https://github.com/Usama0121/ariadne-jwt/blob/master/ariadne_jwt/backends.py

I think the solution would be to modify the admin_client fixture so that users could specify which backend to use, but this would unfortunately be a breaking change as it would change the way the fixture has to work. Below is an example of how the fixture would have to change and how the usage would change:

# fixtures.py
@pytest.fixture()
def admin_client(
    db: None,
    admin_user,
) -> "django.test.client.Client":
    """A Django test client logged in as an admin user."""
    def _inner(backend=None):
        from django.test.client import Client

        client = Client()
        client.force_login(admin_user, backend=backend)
        return client

    return _inner

# my_tests.py
def test_foo(admin_client):
    client = admin_client(backend=my_cool_backend)
    response = admin_client.get('/admin/')
    assert response.status_code == 200

There may be some other solution that is not a breaking change, however.

ryancausey avatar Feb 10 '22 04:02 ryancausey