diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index 055e131954d..7b98a1c460b 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -1,8 +1,8 @@ """Utility functions that are used by both views and celery tasks.""" import itertools -import structlog +import structlog from rest_framework.pagination import PageNumberPagination from readthedocs.builds.constants import ( @@ -15,7 +15,7 @@ STABLE_VERBOSE_NAME, TAG, ) -from readthedocs.builds.models import RegexAutomationRule, Version +from readthedocs.builds.models import Version, VersionAutomationRule log = structlog.get_logger(__name__) @@ -246,7 +246,7 @@ def run_automation_rules(project, added_versions, deleted_active_versions): Currently the versions aren't sorted in any way, the same order is keeped. """ - class_ = RegexAutomationRule + class_ = VersionAutomationRule actions = [ (added_versions, class_.allowed_actions_on_create), (deleted_active_versions, class_.allowed_actions_on_delete), diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 2d81fbc973d..a99a848e102 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -5,7 +5,6 @@ import re from functools import partial -import regex import structlog from django.conf import settings from django.db import models @@ -14,10 +13,7 @@ from django.utils import timezone from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ -from django_extensions.db.fields import ( - CreationDateTimeField, - ModificationDateTimeField, -) +from django_extensions.db.fields import CreationDateTimeField, ModificationDateTimeField from django_extensions.db.models import TimeStampedModel from polymorphic.models import PolymorphicModel @@ -61,6 +57,7 @@ get_github_username_repo, get_gitlab_username_repo, get_vcs_url, + match_regex, ) from readthedocs.builds.version_slug import VersionSlugField from readthedocs.config import LATEST_CONFIGURATION_VERSION @@ -1064,8 +1061,17 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel): (DELETE_VERSION_ACTION, _('Delete version (on branch/tag deletion)')), ) - allowed_actions_on_create = {} - allowed_actions_on_delete = {} + allowed_actions_on_create = { + ACTIVATE_VERSION_ACTION: actions.activate_version, + HIDE_VERSION_ACTION: actions.hide_version, + MAKE_VERSION_PUBLIC_ACTION: actions.set_public_privacy_level, + MAKE_VERSION_PRIVATE_ACTION: actions.set_private_privacy_level, + SET_DEFAULT_VERSION_ACTION: actions.set_default_version, + } + + allowed_actions_on_delete = { + DELETE_VERSION_ACTION: actions.delete_version, + } project = models.ForeignKey( Project, @@ -1137,33 +1143,36 @@ def run(self, version, **kwargs): Run an action if `version` matches the rule. :type version: readthedocs.builds.models.Version - :returns: True if the action was performed + :param kwargs: All extra keywords will be passed to the match and action functions. + :returns: A tuple of (boolean, ANY), where the first element + indicates if the action was performed, and the second is the result + returned by the action. """ if version.type != self.version_type: return False - match, result = self.match(version, self.get_match_arg()) + match, result = self.match(version, self.get_match_arg(), **kwargs) if match: - self.apply_action(version, result) + action_result = self.apply_action(version, result, **kwargs) AutomationRuleMatch.objects.register_match( rule=self, version=version, ) - return True - return False + return True, action_result + return False, None - def match(self, version, match_arg): + def match(self, version, match_arg, **kwargs): """ Returns True and the match result if the version matches the rule. :type version: readthedocs.builds.models.Version :param str match_arg: Additional argument to perform the match - :returns: A tuple of (boolean, match_resul). + :returns: A tuple of (boolean, match_result). The result will be passed to `apply_action`. """ return False, None - def apply_action(self, version, match_result): + def apply_action(self, version, match_result, **kwargs): """ Apply the action from allowed_actions_on_*. @@ -1178,7 +1187,12 @@ def apply_action(self, version, match_result): ) if action is None: raise NotImplementedError - action(version, match_result, self.action_arg) + return action( + version=version, + match_result=match_result, + action_arg=self.action_arg, + **kwargs, + ) def move(self, steps): """ @@ -1280,53 +1294,16 @@ def __str__(self): class RegexAutomationRule(VersionAutomationRule): - TIMEOUT = 1 # timeout in seconds - - allowed_actions_on_create = { - VersionAutomationRule.ACTIVATE_VERSION_ACTION: actions.activate_version, - VersionAutomationRule.HIDE_VERSION_ACTION: actions.hide_version, - VersionAutomationRule.MAKE_VERSION_PUBLIC_ACTION: actions.set_public_privacy_level, - VersionAutomationRule.MAKE_VERSION_PRIVATE_ACTION: actions.set_private_privacy_level, - VersionAutomationRule.SET_DEFAULT_VERSION_ACTION: actions.set_default_version, - } - - allowed_actions_on_delete = { - VersionAutomationRule.DELETE_VERSION_ACTION: actions.delete_version, - } - class Meta: proxy = True - def match(self, version, match_arg): - """ - Find a match using regex.search. - - .. note:: - - We use the regex module with the timeout - arg to avoid ReDoS. - - We could use a finite state machine type of regex too, - but there isn't a stable library at the time of writing this code. - """ - try: - match = regex.search( - match_arg, - version.verbose_name, - # Compatible with the re module - flags=regex.VERSION0, - timeout=self.TIMEOUT, - ) - return bool(match), match - except TimeoutError: - log.warning( - 'Timeout while parsing regex.', - pattern=match_arg, - version_slug=version.slug, - ) - except Exception as e: - log.exception('Error parsing regex.', exc_info=True) - return False, None + def match(self, version, match_arg, **kwargs): + version_name = version.verbose_name + if version.is_external: + version_data = kwargs["version_data"] + version_name = version_data.source_branch + result = match_regex(match_arg, version_name) + return bool(result), result def get_edit_url(self): return reverse( diff --git a/readthedocs/builds/utils.py b/readthedocs/builds/utils.py index 95bd02068fa..3743b170373 100644 --- a/readthedocs/builds/utils.py +++ b/readthedocs/builds/utils.py @@ -1,8 +1,10 @@ """Utilities for the builds app.""" -from time import monotonic from contextlib import contextmanager +from time import monotonic +import regex +import structlog from django.core.cache import cache from readthedocs.builds.constants import EXTERNAL @@ -14,13 +16,15 @@ GITLAB_REGEXS, ) +log = structlog.get_logger(__name__) + LOCK_EXPIRE = 60 * 180 # Lock expires in 3 hours def get_github_username_repo(url): if 'github' in url: - for regex in GITHUB_REGEXS: - match = regex.search(url) + for pattern in GITHUB_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -28,8 +32,8 @@ def get_github_username_repo(url): def get_bitbucket_username_repo(url=None): if 'bitbucket' in url: - for regex in BITBUCKET_REGEXS: - match = regex.search(url) + for pattern in BITBUCKET_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -37,8 +41,8 @@ def get_bitbucket_username_repo(url=None): def get_gitlab_username_repo(url=None): if 'gitlab' in url: - for regex in GITLAB_REGEXS: - match = regex.search(url) + for pattern in GITLAB_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -100,3 +104,33 @@ def memcache_lock(lock_id, oid): # owned by someone else # also don't release the lock if we didn't acquire it cache.delete(lock_id) + + +def match_regex(pattern, text, timeout=1): + """ + Find a match using regex.search. + + .. note:: + + We use the regex module with the timeout arg to avoid ReDoS. + We could use a finite state machine type of regex too, + but there isn't a stable library at the time of writing this code. + """ + try: + match = regex.search( + pattern, + text, + # Compatible with the re module + flags=regex.VERSION0, + timeout=timeout, + ) + return match + except TimeoutError: + log.exception( + "Timeout while parsing regex.", + pattern=pattern, + input=text, + ) + except Exception: + log.exception("Error parsing regex.", exc_info=True) + return None diff --git a/readthedocs/rtd_tests/tests/test_automation_rules.py b/readthedocs/rtd_tests/tests/test_automation_rules.py index ef02ee9b1e9..777844f75b5 100644 --- a/readthedocs/rtd_tests/tests/test_automation_rules.py +++ b/readthedocs/rtd_tests/tests/test_automation_rules.py @@ -93,7 +93,7 @@ def test_match( action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) assert rule.matches.all().count() == (1 if result else 0) @pytest.mark.parametrize( @@ -128,7 +128,7 @@ def test_predefined_match_all_versions(self, trigger_build, version_name, result action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) @pytest.mark.parametrize( 'version_name,result', @@ -164,7 +164,7 @@ def test_predefined_match_semver_versions(self, trigger_build, version_name, res action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) def test_action_activation(self, trigger_build): version = get( @@ -182,7 +182,7 @@ def test_action_activation(self, trigger_build): action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.active is True trigger_build.assert_called_once() @@ -205,7 +205,7 @@ def test_action_delete_version(self, trigger_build, version_type): action=VersionAutomationRule.DELETE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert not self.project.versions.filter(slug=slug).exists() @pytest.mark.parametrize('version_type', [BRANCH, TAG]) @@ -230,7 +230,7 @@ def test_action_delete_version_on_default_version(self, trigger_build, version_t action=VersionAutomationRule.DELETE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert self.project.versions.filter(slug=slug).exists() def test_action_set_default_version(self, trigger_build): @@ -250,7 +250,7 @@ def test_action_set_default_version(self, trigger_build): version_type=TAG, ) assert self.project.get_default_version() == LATEST - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert self.project.get_default_version() == version.slug def test_version_hide_action(self, trigger_build): @@ -270,7 +270,7 @@ def test_version_hide_action(self, trigger_build): action=VersionAutomationRule.HIDE_VERSION_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.active is True assert version.hidden is True trigger_build.assert_called_once() @@ -293,7 +293,7 @@ def test_version_make_public_action(self, trigger_build): action=VersionAutomationRule.MAKE_VERSION_PUBLIC_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.privacy_level == PUBLIC trigger_build.assert_not_called() @@ -315,7 +315,7 @@ def test_version_make_private_action(self, trigger_build): action=VersionAutomationRule.MAKE_VERSION_PRIVATE_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.privacy_level == PRIVATE trigger_build.assert_not_called() @@ -338,7 +338,7 @@ def test_matches_history(self, trigger_build): version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert rule.matches.all().count() == 1 match = rule.matches.first() @@ -350,7 +350,7 @@ def test_matches_history(self, trigger_build): for i in range(1, 31): version.verbose_name = f'test {i}' version.save() - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert rule.matches.all().count() == 15