APIv3: proxy these URLs to be served from El Proxito /_/api/v3/
#11831
Conversation
humitos
commented
Dec 9, 2024
humitos
commented
- Related: API: consider using APIv3 standard endpoints addons#356
- Related: API: use APIv3 endpoint for resources addons#468
|
|
||
| """Mixin to remove conflicting fields from serializers.""" | ||
|
|
||
| FIELDS_TO_REMOVE = ("_links",) |
There was a problem hiding this comment.
Adding links back may cause a snowball of dependencies, requiring to have all endpoints exposed in the proxied API. I wonder if another alternative can be to have a different and more restricted API under the addons umbrella instead of trying to move the whole API.
There was a problem hiding this comment.
I want to avoid having two different API responses one from the dashboard and one from proxito if possible. That will increase maintenance work and also will add confusion. Let's try how it works with this initial implementation that just proxies the APIv3 but read-only using the exact same response for now and see how it goes.
|
I think this PR is going in the direction we want and it's almost ready to replace the addons API endpoint. The only thing I'm not being able to do correctly is to handle the |
|
I did an initial implementation for the required sorting at DB level using a solution I found o a blog post: |
|
@stsewd I think your review here is probably most useful. I looked at the code and it seems reasonable, but not 100% sure about the robustness of the read-only checking, but didn't have an obvious suggestion to improve it. |
| @@ -110,7 +125,7 @@ class APIv3Settings: | |||
| LimitOffsetPagination.default_limit = 10 | |||
|
|
|||
| renderer_classes = (AlphabeticalSortedJSONRenderer, BrowsableAPIRenderer) | |||
| throttle_classes = (UserRateThrottle, AnonRateThrottle) | |||
| # throttle_classes = (UserRateThrottle, AnonRateThrottle) | |||
There was a problem hiding this comment.
I temporary commented this because I was blocked immediately since we have the throttle very low in our settings.
readthedocs.org/readthedocs/settings/base.py
Lines 902 to 905 in e70cf31
We should increase these values to be able to use APIv3 in the way we want with addons.
| if self.request.path.startswith(f"/{settings.DOC_PATH_PREFIX}"): | ||
| permission_classes = [ReadOnlyPermission] |
There was a problem hiding this comment.
This should probably be a kind of override at the view class level or a setting that detects if this is running under proxito instead of relying on the path.
There was a problem hiding this comment.
also, this is only being done for the projects viewsets, it needs to be done for the whole API.
| api_proxied_urls = [ | ||
| path("embed/", ProxiedEmbedAPI.as_view(), name="embed_api_v3"), | ||
| path("search/", ProxiedSearchAPI.as_view(), name="search_api_v3"), | ||
| ] | ||
|
|
||
| urlpatterns = api_proxied_urls | ||
| urlpatterns += router.urls |
There was a problem hiding this comment.
We are proxying the whole API here instead of what addons needs, this has several implications:
- Duplicate endpoints
- Attack surface is greater. Preventing XSS vulnerabilities in doc pages is not under our total control (as it depends on the user more). We won't be exposing write operations, but the attack surface is greater now (but you can argue that the most important information is docs content, project, and version, which is already possible to get, but it's harder to list all projects for example).
- Permissions need to be adapted, especially for .com, as addons needs to work with password and token access, we can't assume we always have a user.
- Caching as well will need to handle per-view depending on what data gets staled.
- I think we did lots of optimizations in the addons response, which we may be loosing when migrating everything to API v3.
I'd suggest we explicitly bring what we need and adapt it to work under proxito (as we have done with search and embed APIs) instead of trying to bring the whole API v3 at once. For example, we don't need to list all projects from docs pages, or redirects.
