Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix non standard implementation of __getattr__ #675

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mluard
Copy link

@mluard mluard commented Feb 17, 2023

According to the the docs, a __getattr__ magic method should either return a value for the requested attribute name, or raise AttributeError.

A __getattr__ method was added to the TokenUser class that allows accessing keys on the token as if they were attributes of the TokenUser class. In the current implementation, if the requested attribute name is not a key in the token dictionary, then None is returned instead of raising AttributeError.

When Django's auth backend ModelBackend does permission checks on a user object, it first checks for an attribute named _perm_cache with hasattr, then builds the cache if it doesn't exist. Since hasattr incorrectly returns True for a TokenUser instance, the cache (which would have resolved to an empty set) is never created. This ultimately results in a TypeError because None is not Iterable.

@Andrew-Chen-Wang
Copy link
Member

Not sure if this is backwards compatible, but makes sense.

@mluard
Copy link
Author

mluard commented Feb 22, 2023

This will break backwards compatibility in the specific case where you try to access an attribute on a token user instance when that attribute does not exist on the instance itself and does not exist as a key in the token. This is a case that should be handled defensively using an exception handler, a hasattr check, or a getattr call with a default value.

In the case where the name of the attribute is known to always exist on the token, this is not a breaking change.

In my case, I was updating the dependencies of an older Django application, and the addition of the __getattr__ method caused the app to crash on has_perm checks for non admin users. If this fix ins't accepted, my other option is to use the TOKEN_USER_CLASS setting to subclass TokenUser and just fix it in my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants