#22623 closed Bug (worksforme)
PermLookupDict behaves dangerously / inconsistently
Reported by: | Owned by: | drtyrsa | |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | PermLookupDict permissions |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
While checking permissions within a template, I noticed that a typo of
{% if perms.auth.change_user %}
to
{% if perms.auth_change_user %}
for example, causes the statement to be evaluated as True if the authenticated user has any permissions, as perms.any_arbitrary_key returns a PermLookupDict, which has a repr method which returns a stringified set of all the user's permissions. This seems dangerous and unusual, as most typos checking user permissions will result is the user being silently given privileges they should not have; it also seems unreasonable that perms.anything_you_want should return the full set of permissions: the PermLookupDict represents permissions for the specified app, not all permissions, and the return value of repr should reflect that (i.e. return a set of permissions within that app, if anything).
The class' repr and bool methods also seem inconsistent in that the latter does behave as I describe, checking that the user has a permission within the module for which the PermLookupDict is constructed.
Change History (7)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
comment:3 by , 10 years ago
Component: | Uncategorized → contrib.auth |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 10 years ago
Resolution: | → worksforme |
---|---|
Status: | assigned → closed |
I can't reproduce any of dangerous parts. The way I test: just adding a line to contrib/admin/templates/admin/index.html and logging in with staff (not superuser) user.
{% if perms.auth_change_user %}BUG!!{% else %}NO BUG!!{% endif %}
The result is "NO BUG!!". Explanation: PermLookupDict does have repr method, but in {% if tag %} it's bool (or nonzero) method which is used. And it works the right way.
{% if perms.auth.anything %}BUG!!{% else %}NO BUG!!{% endif %}
The result is "NO BUG!!" Explanation: it calls user.has_perm method and it works the right way.
I suspect that the source of confusion is that if you try these with superuser, you will have "BUG!!". But it is not a bug, superuser does have all the permissions.
comment:6 by , 10 years ago
Hmm, sorry if that is indeed what I did wrong; I'll repeat your test, drtyrsa, and confirm that. I did spot the bool method and suspected that it was used in template if statements, but my colleague indicated he had checked that and found it was still using repr.
comment:7 by , 10 years ago
Thanks drtyrsa; I've confirmed that it does work as expected and that the problem was most likely because both my colleague and I were testing with superusers. Sorry about that!
While checking perms for existing app but not existing permission (caused e.g. by typo) like {{ perms.auth.anything }} will return True by default, which imho should be defaulted to False as it seems quite dangerous about perms to allow for anything.