Proxito: refactor canocanical redirects

Closes #10065

Author: stsewd

readthedocs/core/resolver.py

@@ -1,12 +1,12 @@
 """URL resolver for documentation."""
-
 from urllib.parse import urlunparse
 
 import structlog
 from django.conf import settings
 
 from readthedocs.builds.constants import EXTERNAL
 from readthedocs.core.utils.extend import SettingsOverrideObject
+from readthedocs.core.utils.url import join_url_path
 
 log = structlog.get_logger(__name__)
 
@@ -214,6 +214,58 @@ def resolve(
         )
         return urlunparse((protocol, domain, path, '', query_params, ''))
 
+    def _get_root_path(self, project):
+        """
+        Returns the root path for a project.
+
+        If the project is a subproject, it will return ``/projects/<project-slug>/``.
+        If the project is a main project, it will return ``/``.
+        This will respect the custom urlconf of the project if it's defined.
+        """
+        custom_prefix = None
+        if project.urlconf:
+            custom_prefix = project.urlconf.split("$", 1)[0]
+
+        if project.is_subproject:
+            prefix = custom_prefix or "projects"
+            return join_url_path(prefix, project.slug, "/")
+
+        prefix = custom_prefix or "/"
+        return join_url_path(prefix, "/")
+
+    def resolve_root(self, project, external_version_slug=None):
+        """
+        Get the root URL from where the documentation of ``project`` is served from.
+
+        This doesn't include the version or language. For example:
+
+        - https://docs.example.com/projects/<project-slug>/
+        - https://docs.readthedocs.io/
+
+        This will respect the custom urlconf of the project if it's defined.
+
+        :param project: Project object to get the root URL from
+        :param external_version_slug: If given, resolve using the external version domain.
+        """
+        canonical_project = self._get_canonical_project(project)
+        use_custom_domain = self._use_cname(canonical_project)
+        custom_domain = canonical_project.get_canonical_custom_domain()
+        if external_version_slug:
+            domain = self._get_external_subdomain(
+                canonical_project, external_version_slug
+            )
+            use_https = settings.PUBLIC_DOMAIN_USES_HTTPS
+        elif use_custom_domain and custom_domain:
+            domain = custom_domain.domain
+            use_https = custom_domain.https
+        else:
+            domain = self._get_project_subdomain(canonical_project)
+            use_https = settings.PUBLIC_DOMAIN_USES_HTTPS
+
+        protocol = "https" if use_https else "http"
+        path = self._get_root_path(project)
+        return urlunparse((protocol, domain, path, "", "", ""))
+
     def _get_canonical_project_data(self, project):
         """
         Returns a tuple with (project, subproject_slug) from the canonical project of `project`.

readthedocs/core/utils/url.py

@@ -0,0 +1,15 @@
+"""URL hadling utilities."""
+
+
+def join_url_path(base, *args):
+    """
+    Joins a base URL path with one or more path components.
+
+    This does a simple join of the base path with the path components,
+    inserting a slash between each component.
+    It does not offer protection against directory traversal attacks.
+    """
+    base = "/" + base.lstrip("/")
+    for path in args:
+        base = base.rstrip("/") + "/" + path.lstrip("/")
+    return base

readthedocs/projects/models.py

@@ -753,7 +753,7 @@ class ProxitoURLConf:
 
         return ProxitoURLConf
 
-    @property
+    @cached_property
     def is_subproject(self):
         """Return whether or not this project is a subproject."""
         return self.superprojects.exists()

readthedocs/proxito/tests/test_full.py

@@ -1395,11 +1395,13 @@ def test_cache_on_private_versions_custom_domain(self):
         self.domain.save()
         self._test_cache_control_header_project(expected_value='private', host=self.domain.domain)
 
-        # HTTPS redirect respects the privacy level of the version.
-        resp = self.client.get('/en/latest/', secure=False, HTTP_HOST=self.domain.domain)
-        self.assertEqual(resp['Location'], f'https://{self.domain.domain}/en/latest/')
-        self.assertEqual(resp.headers['CDN-Cache-Control'], 'private')
-        self.assertEqual(resp.headers['Cache-Tag'], 'project,project:latest')
+        # HTTPS redirects can always be cached.
+        resp = self.client.get(
+            "/en/latest/", secure=False, HTTP_HOST=self.domain.domain
+        )
+        self.assertEqual(resp["Location"], f"https://{self.domain.domain}/en/latest/")
+        self.assertEqual(resp.headers["CDN-Cache-Control"], "public")
+        self.assertEqual(resp.headers["Cache-Tag"], "project")
 
     def test_cache_public_versions(self):
         self.project.versions.update(privacy_level=PUBLIC)
@@ -1427,15 +1429,18 @@ def test_cache_on_private_versions_custom_domain_subproject(self):
         self.domain.save()
         self._test_cache_control_header_subproject(expected_value='private', host=self.domain.domain)
 
-        # HTTPS redirect respects the privacy level of the version.
+        # HTTPS redirects can always be cached.
         resp = self.client.get(
             '/projects/subproject/en/latest/',
             secure=False,
             HTTP_HOST=self.domain.domain,
         )
-        self.assertEqual(resp['Location'], f'https://{self.domain.domain}/projects/subproject/en/latest/')
-        self.assertEqual(resp.headers['CDN-Cache-Control'], 'private')
-        self.assertEqual(resp.headers['Cache-Tag'], 'subproject,subproject:latest')
+        self.assertEqual(
+            resp["Location"],
+            f"https://{self.domain.domain}/projects/subproject/en/latest/",
+        )
+        self.assertEqual(resp.headers["CDN-Cache-Control"], "public")
+        self.assertEqual(resp.headers["Cache-Tag"], "project")
 
     def test_cache_public_versions_subproject(self):
         self.subproject.versions.update(privacy_level=PUBLIC)
@@ -1447,15 +1452,18 @@ def test_cache_public_versions_custom_domain(self):
         self.domain.save()
         self._test_cache_control_header_subproject(expected_value='public', host=self.domain.domain)
 
-        # HTTPS redirect respects the privacy level of the version.
+        # HTTPS redirects can always be cached.
         resp = self.client.get(
             '/projects/subproject/en/latest/',
             secure=False,
             HTTP_HOST=self.domain.domain,
         )
-        self.assertEqual(resp['Location'], f'https://{self.domain.domain}/projects/subproject/en/latest/')
-        self.assertEqual(resp.headers['CDN-Cache-Control'], 'public')
-        self.assertEqual(resp.headers['Cache-Tag'], 'subproject,subproject:latest')
+        self.assertEqual(
+            resp["Location"],
+            f"https://{self.domain.domain}/projects/subproject/en/latest/",
+        )
+        self.assertEqual(resp.headers["CDN-Cache-Control"], "public")
+        self.assertEqual(resp.headers["Cache-Tag"], "project")
 
     def test_cache_disable_on_rtd_header_resolved_project(self):
         get(

readthedocs/proxito/tests/test_middleware.py

@@ -116,8 +116,12 @@ def test_subproject_redirect(self):
         )
         resp = self.client.get(self.url, HTTP_HOST="subproject.dev.readthedocs.io")
         self.assertEqual(resp.status_code, 302)
-        self.assertEqual(resp["location"], f"http://pip.dev.readthedocs.io/")
-        self.assertEqual(resp["X-RTD-Redirect"], RedirectType.to_canonical_domain.name)
+        self.assertEqual(
+            resp["location"], f"http://pip.dev.readthedocs.io/projects/subproject/"
+        )
+        self.assertEqual(
+            resp["X-RTD-Redirect"], RedirectType.subproject_to_main_domain.name
+        )
 
     # We are not canonicalizing custom domains -> public domain for now
     @pytest.mark.xfail(strict=True)

readthedocs/proxito/tests/test_redirects.py

@@ -83,7 +83,8 @@ def test_subproject_redirect(self):
         r = self.client.get('/', HTTP_HOST='subproject.dev.readthedocs.io')
         self.assertEqual(r.status_code, 302)
         self.assertEqual(
-            r['Location'], 'https://project.dev.readthedocs.io/projects/subproject/en/latest/',
+            r["Location"],
+            "https://project.dev.readthedocs.io/projects/subproject/",
         )
 
         r = self.client.get('/en/latest/', HTTP_HOST='subproject.dev.readthedocs.io')

readthedocs/proxito/views/mixins.py

@@ -19,7 +19,8 @@
 from readthedocs.analytics.utils import get_client_ip
 from readthedocs.audit.models import AuditLog
 from readthedocs.builds.constants import EXTERNAL, INTERNAL
-from readthedocs.core.resolver import resolve
+from readthedocs.core.resolver import resolve, resolver
+from readthedocs.core.utils.url import join_url_path
 from readthedocs.projects.constants import MEDIA_TYPE_HTML
 from readthedocs.proxito.constants import RedirectType
 from readthedocs.redirects.exceptions import InfiniteRedirectException
@@ -331,10 +332,8 @@ def canonical_redirect(
         self,
         request,
         final_project,
-        version_slug,
-        filename,
         redirect_type,
-        is_external_version=False,
+        external_version_slug=None,
     ):
         """
         Return a redirect to the canonical domain including scheme.
@@ -352,34 +351,30 @@ def canonical_redirect(
 
         :param request: Request object.
         :param final_project: The current project being served.
-        :param version_slug: The current version slug being served.
-        :param filename: The filename being served.
         :param redirect_type: The type of canonical redirect (https, canonical-cname, subproject-main-domain)
-        :param external: If the version is from a pull request preview.
+        :param external_version_slug: The version slug if the request is from a pull request preview.
         """
         from_url = request.build_absolute_uri()
         parsed_from = urlparse(from_url)
 
         if redirect_type == RedirectType.http_to_https:
             to = parsed_from._replace(scheme="https").geturl()
-        elif redirect_type in [
-            RedirectType.to_canonical_domain,
-            RedirectType.subproject_to_main_domain,
-        ]:
-            to = resolve(
+        elif redirect_type == RedirectType.to_canonical_domain:
+            canonical_domain = final_project.get_canonical_custom_domain()
+            protocol = "https" if canonical_domain.https else "http"
+            to = parsed_from._replace(
+                scheme=protocol, netloc=canonical_domain.domain
+            ).geturl()
+        elif redirect_type == RedirectType.subproject_to_main_domain:
+            project_doc_root = resolver.resolve_root(
                 project=final_project,
-                version_slug=version_slug,
-                filename=filename,
-                query_params=parsed_from.query,
-                external=is_external_version,
+                external_version_slug=external_version_slug,
             )
-            # When a canonical redirect is done, only change the domain.
-            if redirect_type == RedirectType.to_canonical_domain:
-                parsed_to = urlparse(to)
-                to = parsed_from._replace(
-                    scheme=parsed_to.scheme,
-                    netloc=parsed_to.netloc,
-                ).geturl()
+            parsed_doc_root = urlparse(project_doc_root)
+            to = parsed_doc_root._replace(
+                path=join_url_path(parsed_doc_root.path, parsed_from.path),
+                query=parsed_from.query,
+            ).geturl()
         else:
             raise NotImplementedError
 
@@ -391,7 +386,11 @@ def canonical_redirect(
             )
             raise InfiniteRedirectException()
 
-        log.info('Canonical Redirect.', host=request.get_host(), from_url=filename, to_url=to)
+        log.info(
+            "Canonical Redirect.", host=request.get_host(), from_url=from_url, to_url=to
+        )
+        # Canocanical redirects can be cached, since the final URL will check for authz.
+        self.cache_request = True
         resp = HttpResponseRedirect(to)
         resp["X-RTD-Redirect"] = redirect_type.name
         return resp

readthedocs/proxito/views/serve.py

@@ -89,6 +89,23 @@ def get(self,
         ``subproject_slash`` is used to determine if the subproject URL has a slash,
         so that we can decide if we need to serve docs or add a /.
         """
+        unresolved_domain = request.unresolved_domain
+        # Handle requests that need canonicalizing (eg. HTTP -> HTTPS, redirect to canonical domain).
+        redirect_type = self._get_canonical_redirect_type(request)
+        if redirect_type:
+            # Attach the current project to the request.
+            request.path_project_slug = unresolved_domain.project.slug
+            try:
+                return self.canonical_redirect(
+                    request=request,
+                    final_project=unresolved_domain.project,
+                    external_version_slug=unresolved_domain.external_version_slug,
+                    redirect_type=redirect_type,
+                )
+            except InfiniteRedirectException:
+                # Don't redirect in this case, since it would break things.
+                pass
+
         version_slug = self.get_version_from_host(request, version_slug)
         final_project, lang_slug, version_slug, filename = _get_project_data_from_request(  # noqa
             request,
@@ -100,7 +117,7 @@ def get(self,
         )
         version = final_project.versions.filter(slug=version_slug).first()
 
-        is_external = request.unresolved_domain.is_from_external_domain
+        is_external = unresolved_domain.is_from_external_domain
 
         log.bind(
             project_slug=final_project.slug,
@@ -137,26 +154,6 @@ def get(self,
         if spam_response:
             return spam_response
 
-        # Handle requests that need canonicalizing (eg. HTTP -> HTTPS, redirect to canonical domain)
-        redirect_type = self._get_canonical_redirect_type(request)
-        if redirect_type:
-            # A canonical redirect can be cached, if we don't have information
-            # about the version, since the final URL will check for authz.
-            if not version and self._is_cache_enabled(final_project):
-                self.cache_request = True
-            try:
-                return self.canonical_redirect(
-                    request=request,
-                    final_project=final_project,
-                    version_slug=version_slug,
-                    filename=filename,
-                    redirect_type=redirect_type,
-                    is_external_version=is_external,
-                )
-            except InfiniteRedirectException:
-                # Don't redirect in this case, since it would break things
-                pass
-
         # Handle a / redirect when we aren't a single version
         if all([
                 lang_slug is None,
@@ -252,6 +249,16 @@ def _get_canonical_redirect_type(self, request):
                 log.debug("Proxito CNAME HTTPS Redirect.", domain=domain.domain)
                 return RedirectType.http_to_https
 
+        # Check for subprojects before checking for canonical domains,
+        # so we can redirect to the main domain first.
+        # Custom domains on subprojects are not supported.
+        if project.is_subproject:
+            log.debug(
+                "Proxito Public Domain -> Subproject Main Domain Redirect.",
+                project_slug=project.slug,
+            )
+            return RedirectType.subproject_to_main_domain
+
         if unresolved_domain.is_from_public_domain:
             canonical_domain = (
                 Domain.objects.filter(project=project)
@@ -265,13 +272,6 @@ def _get_canonical_redirect_type(self, request):
                 )
                 return RedirectType.to_canonical_domain
 
-        if project.is_subproject:
-            log.debug(
-                "Proxito Public Domain -> Subproject Main Domain Redirect.",
-                project_slug=project.slug,
-            )
-            return RedirectType.subproject_to_main_domain
-
         return None