Opened 14 years ago
Closed 11 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 by , 14 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
comment:2 by , 14 years ago
Resolution: | duplicate |
---|---|
Status: | closed → 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 by , 14 years ago
Component: | Uncategorized → contrib.auth |
---|---|
Triage Stage: | Unreviewed → Design 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 by , 14 years ago
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 by , 14 years ago
Type: | → New feature |
---|
comment:6 by , 14 years ago
Severity: | → Normal |
---|
comment:7 by , 14 years ago
Easy pickings: | unset |
---|---|
Summary: | in authoriation backends, has_perm() should return None if it doesn't know about a pemission → The has_perm() method of authorization backends should be able to explicitly deny permission |
Triage Stage: | Design decision needed → 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 by , 13 years ago
Cc: | added |
---|---|
UI/UX: | unset |
comment:9 by , 12 years ago
Cc: | added |
---|
comment:10 by , 12 years ago
Status: | reopened → new |
---|
comment:11 by , 11 years ago
Cc: | 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 by , 11 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Pull request https://github.com/django/django/pull/2641.
comment:13 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Duplicate of #2550