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.

Sign up for GitHub

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

Jump to bottom

Undocumented (error prone) django-rest-framework authentication side-effect #633

Closed
gbataille opened this issue Aug 15, 2018 · 0 comments
Closed

Undocumented (error prone) django-rest-framework authentication side-effect #633

gbataille opened this issue Aug 15, 2018 · 0 comments
Milestone
1.3

Comments

@gbataille
Copy link
Contributor

Hello,

In here https://github.com/jazzband/django-oauth-toolkit/blob/master/oauth2_provider/contrib/rest_framework/authentication.py, the authenticate method has a hidden that effect. On authentication error, it sets a oauth2_error attribute on the request and then authenticate_header assumes it's there.

This caused me an issue because I have subclasses OAuth2Authentication and to override authenticate. Of course I did not know of this "side effect" and a lot of tests started to fail on my project. Was quite hard to debug.

Granted it's odd that we override this class, we have legacy reasons to do so. I think the authenticate_header method should be more relaxed and look like

if hasattr(request, 'oauth2_error'):
    www_authenticate_attributes.update(request.oauth2_error)

What do you think?
@gbataille gbataille changed the title Undocumented (error prone) django-rest-framework contrib API Undocumented (error prone) django-rest-framework authentication side-effect Aug 15, 2018
auvipy pushed a commit that referenced this issue Jun 7, 2019
@george-pearson
Use getattr for oauth2_error access (#633) (#716)
f86dfb8 
* Use getattr for oauth2_error access (#633)

If the request doesn't have a oauth2_error property the
authenticate_header method errors. This can happen when the
oauthlib_core.verify_request method raises exceptions in authenticate.
It is useful to be able to raise AuthenticationFailed exceptions from
within a custom validate_bearer_token method which causes this.

* Add Test OAuth2Authentication authenticate override

Added a test for if an authenticate method that returns
None is used. This should result in a HTTP 401 response
for any request.
@n2ygk n2ygk mentioned this issue Mar 1, 2020
Merged
@auvipy auvipy added this to the 1.3 milestone Mar 1, 2020
@auvipy auvipy closed this as completed Mar 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3
Development

No branches or pull requests
2 participants
@gbataille @auvipy