Proxito: fix https and canonical redirects

When doing a https redirect we were adding the default language and
version (/en/latest/) if the current url didn't have those components.
And similar when doing a redirect to a canonical domain.

Fix #8183

Author: stsewd

readthedocs/proxito/constants.py

@@ -0,0 +1,3 @@
+REDIRECT_HTTPS = 'https'
+REDIRECT_CANONICAL_CNAME = 'canonical-cname'
+REDIRECT_SUBPROJECT_MAIN_DOMAIN = 'subproject-main-domain'

readthedocs/proxito/middleware.py

@@ -16,6 +16,7 @@
 from django.utils.deprecation import MiddlewareMixin
 
 from readthedocs.projects.models import Domain, Project, ProjectRelationship
+from readthedocs.proxito import constants
 
 log = logging.getLogger(__name__)  # noqa
 
@@ -65,13 +66,13 @@ def map_host_to_project_slug(request):  # pylint: disable=too-many-return-statem
                 https=True,
             ).exists():
                 log.debug('Proxito Public Domain -> Canonical Domain Redirect: host=%s', host)
-                request.canonicalize = 'canonical-cname'
+                request.canonicalize = constants.REDIRECT_CANONICAL_CNAME
             elif (
                 ProjectRelationship.objects.
                 filter(child__slug=project_slug).exists()
             ):
                 log.debug('Proxito Public Domain -> Subproject Main Domain Redirect: host=%s', host)
-                request.canonicalize = 'subproject-main-domain'
+                request.canonicalize = constants.REDIRECT_SUBPROJECT_MAIN_DOMAIN
             return project_slug
 
         # TODO: This can catch some possibly valid domains (docs.readthedocs.io.com) for example
@@ -107,7 +108,7 @@ def map_host_to_project_slug(request):  # pylint: disable=too-many-return-statem
         if domain.https and not request.is_secure():
             # Redirect HTTP -> HTTPS (302) for this custom domain
             log.debug('Proxito CNAME HTTPS Redirect: host=%s', host)
-            request.canonicalize = 'https'
+            request.canonicalize = constants.REDIRECT_HTTPS
 
         # NOTE: consider redirecting non-canonical custom domains to the canonical one
         # Whether that is another custom domain or the public domain

readthedocs/proxito/tests/test_redirects.py

@@ -26,6 +26,28 @@ def test_root_url(self):
             r['Location'], 'https://project.dev.readthedocs.io/en/latest/',
         )
 
+    def test_custom_domain_root_url(self):
+        self.domain.canonical = True
+        self.domain.save()
+
+        r = self.client.get('/', HTTP_HOST=self.domain.domain, secure=True)
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(
+            r['Location'], f'https://{self.domain.domain}/en/latest/',
+        )
+        self.assertEqual(r['X-RTD-Redirect'], 'system')
+
+    def test_custom_domain_root_url_no_slash(self):
+        self.domain.canonical = True
+        self.domain.save()
+
+        r = self.client.get('', HTTP_HOST=self.domain.domain, secure=True)
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(
+            r['Location'], f'https://{self.domain.domain}/en/latest/',
+        )
+        self.assertEqual(r['X-RTD-Redirect'], 'system')
+
     def test_single_version_root_url_doesnt_redirect(self):
         self.project.single_version = True
         self.project.save()
@@ -121,7 +143,7 @@ def test_canonicalize_https_redirect(self):
         r = self.client.get('/', HTTP_HOST=self.domain.domain)
         self.assertEqual(r.status_code, 302)
         self.assertEqual(
-            r['Location'], f'https://{self.domain.domain}/en/latest/',
+            r['Location'], f'https://{self.domain.domain}/',
         )
         self.assertEqual(r['X-RTD-Redirect'], 'https')
 
@@ -141,7 +163,7 @@ def test_canonicalize_public_domain_to_cname_redirect(self):
         r = self.client.get('/', HTTP_HOST='project.dev.readthedocs.io')
         self.assertEqual(r.status_code, 302)
         self.assertEqual(
-            r['Location'], f'https://{self.domain.domain}/en/latest/',
+            r['Location'], f'https://{self.domain.domain}/',
         )
         self.assertEqual(r['X-RTD-Redirect'], 'canonical-cname')
 
@@ -153,6 +175,22 @@ def test_canonicalize_public_domain_to_cname_redirect(self):
         )
         self.assertEqual(r['X-RTD-Redirect'], 'canonical-cname')
 
+    def test_translation_redirect(self):
+        r = self.client.get('/', HTTP_HOST='translation.dev.readthedocs.io')
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(
+            r['Location'], f'https://project.dev.readthedocs.io/es/latest/',
+        )
+        self.assertEqual(r['X-RTD-Redirect'], 'system')
+
+    def test_translation_secure_redirect(self):
+        r = self.client.get('/', HTTP_HOST='translation.dev.readthedocs.io', secure=True)
+        self.assertEqual(r.status_code, 302)
+        self.assertEqual(
+            r['Location'], f'https://project.dev.readthedocs.io/es/latest/',
+        )
+        self.assertEqual(r['X-RTD-Redirect'], 'system')
+
     # We are not canonicalizing custom domains -> public domain for now
     @pytest.mark.xfail(strict=True)
     def test_canonicalize_cname_to_public_domain_redirect(self):

readthedocs/proxito/views/mixins.py

@@ -2,20 +2,24 @@
 import logging
 import mimetypes
 from urllib.parse import urlparse, urlunparse
-from slugify import slugify as unicode_slugify
 
 from django.conf import settings
 from django.http import (
     HttpResponse,
-    HttpResponseRedirect,
     HttpResponsePermanentRedirect,
+    HttpResponseRedirect,
 )
 from django.shortcuts import render
 from django.utils.encoding import iri_to_uri
 from django.views.static import serve
+from slugify import slugify as unicode_slugify
 
 from readthedocs.builds.constants import EXTERNAL, INTERNAL
 from readthedocs.core.resolver import resolve
+from readthedocs.proxito.constants import (
+    REDIRECT_CANONICAL_CNAME,
+    REDIRECT_HTTPS,
+)
 from readthedocs.redirects.exceptions import InfiniteRedirectException
 from readthedocs.storage import build_media_storage
 
@@ -169,19 +173,40 @@ def canonical_redirect(self, request, final_project, version_slug, filename):
         """
         Return a redirect to the canonical domain including scheme.
 
-        This is normally used HTTP -> HTTPS redirects or redirects to/from custom domains.
+        The following cases are covered:
+
+        - Redirect a custom from http to https (if supported)
+          http://project.rtd.io/ -> https://project.rtd.io/
+        - Redirect a domain to a canonical domain (http or https).
+          http://project.rtd.io/ -> https://docs.test.com/
+          http://project.rtd.io/foo/bar/ -> https://docs.test.com/foo/bar/
+        - Redirect from a subproject domain to the main domain
+          https://subproject.rtd.io/en/latest/foo -> https://main.rtd.io/projects/subproject/en/latest/foo  # noqa
+          https://subproject.rtd.io/en/latest/foo -> https://docs.test.com/projects/subproject/en/latest/foo  # noqa
         """
-        full_path = request.get_full_path()
-        urlparse_result = urlparse(full_path)
-        to = resolve(
-            project=final_project,
-            version_slug=version_slug,
-            filename=filename,
-            query_params=urlparse_result.query,
-            external=hasattr(request, 'external_domain'),
-        )
+        from_url = request.build_absolute_uri()
+        parsed_from = urlparse(from_url)
 
-        if full_path == to:
+        redirect_type = getattr(request, 'canonicalize', None)
+        if redirect_type == REDIRECT_HTTPS:
+            to = parsed_from._replace(scheme='https').geturl()
+        else:
+            to = resolve(
+                project=final_project,
+                version_slug=version_slug,
+                filename=filename,
+                query_params=parsed_from.query,
+                external=hasattr(request, 'external_domain'),
+            )
+            # When a canonical redirect is done, only change the domain.
+            if redirect_type == REDIRECT_CANONICAL_CNAME:
+                to_parsed = urlparse(to)
+                to = parsed_from._replace(
+                    scheme=to_parsed.scheme,
+                    netloc=to_parsed.netloc,
+                ).geturl()
+
+        if from_url == to:
             # check that we do have a response and avoid infinite redirect
             log.warning(
                 'Infinite Redirect: FROM URL is the same than TO URL. url=%s',