Opened 4 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#37021 closed New feature (fixed)

Add a helper function to return the permission string representation to be used with `user.has_perm()`

Reported by: Mariatta Owned by: Mariatta
Component: contrib.auth Version: 6.0
Severity: Normal Keywords: codename, has_perm
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Mariatta)

When checking user's permission using user.has_perm(), I need to pass a string in the format of {Permission.content_type.app_label}.{Permission.codename}

However, the Permission object itself does not provide an easy way to return this string.
The __str__ method of the Permission class returns a string of content_type | name.

I think it would be great if the Permission class can provide the string representation that will allow us to check the permission without having to manually construct it ourselves.

My desired outcome is to be able to do the following.

user_permissions = Permission.objects.filter(...) 
for perm in user_permissions:
    if user.has_perm(perm.perm_string()):
        # let them do stuff

Currently I am doing it as follows:

if user.has_perm(f"{perm.content_type.app_label}.{perm.codename}"):
        # let them do stuff

Proposal:

To add a function within Permission class:

def perm_string(self): # feel free to suggest other name
    return f"{self.content_type.app_label}.{self.codename}"

If you agree, I would like to try to create the PR for this.

Change History (13)

comment:1 by Mariatta, 4 weeks ago

Description: modified (diff)

comment:2 by Jacob Walls, 4 weeks ago

Keywords: codename has_perm added
Resolution: needsnewfeatureprocess
Status: newclosed

Thanks for the idea. I'm very tempted to accept this, because the lack of this feature can lead to the temptation to hard-code the app label. (#26547 points out this is incorrect if the AppConfig overrides the label).

However, although it's just a one-liner, forcing the user to write it themselves will probably alert them to the need to JOIN the content types, or otherwise do an annotation, to avoid an N+1 query problem.

As silly as it might sound to go through the new feature process for this, I think getting some confirming views there would be useful. Can I ask you to repeat your request there, and mention the temptation to do it the "wrong" way by hard-coding the app label?

comment:3 by Jacob Walls, 4 weeks ago

I forgot to link you to the new-features repo, which is only about a year old: https://github.com/django/new-features/issues

comment:4 by Jacob Walls, 4 weeks ago

Resolution: needsnewfeatureprocess
Status: closednew
Triage Stage: UnreviewedAccepted

Thanks for doing that! New-features discussion is here, reception looks positive all around, so I'm happy to accept. Thanks for offering a PR!

comment:5 by Jacob Walls, 4 weeks ago

Owner: set to Mariatta
Status: newassigned

comment:7 by Mariatta, 4 weeks ago

Has patch: set

comment:8 by JaeHyuckSa, 4 weeks ago

Patch needs improvement: set

comment:9 by Jacob Walls, 3 weeks ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Jacob Walls <jacobtylerwalls@…>, 3 weeks ago

Resolution: fixed
Status: assignedclosed

In e2abe32:

Fixed #37021 -- Added Permission.user_perm_str property.

For use in checking user permissions via has_perm().

Co-authored-by: 사재혁 <jaehyuck.sa.dev@…>

comment:11 by Adam Johnson, 3 weeks ago

Hey, not to discourage here, but there was a lot of support for the alternative suggestion in the new-features discussion: https://github.com/django/new-features/issues/137#issuecomment-4182093695 . The alternative is to extend User.has_perm() to accept Permission objects directly, so users don't need to think about formatting at all.

Here’s a PoC PR with some tests; https://github.com/django/django/pull/21071

I think this is a better approach, and we could look into rolling back the new property in favour of it…

comment:12 by Mariatta, 3 weeks ago

Yeah I wasn't sure if modifying User.has_perm to accept a Permission class would require a different feature request or not. My original proposal was scoped to adding the helper function/property. Therefore I went ahead to implement it after Jacob accepted the issue.

Can't we keep both?

No hard feelings even if you decide to revert this change :)

comment:13 by Jacob Walls, 3 weeks ago

I just figured we'd use this new property if someone implemented accepting Permissions directly. But I see your PR inlines the string interpolation in has_perm(). I was assuming we had other methods in builtin backends (and thus 3rd party backends as well) that would find the property useful?

https://github.com/django/django/blob/e2abe321a6f1370e05c1a89a742125c9eafcac8c/django/contrib/auth/backends.py#L121

(Ah, but looking at that example, it uses values_list().)

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