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

Add ability to revoke access to specific Openverse API registered client applications #4321

Closed
sarayourfriend opened this issue May 14, 2024 · 2 comments · Fixed by #4334
Closed

Add ability to revoke access to specific Openverse API registered client applications #4321

sarayourfriend opened this issue May 14, 2024 · 2 comments · Fixed by #4334
Assignees
@madewithkode
Labels
🕹 aspect: interface Concerns end-users' experience with the software 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API

Comments

@sarayourfriend
Copy link
Contributor

sarayourfriend commented May 14, 2024 •
edited

Problem

Our main tool to fight ToS violations at the moment is to employ Cloudflare rules targeting particular request/response headers. This works well, but isn't necessarily intuitive, and we can do more in this regard. We have no way built into Django admin to revoke access to a client application.

Description

Add a new throttle scope "revoked" that simply disallows access. Either raise a 401 in the throttle itself, or probably better, augment conf.oauth2_extensions.OAuth2Authentication to check the scope and raise DRF's unauthorised exception when the throttle scope is "revoked".

We'll be adding a new boolean revoked to the throttled application instead. See context in this comment: #4334 (comment)

Alternatives

A workaround is to set the throttled application back to unverified. The registrant would not receive a new email to re-verify, so this effectively revokes access, as unverified applications receive 401s as well. However, this is only a workaround, and is indirect (there's no way to make this clear in Django admin, nowhere near as clear as an explicit throttle scope). As such, it is not a satisfactory long-term solution.
@sarayourfriend sarayourfriend added 🟧 priority: high Stalls work on the project or its dependents 🕹 aspect: interface Concerns end-users' experience with the software 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🧱 stack: api Related to the Django API labels May 14, 2024
@madewithkode
Copy link
Contributor

Hi @sarayourfriend, I'd like to take on this.
I'd try to open a draft PR as early as possible and I'd appreciate as much guidance as I can get as I don't have it all figured out.

@sarayourfriend
Copy link
Contributor Author

Sure thing @madewithkode. You've seen it is high priority, so we'll be working on a quick turnaround for reviews and requested changes. You're welcome to work on it, but I'll ask you to be extra proactive in communicating any potential delays so we can hand off if needed. You've been a proactive communicator in the past, so I think it should be fine, just wanted to clarify expectations on this particular issue.

For implementation, to add a new throttle scope, create a new class RevokedOAuth2IdRateThrottle. It should be essentially a copy/paste of the Exempt throttle class, but with exempt replaced with "revoked" in the rate limit model set and scope string.

Here's the code of the exempt throttle class:

openverse/api/api/utils/throttle.py

Lines 186 to 188 in c92d484

class ExemptOAuth2IdRateThrottle(AbstractOAuth2IdRateThrottle):
applies_to_rate_limit_model = {"exempt"}
scope = "exempt_oauth2_client_credentials"

You'll also need to add a "revoked" entry to the RATE_LIMIT_MODELS attribute on ThrottledApplication:

openverse/api/api/models/oauth.py

Lines 30 to 36 in 098925a

RATE_LIMIT_MODELS = [
("standard", "standard"), # Default rate limit for all API keys.
("enhanced", "enhanced"), # Rate limits for "super" keys, granted on a
# case-by-case basis.
("exempt", "exempt"), # Rate limits used for internal infrastructure to
# by-pass rate limiting entirely.
]

Django admin is already configured for the rate_limit_model field to be configurable by admins, so there's no need to make changes to Django admin for this issue.

Finally, in OAuth2Authentication, you'll need to add an additional condition to raise rest_framework.exceptions.PermissionDenied when request.auth.application.rate_limit_model == "revoked". You'll need to check if request.auth and request.auth.application exist too or use a combination of calls to getattr.

That's the implementation work, I think. Last part is to add and update test_auth to account for the revoked throttle class:

openverse/api/test/integration/test_auth.py

Line 135 in 575f529

def test_auth_rate_limit_reporting(

This is a significant issue with a few different moving parts, and the unit tests will probably almost certainly require adjustments to existing tests (at the very least to exclude the "revoked" throttle class from tests that expect authentication to pass), as well as brand new tests. test_auth is a bit of a gnarly module, so please reach out early if you run into difficulties with that. The tests are almost certain to be the most complex and tedious part of this issue.

@sarayourfriend sarayourfriend mentioned this issue May 15, 2024
Open
@madewithkode madewithkode mentioned this issue May 15, 2024
Merged
8 tasks
@sarayourfriend sarayourfriend changed the title Add "revoked" throttle scope to enable easily revoking access to client applications violating Openverse terms of service Add ability to revoke access to specific Openverse API registered client applications May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@madewithkode madewithkode

Labels
🕹 aspect: interface Concerns end-users' experience with the software 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟧 priority: high Stalls work on the project or its dependents 🧱 stack: api Related to the Django API
Projects
Openverse Backlog
Archived in project
Milestone
No milestone
2 participants
@sarayourfriend @madewithkode