From 00d188efa118787e187bcc03eec46093a9a75306 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 24 Oct 2023 10:54:42 +0200 Subject: [PATCH] Addons: accept `project-slug` and `version-slug` on endpoint (#10823) * Addons: accept `project-slug` and `version-slug` on endpoint `/_/addons/` API endpoint now accepts `project-slug` and `version-slug`. This is useful to be able to cache these requests in our CDN. It still supports sending `url=` when necessary for those requests that relies on the full URL to generate specific data in the response. The NGINX config is updated to inject `meta` tags with project/version slugs, emulating what our CF worker does on production. Closes #10536 * Add tests and update logic for API permissions --- dockerfiles/nginx/proxito.conf.template | 5 +- readthedocs/api/v2/permissions.py | 11 ++ readthedocs/proxito/tests/test_hosting.py | 78 ++++++++++ readthedocs/proxito/views/hosting.py | 178 +++++++++++++--------- 4 files changed, 198 insertions(+), 74 deletions(-) diff --git a/dockerfiles/nginx/proxito.conf.template b/dockerfiles/nginx/proxito.conf.template index f77b84cee26..a4f043d7b60 100644 --- a/dockerfiles/nginx/proxito.conf.template +++ b/dockerfiles/nginx/proxito.conf.template @@ -111,9 +111,10 @@ server { set $rtd_force_addons $upstream_http_x_rtd_force_addons; add_header X-RTD-Force-Addons $rtd_force_addons always; - # Inject our own script dynamically + # Inject our own script dynamically and project/version slugs into the HTML to emulate what CF worker does # TODO: find a way to make this work _without_ running `npm run dev` from the `addons` repository - sub_filter '' '\n'; + sub_filter '' '\n\n\n'; + sub_filter_types text/html; sub_filter_last_modified on; sub_filter_once on; } diff --git a/readthedocs/api/v2/permissions.py b/readthedocs/api/v2/permissions.py index 8bc30ecd4e2..d3ec67d8847 100644 --- a/readthedocs/api/v2/permissions.py +++ b/readthedocs/api/v2/permissions.py @@ -40,6 +40,17 @@ class IsAuthorizedToViewVersion(permissions.BasePermission): def has_permission(self, request, view): project = view._get_project() version = view._get_version() + + # I had to add this condition here because I want to return a 400 when + # the `project-slug` or `version-slug` are not sent to the API + # endpoint. In those cases, we don't have a Project/Version and this + # function was failing. + # + # I think it's a valid use case when Project/Version is invalid to be + # able to return a good response from the API view. + if project is None or version is None: + return True + has_access = ( Version.objects.public( user=request.user, diff --git a/readthedocs/proxito/tests/test_hosting.py b/readthedocs/proxito/tests/test_hosting.py index fbe731bd8af..44f56ed879b 100644 --- a/readthedocs/proxito/tests/test_hosting.py +++ b/readthedocs/proxito/tests/test_hosting.py @@ -498,3 +498,81 @@ def test_flyout_subproject_urls(self): }, ] assert r.json()["addons"]["flyout"]["translations"] == expected_translations + + def test_send_project_not_version_slugs(self): + r = self.client.get( + reverse("proxito_readthedocs_docs_addons"), + { + "api-version": "0.1.0", + "client-version": "0.6.0", + "project-slug": self.project.slug, + }, + secure=True, + headers={ + "host": "project.dev.readthedocs.io", + }, + ) + assert r.status_code == 400 + assert r.json() == { + "error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'" + } + + def test_send_version_not_project_slugs(self): + r = self.client.get( + reverse("proxito_readthedocs_docs_addons"), + { + "api-version": "0.1.0", + "client-version": "0.6.0", + "version-slug": self.version.slug, + }, + secure=True, + headers={ + "host": "project.dev.readthedocs.io", + }, + ) + assert r.status_code == 400 + assert r.json() == { + "error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'" + } + + def test_send_project_version_slugs(self): + r = self.client.get( + reverse("proxito_readthedocs_docs_addons"), + { + "api-version": "0.1.0", + "client-version": "0.6.0", + "project-slug": self.project.slug, + "version-slug": self.version.slug, + }, + secure=True, + headers={ + "host": "project.dev.readthedocs.io", + }, + ) + assert r.status_code == 200 + expected_response = self._get_response_dict("v0") + # Remove `addons.doc_diff` from the response because it's not present when `url=` is not sent + expected_response["addons"].pop("doc_diff") + + assert self._normalize_datetime_fields(r.json()) == expected_response + + def test_send_project_version_slugs_and_url(self): + r = self.client.get( + reverse("proxito_readthedocs_docs_addons"), + { + "api-version": "0.1.0", + "client-version": "0.6.0", + "url": "https://project.dev.readthedocs.io/en/latest/", + # When sending `url=`, slugs are ignored + "project-slug": "different-project-slug-than-url", + "version-slug": "different-version-slug-than-url", + }, + secure=True, + headers={ + "host": "project.dev.readthedocs.io", + }, + ) + assert r.status_code == 200 + assert self._normalize_datetime_fields(r.json()) == self._get_response_dict( + "v0" + ) diff --git a/readthedocs/proxito/views/hosting.py b/readthedocs/proxito/views/hosting.py index 35c754397f9..0970dc6f134 100644 --- a/readthedocs/proxito/views/hosting.py +++ b/readthedocs/proxito/views/hosting.py @@ -22,7 +22,7 @@ from readthedocs.core.resolver import resolver from readthedocs.core.unresolver import UnresolverError, unresolver from readthedocs.core.utils.extend import SettingsOverrideObject -from readthedocs.projects.models import Feature +from readthedocs.projects.models import Feature, Project log = structlog.get_logger(__name__) # noqa @@ -47,10 +47,19 @@ class BaseReadTheDocsConfigJson(CDNCacheTagsMixin, APIView): Attributes: - url (required): absolute URL from where the request is performed - (e.g. ``window.location.href``) - api-version (required): API JSON structure version (e.g. ``0``, ``1``, ``2``). + + project-slug (required): slug of the project. + Optional if "url" is sent. + + version-slug (required): slug of the version. + Optional if "url" is sent. + + url (optional): absolute URL from where the request is performed. + When sending "url" attribute, "project-slug" and "version-slug" are ignored. + (e.g. ``window.location.href``). + + client-version (optional): JavaScript client version (e.g. ``0.6.0``). """ http_method_names = ["get"] @@ -61,61 +70,73 @@ class BaseReadTheDocsConfigJson(CDNCacheTagsMixin, APIView): @lru_cache(maxsize=1) def _resolve_resources(self): url = self.request.GET.get("url") - if not url: - # TODO: not sure what to return here when it fails on the `has_permission` - return None, None, None, None + project_slug = self.request.GET.get("project-slug") + version_slug = self.request.GET.get("version-slug") + + project = None + version = None + build = None + filename = None unresolved_domain = self.request.unresolved_domain # Main project from the domain. project = unresolved_domain.project - try: - unresolved_url = unresolver.unresolve_url(url) - # Project from the URL: if it's a subproject it will differ from - # the main project got from the domain. - project = unresolved_url.project - version = unresolved_url.version - filename = unresolved_url.filename - # This query should use a particular index: - # ``builds_build_version_id_state_date_success_12dfb214_idx``. - # Otherwise, if the index is not used, the query gets too slow. - build = version.builds.filter( - success=True, - state=BUILD_STATE_FINISHED, - ).last() - - except UnresolverError as exc: - # If an exception is raised and there is a ``project`` in the - # exception, it's a partial match. This could be because of an - # invalid URL path, but on a valid project domain. In this case, we - # continue with the ``project``, but without a ``version``. - # Otherwise, we return 404 NOT FOUND. - project = getattr(exc, "project", None) - if not project: - raise Http404() from exc - - version = None - filename = None - build = None + if url: + try: + unresolved_url = unresolver.unresolve_url(url) + # Project from the URL: if it's a subproject it will differ from + # the main project got from the domain. + project = unresolved_url.project + version = unresolved_url.version + filename = unresolved_url.filename + # This query should use a particular index: + # ``builds_build_version_id_state_date_success_12dfb214_idx``. + # Otherwise, if the index is not used, the query gets too slow. + build = version.builds.filter( + success=True, + state=BUILD_STATE_FINISHED, + ).last() + + except UnresolverError as exc: + # If an exception is raised and there is a ``project`` in the + # exception, it's a partial match. This could be because of an + # invalid URL path, but on a valid project domain. In this case, we + # continue with the ``project``, but without a ``version``. + # Otherwise, we return 404 NOT FOUND. + project = getattr(exc, "project", None) + if not project: + raise Http404() from exc + + else: + project = Project.objects.filter(slug=project_slug).first() + version = Version.objects.filter(slug=version_slug, project=project).first() + if version: + build = version.builds.last() return project, version, build, filename def _get_project(self): - project, version, build, filename = self._resolve_resources() + project, _, _, _ = self._resolve_resources() return project def _get_version(self): - project, version, build, filename = self._resolve_resources() + _, version, _, _ = self._resolve_resources() return version def get(self, request, format=None): url = request.GET.get("url") + project_slug = request.GET.get("project-slug") + version_slug = request.GET.get("version-slug") if not url: - return JsonResponse( - {"error": "'url' GET attribute is required"}, - status=400, - ) + if not project_slug or not version_slug: + return JsonResponse( + { + "error": "'project-slug' and 'version-slug' GET attributes are required when not sending 'url'" + }, + status=400, + ) addons_version = request.GET.get("api-version") if not addons_version: @@ -148,6 +169,7 @@ def get(self, request, format=None): version, build, filename, + url, user=request.user, ) return JsonResponse(data, json_dumps_params={"indent": 4, "sort_keys": True}) @@ -199,6 +221,7 @@ def get( version=None, build=None, filename=None, + url=None, user=None, ): """ @@ -208,12 +231,12 @@ def get( best JSON structure for that particular version. """ if addons_version.major == 0: - return self._v0(project, version, build, filename, user) + return self._v0(project, version, build, filename, url, user) if addons_version.major == 1: - return self._v1(project, version, build, filename, user) + return self._v1(project, version, build, filename, url, user) - def _v0(self, project, version, build, filename, user): + def _v0(self, project, version, build, filename, url, user): """ Initial JSON data structure consumed by the JavaScript client. @@ -308,27 +331,6 @@ def _v0(self, project, version, build, filename, user): versions_active_built_not_hidden.values_list("slug", flat=True) ), }, - "doc_diff": { - "enabled": Feature.ADDONS_DOC_DIFF_DISABLED not in project_features, - # "http://test-builds-local.devthedocs.org/en/latest/index.html" - "base_url": resolver.resolve( - project=project, - # NOTE: we are using LATEST version to compare against to for now. - # Ideally, this should be configurable by the user. - version_slug=LATEST, - language=project.language, - filename=filename, - ) - if filename - else None, - "root_selector": "[role=main]", - "inject_styles": True, - # NOTE: `base_host` and `base_page` are not required, since - # we are constructing the `base_url` in the backend instead - # of the frontend, as the doc-diff extension does. - "base_host": "", - "base_page": "", - }, "flyout": { "enabled": Feature.ADDONS_FLYOUT_DISABLED not in project_features, "translations": [ @@ -347,22 +349,22 @@ def _v0(self, project, version, build, filename, user): "versions": [ { # TODO: name this field "display_name" - "slug": version.slug, + "slug": version_.slug, "url": resolver.resolve( project=project, - version_slug=version.slug, - external=version.type == EXTERNAL, + version_slug=version_.slug, + external=version_.type == EXTERNAL, ), } - for version in versions_active_built_not_hidden + for version_ in versions_active_built_not_hidden ], "downloads": [ { # TODO: name this field "display_name" "name": name, - "url": url, + "url": url_, } - for name, url in version_downloads + for name, url_ in version_downloads ], # TODO: find a way to get this data in a reliably way. # We don't have a simple way to map a URL to a file in the repository. @@ -417,6 +419,38 @@ def _v0(self, project, version, build, filename, user): }, } + # DocDiff depends on `url=` GET attribute. + # This attribute allows us to know the exact filename where the request was made. + # If we don't know the filename, we cannot return the data required by DocDiff to work. + # In that case, we just don't include the `doc_diff` object in the response. + if url: + data["addons"].update( + { + "doc_diff": { + "enabled": Feature.ADDONS_DOC_DIFF_DISABLED + not in project_features, + # "http://test-builds-local.devthedocs.org/en/latest/index.html" + "base_url": resolver.resolve( + project=project, + # NOTE: we are using LATEST version to compare against to for now. + # Ideally, this should be configurable by the user. + version_slug=LATEST, + language=project.language, + filename=filename, + ) + if filename + else None, + "root_selector": "[role=main]", + "inject_styles": True, + # NOTE: `base_host` and `base_page` are not required, since + # we are constructing the `base_url` in the backend instead + # of the frontend, as the doc-diff extension does. + "base_host": "", + "base_page": "", + }, + } + ) + # Update this data with the one generated at build time by the doctool if version and version.build_data: data.update(version.build_data) @@ -450,7 +484,7 @@ def _v0(self, project, version, build, filename, user): return data - def _v1(self, project, version, build, filename, user): + def _v1(self, project, version, build, filename, url, user): return { "api_version": "1", "comment": "Undefined yet. Use v0 for now",