Opened 5 years ago

Closed 2 years ago

Last modified 2 years ago

#17903 closed Bug (fixed)

`ModelBackend.get_all_permissions` returns permissions for inactive users

Reported by: Chris Beaven Owned by: jorgecarleitao
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: jorgecarleitao Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


The documentation states:

Returns a set of permission strings that the user has, both through group and user permissions

Since has_perm returns False for an inactive user, I wouldn't think it should return an empty string.
The behaviour is not as explicitly stated as the other perm methods though, they say:

If the user is inactive, this method will always return False.

r14797 changed get_all_permissions to return all strings for superusers, which seems to imply this should work the same way that has_perm does, not just provide a list of permission strings directly assigned (or via groups) to the user.

Change History (11)

comment:1 Changed 5 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

comment:3 Changed 3 years ago by jorgecarleitao

I checked the code, and get_all_permissions does the following:

If user is anonymous: returns empty set.
else: joins "user permissions" with "user groups permissions".

In "user groups permissions", if the user is superuser, this returns all permissions.

So, in has_perm and has_module_perms we check for is_active, but in get_all_permissions we check for is_anonymous.
Shouldn't this be consistent?

comment:4 Changed 3 years ago by jorgecarleitao

Cc: jorgecarleitao added

comment:5 Changed 2 years ago by jorgecarleitao

Owner: changed from nobody to jorgecarleitao
Status: newassigned

comment:6 Changed 2 years ago by jorgecarleitao

Has patch: set

comment:7 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:8 Changed 2 years ago by jorgecarleitao

Patch needs improvement: unset

To be consistent, I made both get_all_permissions, get_user_permissions and get_group_permissions to require the user to be active.

comment:9 Changed 2 years ago by Tim Graham

Patch needs improvement: set

Left comments for how to improve the tests.

comment:10 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In c33447a50c1b0a96c6e2261f7c45d2522a3fe28d:

Fixed #17903 -- Modified ModelBackend to eliminate permissions on inactive users.

Thanks to @SmileyChris for the report and @timgraham for review.

comment:11 Changed 2 years ago by Tim Graham <timograham@…>

In 150d88cc2c0866ef65f077387e3e560e9c9c3f80:

Restored is_anonymous() check in ModelBackend permission checking removed in refs #17903.

Thanks Florian Apolloner for raising the issue.

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