Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#22623 closed Bug (worksforme)

PermLookupDict behaves dangerously / inconsistently

Reported by: rob.moore@… Owned by: drtyrsa
Component: contrib.auth Version: master
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 Changed 3 years ago by anonymous

comment:2 Changed 3 years ago by evolter

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.

comment:3 Changed 3 years ago by Tim Graham

Component: Uncategorizedcontrib.auth
Triage Stage: UnreviewedAccepted

comment:4 Changed 3 years ago by drtyrsa

Owner: changed from nobody to drtyrsa
Status: newassigned

comment:5 Changed 3 years ago by drtyrsa

Resolution: worksforme
Status: assignedclosed

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.

  1. {% 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.

  1. {% 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 Changed 3 years ago by rob.moore@…

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 Changed 3 years ago by rob.moore@…

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!

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