Fix: Return 401 for API request with Invalid Authorization #3663
Conversation
…rbidden when invalid Auth headers
openverse-bot
added
the
🚦 status: awaiting triage
openverse-bot
added
🟨 priority: medium
dhruvkb
added
🧱 stack: api
|
Thanks for the help! @dhruvkb |
There was a problem hiding this comment.
No problem @carlosm22700. Thanks for the contribution!
A more elegant approach to this would be to use permission_classes (I believe L176 was a typo).
+ from oauth2_provider.contrib.rest_framework import TokenHasScope
@extend_schema(tags=["auth"])
class CheckRates(APIView):
throttle_classes = (OnePerSecond,)
+ permission_classes = (TokenHasScope,)
+ required_scopes = ('read',)This automatically raises 401 when the token is not provided, invalid or expired (and also provides the reason in the WWW-Authenticated header as recommended by DRF).
E.g.
WWW-Authenticate: Bearer realm="api",error="invalid_token",error_description="The access token has expired."
or
WWW-Authenticate: Bearer realm="api",error="invalid_token",error_description="The access token is invalid."
|
@carlosm22700 Do you think you could resume this work in the following days? We would appreciate it if you let us know in any case. |
There was a problem hiding this comment.
Thanks for the contribution @carlosm22700 and for implementing the changes from the review. LGTM!
There was a problem hiding this comment.
Thanks for your contribution, @carlosm22700! Would you mind updating the PR description to include testing instructions? It looks like the description might also be slightly out of date.
If I'm reading the linked issue correctly, I think we want to return a 401 when an invalid authorization header is sent to any API endpoint. The code changes here only address the CheckRates view. For example, when testing I would expect that if I try:
> curl -H "Authorization: Bearer faketoken123" \
-I "http://localhost:50280/v1/images/"
I should get a 401. Instead it continues to silently downgrade it to an unauthorized request (returns successfully until the unauthorized rate limit is exceeded, then 429s).
I'm also still getting a 403 when I call the CheckRates endpoint with a bad token, too 🤔 Though I may be misunderstanding the intended behavior there. @dhruvkb can you weigh in if I'm off base?
|
You're right about the intended scope, Staci. Any API endpoint sent with invalid authorisation header should raise a 401 to indicate the credentials are invalid, without downgrading the request to anonymous. What this means is:
To accomplish that, we need a wrapper around the authentication middleware, not a permissions class. The permissions class would be the right choice if we were trying to guard access to particular resources based on the scope of the authenticated request. That's not the goal, though. If it helps clarify the intention and goal, we could use 400 instead of 401 for these, with a message indicating the authorization header credentials were invalid. |
…ting custom middleware wrapper
…y implementing middleware wrapper
|
Thank you all for your patience! If anyone could take a moment to review the updated PR or provide feedback, I'd greatly appreciate it! Your guidance means a lot while i continue to learn and contribute :) |
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @sarayourfriend Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @carlosm22700, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
There was a problem hiding this comment.
You are on the right track, @carlosm22700, but there are two requested changes I've left. This is a bit of a tricky issue, and to understand the approach I've suggested, I had to dig through the code for django-oauth-toolkit to understand how it handles errors.
With these requested changes, I'd also like to request new unit tests to cover this, added in test_auth.py. The test should make a request with a bad token, and confirm that the response is a 401.
| "oauth2_provider.middleware.OAuth2TokenMiddleware", | ||
| "api.middleware.strict_auth_middleware.strict_auth_middleware", |
There was a problem hiding this comment.
The logging middleware must not be removed. The OAuth2 middleware is conditionally added in the settings/oauth.py module (
openverse/api/conf/settings/oauth2.py
Lines 9 to 11 in 962c556
| "oauth2_provider.middleware.OAuth2TokenMiddleware", | |
| "api.middleware.strict_auth_middleware.strict_auth_middleware", | |
| "api.middleware.response_headers_middleware.response_headers_middleware", |
To fully resolve this issue, please update api/conf/settings/oauth.py to append strict_auth_middleware when we append the OAuth2 token middleware (in the same conditional).
| permission_classes = (TokenHasScope,) | ||
| required_scopes = ["read"] |
There was a problem hiding this comment.
This can be removed and the TODO later in the file added back. The precise condition is already handled where the TODO was, and we should address this in a separate PR to avoid complicating this one.
| # Extract the Authorization header from the request | ||
| auth_header = request.headers.get('Authorization', None) | ||
|
|
||
| # If the Authorization header is present | ||
| if auth_header: | ||
| # Check if the user is anonymous or authentication failed | ||
| if request.user.is_anonymous or request.auth is None: |
There was a problem hiding this comment.
Because of how the OAuth2 DRF authenticator works, we can replace all of these checks with a check against oauth2_error on the request object:
The request attribute comes from this method, where you can see various error messages are turned as part of it:
We haven't defined set scopes anywhere, so we don't need to worry about insufficient_scope, and can just assume the reason is for an invalid token. It'd be nice to return the error description as well, so users understand why it's invalid.
Something along these lines is where I would start:
oauth2_error = getattr(request, "oauth2_error", None)
if oauth2_error is None:
return get_response(request)
raise AuthenticationFailed(
detail=oauth2_error.get("error_description", None),
code=oauth2_error.get("error", None),
)
|
working on this today, but I've already started implementing the changes! I've been having some issues with booting up my api using both just/api up and just/api init, so I was hoping to also address that on my end. |
|
@carlosm22700 have you moved this PR elsewhere/do you plan to continue working on it? Just wanted to clarify since you closed the PR, just let us know 🙂 |
Fixes
Related to #3626 by @sarayourfriend
Description
This PR implements middleware 'StrictAuthMiddleware' as a solution to issue #3626. The goal is to enforce a consistent response across the API for requests with invalid Authorization headers by returning a 401 Unauthorized status. If the request does not contain a header, it proceeds as anonymous.
During the development and testing phases, I tested different endpoints to see how they would handle various scenarios, including valid and invalid Authorization headers (tokens), as well as requests lacking these headers. By implementing this middleware, my goal was to avoid the need for repetitive modifications across individual views. It also avoids using individual permissions for specific resources and uses a wrapper instead.
However, I ran into some challenges with authenticated requests also returning 401 and the auth tests not passing when running just/api test. I would very much appreciate any feedback on my approach to this!
Testing
Invalid Token:
curl -H "Authorization: Bearer invalid_token" -I "http://localhost:50280/v1/images/"curl -H "Authorization: Bearer invalid_token" -I "http://localhost:50280/v1/checkrates/"Expect a 401 Unauthorized response.
No Authorization Header (Anonymous request):
curl -I "http://localhost:50280/v1/images/"Expect a 200 OK response, treating the request as anonymous.
Checklist
main).Developer Certificate of Origin
Developer Certificate of Origin