Opened 6 years ago

Closed 3 years ago

#15716 closed New feature (fixed)

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

Reported by: German M. Bravo Owned by: jorgecarleitao
Component: contrib.auth Version:
Severity: Normal Keywords:
Cc: German M. Bravo, 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.

Change History (13)

comment:1 Changed 6 years ago by Aymeric Augustin

Resolution: duplicate
Status: newclosed

Duplicate of #2550

comment:2 Changed 6 years ago by German M. Bravo

Resolution: duplicate
Status: closedreopened

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 6 years ago by Chris Beaven

Component: Uncategorizedcontrib.auth
Triage Stage: UnreviewedDesign decision needed
Version: 1.2

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 6 years ago by German M. Bravo

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 6 years ago by Luke Plant

Type: New feature

comment:6 Changed 6 years ago by Luke Plant

Severity: Normal

comment:7 Changed 6 years ago by Chris Beaven

Easy pickings: unset
Summary: in authoriation backends, has_perm() should return None if it doesn't know about a pemissionThe has_perm() method of authorization backends should be able to explicitly deny permission
Triage Stage: Design decision neededAccepted

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 5 years ago by German M. Bravo

Cc: German M. Bravo added
UI/UX: unset

comment:9 Changed 4 years ago by Andi Albrecht

Cc: albrecht.andi@… added

comment:10 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:11 Changed 3 years 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 3 years ago by jorgecarleitao

Has patch: set
Owner: changed from nobody to jorgecarleitao
Status: newassigned

comment:13 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

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.

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