Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32255 closed New feature (wontfix)

User.has_perm should forward **kwargs to allow more flexibility in authentication backends

Reported by: Matteo Parrucci Owned by: Matteo Parrucci
Component: contrib.auth Version: 3.1
Severity: Normal Keywords: auth, django.contrib.auth, authentication, request, has_perm, has_perms, sites, django.contrib.sites
Cc: Florian Apolloner Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Matteo Parrucci)

The situation
I would like to check user permission based on the django.contrib.sites I'm in. To get the active site, django.contrib.sites needs the request so he can match the hostname. I'm using django-rules at the moment but It's quite generic.

The Problem
Not having the request in has_perm/has_perms I cannot check permissions based on the currently active site
The "call stack" detail is here: https://github.com/dfunckt/django-rules/issues/130

A possible solution
Adding **kwargs to has_perm and has_perms signatures would allow greater flexibility for the custom authentication backends leaving intact what we have now. In my case, for example, I could check permission in my django-rules predicates having the the request passed in and check if I have rights for the current site, but I think my case is quite limited and all new authentication backends would benefit from this change.

Change History (13)

comment:1 by Matteo Parrucci, 4 years ago

Description: modified (diff)
Summary: User.has_perm should forward *args and **kwargs to allow more flexibility in authentication backendsUser.has_perm should forward **kwargs to allow more flexibility in authentication backends

comment:2 by Matteo Parrucci, 4 years ago

I've implemented it in a demo project and it now looks like this:

Into django.contrib.auth.models.AbstractUser

    def has_perm(self, perm, obj=None, **kwargs):
        return _user_has_perm(self, perm, obj=obj, **kwargs)

    def has_perms(self, perm_list, obj=None, **kwargs):
        return all(self.has_perm(perm, obj, **kwargs) for perm in perm_list)

In the same module the checking permission function:

def _user_has_perm(user, perm, obj, **kwargs):
    """
    A backend can raise `PermissionDenied` to short-circuit permission checking.
    """
    for backend in get_backends():
        if not hasattr(backend, 'has_perm'):
            continue
        try:
            if backend.has_perm(user, perm, obj, **kwargs):
                return True
        except PermissionDenied:
            return False
    return False

Into django.contrib.auth.backends.* change the has_perm signature to the following leaving the implementation as is.

def has_perm(self, user_obj, perm, obj=None, **kwargs):

And it works for me now. It is a small change and I think it could be useful for others too. Basically I only added kwargs to the signatures and forwarded it to the auth backends.

Last edited 4 years ago by Matteo Parrucci (previous) (diff)

comment:3 by keshav pareek, 4 years ago

Owner: changed from nobody to keshav pareek
Status: newassigned

comment:4 by keshav pareek, 4 years ago

Owner: keshav pareek removed

comment:5 by Carlton Gibson, 4 years ago

Triage Stage: UnreviewedAccepted

Hey Matteo — could you put the proposed change into a PR so we can see the diff? Essentially, if it's not a breaking change then OK, otherwise it's difficult — much easier to see that in a PR. Thanks.

Seems reasonable from the description. I'll provisionally Accept on that basis. (Can't immediately see how adding **kwargs would be breaking change so …)

comment:6 by Matteo Parrucci, 4 years ago

Here it is the pull request:

https://github.com/django/django/pull/13785

It is somehow a breaking change in the meaning that all auth backends should change and also accept a **kwargs from now on.
I'm open to suggestions to avoid this.

Last edited 4 years ago by Matteo Parrucci (previous) (diff)

comment:7 by Jacob Walls, 4 years ago

Has patch: set
Needs tests: set

comment:8 by Jacob Walls, 4 years ago

Owner: set to Matteo Parrucci

comment:9 by Florian Apolloner, 4 years ago

Adding **kwargs to has_perm and has_perms signatures would allow greater flexibility for the custom authentication backends leaving intact what we have now.

But how would a 3rd party application that uses has_perm know which **kwargs to pass in? This is all nice and well if you control the code, but as soon as 3rd party apps (like the admin, even though there you can probably manually override the check) come into play you no longer get anything passed. What would you fall back to then?

What we really need would be to pass in some context (or make available) that can be populate from elsewhere (maybe already from a middleware). But to pass that down to the auth backends we would most likely need to use thread locals :/

in reply to:  9 comment:10 by Matteo Parrucci, 4 years ago

Replying to Florian Apolloner:

But how would a 3rd party application that uses has_perm know which **kwargs to pass in? This is all nice and well if you control the code, but as soon as 3rd party apps (like the admin, even though there you can probably manually override the check) come into play you no longer get anything passed. What would you fall back to then?

Ok, you are right on this, I was thinking it with a project-wide scope but thinking it in the reusable apps scope it makes no sense.

What we really need would be to pass in some context (or make available) that can be populate from elsewhere (maybe already from a middleware). But to pass that down to the auth backends we would most likely need to use thread locals :/

And what if there are no more kwargs and has_perm gets and forwards to the backends simply the request as context?

  • It would be more than enough to check the active site
  • Could be populated through middlewares in case we need something else.
  • In the admin and in the views that uses permissionmixin we always have the request to pass to has_perm

comment:11 by Florian Apolloner, 4 years ago

Cc: Florian Apolloner added

And what if there are no more kwargs and has_perm gets and forwards to the backends simply the request as context?

That would work, but would add strong coupling to the request cycle, which is imo rather ugly. And yes, we already have it for authenticate but I still don't like it -- although it might be the better option in that regard :/ And even if we were to do that, we'd need to adjust https://github.com/django/django/blob/master/django/contrib/auth/decorators.py otherwise you'd probably get the request passed in 10% of the cases.

comment:12 by Carlton Gibson, 4 years ago

Resolution: wontfix
Status: assignedclosed

Given the difficulties here (and on the PR) let's move this to wontfix — pending an acceptable solution, it doesn't look feasible. Thanks all.

comment:13 by Carlton Gibson, 4 years ago

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