Opened 3 weeks ago

Closed 13 days ago

#36837 closed New feature (needsnewfeatureprocess)

Client.force_login won't work for permission-only backends inheriting from BaseBackend

Reported by: Christian Hartung Owned by:
Component: contrib.auth Version: 6.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is a follow up to #27542.

Take the MagicAdminBackend from the documentation. It is an Authentication Backend that inherits from BaseBackend and overrides the has_perm method. This leaves it with the default get_user method returning None, making it eligible for Client.force_login.

Some libraries like django-rules and django-guardian work around this by not inheriting from BaseBackend, and not adding a get_user method, but:

  • this goes against the documentation, which states that both get_user and authenticate are required, and permission related methods are optional
  • now they don't automatically gain the async functionality, making it harder for users to integrate those libraries in an async context

I don't know a good way around this. Some ideas are:

Add some introspection capabilities to BaseBackend
I was think something like:

class BaseBackend:
    supports_authentication = True
    supports_permissions = True

class ObjectPermissionBackend(BaseBackend):
    supports_authentication = False
    supports_permissions = True

This way we could change (a)authenticate and force_login to skip backends that do not support autentication, and (a)has_perm to skip backends that do not support permissions.

def _get_compatible_backends(request, **credentials):
    for backend, backend_path in _get_backends(return_tuples=True):
        if not getattr(backend, "supports_authentication", True):
            continue

        ... Check signature and so on

def _user_has_perm(user, perm, obj):
    for backend in auth.get_backends():
        if not getattr(backend, "supports_permissions", True):
            continue

        if not hasattr(backend, "has_perm"):
            continue
        
        ... Check permission

Call get_user inside force_login
Use the first backend that returns something. This was suggested on #27542, but it could be really slow and might introduce some side effects

Updating the documentation
Simply update the documentation stating that:

  • get_user required only if the backend can authenticate users (authenticate returns something)
  • Client.force_login selects the first backend that has a get_user.

Change History (3)

comment:1 by VIZZARD-X, 3 weeks ago

Has patch: set

comment:2 by VIZZARD-X, 3 weeks ago

I have opened a Pull Request with the fix and regression tests:
https://github.com/django/django/pull/20485

  • The fix ensures Client.force_login and AsyncClient.aforce_login verify that the selected backend can actually retrieve a user (via get_user) before using it. This resolves the crash/incompatibility with permission-only backends like django-rules.

comment:3 by Jacob Walls, 13 days ago

Has patch: unset
Resolution: needsnewfeatureprocess
Status: newclosed
Type: Cleanup/optimizationNew feature

Another option is to statically declare that get_user() returns None without having to call it to find out. Not so ordinary, but we have a precedent in Django of doing this, see the doc'd way to declare a dependency on the natural_key method.

Then with a small-scoped change in force_login(), a rules backend could do:

get_user.noop = True

It's a lot like your option 1, but it seems like a smaller lift.

I don't think the linked PR (which calls get_user()) is acceptable, for the reason you articulate (introduces db queries, defeating the point of the test utility).

A tiny hint for the test client seems like a not disruptive change for the auth backends. It is new API, though, so we need a new features discussion to make sure we're not missing anything. It would also be nice to know if django-rules and django-guardian are willing to use the feature we build for them here.

My recommendation is to kick off a new-features discussion. Thanks.

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