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

Author: humitos

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 '</head>' '<script language="javascript" src="http://localhost:8000/readthedocs-addons.js"></script>\n</head>';
+        sub_filter '</head>' '<script async language="javascript" src="http://localhost:8000/readthedocs-addons.js"></script>\n<meta name="readthedocs-project-slug" content="$rtd_project" />\n<meta name="readthedocs-version-slug" content="latest" />\n</head>';
+        sub_filter_types text/html;
         sub_filter_last_modified on;
         sub_filter_once on;
     }

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,

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"
+        )

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",