Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Fix AttributeError hiding on request authenticators#5600
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Fix AttributeError hiding on request authenticators#5600
Uh oh!
There was an error while loading.Please reload this page.
Conversation
01f5ef5 tod69f44aCompare16ec7ae to746bb17Comparerest_framework/request.py Outdated
| # potentially hides AttributeError's raised in `_authenticate`. | ||
| if attr in ['user', 'auth']: | ||
| raise | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
+1 For not doing this. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, wanted to demonstrate that this does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@rpkilby Great stuff!
I think we just need to document the existence ofWrappedAttributeError — the why and what you'd do with it.
A good place for that would be in theCustom Authentication section.
@rpkilby we cannot really reproduce it now any more since our code has fixed the AttributeError and changed quite a bit since we originally reported this. But yes, just raising an Looking at the PR and test I think this PR does the right thing :) |
746bb17 toebd1e85Compare* Update assertion style in user logout test* Apply middlewares to django request object* Fix test for request auth hiding AttributeErrors* Re-raise/wrap auth attribute errors* Fix test for py2k* Add docs for WrappedAttributeError
Fixes#4033. There is an existing test for the issue, however there are a couple of issues:
request.useris never set. Because of this,__getattribute__will re-raise theAttributeError. However, typical use does include an authentication middleware and will yield auseron the underlying request, as demonstrated bySuppressed AttributeError When Authenticating #4033.SessionMiddlewareis applied to the DRF request object instead of the underlying Django request, which doesn't mirror how middleware is actually applied.