From 177566643517a911a3068803d15c2e7f4a5c130b Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 11 Jan 2024 12:29:59 -0500 Subject: [PATCH 1/7] Redirects: limit to 100 per project --- readthedocs/redirects/validators.py | 30 ++++++++++++++++++++++++++ readthedocs/settings/base.py | 2 ++ readthedocs/subscriptions/constants.py | 2 ++ 3 files changed, 34 insertions(+) diff --git a/readthedocs/redirects/validators.py b/readthedocs/redirects/validators.py index b03706e558b..dddd0f28dc1 100644 --- a/readthedocs/redirects/validators.py +++ b/readthedocs/redirects/validators.py @@ -1,4 +1,6 @@ +from django.conf import settings from django.core.exceptions import ValidationError +from django.utils.translation import gettext_lazy as _ from readthedocs.redirects.constants import ( CLEAN_URL_TO_HTML_REDIRECT, @@ -6,6 +8,8 @@ HTML_TO_CLEAN_URL_REDIRECT, PAGE_REDIRECT, ) +from readthedocs.subscriptions.constants import TYPE_REDIRECTS_LIMIT +from readthedocs.subscriptions.products import get_feature def validate_redirect( @@ -18,6 +22,10 @@ def validate_redirect( (used in forms), and in the Django Rest Framework serializer (used in the API). Since DRF doesn't call the clean method of the model. """ + # Check for the limit if we are creating a new redirect. + if not pk: + _check_redirects_limit(project, error_class) + if redirect_type in [EXACT_REDIRECT, PAGE_REDIRECT]: if from_url.endswith("$rest"): raise error_class("The $rest wildcard has been removed in favor of *.") @@ -38,3 +46,25 @@ def validate_redirect( raise error_class( f"Only one redirect of type `{redirect_type}` is allowed per project." ) + + +def _check_redirects_limit(project, error_class): + """ + Check if the project has reached the limit on the number of redirects. + """ + feature = get_feature(project, TYPE_REDIRECTS_LIMIT) + if feature.unlimited: + return + + if project.redirects.count() >= feature.value: + msg = _( + f"This project has reached the limit of {feature.value} redirects." + " Consider replacing some of your redirects with a wildcard redirect." + ) + if settings.ALLOW_PRIVATE_REPOS: + msg = _( + f"This project has reached the limit of {feature.value} redirects." + " Consider replacing some of your redirects with a wildcard redirect," + " or upgrade your plan." + ) + raise error_class(msg) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 24fb48d56b1..abfcf661f4a 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -174,6 +174,8 @@ def RTD_DEFAULT_FEATURES(self): RTDProductFeature(type=constants.TYPE_AUDIT_LOGS, value=self.RTD_AUDITLOGS_DEFAULT_RETENTION_DAYS).to_item(), # Max number of concurrent builds. RTDProductFeature(type=constants.TYPE_CONCURRENT_BUILDS, value=self.RTD_MAX_CONCURRENT_BUILDS).to_item(), + # Max number of redirects allowed per project. + RTDProductFeature(type=constants.TYPE_REDIRECTS_LIMIT, value=100).to_item(), )) # A dictionary of Stripe products mapped to a RTDProduct object. diff --git a/readthedocs/subscriptions/constants.py b/readthedocs/subscriptions/constants.py index d46514a1fe4..ffaf33cd1da 100644 --- a/readthedocs/subscriptions/constants.py +++ b/readthedocs/subscriptions/constants.py @@ -18,6 +18,7 @@ TYPE_CUSTOM_URL = "urls" TYPE_AUDIT_LOGS = "audit-logs" TYPE_AUDIT_PAGEVIEWS = "audit-pageviews" +TYPE_REDIRECTS_LIMIT = "redirects-limit" FEATURE_TYPES = ( (TYPE_CNAME, _("Custom domain")), @@ -33,4 +34,5 @@ (TYPE_CUSTOM_URL, _("Custom URLs")), (TYPE_AUDIT_LOGS, _("Audit logs")), (TYPE_AUDIT_PAGEVIEWS, _("Audit logs for every page view")), + (TYPE_REDIRECTS_LIMIT, _("Redirects limit")), ) From 9344a289d4373e1fb0f35ba91acd2c82af868442 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 11 Jan 2024 13:30:06 -0500 Subject: [PATCH 2/7] Test --- readthedocs/redirects/tests/test_views.py | 62 +++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/readthedocs/redirects/tests/test_views.py b/readthedocs/redirects/tests/test_views.py index 5423eba795b..d7510237cd2 100644 --- a/readthedocs/redirects/tests/test_views.py +++ b/readthedocs/redirects/tests/test_views.py @@ -12,6 +12,8 @@ PAGE_REDIRECT, ) from readthedocs.redirects.models import Redirect +from readthedocs.subscriptions.constants import TYPE_REDIRECTS_LIMIT +from readthedocs.subscriptions.products import RTDProductFeature @override_settings(RTD_ALLOW_ORGANIZATIONS=False) @@ -224,6 +226,66 @@ def test_redirects_validations(self): "Only one redirect of type `html_to_clean_url` is allowed per project.", ) + @override_settings( + RTD_DEFAULT_FEATURES=dict( + ( + RTDProductFeature( + type=TYPE_REDIRECTS_LIMIT, + value=2, + ).to_item(), + ) + ) + ) + def test_redirects_limit(self): + self.assertEqual(self.project.redirects.all().count(), 1) + url = reverse("projects_redirects_create", args=[self.project.slug]) + resp = self.client.post( + url, + data={ + "redirect_type": EXACT_REDIRECT, + "from_url": "a", + "to_url": "b", + "http_status": 302, + }, + ) + self.assertEqual(resp.status_code, 302) + + resp = self.client.post( + url, + data={ + "redirect_type": EXACT_REDIRECT, + "from_url": "c", + "to_url": "d", + "http_status": 302, + }, + ) + self.assertEqual(resp.status_code, 200) + self.assertContains( + resp, + "This project has reached the limit of 2 redirects.", + ) + self.assertEqual(self.project.redirects.all().count(), 2) + + # Update works + resp = self.client.post( + reverse( + "projects_redirects_edit", args=[self.project.slug, self.redirect.pk] + ), + data={ + "redirect_type": PAGE_REDIRECT, + "http_status": 302, + }, + ) + self.assertEqual(resp.status_code, 302) + + # Delete works + resp = self.client.post( + reverse( + "projects_redirects_delete", args=[self.project.slug, self.redirect.pk] + ), + ) + self.assertEqual(resp.status_code, 302) + @override_settings(RTD_ALLOW_ORGANIZATIONS=True) class TestViewsWithOrganizations(TestViews): From 3019609a17e284148b51cf9c08a9fafa1e9dcd3f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 11 Jan 2024 13:39:19 -0500 Subject: [PATCH 3/7] Allow forced redirects for everyone --- readthedocs/api/v3/serializers.py | 2 -- readthedocs/projects/forms.py | 11 ++++------- readthedocs/projects/models.py | 5 ----- 3 files changed, 4 insertions(+), 14 deletions(-) diff --git a/readthedocs/api/v3/serializers.py b/readthedocs/api/v3/serializers.py index 21b1c30eced..ffa742a870b 100644 --- a/readthedocs/api/v3/serializers.py +++ b/readthedocs/api/v3/serializers.py @@ -944,8 +944,6 @@ class Meta: "position", "_links", ] - # TODO: allow editing this field for projects that have this feature enabled. - read_only_fields = ["force"] def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index e5051dc7a71..2653fecc834 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -785,13 +785,10 @@ def __init__(self, *args, **kwargs): self.fields["enabled"].widget = forms.CheckboxInput() self.fields["enabled"].empty_value = False - if self.project.has_feature(Feature.ALLOW_FORCED_REDIRECTS): - # Remove the nullable option from the form. - # TODO: remove after migration. - self.fields["force"].widget = forms.CheckboxInput() - self.fields["force"].empty_value = False - else: - self.fields.pop("force") + # Remove the nullable option from the form. + # TODO: remove after migration. + self.fields["force"].widget = forms.CheckboxInput() + self.fields["force"].empty_value = False def clean_project(self): return self.project diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 5dea927ed40..5b06c535444 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1951,7 +1951,6 @@ def add_features(sender, **kwargs): CONDA_APPEND_CORE_REQUIREMENTS = "conda_append_core_requirements" ALL_VERSIONS_IN_HTML_CONTEXT = "all_versions_in_html_context" RECORD_404_PAGE_VIEWS = "record_404_page_views" - ALLOW_FORCED_REDIRECTS = "allow_forced_redirects" DISABLE_PAGEVIEWS = "disable_pageviews" RESOLVE_PROJECT_FROM_HEADER = "resolve_project_from_header" USE_PROXIED_APIS_WITH_PREFIX = "use_proxied_apis_with_prefix" @@ -2020,10 +2019,6 @@ def add_features(sender, **kwargs): RECORD_404_PAGE_VIEWS, _("Proxito: Record 404s page views."), ), - ( - ALLOW_FORCED_REDIRECTS, - _("Proxito: Allow forced redirects."), - ), ( DISABLE_PAGEVIEWS, _("Proxito: Disable all page views"), From 85f609c3820613de64791279dc1610cb7fe115d2 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 11 Jan 2024 13:43:15 -0500 Subject: [PATCH 4/7] Update docs --- docs/user/api/v3.rst | 2 -- docs/user/user-defined-redirects.rst | 4 +--- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/user/api/v3.rst b/docs/user/api/v3.rst index 3783b3c5cd5..4c98eb28d98 100644 --- a/docs/user/api/v3.rst +++ b/docs/user/api/v3.rst @@ -1244,8 +1244,6 @@ Redirect create * ``page`` and ``exact`` types require ``from_url`` and ``to_url``. * ``clean_url_to_html`` and ``html_to_clean_url`` types do not require ``from_url`` and ``to_url``. - - Forced redirects are not enabled by default, - you can ask for them to be enabled via support. - Position starts at 0 and is used to order redirects. **Example response**: diff --git a/docs/user/user-defined-redirects.rst b/docs/user/user-defined-redirects.rst index 620c6a45bee..cdb0b371d68 100644 --- a/docs/user/user-defined-redirects.rst +++ b/docs/user/user-defined-redirects.rst @@ -127,9 +127,7 @@ Limitations and observations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - By default, redirects only apply on pages that don't exist. - **Forced redirects** allow you to apply redirects on existing pages, - but incur a small performance penalty, so aren't enabled by default. - You can ask for them to be enabled via support. + **Forced redirects** allow you to apply redirects on existing pages. - Redirects aren't applied on :doc:`previews of pull requests `. You should treat these domains as ephemeral and not rely on them for user-facing content. - You can redirect to URLs outside Read the Docs, From 0d98cee83d8a53244192611a925d0f7e071be467 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 11 Jan 2024 13:46:05 -0500 Subject: [PATCH 5/7] Linter --- readthedocs/redirects/validators.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/readthedocs/redirects/validators.py b/readthedocs/redirects/validators.py index dddd0f28dc1..a627d8f82be 100644 --- a/readthedocs/redirects/validators.py +++ b/readthedocs/redirects/validators.py @@ -49,9 +49,7 @@ def validate_redirect( def _check_redirects_limit(project, error_class): - """ - Check if the project has reached the limit on the number of redirects. - """ + """Check if the project has reached the limit on the number of redirects.""" feature = get_feature(project, TYPE_REDIRECTS_LIMIT) if feature.unlimited: return From 4159b6c15de56db54fa06ed57a001266a67e86aa Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 15 Jan 2024 14:30:55 -0500 Subject: [PATCH 6/7] Mention redirect limits --- docs/user/user-defined-redirects.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/user/user-defined-redirects.rst b/docs/user/user-defined-redirects.rst index cdb0b371d68..808e2ed1d2e 100644 --- a/docs/user/user-defined-redirects.rst +++ b/docs/user/user-defined-redirects.rst @@ -126,6 +126,8 @@ users will be redirected to the new URL. Limitations and observations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +- |org_brand| users are limited to 100 redirects per project, + and |com_brand| users have a number of redirects limited by their plan. - By default, redirects only apply on pages that don't exist. **Forced redirects** allow you to apply redirects on existing pages. - Redirects aren't applied on :doc:`previews of pull requests `. From b80ea22beaebfebabc85a798480a2fc275545698 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 15 Jan 2024 14:35:26 -0500 Subject: [PATCH 7/7] Remove trailing white space --- docs/user/user-defined-redirects.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user/user-defined-redirects.rst b/docs/user/user-defined-redirects.rst index 808e2ed1d2e..a01b40d6834 100644 --- a/docs/user/user-defined-redirects.rst +++ b/docs/user/user-defined-redirects.rst @@ -127,7 +127,7 @@ Limitations and observations ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - |org_brand| users are limited to 100 redirects per project, - and |com_brand| users have a number of redirects limited by their plan. + and |com_brand| users have a number of redirects limited by their plan. - By default, redirects only apply on pages that don't exist. **Forced redirects** allow you to apply redirects on existing pages. - Redirects aren't applied on :doc:`previews of pull requests `.