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_userandauthenticateare required, and permission related methods are optional - now they don't automatically gain the
asyncfunctionality, making it harder for users to integrate those libraries in anasynccontext
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_userrequired only if the backend can authenticate users (authenticatereturns something)Client.force_loginselects the first backend that has aget_user.
Change History (3)
comment:1 by , 3 weeks ago
| Has patch: | set |
|---|
comment:2 by , 3 weeks ago
comment:3 by , 13 days ago
| Has patch: | unset |
|---|---|
| Resolution: | → needsnewfeatureprocess |
| Status: | new → closed |
| Type: | Cleanup/optimization → New 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.
I have opened a Pull Request with the fix and regression tests:
https://github.com/django/django/pull/20485
Client.force_loginandAsyncClient.aforce_loginverify that the selected backend can actually retrieve a user (viaget_user) before using it. This resolves the crash/incompatibility with permission-only backends likedjango-rules.