From 40bfe63fd9db2bfa86f83c412cbb3baca104373a Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 28 Aug 2023 13:06:26 -0500 Subject: [PATCH] Proxito: remove old implementation (#10660) Mostly removing old code, there is only one small custom addition https://github.com/readthedocs/readthedocs.org/blob/6595bdac198b482e1279c0e3b1925198f6baa631/readthedocs/proxito/views/serve.py#L144-L146 Django doesn't capture the first slash, previously we were getting the path from request.path_info, that includes the slash! When a request is done to `/` the path is set to "" (empty), our code relies on that being `/`. - Closes https://github.com/readthedocs/readthedocs.org/issues/10408 - Closes https://github.com/readthedocs/readthedocs.org/issues/8399 ### How to deploy this change - Deploy everything as usual, but don't run the `projects 0105` migration!!! - After deploy is done, run the `projects 0105` migration --- readthedocs/api/v2/serializers.py | 1 - readthedocs/core/resolver.py | 35 -- readthedocs/core/unresolver.py | 2 + readthedocs/projects/fixtures/test_data.json | 17 - .../migrations/0105_remove_project_urlconf.py | 20 + readthedocs/projects/models.py | 135 +----- readthedocs/proxito/middleware.py | 22 - .../tests/test_custom_path_prefixes.py | 10 - readthedocs/proxito/tests/test_full.py | 70 +-- readthedocs/proxito/tests/test_headers.py | 15 +- readthedocs/proxito/tests/test_middleware.py | 196 -------- .../proxito/tests/test_old_redirects.py | 159 ++----- readthedocs/proxito/tests/test_redirects.py | 13 - readthedocs/proxito/tests/test_urls.py | 127 ----- readthedocs/proxito/urls.py | 48 +- readthedocs/proxito/views/decorators.py | 75 --- readthedocs/proxito/views/mixins.py | 16 +- readthedocs/proxito/views/serve.py | 440 ++++-------------- readthedocs/proxito/views/utils.py | 69 +-- readthedocs/rtd_tests/tests/test_api.py | 1 - readthedocs/rtd_tests/tests/test_resolver.py | 16 - 21 files changed, 189 insertions(+), 1298 deletions(-) create mode 100644 readthedocs/projects/migrations/0105_remove_project_urlconf.py delete mode 100644 readthedocs/proxito/tests/test_urls.py delete mode 100644 readthedocs/proxito/views/decorators.py diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index c48e6ff1305..2de36eec55c 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -30,7 +30,6 @@ class Meta: "documentation_type", "users", "canonical_url", - "urlconf", "custom_prefix", ) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index dd569a18f14..650ab417c53 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -64,7 +64,6 @@ def base_resolve_path( project_relationship=None, subdomain=None, cname=None, - urlconf=None, custom_prefix=None, ): """ @@ -97,38 +96,6 @@ def base_resolve_path( else: path = unsafe_join_url_path(path, "{language}/{version}/{filename}") - # TODO: remove this when all projects have migrated to path prefixes. - # Allow users to override their own URLConf - # If a custom prefix is given, we don't use the custom URLConf, - # since they are not compatible with each other. - # We also don't check if the project has the new proxito implementation - # enabled, this is so we can start generating links with the new - # custom prefixes without starting to serve docs with it (this helps to ease - # the migration from urlconf to custom prefixes). - if urlconf and not custom_prefix: - path = urlconf - path = path.replace( - "$version", - "{version}", - ) - path = path.replace( - '$language', - '{language}', - ) - path = path.replace( - '$filename', - '{filename}', - ) - path = path.replace( - "$subproject", - "{subproject}", - ) - if "$" in path: - log.warning( - "Unconverted variable in a resolver URLConf.", - path=path, - ) - subproject_alias = project_relationship.alias if project_relationship else "" return path.format( project=project_slug, @@ -147,7 +114,6 @@ def resolve_path( single_version=None, subdomain=None, cname=None, - urlconf=None, ): """Resolve a URL with a subset of fields defined.""" version_slug = version_slug or project.get_default_version() @@ -181,7 +147,6 @@ def resolve_path( project_relationship=project_relationship, cname=cname, subdomain=subdomain, - urlconf=urlconf or project.urlconf, custom_prefix=custom_prefix, ) diff --git a/readthedocs/core/unresolver.py b/readthedocs/core/unresolver.py index d65f5d42d31..82a469aca29 100644 --- a/readthedocs/core/unresolver.py +++ b/readthedocs/core/unresolver.py @@ -214,6 +214,8 @@ def unresolve_path(self, unresolved_domain, path, append_indexhtml=True): :param append_indexhtml: If `True` directories will be normalized to end with ``/index.html``. """ + # Make sure we always have a leading slash. + path = self._normalize_filename(path) # We don't call unparse() on the path, # since it could be parsed as a full URL if it starts with a protocol. parsed_url = ParseResult( diff --git a/readthedocs/projects/fixtures/test_data.json b/readthedocs/projects/fixtures/test_data.json index b87506c1b24..c3f28045f75 100644 --- a/readthedocs/projects/fixtures/test_data.json +++ b/readthedocs/projects/fixtures/test_data.json @@ -17,7 +17,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -70,7 +69,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -123,7 +121,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -176,7 +173,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -229,7 +225,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -282,7 +277,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -335,7 +329,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -388,7 +381,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -441,7 +433,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -494,7 +485,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -547,7 +537,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -600,7 +589,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -653,7 +641,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -706,7 +693,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -757,7 +743,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -808,7 +793,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, @@ -859,7 +843,6 @@ "default_branch": null, "requirements_file": null, "documentation_type": "sphinx", - "urlconf": null, "external_builds_enabled": false, "external_builds_privacy_level": "public", "cdn_enabled": false, diff --git a/readthedocs/projects/migrations/0105_remove_project_urlconf.py b/readthedocs/projects/migrations/0105_remove_project_urlconf.py new file mode 100644 index 00000000000..e5a20a60771 --- /dev/null +++ b/readthedocs/projects/migrations/0105_remove_project_urlconf.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.4 on 2023-08-23 20:35 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("projects", "0104_alter_httpheader_value"), + ] + + operations = [ + migrations.RemoveField( + model_name="historicalproject", + name="urlconf", + ), + migrations.RemoveField( + model_name="project", + name="urlconf", + ), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index c4449802f26..729327c2c3f 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -3,7 +3,6 @@ import hashlib import hmac import os -import re from shlex import quote from urllib.parse import urlparse @@ -15,18 +14,16 @@ from django.core.validators import MaxValueValidator, MinValueValidator from django.db import models from django.db.models import Prefetch -from django.urls import include, re_path, reverse +from django.urls import reverse from django.utils import timezone from django.utils.crypto import get_random_string from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ -from django.views import defaults from django_extensions.db.fields import CreationDateTimeField, ModificationDateTimeField from django_extensions.db.models import TimeStampedModel from taggit.managers import TaggableManager from readthedocs.builds.constants import EXTERNAL, INTERNAL, LATEST, STABLE -from readthedocs.constants import pattern_opts from readthedocs.core.history import ExtraHistoricalRecords from readthedocs.core.resolver import resolve, resolve_domain from readthedocs.core.utils import extract_valid_attributes_for_model, slugify @@ -230,18 +227,6 @@ class Project(models.Model): 'DirectoryHTMLBuilder">More info on sphinx builders.', ), ) - # NOTE: This is deprecated, use the `custom_prefix*` attributes instead. - urlconf = models.CharField( - _('Documentation URL Configuration'), - max_length=255, - default=None, - blank=True, - null=True, - help_text=_( - 'Supports the following keys: $language, $version, $subproject, $filename. ' - 'An example: `$language/$version/$filename`.' - ), - ) custom_prefix = models.CharField( _("Custom path prefix"), @@ -718,7 +703,7 @@ def proxied_api_prefix(self): """ Get the path prefix for proxied API paths (``/_/``). - Returns `None` if the project doesn't have a custom urlconf. + Returns `None` if the project doesn't have a custom prefix. """ # When using a custom prefix, we can only handle serving # docs pages under the prefix, not special paths like `/_/`. @@ -730,10 +715,6 @@ def proxied_api_prefix(self): Feature.USE_PROXIED_APIS_WITH_PREFIX ): return self.custom_prefix - if self.urlconf: - # Return the value before the first defined variable, - # as that is a prefix and not part of our normal doc patterns. - return self.urlconf.split("$", 1)[0] return None @cached_property @@ -752,111 +733,6 @@ def subproject_prefix(self): return None return parent_relationship.subproject_prefix - @property - def regex_urlconf(self): - """ - Convert User's URLConf into a proper django URLConf. - - This replaces the user-facing syntax with the regex syntax. - """ - to_convert = re.escape(self.urlconf) - - # We should standardize these names so we can loop over them easier - to_convert = to_convert.replace( - '\\$version', - '(?P{regex})'.format(regex=pattern_opts['version_slug']) - ) - to_convert = to_convert.replace( - '\\$language', - '(?P{regex})'.format(regex=pattern_opts['lang_slug']) - ) - to_convert = to_convert.replace( - '\\$filename', - '(?P{regex})'.format(regex=pattern_opts['filename_slug']) - ) - to_convert = to_convert.replace( - '\\$subproject', - '(?P{regex})'.format(regex=pattern_opts['project_slug']) - ) - - if '\\$' in to_convert: - log.warning( - 'Unconverted variable in a project URLConf.', - project_slug=self.slug, - to_convert=to_convert, - ) - return to_convert - - @property - def proxito_urlconf(self): - """ - Returns a URLConf class that is dynamically inserted via proxito. - - It is used for doc serving on projects that have their own ``urlconf``. - """ - from readthedocs.projects.views.public import ProjectDownloadMedia - from readthedocs.proxito.urls import core_urls - from readthedocs.proxito.views.serve import ServeDocs, ServeStaticFiles - from readthedocs.proxito.views.utils import proxito_404_page_handler - - class ProxitoURLConf: - - """A URLConf dynamically inserted by Proxito.""" - - proxied_urls = [ - re_path( - r'{proxied_api_url}api/v2/'.format( - proxied_api_url=re.escape(self.proxied_api_url), - ), - include('readthedocs.api.v2.proxied_urls'), - name='user_proxied_api' - ), - re_path( - r'{proxied_api_url}downloads/' - r'(?P{lang_slug})/' - r'(?P{version_slug})/' - r'(?P[-\w]+)/$'.format( - proxied_api_url=re.escape(self.proxied_api_url), - **pattern_opts), - ProjectDownloadMedia.as_view(same_domain_url=True), - name='user_proxied_downloads' - ), - re_path( - r"{proxied_api_url}static/" - r"(?P{filename_slug})$".format( - proxied_api_url=re.escape(self.proxied_api_url), - **pattern_opts, - ), - ServeStaticFiles.as_view(), - name="proxito_static_files", - ), - ] - docs_urls = [ - re_path( - '^{regex_urlconf}$'.format(regex_urlconf=self.regex_urlconf), - ServeDocs.as_view(), - name='user_proxied_serve_docs' - ), - # paths for redirects at the root - re_path( - '^{proxied_api_url}$'.format( - proxied_api_url=re.escape(self.urlconf.split('$', 1)[0]), - ), - ServeDocs.as_view(), - name='user_proxied_serve_docs_subpath_redirect' - ), - re_path( - '^(?P{regex})$'.format(regex=pattern_opts['filename_slug']), - ServeDocs.as_view(), - name='user_proxied_serve_docs_root_redirect' - ), - ] - urlpatterns = proxied_urls + core_urls + docs_urls - handler404 = proxito_404_page_handler - handler500 = defaults.server_error - - return ProxitoURLConf - @cached_property def is_subproject(self): """Return whether or not this project is a subproject.""" @@ -1935,7 +1811,6 @@ def add_features(sender, **kwargs): ALLOW_FORCED_REDIRECTS = "allow_forced_redirects" DISABLE_PAGEVIEWS = "disable_pageviews" RESOLVE_PROJECT_FROM_HEADER = "resolve_project_from_header" - USE_UNRESOLVER_WITH_PROXITO = "use_unresolver_with_proxito" USE_PROXIED_APIS_WITH_PREFIX = "use_proxied_apis_with_prefix" ALLOW_VERSION_WARNING_BANNER = "allow_version_warning_banner" @@ -2006,12 +1881,6 @@ def add_features(sender, **kwargs): RESOLVE_PROJECT_FROM_HEADER, _("Proxito: Allow usage of the X-RTD-Slug header"), ), - ( - USE_UNRESOLVER_WITH_PROXITO, - _( - "Proxito: Use new unresolver implementation for serving documentation files." - ), - ), ( USE_PROXIED_APIS_WITH_PREFIX, _( diff --git a/readthedocs/proxito/middleware.py b/readthedocs/proxito/middleware.py index 1de3b1cfbea..d779972124a 100644 --- a/readthedocs/proxito/middleware.py +++ b/readthedocs/proxito/middleware.py @@ -6,7 +6,6 @@ Additional processing is done to get the project from the URL in the ``views.py`` as well. """ import re -import sys from urllib.parse import urlparse import structlog @@ -26,7 +25,6 @@ unresolver, ) from readthedocs.core.utils import get_cache_tag -from readthedocs.projects.models import Feature from readthedocs.proxito.cache import add_cache_tags, cache_response, private_response from readthedocs.proxito.redirects import redirect_to_https @@ -264,26 +262,6 @@ def process_request(self, request): # noqa project_slug=project.slug, ) - # This is hacky because Django wants a module for the URLConf, - # instead of also accepting string - if project.urlconf and not project.has_feature( - Feature.USE_UNRESOLVER_WITH_PROXITO - ): - # Stop Django from caching URLs - # https://github.com/django/django/blob/7cf7d74/django/urls/resolvers.py#L65-L69 # noqa - project_timestamp = project.modified_date.strftime("%Y%m%d.%H%M%S%f") - url_key = f"readthedocs.urls.fake.{project.slug}.{project_timestamp}" - - log.info( - "Setting URLConf", - project_slug=project.slug, - url_key=url_key, - urlconf=project.urlconf, - ) - if url_key not in sys.modules: - sys.modules[url_key] = project.proxito_urlconf - request.urlconf = url_key - return None def add_hosting_integrations_headers(self, request, response): diff --git a/readthedocs/proxito/tests/test_custom_path_prefixes.py b/readthedocs/proxito/tests/test_custom_path_prefixes.py index 6f8574ed6dd..f74eee7c7f4 100644 --- a/readthedocs/proxito/tests/test_custom_path_prefixes.py +++ b/readthedocs/proxito/tests/test_custom_path_prefixes.py @@ -1,7 +1,5 @@ from django.test.utils import override_settings -from django_dynamic_fixture import get -from readthedocs.projects.models import Feature from readthedocs.proxito.tests.base import BaseDocServing @@ -11,14 +9,6 @@ RTD_EXTERNAL_VERSION_DOMAIN="readthedocs.build", ) class TestCustomPathPrefixes(BaseDocServing): - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) def test_custom_prefix_multi_version_project(self): self.project.custom_prefix = "/custom/prefix/" diff --git a/readthedocs/proxito/tests/test_full.py b/readthedocs/proxito/tests/test_full.py index 9fec6d93b0f..715bc79c92a 100644 --- a/readthedocs/proxito/tests/test_full.py +++ b/readthedocs/proxito/tests/test_full.py @@ -362,18 +362,6 @@ def test_custom_domain_response_hsts(self): response['strict-transport-security'], 'max-age=3600; includeSubDomains; preload', ) - -class ProxitoV2TestFullDocServing(TestFullDocServing): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - def test_single_version_serving_projects_dir(self): self.project.single_version = True self.project.save() @@ -385,6 +373,28 @@ def test_single_version_serving_projects_dir(self): "/proxito/media/html/project/latest/projects/awesome.html", ) + def test_single_version_serving_language_like_subdir(self): + self.project.single_version = True + self.project.save() + url = "/en/api/awesome.html" + host = "project.dev.readthedocs.io" + resp = self.client.get(url, headers={"host": host}) + self.assertEqual( + resp["x-accel-redirect"], + "/proxito/media/html/project/latest/en/api/awesome.html", + ) + + def test_single_version_serving_language_like_dir(self): + self.project.single_version = True + self.project.save() + url = "/en/awesome.html" + host = "project.dev.readthedocs.io" + resp = self.client.get(url, headers={"host": host}) + self.assertEqual( + resp["x-accel-redirect"], + "/proxito/media/html/project/latest/en/awesome.html", + ) + @override_settings( PUBLIC_DOMAIN="dev.readthedocs.io", @@ -683,18 +693,6 @@ def test_track_downloads(self): self.assertEqual(log.action, AuditLog.DOWNLOAD) -class ProxitoV2TestDocServingBackends(TestDocServingBackends): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - @override_settings( PYTHON_MEDIA=False, PUBLIC_DOMAIN='readthedocs.io', @@ -1532,18 +1530,6 @@ def test_serve_invalid_static_file(self, staticfiles_storage): ) self.assertEqual(resp.status_code, 404) - -class ProxitoV2TestAdditionalDocViews(TestAdditionalDocViews): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - def test_404_download(self): response = self.client.get( reverse( @@ -1783,15 +1769,3 @@ def test_cache_disable_on_rtd_header_resolved_project(self): ) self.assertEqual(resp.headers["CDN-Cache-Control"], "private") self.assertEqual(resp.headers["Cache-Tag"], "project,project:latest") - - -class ProxitoV2TestCDNCache(TestCDNCache): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) diff --git a/readthedocs/proxito/tests/test_headers.py b/readthedocs/proxito/tests/test_headers.py index 28a0a8608dc..2f8fd6626d6 100644 --- a/readthedocs/proxito/tests/test_headers.py +++ b/readthedocs/proxito/tests/test_headers.py @@ -1,10 +1,9 @@ import django_dynamic_fixture as fixture from django.test import override_settings from django.urls import reverse -from django_dynamic_fixture import get from readthedocs.builds.constants import LATEST -from readthedocs.projects.models import Domain, Feature, HTTPHeader +from readthedocs.projects.models import Domain, HTTPHeader from .base import BaseDocServing @@ -210,15 +209,3 @@ def test_cache_headers_robots_txt_with_private_projects_allowed(self): self.assertEqual(r.status_code, 200) self.assertEqual(r["CDN-Cache-Control"], "public") self.assertEqual(r["Cache-Tag"], "project,project:sitemap.xml") - - -class ProxitoV2HeaderTests(ProxitoHeaderTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) diff --git a/readthedocs/proxito/tests/test_middleware.py b/readthedocs/proxito/tests/test_middleware.py index ab773baf815..bc4be6951be 100644 --- a/readthedocs/proxito/tests/test_middleware.py +++ b/readthedocs/proxito/tests/test_middleware.py @@ -1,5 +1,4 @@ # Copied from test_middleware.py -from unittest import mock import pytest from django.core.exceptions import SuspiciousOperation @@ -8,14 +7,12 @@ from django.test.utils import override_settings from django_dynamic_fixture import get -from readthedocs.builds.models import Version from readthedocs.projects.constants import PUBLIC from readthedocs.projects.models import Domain, Feature, Project, ProjectRelationship from readthedocs.proxito.constants import RedirectType from readthedocs.proxito.exceptions import DomainDNSHttp404 from readthedocs.proxito.middleware import ProxitoMiddleware from readthedocs.rtd_tests.base import RequestFactoryTestMixin -from readthedocs.rtd_tests.storage import BuildMediaFileSystemStorageTest from readthedocs.rtd_tests.utils import create_user from readthedocs.subscriptions.constants import TYPE_CNAME from readthedocs.subscriptions.products import RTDProductFeature @@ -256,196 +253,3 @@ def test_front_slash_url(self): self.assertEqual( res['Location'], '/google.com', ) - - -class ProxitoV2MiddlewareTests(MiddlewareTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - -@pytest.mark.proxito -@override_settings(PUBLIC_DOMAIN='dev.readthedocs.io') -class MiddlewareURLConfTests(TestCase): - - def setUp(self): - self.owner = create_user(username='owner', password='test') - self.domain = 'pip.dev.readthedocs.io' - self.pip = get( - Project, - slug='pip', - users=[self.owner], - privacy_level=PUBLIC, - urlconf='subpath/to/$version/$language/$filename' # Flipped - ) - self.testing_version = get( - Version, - slug='testing', - project=self.pip, - built=True, - active=True, - ) - self.pip.versions.update(privacy_level=PUBLIC) - - def test_proxied_api_methods(self): - # This is mostly a unit test, but useful to make sure the below tests work - self.assertEqual(self.pip.proxied_api_url, 'subpath/to/_/') - self.assertEqual(self.pip.proxied_api_host, '/subpath/to/_') - - def test_middleware_urlconf(self): - resp = self.client.get( - "/subpath/to/testing/en/foodex.html", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 200) - self.assertEqual( - resp['X-Accel-Redirect'], - '/proxito/media/html/pip/testing/foodex.html', - ) - - def test_middleware_urlconf_redirects_subpath_root(self): - resp = self.client.get("/subpath/to/", headers={"host": self.domain}) - self.assertEqual(resp.status_code, 302) - self.assertEqual( - resp['Location'], - 'http://pip.dev.readthedocs.io/subpath/to/latest/en/', - ) - - def test_middleware_urlconf_redirects_root(self): - resp = self.client.get("/", headers={"host": self.domain}) - self.assertEqual(resp.status_code, 302) - self.assertEqual( - resp['Location'], - 'http://pip.dev.readthedocs.io/subpath/to/latest/en/', - ) - - def test_middleware_urlconf_invalid(self): - resp = self.client.get( - "/subpath/to/latest/index.html", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 404) - - def test_middleware_urlconf_subpath_downloads(self): - # These aren't configurable yet - resp = self.client.get( - "/subpath/to/_/downloads/en/latest/pdf/", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 200) - self.assertEqual( - resp['X-Accel-Redirect'], - '/proxito/media/pdf/pip/latest/pip.pdf', - ) - - def test_middleware_urlconf_subpath_api(self): - # These aren't configurable yet - resp = self.client.get( - "/subpath/to/_/api/v2/footer_html/?project=pip&version=latest&language=en&page=index", - headers={"host": self.domain}, - ) - self.assertEqual(resp.status_code, 200) - self.assertContains( - resp, - 'Inserted RTD Footer', - ) - - @mock.patch( - "readthedocs.proxito.views.mixins.staticfiles_storage", - new=BuildMediaFileSystemStorageTest(), - ) - def test_middleware_urlconf_subpath_static_files(self): - resp = self.client.get( - "/subpath/to/_/static/javascript/readthedocs-doc-embed.js", - headers={"host": self.domain}, - ) - self.assertEqual(resp.status_code, 200) - - def test_urlconf_is_escaped(self): - self.pip.urlconf = '3.6/$version/$language/$filename' - self.pip.save() - - self.assertEqual(self.pip.proxied_api_url, '3.6/_/') - self.assertEqual(self.pip.proxied_api_host, '/3.6/_') - - resp = self.client.get( - "/316/latest/en/index.html", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 404) - resp = self.client.get( - "/3.6/latest/en/index.html", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 200) - - resp = self.client.get( - "/316/_/downloads/en/latest/pdf/", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 404) - resp = self.client.get( - "/3.6/_/downloads/en/latest/pdf/", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 200) - - resp = self.client.get( - "/316/_/api/v2/footer_html/?project=pip&version=latest&language=en&page=index", - headers={"host": self.domain}, - ) - self.assertEqual(resp.status_code, 404) - resp = self.client.get( - "/3.6/_/api/v2/footer_html/?project=pip&version=latest&language=en&page=index", - headers={"host": self.domain}, - ) - self.assertEqual(resp.status_code, 200) - - - -@pytest.mark.proxito -@override_settings(PUBLIC_DOMAIN='dev.readthedocs.io') -class MiddlewareURLConfSubprojectTests(TestCase): - - def setUp(self): - self.owner = create_user(username='owner', password='test') - self.domain = 'pip.dev.readthedocs.io' - self.pip = get( - Project, - name='pip', - slug='pip', - users=[self.owner], - privacy_level=PUBLIC, - urlconf='subpath/$subproject/$version/$language/$filename' # Flipped - ) - self.pip.versions.update(privacy_level=PUBLIC) - self.subproject = get( - Project, - name='subproject', - slug='subproject', - users=[self.owner], - privacy_level=PUBLIC, - main_language_project=None, - ) - self.testing_version = get( - Version, - slug='testing', - project=self.subproject, - built=True, - active=True, - ) - self.subproject.versions.update(privacy_level=PUBLIC) - self.relationship = get( - ProjectRelationship, - parent=self.pip, - child=self.subproject, - ) - - def test_middleware_urlconf_subproject(self): - resp = self.client.get( - "/subpath/subproject/testing/en/foodex.html", headers={"host": self.domain} - ) - self.assertEqual(resp.status_code, 200) - self.assertEqual( - resp['X-Accel-Redirect'], - '/proxito/media/html/subproject/testing/foodex.html', - ) diff --git a/readthedocs/proxito/tests/test_old_redirects.py b/readthedocs/proxito/tests/test_old_redirects.py index 559ba08fde0..b9d420a1f62 100644 --- a/readthedocs/proxito/tests/test_old_redirects.py +++ b/readthedocs/proxito/tests/test_old_redirects.py @@ -13,8 +13,6 @@ import pytest from django.http import Http404 from django.test.utils import override_settings -from django.urls import Resolver404 -from django_dynamic_fixture import get from readthedocs.builds.models import Version from readthedocs.projects.models import Feature @@ -140,18 +138,6 @@ def test_root_redirect_with_query_params(self): ) -class ProxitoV2InternalRedirectTests(InternalRedirectTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - # Use ``PYTHON_MEDIA`` here to raise a 404 when trying to serve the file # from disk and execute the code for the handler404 (the file does not # exist). On production, this will happen when trying to serve from @@ -626,55 +612,6 @@ def test_not_found_page_without_trailing_slash(self): ) -class ProxitoV2UserRedirectTests(UserRedirectTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - def test_redirect_using_projects_prefix(self): - """ - Test that we can support redirects using the ``/projects/`` prefix. - - https://github.com/readthedocs/readthedocs.org/issues/7552 - """ - redirect = fixture.get( - Redirect, - project=self.project, - redirect_type="exact", - from_url="/projects/$rest", - to_url="https://example.com/projects/", - ) - self.assertEqual(self.project.redirects.count(), 1) - r = self.client.get( - "/projects/deleted-subproject/en/latest/guides/install.html", - headers={"host": "project.dev.readthedocs.io"}, - ) - self.assertEqual(r.status_code, 302) - self.assertEqual( - r["Location"], - "https://example.com/projects/deleted-subproject/en/latest/guides/install.html", - ) - - redirect.from_url = "/projects/not-found/$rest" - redirect.to_url = "/projects/subproject/" - redirect.save() - r = self.client.get( - "/projects/not-found/en/latest/guides/install.html", - headers={"host": "project.dev.readthedocs.io"}, - ) - self.assertEqual(r.status_code, 302) - self.assertEqual( - r["Location"], - "http://project.dev.readthedocs.io/projects/subproject/en/latest/guides/install.html", - ) - - @override_settings(PUBLIC_DOMAIN="dev.readthedocs.io") class UserForcedRedirectTests(BaseDocServing): def test_no_forced_redirect(self): @@ -952,18 +889,6 @@ def test_redirect_with_301_status(self): ) -class ProxitoV2UserForcedRedirectTests(UserForcedRedirectTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - @override_settings( PYTHON_MEDIA=True, PUBLIC_DOMAIN="dev.readthedocs.io", @@ -1025,17 +950,20 @@ def test_redirect_prefix_crossdomain_with_newline_chars(self): redirect_type="prefix", from_url="/", ) - # We make use of Django's URL resolve in the current implementation, - # which doesn't handle these chars and raises an exception - # instead of redirecting. urls = [ - # Trying to bypass the protocol check by including a `\n` char. - "http://project.dev.readthedocs.io/http:/%0A/my.host/path.html", - "http://project.dev.readthedocs.io/%0A/my.host/path.html", + ( + "http://project.dev.readthedocs.io/http:/%0A/my.host/path.html", + "http://project.dev.readthedocs.io/en/latest/http://my.host/path.html", + ), + ( + "http://project.dev.readthedocs.io/%0A/my.host/path.html", + "http://project.dev.readthedocs.io/en/latest/my.host/path.html", + ), ] - for url in urls: - with pytest.raises(Resolver404): - self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + for url, expected_location in urls: + r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) + self.assertEqual(r.status_code, 302, url) + self.assertEqual(r["Location"], expected_location, url) def test_redirect_sphinx_htmldir_crossdomain(self): """ @@ -1104,42 +1032,39 @@ def test_redirect_sphinx_html_crossdomain(self): self.assertEqual(r.status_code, 302, url) self.assertEqual(r["Location"], expected_location, url) - -class ProxitoV2UserRedirectCrossdomainTest(UserRedirectCrossdomainTest): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - - def test_redirect_prefix_crossdomain_with_newline_chars(self): + def test_redirect_using_projects_prefix(self): """ - Overridden to make it compatible with the new implementation. + Test that we can support redirects using the ``/projects/`` prefix. - In the new implementation we will correctly redirect, - instead of raising an exception. + https://github.com/readthedocs/readthedocs.org/issues/7552 """ - fixture.get( + redirect = fixture.get( Redirect, project=self.project, - redirect_type="prefix", - from_url="/", + redirect_type="exact", + from_url="/projects/$rest", + to_url="https://example.com/projects/", + ) + self.assertEqual(self.project.redirects.count(), 1) + r = self.client.get( + "/projects/deleted-subproject/en/latest/guides/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "https://example.com/projects/deleted-subproject/en/latest/guides/install.html", + ) + + redirect.from_url = "/projects/not-found/$rest" + redirect.to_url = "/projects/subproject/" + redirect.save() + r = self.client.get( + "/projects/not-found/en/latest/guides/install.html", + headers={"host": "project.dev.readthedocs.io"}, + ) + self.assertEqual(r.status_code, 302) + self.assertEqual( + r["Location"], + "http://project.dev.readthedocs.io/projects/subproject/en/latest/guides/install.html", ) - urls = [ - ( - "http://project.dev.readthedocs.io/http:/%0A/my.host/path.html", - "http://project.dev.readthedocs.io/en/latest/http://my.host/path.html", - ), - ( - "http://project.dev.readthedocs.io/%0A/my.host/path.html", - "http://project.dev.readthedocs.io/en/latest/my.host/path.html", - ), - ] - for url, expected_location in urls: - r = self.client.get(url, headers={"host": "project.dev.readthedocs.io"}) - self.assertEqual(r.status_code, 302, url) - self.assertEqual(r["Location"], expected_location, url) diff --git a/readthedocs/proxito/tests/test_redirects.py b/readthedocs/proxito/tests/test_redirects.py index 0f8cf6a0984..4ecd0d20cd6 100644 --- a/readthedocs/proxito/tests/test_redirects.py +++ b/readthedocs/proxito/tests/test_redirects.py @@ -4,7 +4,6 @@ from readthedocs.builds.models import Version from readthedocs.projects.constants import PUBLIC -from readthedocs.projects.models import Feature from readthedocs.proxito.constants import RedirectType from readthedocs.subscriptions.constants import TYPE_CNAME from readthedocs.subscriptions.products import RTDProductFeature @@ -437,18 +436,6 @@ def test_http_public_domain_https_redirect(self): self.assertEqual(r.headers["Cache-Tag"], "project") self.assertEqual(r["X-RTD-Redirect"], RedirectType.system.name) - -class ProxitoV2RedirectTests(RedirectTests): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) - def test_redirect_from_root_language_to_default_version(self): paths = [ "/en", diff --git a/readthedocs/proxito/tests/test_urls.py b/readthedocs/proxito/tests/test_urls.py deleted file mode 100644 index 368b87ce6d5..00000000000 --- a/readthedocs/proxito/tests/test_urls.py +++ /dev/null @@ -1,127 +0,0 @@ -# Copied from .com codebase - -"""Test URL config.""" - -import pytest -from django.test import TestCase -from django.urls import resolve -from django_dynamic_fixture import get - -from readthedocs.projects.models import Feature - - -@pytest.mark.proxito -class TestSingleVersionURLs(TestCase): - def test_root(self): - match = resolve("/") - self.assertEqual(match.url_name, "docs_detail_singleversion_subproject") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "filename": "", - }, - ) - - def test_normal_root(self): - match = resolve("/en/latest/") - self.assertEqual(match.url_name, "docs_detail") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "lang_slug": "en", - "version_slug": "latest", - "filename": "", - }, - ) - - def test_normal_root_with_file(self): - match = resolve("/en/latest/foo.html") - self.assertEqual(match.url_name, "docs_detail") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "lang_slug": "en", - "version_slug": "latest", - "filename": "foo.html", - }, - ) - - def test_subproject_with_lang_and_version(self): - match = resolve("/projects/bar/en/latest/") - self.assertEqual(match.url_name, "docs_detail") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "subproject_slug": "bar", - "lang_slug": "en", - "version_slug": "latest", - "filename": "", - }, - ) - - def test_subproject_with_lang_and_version_and_filename(self): - match = resolve("/projects/bar/en/latest/index.html") - self.assertEqual(match.url_name, "docs_detail") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "subproject_slug": "bar", - "lang_slug": "en", - "version_slug": "latest", - "filename": "index.html", - }, - ) - - def test_subproject_single_version(self): - match = resolve("/projects/bar/index.html") - self.assertEqual(match.url_name, "docs_detail_singleversion_subproject") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "subproject_slug": "bar", - "subproject_slash": "/", - "filename": "index.html", - }, - ) - - def test_subproject_root(self): - match = resolve("/projects/bar/") - self.assertEqual(match.url_name, "docs_detail_singleversion_subproject") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "subproject_slug": "bar", - "subproject_slash": "/", - "filename": "", - }, - ) - - def test_single_version(self): - match = resolve("/some/path/index.html") - self.assertEqual(match.url_name, "docs_detail_singleversion_subproject") - self.assertEqual(match.args, ()) - self.assertEqual( - match.kwargs, - { - "filename": "some/path/index.html", - }, - ) - - -class ProxitoV2TestSingleVersionURLs(TestSingleVersionURLs): - # TODO: remove this class once the new implementation is the default. - def setUp(self): - super().setUp() - get( - Feature, - feature_id=Feature.USE_UNRESOLVER_WITH_PROXITO, - default_true=True, - future_default_true=True, - ) diff --git a/readthedocs/proxito/urls.py b/readthedocs/proxito/urls.py index 7177a29c0e4..fd636e7fd92 100644 --- a/readthedocs/proxito/urls.py +++ b/readthedocs/proxito/urls.py @@ -49,7 +49,7 @@ ServeSitemapXML, ServeStaticFiles, ) -from readthedocs.proxito.views.utils import fast_404, proxito_404_page_handler +from readthedocs.proxito.views.utils import proxito_404_page_handler DOC_PATH_PREFIX = getattr(settings, "DOC_PATH_PREFIX", "") @@ -148,51 +148,7 @@ ServePageRedirect.as_view(), name="redirect_page_with_filename", ), - # (Sub)project w/ translation and versions - re_path( - ( - r"^(?:projects/(?P{project_slug})/)?" - r"(?P{lang_slug})/" - r"(?P{version_slug})/" - r"(?P{filename_slug})$".format(**pattern_opts) - ), - ServeDocs.as_view(), - name="docs_detail", - ), - # Hack /en/latest so it redirects properly - # We don't want to serve the docs here, - # because it's at a different level of serving so relative links break. - re_path( - ( - r"^(?:projects/(?P{project_slug})/)?" - r"(?P{lang_slug})/" - r"(?P{version_slug})$".format(**pattern_opts) - ), - fast_404, - name="docs_detail_directory_indexing", - ), - # # TODO: Support this? - # # (Sub)project translation and single version - # re_path( - # ( - # r'^(?:|projects/(?P{project_slug})/)' - # r'(?P{lang_slug})/' - # r'(?P{filename_slug})$'.format(**pattern_opts) - # ), - # serve_docs, - # name='docs_detail', - # ), - # (Sub)project single version - re_path( - ( - # subproject_slash variable at the end of this regex is for ``/projects/subproject`` - # so that it will get captured here and redirect properly. - r"^(?:projects/(?P{project_slug})(?P/?))?" - r"(?P{filename_slug})$".format(**pattern_opts) - ), - ServeDocs.as_view(), - name="docs_detail_singleversion_subproject", - ), + re_path(r"^(?P.*)$", ServeDocs.as_view(), name="docs_detail"), ] urlpatterns = health_check_urls + proxied_urls + core_urls + docs_urls diff --git a/readthedocs/proxito/views/decorators.py b/readthedocs/proxito/views/decorators.py deleted file mode 100644 index f4e8e34e245..00000000000 --- a/readthedocs/proxito/views/decorators.py +++ /dev/null @@ -1,75 +0,0 @@ -from functools import wraps - -import structlog -from django.db.models import Q -from django.http import Http404 - -from readthedocs.projects.models import Project, ProjectRelationship - -log = structlog.get_logger(__name__) # noqa - - -def map_subproject_slug(view_func): - """ - A decorator that maps a ``subproject_slug`` URL param into a Project. - - :raises: Http404 if the Project doesn't exist - - .. warning:: Does not take into account any kind of privacy settings. - """ - - @wraps(view_func) - def inner_view( # noqa - request, subproject=None, subproject_slug=None, *args, **kwargs - ): - if subproject is None and subproject_slug: - # Try to fetch by subproject alias first, otherwise we might end up - # redirected to an unrelated project. - # Depends on a project passed into kwargs - rel = ( - ProjectRelationship.objects.filter(parent=kwargs["project"]) - .filter(Q(alias=subproject_slug) | Q(child__slug=subproject_slug)) - .first() - ) - if rel: - subproject = rel.child - else: - log.warning( - "The slug is not subproject of project.", - subproject_slug=subproject_slug, - project_slug=kwargs["project"].slug, - ) - raise Http404("Invalid subproject slug") - return view_func(request, subproject=subproject, *args, **kwargs) - - return inner_view - - -def map_project_slug(view_func): - """ - A decorator that maps a ``project_slug`` URL param into a Project. - - :raises: Http404 if the Project doesn't exist - - .. warning:: Does not take into account any kind of privacy settings. - """ - - @wraps(view_func) - def inner_view(request, project=None, project_slug=None, *args, **kwargs): # noqa - if project is None: - # Get the project from the request if it can't be found in the URL - unresolved_domain = request.unresolved_domain - if unresolved_domain and not project_slug: - log.debug( - "Inserting project slug from request.", - project_slug=project_slug, - ) - project = unresolved_domain.project - elif project_slug: - try: - project = Project.objects.get(slug=project_slug) - except Project.DoesNotExist: - raise Http404("Project does not exist.") - return view_func(request, project=project, *args, **kwargs) - - return inner_view diff --git a/readthedocs/proxito/views/mixins.py b/readthedocs/proxito/views/mixins.py index 184131f09d0..d46c22395f0 100644 --- a/readthedocs/proxito/views/mixins.py +++ b/readthedocs/proxito/views/mixins.py @@ -18,7 +18,7 @@ from readthedocs.analytics.tasks import analytics_event from readthedocs.analytics.utils import get_client_ip from readthedocs.audit.models import AuditLog -from readthedocs.builds.constants import EXTERNAL, INTERNAL +from readthedocs.builds.constants import INTERNAL from readthedocs.core.resolver import resolve from readthedocs.projects.constants import MEDIA_TYPE_HTML from readthedocs.proxito.constants import RedirectType @@ -284,20 +284,6 @@ def _serve_401(self, request, project): def allowed_user(self, request, version): return True - def get_version_from_host(self, request, version_slug): - # Handle external domain - unresolved_domain = request.unresolved_domain - if unresolved_domain and unresolved_domain.is_from_external_domain: - external_version_slug = unresolved_domain.external_version_slug - self.version_type = EXTERNAL - log.warning( - 'Using version slug from host.', - version_slug=version_slug, - host_version=external_version_slug, - ) - return external_version_slug - return version_slug - def _spam_response(self, request, project): if 'readthedocsext.spamfighting' in settings.INSTALLED_APPS: from readthedocsext.spamfighting.utils import is_serve_docs_denied # noqa diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 35be9c813b6..35040c025fa 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -6,12 +6,11 @@ from django.conf import settings from django.http import Http404, HttpResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404, render -from django.urls import resolve as url_resolve from django.views import View from readthedocs.analytics.models import PageView from readthedocs.api.mixins import CDNCacheTagsMixin -from readthedocs.builds.constants import EXTERNAL, INTERNAL, LATEST, STABLE +from readthedocs.builds.constants import EXTERNAL, LATEST, STABLE from readthedocs.builds.models import Version from readthedocs.core.mixins import CDNCacheControlMixin from readthedocs.core.resolver import resolve_path, resolver @@ -44,8 +43,6 @@ from readthedocs.redirects.exceptions import InfiniteRedirectException from readthedocs.storage import build_media_storage -from .utils import _get_project_data_from_request - log = structlog.get_logger(__name__) # noqa @@ -101,23 +98,27 @@ class ServeDocsBase(CDNCacheControlMixin, ServeRedirectMixin, ServeDocsMixin, Vi and handles canonical redirects. """ - def get( - self, - request, - project_slug=None, - subproject_slug=None, - subproject_slash=None, - lang_slug=None, - version_slug=None, - filename="", - ): + def get(self, request, path): """ - Take the incoming parsed URL's and figure out what file to serve. + Serve a file from the resolved project and version from the path. + + Before trying to serve the file, we check for canonical redirects. + + If the path isn't valid for the current project, or if the version/translation + doesn't exist, we raise a 404. This will be handled by the ``ServeError404`` + view. + + This view handles the following redirects: + + - Redirect to the default version of the project + from the root path or translation + (/ -> /en/latest/, /en/ -> /en/latest/). + - Trailing slash redirect (/en/latest -> /en/latest/). + - Forced redirects (apply a user defined redirect even if the path exists). - ``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 /. + This view checks if the user is allowed to access the current version, + and if the project is marked as spam. """ - # pylint: disable=too-many-locals unresolved_domain = request.unresolved_domain # Handle requests that need canonicalizing first, # e.g. HTTP -> HTTPS, redirect to canonical domain, etc. @@ -140,152 +141,9 @@ def get( # and we don't want to issue infinite redirects. pass - if unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO): - return self.get_using_unresolver(request) - - original_version_slug = version_slug - version_slug = self.get_version_from_host(request, version_slug) - - ( - final_project, - lang_slug, - version_slug, - filename, - ) = _get_project_data_from_request( # noqa - request, - project_slug=project_slug, - subproject_slug=subproject_slug, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - ) - - is_external = unresolved_domain.is_from_external_domain - if ( - is_external - and original_version_slug - and original_version_slug != version_slug - ): - raise Http404("Version doesn't match the version from the domain.") - - manager = EXTERNAL if is_external else INTERNAL - version = ( - final_project.versions(manager=manager).filter(slug=version_slug).first() - ) - - log.bind( - project_slug=final_project.slug, - subproject_slug=subproject_slug, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - external=is_external, - ) - - # Skip serving versions that are not active (return 404). This is to - # avoid serving files that we have in the storage, but its associated - # version does not exist anymore or it was de-activated. - # - # Note that we want to serve the page when `version is None` because it - # could be a valid URL, like `/` or `` (empty) that does not have a - # version associated to it. - # - # However, if there is a `version_slug` in the URL but there is no - # version on the database we want to return 404. - if (version and not version.active) or (version_slug and not version): - log.warning("Version does not exist or is not active.") - raise Http404("Version does not exist or is not active.") - - if version: - # All public versions can be cached. - self.cache_response = version.is_public - - log.bind(cache_response=self.cache_response) - log.debug('Serving docs.') - - # Verify if the project is marked as spam and return a 401 in that case - spam_response = self._spam_response(request, final_project) - if spam_response: - # If a project was marked as spam, - # all of their responses can be cached. - self.cache_response = True - return spam_response - - # Handle a / redirect when we aren't a single version - if all([ - lang_slug is None, - # External versions/builds will always have a version, - # because it is taken from the host name - version_slug is None or is_external, - filename == '', - not final_project.single_version, - ]): - return self.system_redirect( - request=request, - final_project=final_project, - version_slug=version_slug, - filename=filename, - is_external_version=is_external, - ) - - # Handle `/projects/subproject` URL redirection: - # when there _is_ a subproject_slug but not a subproject_slash - if all([ - final_project.single_version, - filename == '', - subproject_slug, - not subproject_slash, - ]): - return self.system_redirect( - request=request, - final_project=final_project, - version_slug=version_slug, - filename=filename, - is_external_version=is_external, - ) - - if all([ - (lang_slug is None or version_slug is None), - not final_project.single_version, - self.version_type != EXTERNAL, - ]): - log.debug( - 'Invalid URL for project with versions.', - filename=filename, - ) - raise Http404("Invalid URL for project with versions") - - redirect_path, http_status = self.get_redirect( - project=final_project, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - full_path=request.path, - forced_only=True, - ) - if redirect_path and http_status: - log.bind(forced_redirect=True) - try: - return self.get_redirect_response( - request=request, - redirect_path=redirect_path, - proxito_path=request.path, - http_status=http_status, - ) - except InfiniteRedirectException: - # Continue with our normal serve. - pass - - # Check user permissions and return an unauthed response if needed - if not version or not self.allowed_user(request, version): - return self.get_unauthed_response(request, final_project) - - return self._serve_docs( - request=request, - project=final_project, - version=version, - filename=filename, - ) + # Django doesn't include the leading slash in the path, so we normalize it here. + path = "/" + path + return self.serve_path(request, path) def _get_canonical_redirect_type(self, request): """If the current request needs a redirect, return the type of redirect to perform.""" @@ -317,17 +175,8 @@ def _get_canonical_redirect_type(self, request): return None - def get_using_unresolver(self, request): - """ - Resolve the current request using the new proxito implementation. - - This is basically a copy of the get() method, - but adapted to make use of the unresolved to extract the current project, version, and file. - """ + def serve_path(self, request, path): unresolved_domain = request.unresolved_domain - # TODO: We shouldn't use path_info to the get the proxito path, - # it should be captured in proxito/urls.py. - path = request.path_info # We force all storage calls to use the external versions storage, # since we are serving an external version. @@ -507,8 +356,7 @@ class ServeError404Base(CDNCacheControlMixin, ServeRedirectMixin, ServeDocsMixin This view is called by an internal nginx redirect when there is a 404. """ - # pylint: disable=unused-argument - def get(self, request, proxito_path, template_name="404.html"): + def get(self, request, proxito_path): """ Handler for 404 pages on subdomains. @@ -526,41 +374,72 @@ def get(self, request, proxito_path, template_name="404.html"): """ log.bind(proxito_path=proxito_path) log.debug('Executing 404 handler.') - unresolved_domain = request.unresolved_domain - if unresolved_domain.project.has_feature(Feature.USE_UNRESOLVER_WITH_PROXITO): - return self.get_using_unresolver(request, proxito_path) + # We force all storage calls to use the external versions storage, + # since we are serving an external version. + # The version that results from the unresolve_path() call already is + # validated to use the correct manager, this is here to add defense in + # depth against serving the wrong version. + if unresolved_domain.is_from_external_domain: + self.version_type = EXTERNAL - # Parse the URL using the normal urlconf, so we get proper subdomain/translation data - _, __, kwargs = url_resolve( - proxito_path, - urlconf='readthedocs.proxito.urls', - ) + project = None + version = None + # If we weren't able to resolve a filename, + # then the path is the filename. + filename = proxito_path + lang_slug = None + version_slug = None + # Try to map the current path to a project/version/filename. + # If that fails, we fill the variables with the information we have + # available in the exceptions. - version_slug = kwargs.get('version_slug') - version_slug = self.get_version_from_host(request, version_slug) - ( - final_project, - lang_slug, - version_slug, - filename, - ) = _get_project_data_from_request( # noqa - request, - project_slug=kwargs.get("project_slug"), - subproject_slug=kwargs.get("subproject_slug"), - lang_slug=kwargs.get("lang_slug"), - version_slug=version_slug, - filename=kwargs.get("filename", ""), - ) + contextualized_404_class = ContextualizedHttp404 + + try: + unresolved = unresolver.unresolve_path( + unresolved_domain=unresolved_domain, + path=proxito_path, + append_indexhtml=False, + ) + project = unresolved.project + version = unresolved.version + filename = unresolved.filename + lang_slug = project.language + version_slug = version.slug + contextualized_404_class = ProjectFilenameHttp404 + except VersionNotFoundError as exc: + project = exc.project + lang_slug = project.language + version_slug = exc.version_slug + filename = exc.filename + contextualized_404_class = ProjectVersionHttp404 + except TranslationNotFoundError as exc: + project = exc.project + lang_slug = exc.language + version_slug = exc.version_slug + filename = exc.filename + contextualized_404_class = ProjectTranslationHttp404 + except TranslationWithoutVersionError as exc: + project = exc.project + lang_slug = exc.language + # TODO: Use a contextualized 404 + except InvalidExternalVersionError as exc: + project = exc.project + # TODO: Use a contextualized 404 + except InvalidPathForVersionedProjectError as exc: + project = exc.project + filename = exc.path + # TODO: Use a contextualized 404 log.bind( - project_slug=final_project.slug, + project_slug=project.slug, version_slug=version_slug, ) - version = Version.objects.filter( - project=final_project, slug=version_slug - ).first() + # TODO: find a better way to pass this to the middleware. + request.path_project_slug = project.slug + request.path_version_slug = version_slug # If we were able to resolve to a valid version, it means that the # current file doesn't exist. So we check if we can redirect to its @@ -569,7 +448,7 @@ def get(self, request, proxito_path, template_name="404.html"): if version: response = self._get_index_file_redirect( request=request, - project=final_project, + project=project, version=version, filename=filename, full_path=proxito_path, @@ -582,7 +461,7 @@ def get(self, request, proxito_path, template_name="404.html"): # ``index.html`` and ``README.html`` to emulate the behavior we had when # serving directly from NGINX without passing through Python. redirect_path, http_status = self.get_redirect( - project=final_project, + project=project, lang_slug=lang_slug, version_slug=version_slug, filename=filename, @@ -590,7 +469,9 @@ def get(self, request, proxito_path, template_name="404.html"): ) if redirect_path and http_status: try: - return self.get_redirect_response(request, redirect_path, proxito_path, http_status) + return self.get_redirect_response( + request, redirect_path, proxito_path, http_status + ) except InfiniteRedirectException: # ``get_redirect_response`` raises this when it's redirecting back to itself. # We can safely ignore it here because it's logged in ``canonical_redirect``, @@ -599,7 +480,7 @@ def get(self, request, proxito_path, template_name="404.html"): # Register 404 pages into our database for user's analytics self._register_broken_link( - project=final_project, + project=project, version=version, path=filename, full_path=proxito_path, @@ -607,13 +488,19 @@ def get(self, request, proxito_path, template_name="404.html"): response = self._get_custom_404_page( request=request, - project=final_project, + project=project, version=version, ) if response: return response - raise Http404("No custom 404 page found.") + # Don't use the custom 404 page, use our general contextualized 404 response + # Several additional context variables can be added if the templates + # or other error handling is developed (version, language, filename). + raise contextualized_404_class( + project=project, + path_not_found=proxito_path, + ) def _register_broken_link(self, project, version, path, full_path): try: @@ -765,141 +652,6 @@ def _get_index_file_redirect(self, request, project, version, filename, full_pat return None - def get_using_unresolver(self, request, path): - """ - 404 handler using the new proxito implementation. - - This is basically a copy of the get() method, but adapted to make use - of the unresolver to extract the current project, version, and file. - """ - unresolved_domain = request.unresolved_domain - # We force all storage calls to use the external versions storage, - # since we are serving an external version. - # The version that results from the unresolve_path() call already is - # validated to use the correct manager, this is here to add defense in - # depth against serving the wrong version. - if unresolved_domain.is_from_external_domain: - self.version_type = EXTERNAL - - project = None - version = None - # If we weren't able to resolve a filename, - # then the path is the filename. - filename = path - lang_slug = None - version_slug = None - # Try to map the current path to a project/version/filename. - # If that fails, we fill the variables with the information we have - # available in the exceptions. - - contextualized_404_class = ContextualizedHttp404 - - try: - unresolved = unresolver.unresolve_path( - unresolved_domain=unresolved_domain, - path=path, - append_indexhtml=False, - ) - project = unresolved.project - version = unresolved.version - filename = unresolved.filename - lang_slug = project.language - version_slug = version.slug - contextualized_404_class = ProjectFilenameHttp404 - except VersionNotFoundError as exc: - project = exc.project - lang_slug = project.language - version_slug = exc.version_slug - filename = exc.filename - contextualized_404_class = ProjectVersionHttp404 - except TranslationNotFoundError as exc: - project = exc.project - lang_slug = exc.language - version_slug = exc.version_slug - filename = exc.filename - contextualized_404_class = ProjectTranslationHttp404 - except TranslationWithoutVersionError as exc: - project = exc.project - lang_slug = exc.language - # TODO: Use a contextualized 404 - except InvalidExternalVersionError as exc: - project = exc.project - # TODO: Use a contextualized 404 - except InvalidPathForVersionedProjectError as exc: - project = exc.project - filename = exc.path - # TODO: Use a contextualized 404 - - log.bind( - project_slug=project.slug, - version_slug=version_slug, - ) - - # TODO: find a better way to pass this to the middleware. - request.path_project_slug = project.slug - request.path_version_slug = version_slug - - # If we were able to resolve to a valid version, it means that the - # current file doesn't exist. So we check if we can redirect to its - # index file if it exists before doing anything else. - # This is /en/latest/foo -> /en/latest/foo/index.html. - if version: - response = self._get_index_file_redirect( - request=request, - project=project, - version=version, - filename=filename, - full_path=path, - ) - if response: - return response - - # Check and perform redirects on 404 handler - # NOTE: this redirect check must be done after trying files like - # ``index.html`` and ``README.html`` to emulate the behavior we had when - # serving directly from NGINX without passing through Python. - redirect_path, http_status = self.get_redirect( - project=project, - lang_slug=lang_slug, - version_slug=version_slug, - filename=filename, - full_path=path, - ) - if redirect_path and http_status: - try: - return self.get_redirect_response( - request, redirect_path, path, http_status - ) - except InfiniteRedirectException: - # ``get_redirect_response`` raises this when it's redirecting back to itself. - # We can safely ignore it here because it's logged in ``canonical_redirect``, - # and we don't want to issue infinite redirects. - pass - - # Register 404 pages into our database for user's analytics - self._register_broken_link( - project=project, - version=version, - path=filename, - full_path=path, - ) - - response = self._get_custom_404_page( - request=request, - project=project, - version=version, - ) - if response: - return response - - # Don't use the custom 404 page, use our general contextualized 404 response - # Several additional context variables can be added if the templates - # or other error handling is developed (version, language, filename). - raise contextualized_404_class( - project=project, - path_not_found=path, - ) - class ServeError404(SettingsOverrideObject): _default_class = ServeError404Base diff --git a/readthedocs/proxito/views/utils.py b/readthedocs/proxito/views/utils.py index 3501ea1de3f..c33a76a3833 100644 --- a/readthedocs/proxito/views/utils.py +++ b/readthedocs/proxito/views/utils.py @@ -1,11 +1,8 @@ -import os - import structlog from django.http import HttpResponse -from django.shortcuts import get_object_or_404, render +from django.shortcuts import render from ..exceptions import ContextualizedHttp404 -from .decorators import map_project_slug, map_subproject_slug log = structlog.get_logger(__name__) # noqa @@ -59,67 +56,3 @@ def proxito_404_page_handler( ) r.status_code = http_status return r - - -@map_project_slug -@map_subproject_slug -def _get_project_data_from_request( - request, - project, - subproject, - lang_slug=None, - version_slug=None, - filename="", -): - """ - Get the proper project based on the request and URL. - - This is used in a few places and so we break out into a utility function. - """ - # Take the most relevant project so far - current_project = subproject or project - - # Handle single-version projects that have URLs like a real project - if current_project.single_version: - if lang_slug and version_slug: - filename = os.path.join(lang_slug, version_slug, filename) - log.warning( - "URL looks like versioned on a single version project. " - "Changing filename to match.", - filename=filename, - ) - lang_slug = version_slug = None - - # Check to see if we need to serve a translation - if not lang_slug or lang_slug == current_project.language: - final_project = current_project - else: - final_project = get_object_or_404( - current_project.translations.all(), language=lang_slug - ) - - # Handle single version by grabbing the default version - # We might have version_slug when we're serving a PR - if any( - [ - not version_slug and final_project.single_version, - not version_slug and project.urlconf and "$version" not in project.urlconf, - ] - ): - version_slug = final_project.get_default_version() - - # Automatically add the default language if it isn't defined in urlconf - if not lang_slug and project.urlconf and "$language" not in project.urlconf: - lang_slug = final_project.language - - # ``final_project`` is now the actual project we want to serve docs on, - # accounting for: - # * Project - # * Subproject - # * Translations - - # Set the project and version slug on the request so we can log it in middleware - request.path_project_slug = final_project.slug - request.path_version_slug = version_slug - - return final_project, lang_slug, version_slug, filename diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 0898621e1bc..961252ea9fd 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -3174,7 +3174,6 @@ def test_get_version_by_id(self): "skip": False, "slug": "pip", "users": [1], - "urlconf": None, "custom_prefix": None, }, "privacy_level": "public", diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index e76a5ae94fb..1f6bbe4dc7c 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -190,22 +190,6 @@ def test_resolver_translation(self): url = resolve_path(project=self.translation, filename="index.html") self.assertEqual(url, "/ja/latest/index.html") - def test_resolver_urlconf(self): - url = resolve_path( - project=self.translation, - filename="index.html", - urlconf="$version/$filename", - ) - self.assertEqual(url, "latest/index.html") - - def test_resolver_urlconf_extra(self): - url = resolve_path( - project=self.translation, - filename="index.html", - urlconf="foo/bar/$version/$filename", - ) - self.assertEqual(url, "foo/bar/latest/index.html") - class ResolverPathOverrideTests(ResolverBase):