-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add 'revoked' field to ThrottledApplication to enable easily revoking access to client applications violating openverse TOS #4334
Conversation
Hi @sarayourfriend, this is my first attempt at navigating this issue, many thanks for the headstart! I was able to get the tests in Furthermore, I have a few questions as to how things work. First is, how are we able to get the new |
The questions you're running into are good ones, and I think expose the fact that maybe a throttle isn't the right place to handle authorisation. The use of a throttle was a bit of a hack, and actually I'm realising that a new throttle class isn't necessary at all, because the For what it's worth, if we did need the throttle class to run, it would need to be added to the openverse/api/conf/settings/rest_framework.py Lines 43 to 53 in 575f529
We don't define specific throttle classes on any routes except on a few routes that are more restrictive like thumbnails. However, with your questions in mind, it really seems to me a throttle scope isn't the right choice, and a new boolean field on
This makes sense, I hadn't realised it would happen, but I see now that it's unavoidable with the exact conditional check I suggested. The problem is that https://github.com/encode/django-rest-framework/blob/HEAD/rest_framework/request.py#L247-L256 https://github.com/encode/django-rest-framework/blob/HEAD/rest_framework/request.py#L377-L394 Those are the two code places to keep in mind. So the authentication runs the first time some code accesses the We can get around this by knowing that diff --git a/api/conf/oauth2_extensions.py b/api/conf/oauth2_extensions.py
index 2ece1b497..4ebe41a45 100644
--- a/api/conf/oauth2_extensions.py
+++ b/api/conf/oauth2_extensions.py
@@ -11,7 +11,7 @@ class OAuth2Authentication(BaseOAuth2Authentication):
keyword = "Bearer"
def authenticate(self, request):
- result = super().authenticate(request)
+ user, auth = super().authenticate(request)
if getattr(request, "oauth2_error", None):
# oauth2_error is only defined on requests that had errors
# it will be undefined or empty for anonymous requests and
@@ -19,7 +19,11 @@ class OAuth2Authentication(BaseOAuth2Authentication):
# `request` is mutated by `super().authenticate`
raise AuthenticationFailed()
- return result
+ if application := getattr(auth, "application", None):
+ if application.revoked:
+ raise PermissionDenied()
+
+ return user, auth
class OAuth2OpenApiAuthenticationExtension(TokenScheme): To summarise: your questions are excellent and expose a deep problem with my suggested implementation. Instead of a new throttle class, let's add a boolean openverse/api/api/admin/__init__.py Lines 69 to 87 in 0a7d31b
Sorry for leading you astray with my implementation suggestion! Please let me know if this new approach makes sense, and thank you very much for asking these questions. I wouldn't have realised the problem with my suggested implementation otherwise. |
Hi @sarayourfriend, Once again, thank you for this detailed insights and explanations. Glad to hear my question helped make things more clearer. And yes, your new suggested approach is very clear and intuitive. |
ad8bdc1
to
47a402c
Compare
Hi @sarayourfriend all suggested changes have been implemented and I can confirm that things work as expected locally. Would go ahead and undraft this PR. |
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.
Awesome @madewithkode! This LGTM, fantastic work getting this turned around after my initial misguidance 🎉 🚀. Thank you very much for the contribution 🙏
if getattr(request, "oauth2_error", None): | ||
# oauth2_error is only defined on requests that had errors | ||
# it will be undefined or empty for anonymous requests and | ||
# requests with valid credentials | ||
# `request` is mutated by `super().authenticate` | ||
raise AuthenticationFailed() | ||
# if this is an authed request, check and |
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.
Very minor nit-pick for extra whitespace to help visually break the function up.
# if this is an authed request, check and | |
# if this is an authed request, check and |
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.
@sarayourfriend Updated!
47a402c
to
5d48b22
Compare
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.
LGTM, works great locally! @sarayourfriend I wanted to ask a question but feel free to merge this. Should we send a different response to the user other than
{
"detail": "You do not have permission to perform this action."
}
I can see this response leading to folks generating new credentials. Maybe something like
{
"detail": "Your registered application has been revoked for violating Openverse's Terms of Service. Learn more here: https://docs.openverse.org/terms_of_service.html."
}
To be fair, either response would provoke a bad actor to do so; this is really just a signal of "we're aware of your behaviour" more than anything. I don't think we necessarily need to give the reason why the request was forbidden, and we should discuss internally what the policy would be if someone asked us directly about a specific client application. |
@madewithkode this just needs a rebase to resolve the conflict. I'll do that later during my workday today (it's 10 AM here for me) if you don't get to it first, then I'll merge afterwards. Thanks again! |
…#4364) Co-authored-by: krysal <9145885+krysal@users.noreply.github.com>
5d48b22
to
8f35bf2
Compare
@sarayourfriend rebased and pushed the changes. |
Thanks! Sorry I wasn't able to get to it yesterday in the end. Merged 🚀 |
Fixes
Fixes #4321 by @sarayourfriend
Description
Add 'revoked' field to
ThrottledApplication
to enable easily revoking access to client applications violating openverse TOS.Testing Instructions
Create a
ThrottledApplication
on Django admin and go ahead to use the generated credentials to authenticate and make request(s) to the API. This should work as expected and return results.Head back to your ThrottledApplication on Django admin, edit this application by checking the 'revoked' field and save. Try making requests to the API and you should get a
401
PermissionDenied
.Additionally, a unit test exists for this at
api/test/integration/test_auth.py::test_revoked_application_access
Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin