Code

Opened 3 years ago

Closed 2 months ago

#15716 closed New feature (fixed)

The has_perm() method of authorization backends should be able to explicitly deny permission

Reported by: Kronuz Owned by: jorgecarleitao
Component: contrib.auth Version:
Severity: Normal Keywords:
Cc: Kronuz, albrecht.andi@…, jorgecarleitao Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

For authorization backends, has_perm() should return True if the authorization is granted, False if it's not, and None if it doesn't know about certain permission.

Reading django's source code, I see that if has_perm(), in the backends, returns False, it keeps trying other backends for a successful authorization (i.e. for any other has_perm() in any other backends returning True)

I was thinking, shouldn't a has_perm() returning False be a definitive "False" (as in no permission or permission denied and stop trying?) My take would be perhaps has_perm() returning None in case the backend simply doesn't know about the asked permission, so only then django keeps on trying in other backends, but if has_perm() otherwise returns False, it should always mean an absolute Permission Denied.

This would allow cases where several authorization backends are set up and some backends handle explicit permission denied rules.

Attachments (0)

Change History (13)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #2550

comment:2 Changed 3 years ago by Kronuz

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I reopened the ticket since #15716 is not entirely a duplicate of #2550, since ticket #15716 is about authorization, and #2550 is about authentication... they are both related nonetheless.

I would suggest, in my opinion, for the case of authorization in this ticket, to use an irrevocable False for permission denied, True for permission granted and either None or an Exception (or both) for "Permission Unknown" or "Don't care".

In case of has_perm() returning None or raising a PermissionUnkown Exception, django would keep on looking for the permission in other backends, otherwise return the permission from the backend.

comment:3 Changed 3 years ago by SmileyChris

  • Component changed from Uncategorized to contrib.auth
  • Triage Stage changed from Unreviewed to Design decision needed
  • Version 1.2 deleted

We can't just change False to mean "permission denied" since that would be entirely backwards incompatible. I'd say it would probably be better to follow the example of #2550 and allow has_perm to raise PermissionDenied, short-circuiting the remainder of the permissions checks.

comment:4 Changed 3 years ago by Kronuz

I absolutely agree SmileyChris, just to keep backward compatibility, though not being this the same case as in #2550, where authenticate() returns a User object or None (in which the PermissionDenied exception is the obvious solution) and had we had the opportunity to choose, False meaning Permission Denied, I'd say is better.

The edge case in which using False to mean Permission Denied here could hit backward compatibility would be only when multiple authorization backends are used, chaining multiple fallback calls to has_perm(), which I don't know how often or serious that'd be. In all other cases, it seems using False to mean Permission Denied shouldn't pose a problem, and by design we'd have a more solid, elegant, intuitive and logical implementation of has_perm(), not allowing, or denying, a permission when it returns False, as it should. But I suppose the consequences of this decision call for further discussion.

comment:5 Changed 3 years ago by lukeplant

  • Type set to New feature

comment:6 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:7 Changed 3 years ago by SmileyChris

  • Easy pickings unset
  • Summary changed from in authoriation backends, has_perm() should return None if it doesn't know about a pemission to The has_perm() method of authorization backends should be able to explicitly deny permission
  • Triage Stage changed from Design decision needed to Accepted

I reassert that I want this feature to use an exception to explicitly deny that permission.

I'm leaving it open as a close relative to #2550, since this deals with authorization rather than authentication.

comment:8 Changed 3 years ago by Kronuz

  • Cc Kronuz added
  • UI/UX unset

comment:9 Changed 2 years ago by aalbrecht

  • Cc albrecht.andi@… added

comment:10 Changed 16 months ago by aaugustin

  • Status changed from reopened to new

comment:11 Changed 2 months ago by jorgecarleitao

  • Cc jorgecarleitao added

I believe the option of raising an exception is superior to return False since it allows exceptions that tell us why the permission was denied. A False would only provide information on whether it was forbidden or not, which can be too restrictive for future development of backends.

comment:12 Changed 2 months ago by jorgecarleitao

  • Has patch set
  • Owner changed from nobody to jorgecarleitao
  • Status changed from new to assigned

comment:13 Changed 2 months ago by Tim Graham <timograham@…>

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

In 2e364a0aacb49a5160896b1ca5a2619baa3f4d9b:

Fixed #15716 - Authentication backends can short-circuit authorization.

Authorization backends can now raise PermissionDenied in "has_perm"
and "has_module_perms" to short-circuit authorization process.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.