Opened 4 years ago

Closed 16 months ago

Last modified 16 months ago

#17903 closed Bug (fixed)

`ModelBackend.get_all_permissions` returns permissions for inactive users

Reported by: SmileyChris 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 4 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 3 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

comment:3 Changed 22 months 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 22 months ago by jorgecarleitao

  • Cc jorgecarleitao added

comment:5 Changed 17 months ago by jorgecarleitao

  • Owner changed from nobody to jorgecarleitao
  • Status changed from new to assigned

comment:6 Changed 16 months ago by jorgecarleitao

  • Has patch set

comment:7 Changed 16 months ago by timo

  • Patch needs improvement set

comment:8 Changed 16 months 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 16 months ago by timo

  • Patch needs improvement set

Left comments for how to improve the tests.

comment:10 Changed 16 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 16 months 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