diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index ad69ee822ff..595a16fd733 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -14,7 +14,7 @@ STABLE_VERBOSE_NAME, TAG, ) -from readthedocs.builds.models import RegexAutomationRule, Version +from readthedocs.builds.models import Version, VersionAutomationRule log = logging.getLogger(__name__) @@ -210,7 +210,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 3e12d5b8f60..7da0eb4b946 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -6,7 +6,6 @@ import re from shutil import rmtree -import regex from django.conf import settings from django.core.files.storage import get_storage_class from django.db import models @@ -62,6 +61,7 @@ get_bitbucket_username_repo, get_github_username_repo, get_gitlab_username_repo, + match_regex, ) from readthedocs.builds.version_slug import VersionSlugField from readthedocs.config import LATEST_CONFIGURATION_VERSION @@ -979,8 +979,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, @@ -1047,32 +1056,35 @@ def get_match_arg(self): ) return match_arg or self.match_arg - def run(self, version, *args, **kwargs): + 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: - 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) - return True - return False + action_result = self.apply_action(version, result, **kwargs) + 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_*. @@ -1087,7 +1099,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): """ @@ -1189,52 +1206,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 writting 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=%s, input=%s', - match_arg, version.verbose_name, - ) - except Exception as e: - log.info('Error parsing regex: %s', e) - 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 0702e460690..6b6a978acf5 100644 --- a/readthedocs/builds/utils.py +++ b/readthedocs/builds/utils.py @@ -1,7 +1,8 @@ """Utilities for the builds app.""" - +import logging from contextlib import contextmanager +import regex from celery.five import monotonic from django.core.cache import cache @@ -11,13 +12,15 @@ GITLAB_REGEXS, ) +log = logging.getLogger(__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) @@ -25,8 +28,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) @@ -34,8 +37,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) @@ -61,3 +64,31 @@ def memcache_lock(lock_id, oid): # to lessen the chance of releasing an expired lock # owned by someone else. 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 writting 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=%s, input=%s', + pattern, text, + ) + except Exception as e: + log.exception('Error parsing regex: %s', e) + return None diff --git a/readthedocs/rtd_tests/tests/test_automation_rules.py b/readthedocs/rtd_tests/tests/test_automation_rules.py index 0e87977685f..9c915e607ab 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) @pytest.mark.parametrize( 'version_name,result', @@ -127,7 +127,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', @@ -163,7 +163,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( @@ -181,7 +181,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() @@ -204,7 +204,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]) @@ -229,7 +229,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): @@ -249,7 +249,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): @@ -269,7 +269,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() @@ -292,7 +292,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() @@ -314,7 +314,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()