From 312ed8953b89096a6fb2a65fec60078dd81fb713 Mon Sep 17 00:00:00 2001 From: Laura Barcziova Date: Wed, 10 May 2023 20:12:20 +0200 Subject: [PATCH] Implement checking of denied project and actor We already had the 'denied' value in allowlist, let's utilise it both for checking the project and actor. For project, check all parent namespaces for denied value. Related to #1964 --- packit_service/constants.py | 6 +- packit_service/models.py | 2 +- packit_service/worker/allowlist.py | 243 +++++++++----- packit_service/worker/handlers/forges.py | 11 +- .../test_github_fas_verification.py | 18 +- tests/integration/test_installation.py | 43 ++- tests/unit/test_allowlist.py | 307 +++++++++++++++++- tests_openshift/database/test_allowlist.py | 4 +- 8 files changed, 533 insertions(+), 101 deletions(-) diff --git a/packit_service/constants.py b/packit_service/constants.py index 8423d693d..d4f6e1604 100644 --- a/packit_service/constants.py +++ b/packit_service/constants.py @@ -74,7 +74,6 @@ ) NOTIFICATION_REPO = "https://github.com/packit/notifications" - PERMISSIONS_ERROR_WRITE_OR_ADMIN = ( "Only users with write or admin permissions to the repository " "can trigger Packit-as-a-Service" @@ -250,3 +249,8 @@ def from_number(number: int): "s390x", "x86_64", ] + +DENIED_MSG = ( + f"You were denied from using Packit Service. If you think this was done by mistake, " + f"please, [let us know]({CONTACTS_URL})." +) diff --git a/packit_service/models.py b/packit_service/models.py index 0651abe52..aad128c4b 100644 --- a/packit_service/models.py +++ b/packit_service/models.py @@ -1998,7 +1998,7 @@ def get_namespace(cls, namespace: str) -> Optional["AllowlistModel"]: return sa_session().query(AllowlistModel).filter_by(namespace=namespace).first() @classmethod - def get_namespaces_by_status(cls, status: str) -> Iterable["AllowlistModel"]: + def get_by_status(cls, status: str) -> Iterable["AllowlistModel"]: """ Get list of namespaces with specific status. diff --git a/packit_service/worker/allowlist.py b/packit_service/worker/allowlist.py index b8f1ce59c..269b1f6b1 100644 --- a/packit_service/worker/allowlist.py +++ b/packit_service/worker/allowlist.py @@ -3,11 +3,12 @@ import logging from typing import Any, Iterable, Optional, Union, Callable, List, Tuple, Dict, Type +from urllib.parse import urlparse from fasjson_client import Client from fasjson_client.errors import APIError -from ogr.abstract import GitProject +from ogr.abstract import GitProject from packit.api import PackitAPI from packit.config.job_config import JobConfig, JobType from packit.exceptions import PackitException, PackitCommandFailedError @@ -18,6 +19,7 @@ NAMESPACE_NOT_ALLOWED_MARKDOWN_ISSUE_INSTRUCTIONS, NOTIFICATION_REPO, DOCS_APPROVAL_URL, + DENIED_MSG, ) from packit_service.models import AllowlistModel, AllowlistStatus from packit_service.worker.events import ( @@ -162,9 +164,22 @@ def approve_namespace(namespace: str): logger.info(f"Account {namespace!r} approved successfully.") @staticmethod - def is_approved(namespace: str) -> bool: + def deny_namespace(namespace: str): """ - Checks if namespace is approved in the allowlist. + Deny namespace. + + Args: + namespace (str): Namespace in the format of `github.com/namespace` or + `github.com/namespace/repository.git`. + """ + AllowlistModel.add_namespace(namespace=namespace, status=AllowlistStatus.denied) + + logger.info(f"Account {namespace!r} denied successfully.") + + @staticmethod + def is_namespace_or_parent_approved(namespace: str) -> bool: + """ + Checks if namespace or any parent namespace is approved in the allowlist. Args: namespace (str): Namespace in format `example.com/namespace/repository.git`, @@ -188,9 +203,42 @@ def is_approved(namespace: str) -> bool: separated_path = separated_path[0].rsplit("/", 1) - logger.info(f"Could not find entry for: {namespace}") + logger.info(f"Could not find approved entry for: {namespace}") + return False + + @staticmethod + def is_namespace_or_parent_denied(namespace: str) -> bool: + """ + Checks if namespace or any parent namespace is denied in the allowlist. + + Args: + namespace (str): Namespace in format `example.com/namespace/repository.git`, + where `/repository.git` is optional. + + Returns: + `True` if namespace is approved, `False` otherwise. + """ + if not namespace: + return False + + separated_path = [namespace, None] + while len(separated_path) > 1: + if matching_namespace := AllowlistModel.get_namespace(separated_path[0]): + status = AllowlistStatus(matching_namespace.status) + if status == AllowlistStatus.denied: + logger.info(f"Namespace {namespace} is denied.") + return True + + separated_path = separated_path[0].rsplit("/", 1) + + logger.info(f"Could not find denied entry for: {namespace}") return False + @staticmethod + def is_denied(namespace: str) -> bool: + model = AllowlistModel.get_namespace(namespace) + return bool(model) and model.status == AllowlistStatus.denied + @staticmethod def remove_namespace(namespace: str) -> bool: """ @@ -212,6 +260,12 @@ def remove_namespace(namespace: str) -> bool: return True + @staticmethod + def get_namespaces_by_status(status: AllowlistStatus) -> List[str]: + return [ + account.namespace for account in AllowlistModel.get_by_status(status.value) + ] + @staticmethod def waiting_namespaces() -> List[str]: """ @@ -220,12 +274,17 @@ def waiting_namespaces() -> List[str]: Returns: List of namespaces that are waiting for approval. """ - return [ - account.namespace - for account in AllowlistModel.get_namespaces_by_status( - AllowlistStatus.waiting.value - ) - ] + return Allowlist.get_namespaces_by_status(AllowlistStatus.waiting) + + @staticmethod + def denied_namespaces() -> List[str]: + """ + Get denied namespace. + + Returns: + List of namespaces that are denied. + """ + return Allowlist.get_namespaces_by_status(AllowlistStatus.denied) def _check_unchecked_event( self, @@ -247,8 +306,11 @@ def _check_release_push_event( project_url = self._strip_protocol_and_add_git(event.project_url) if not project_url: raise KeyError(f"Failed to get namespace from {type(event)!r}") + if self.is_namespace_or_parent_denied(project_url): + logger.info("Refusing release event on denied repo namespace.") + return False - if self.is_approved(project_url): + if self.is_namespace_or_parent_approved(project_url): return True logger.info("Refusing release event on not allowlisted repo namespace.") @@ -270,68 +332,93 @@ def _check_pr_event( raise KeyError(f"Failed to get login of the actor from {type(event)}") project_url = self._strip_protocol_and_add_git(event.project_url) - - namespace_approved = self.is_approved(project_url) - user_approved = ( - project.can_merge_pr(actor_name) - or project.get_pr(event.pr_id).author == actor_name - ) - - if namespace_approved and user_approved: + user_namespace = f"{urlparse(event.project_url).netloc}/{actor_name}" + + if user_or_project_denied := Allowlist.is_denied(user_namespace): + msg = f"User namespace {actor_name} denied!" + short_msg = "User namespace denied!" + elif user_or_project_denied := self.is_namespace_or_parent_denied(project_url): + msg = f"{project_url} or parent namespaces denied!" + short_msg = "Project or its namespace denied!" + else: + namespace_approved = self.is_namespace_or_parent_approved(project_url) + user_approved = ( + project.can_merge_pr(actor_name) + or project.get_pr(event.pr_id).author == actor_name + ) # TODO: clear failing check when present - return True + if namespace_approved and user_approved: + return True + msg = ( + ( + f"Project {project_url} is not on our allowlist! " + "See https://packit.dev/docs/guide/#2-approval" + ) + if not namespace_approved + else f"Account {actor_name} has no write access nor is author of PR!" + ) + short_msg = ( + f"{project_url} not allowed!" + if not namespace_approved + else "User cannot trigger!" + ) - msg = ( - f"Project {project_url} is not on our allowlist! " - "See https://packit.dev/docs/guide/#2-approval" - if not namespace_approved - else f"Account {actor_name} has no write access nor is author of PR!" - ) logger.debug(msg) if isinstance( event, (PullRequestCommentGithubEvent, MergeRequestCommentGitlabEvent) ): project.get_pr(event.pr_id).comment(msg) else: - for job_config in job_configs: - job_helper_kls: Type[Union[TestingFarmJobHelper, CoprBuildJobHelper]] - if job_config.type == JobType.tests: - job_helper_kls = TestingFarmJobHelper - else: - job_helper_kls = CoprBuildJobHelper - - job_helper = job_helper_kls( - service_config=self.service_config, - package_config=event.get_packages_config().get_package_config_for( - job_config - ), - project=project, - metadata=EventData.from_event_dict(event.get_dict()), - db_trigger=event.db_trigger, - job_config=job_config, - build_targets_override=event.build_targets_override, - tests_targets_override=event.tests_targets_override, - ) - msg = ( - f"{project.service.hostname}/{project.namespace} not allowed!" - if not namespace_approved - else "User cannot trigger!" - ) + self._check_pr_report_status( + job_configs=job_configs, + event=event, + project=project, + user_or_project_denied=user_or_project_denied, + short_msg=short_msg, + ) + return False + + def _check_pr_report_status( + self, job_configs, event, project, user_or_project_denied, short_msg + ): + for job_config in job_configs: + job_helper_kls: Type[Union[TestingFarmJobHelper, CoprBuildJobHelper]] + if job_config.type == JobType.tests: + job_helper_kls = TestingFarmJobHelper + else: + job_helper_kls = CoprBuildJobHelper + + job_helper = job_helper_kls( + service_config=self.service_config, + package_config=event.get_packages_config().get_package_config_for( + job_config + ), + project=project, + metadata=EventData.from_event_dict(event.get_dict()), + db_trigger=event.db_trigger, + job_config=job_config, + build_targets_override=event.build_targets_override, + tests_targets_override=event.tests_targets_override, + ) + if user_or_project_denied: + url = None + markdown_content = DENIED_MSG + else: issue_url = self.get_approval_issue(namespace=project.namespace) - job_helper.report_status_to_configured_job( - description=msg, - state=BaseCommitStatus.neutral, - url=issue_url or DOCS_APPROVAL_URL, - markdown_content=NAMESPACE_NOT_ALLOWED_MARKDOWN_DESCRIPTION.format( - instructions=NAMESPACE_NOT_ALLOWED_MARKDOWN_ISSUE_INSTRUCTIONS.format( - issue_url=issue_url - ) - if issue_url - else "" - ), + url = issue_url or DOCS_APPROVAL_URL + markdown_content = NAMESPACE_NOT_ALLOWED_MARKDOWN_DESCRIPTION.format( + instructions=NAMESPACE_NOT_ALLOWED_MARKDOWN_ISSUE_INSTRUCTIONS.format( + issue_url=issue_url + ) + if issue_url + else "" ) - - return False + job_helper.report_status_to_configured_job( + description=short_msg, + state=BaseCommitStatus.neutral, + url=url, + markdown_content=markdown_content, + ) def _check_issue_comment_event( self, @@ -343,19 +430,27 @@ def _check_issue_comment_event( if not actor_name: raise KeyError(f"Failed to get login of the actor from {type(event)}") project_url = self._strip_protocol_and_add_git(event.project_url) + user_namespace = f"{urlparse(event.project_url).netloc}/{actor_name}" - namespace_approved = self.is_approved(project_url) - user_approved = project.can_merge_pr(actor_name) - - if namespace_approved and user_approved: - return True + if Allowlist.is_denied(user_namespace): + msg = f"User namespace {actor_name} denied!" + elif self.is_namespace_or_parent_denied(project_url): + msg = f"{project_url} or parent namespaces denied!" + else: + namespace_approved = self.is_namespace_or_parent_approved(project_url) + user_approved = project.can_merge_pr(actor_name) + # TODO: clear failing check when present + if namespace_approved and user_approved: + return True + msg = ( + ( + f"Project {project_url} is not on our allowlist! " + "See https://packit.dev/docs/guide/#2-approval" + ) + if not namespace_approved + else f"Account {actor_name} has no write access!" + ) - msg = ( - f"Project {project_url} is not on our allowlist! " - "See https://packit.dev/docs/guide/#2-approval" - if not namespace_approved - else f"Account {actor_name} has no write access!" - ) logger.debug(msg) project.get_issue(event.issue_id).comment(msg) return False diff --git a/packit_service/worker/handlers/forges.py b/packit_service/worker/handlers/forges.py index 7e56206e7..43a44f253 100644 --- a/packit_service/worker/handlers/forges.py +++ b/packit_service/worker/handlers/forges.py @@ -92,7 +92,7 @@ def run(self) -> TaskResults: existing_allowlist_entry = AllowlistModel.get_namespace(namespace) if not existing_allowlist_entry or ( previous_installation is not None - and not allowlist.is_approved(namespace) + and not allowlist.is_namespace_or_parent_approved(namespace) and previous_sender_login != self.sender_login ): if allowlist.is_github_username_from_fas_account_matching( @@ -220,11 +220,14 @@ def verify(self, namespace: str, fas_account: str) -> TaskResults: to match fas_account. """ allowlist = Allowlist(service_config=self.service_config) - if allowlist.is_approved(namespace): - msg = f"Namespace `{namespace}` was already approved." + if ( + approved := allowlist.is_namespace_or_parent_approved(namespace) + ) or allowlist.is_denied(namespace): + msg = f"Namespace `{namespace}` {'was already approved' if approved else 'is denied'}." logger.debug(msg) self.issue.comment(msg) - self.issue.close() + if approved: + self.issue.close() return TaskResults(success=True, details={"msg": msg}) if allowlist.is_github_username_from_fas_account_matching( diff --git a/tests/integration/test_github_fas_verification.py b/tests/integration/test_github_fas_verification.py index 4c78fddb8..5b479cbf0 100644 --- a/tests/integration/test_github_fas_verification.py +++ b/tests/integration/test_github_fas_verification.py @@ -60,7 +60,10 @@ def test_verification_successful(): ) assert json.dumps(event_dict) - flexmock(Allowlist).should_receive("is_approved").and_return(False) + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").and_return( + False + ) + flexmock(Allowlist).should_receive("is_denied").and_return(False) flexmock(Allowlist).should_receive( "is_github_username_from_fas_account_matching" ).with_args(fas_account="my-fas-account", sender_login="phracek").and_return(True) @@ -119,7 +122,10 @@ def test_verification_not_successful(): ) assert json.dumps(event_dict) - flexmock(Allowlist).should_receive("is_approved").and_return(False) + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").and_return( + False + ) + flexmock(Allowlist).should_receive("is_denied").and_return(False) flexmock(Allowlist).should_receive( "is_github_username_from_fas_account_matching" ).with_args(fas_account="my-fas-account", sender_login="phracek").and_return(False) @@ -240,7 +246,9 @@ def test_verification_already_approved(): ) assert json.dumps(event_dict) - flexmock(Allowlist).should_receive("is_approved").and_return(True) + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").and_return( + True + ) flexmock(AllowlistModel).should_receive("add_namespace").never() flexmock(GithubInstallationModel).should_receive("get_by_account_login").with_args( "example-user" @@ -343,7 +351,9 @@ def test_verification_not_original_triggerer(): ) assert json.dumps(event_dict) - flexmock(Allowlist).should_receive("is_approved").and_return(True) + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").and_return( + True + ) flexmock(AllowlistModel).should_receive("add_namespace").never() flexmock(GithubInstallationModel).should_receive("get_by_account_login").with_args( "example-user" diff --git a/tests/integration/test_installation.py b/tests/integration/test_installation.py index 9d2b480ee..9d439c042 100644 --- a/tests/integration/test_installation.py +++ b/tests/integration/test_installation.py @@ -71,7 +71,43 @@ def test_reinstallation_already_approved_namespace(): flexmock(AllowlistModel).should_receive("get_namespace").with_args( "github.com/packit-service" ).and_return(flexmock()) - flexmock(Allowlist).should_receive("is_approved").with_args( + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").with_args( + "github.com/packit-service" + ).and_return(True) + flexmock(PackageConfigGetter).should_receive("create_issue_if_needed").never() + + flexmock(Signature).should_receive("apply_async").once() + flexmock(Pushgateway).should_receive("push").once().and_return() + processing_results = SteveJobs().process_message(installation_event()) + event_dict, job, job_config, package_config = get_parameters_from_results( + processing_results + ) + assert json.dumps(event_dict) + + results = run_installation_handler( + package_config=package_config, + event=event_dict, + job_config=job_config, + ) + + assert first_dict_value(results["job"])["success"] + + +def test_reinstallation_denied_namespace(): + config = ServiceConfig() + config.command_handler_work_dir = SANDCASTLE_WORK_DIR + flexmock(ServiceConfig).should_receive("get_service_config").and_return(config) + flexmock(GithubInstallationModel).should_receive("get_by_account_login").and_return( + flexmock(sender_login="jpopelka") + ) + flexmock(GithubInstallationModel).should_receive("create_or_update").once() + flexmock(AllowlistModel).should_receive("get_namespace").with_args( + "github.com/packit-service" + ).and_return(flexmock()) + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").with_args( + "github.com/packit-service" + ).and_return(False) + flexmock(Allowlist).should_receive("is_denied").with_args( "github.com/packit-service" ).and_return(True) flexmock(PackageConfigGetter).should_receive("create_issue_if_needed").never() @@ -107,7 +143,10 @@ def test_reinstallation_not_approved_namespace(previous_sender_login, create_iss flexmock(AllowlistModel).should_receive("get_namespace").with_args( "github.com/packit-service" ).and_return(flexmock()) - flexmock(Allowlist).should_receive("is_approved").with_args( + flexmock(Allowlist).should_receive("is_namespace_or_parent_approved").with_args( + "github.com/packit-service" + ).and_return(False) + flexmock(Allowlist).should_receive("is_denied").with_args( "github.com/packit-service" ).and_return(False) if create_issue: diff --git a/tests/unit/test_allowlist.py b/tests/unit/test_allowlist.py index 350cdd492..5251874af 100644 --- a/tests/unit/test_allowlist.py +++ b/tests/unit/test_allowlist.py @@ -19,6 +19,7 @@ from packit_service.constants import ( DOCS_APPROVAL_URL, NOTIFICATION_REPO, + DENIED_MSG, ) from packit_service.models import ( AllowlistModel as DBAllowlist, @@ -54,12 +55,8 @@ def allowlist(): @pytest.fixture(scope="module") def allowlist_entries(): return { - "github.com": flexmock( - id=1, namespace="github.com", status=AllowlistStatus.denied.value - ), - "gitlab.com": flexmock( - id=2, namespace="gitlab.com", status=AllowlistStatus.denied.value - ), + "github.com": None, + "gitlab.com": None, "github.com/fero": flexmock( id=3, namespace="github.com/fero", @@ -193,12 +190,48 @@ def mocked_model(allowlist_entries, request): ), indirect=["mocked_model"], ) -def test_is_approved(allowlist, account_name, mocked_model, is_approved): - assert allowlist.is_approved(account_name) == is_approved +def test_is_namespace_or_parent_approved( + allowlist, account_name, mocked_model, is_approved +): + assert allowlist.is_namespace_or_parent_approved(account_name) == is_approved @pytest.mark.parametrize( - "event, mocked_model, approved", + "account_name, mocked_model, is_denied", + ( + ( + "github.com/fero", + ("github.com/fero", "github.com"), + False, + ), + ( # gitlab.com/packit-service denied + "gitlab.com/packit-service/src/glibc.git", + ( + "gitlab.com/packit-service/src/glibc.git", + "gitlab.com/packit-service/src", + "gitlab.com/packit-service", + ), + True, + ), + ( + "github.com/src/glibc.git", + ("github.com/src/glibc.git", "github.com/src", "github.com"), + False, + ), + ( # gitlab.com/packit denied + "gitlab.com/packit/packit.git", + ("gitlab.com/packit/packit.git", "gitlab.com/packit"), + True, + ), + ), + indirect=["mocked_model"], +) +def test_is_denied(allowlist, account_name, mocked_model, is_denied): + assert allowlist.is_namespace_or_parent_denied(account_name) == is_denied + + +@pytest.mark.parametrize( + "event, mocked_model, approved, user_namespace", [ ( PullRequestCommentGithubEvent( @@ -216,6 +249,7 @@ def test_is_approved(allowlist, account_name, mocked_model, is_approved): ), ("github.com/foo/bar.git", "github.com/foo", "github.com"), False, + "github.com/bar", ), ( IssueCommentEvent( @@ -231,6 +265,7 @@ def test_is_approved(allowlist, account_name, mocked_model, is_approved): ), ("github.com/foo/bar.git", "github.com/foo", "github.com"), False, + "github.com/baz", ), ( PullRequestCommentGithubEvent( @@ -248,6 +283,7 @@ def test_is_approved(allowlist, account_name, mocked_model, is_approved): ), ("github.com/fero/dwm.git", "github.com/fero"), True, + "github.com/lojzo", ), ( IssueCommentEvent( @@ -266,6 +302,7 @@ def test_is_approved(allowlist, account_name, mocked_model, is_approved): "gitlab.com/packit-service/src", ), True, + "gitlab.com/lojzo", ), ( PullRequestCommentGithubEvent( @@ -283,16 +320,24 @@ def test_is_approved(allowlist, account_name, mocked_model, is_approved): ), [], True, + "github.com/admin", ), ], indirect=["mocked_model"], ) -def test_check_and_report_calls_method(allowlist, event, mocked_model, approved): +def test_check_and_report_calls_method( + allowlist, event, mocked_model, approved, user_namespace +): gp = GitProject("", GitService(), "") - + flexmock(DBAllowlist).should_receive("get_namespace").with_args( + user_namespace + ).and_return() flexmock(gp).should_receive("can_merge_pr").with_args(event.actor).and_return( approved ) + flexmock(Allowlist).should_receive("is_namespace_or_parent_denied").and_return( + False + ) mocked_pr_or_issue = flexmock(author=None) if isinstance(event, IssueCommentEvent): flexmock(gp).should_receive("get_issue").and_return(mocked_pr_or_issue) @@ -315,6 +360,96 @@ def test_check_and_report_calls_method(allowlist, event, mocked_model, approved) ) +@pytest.mark.parametrize( + "event", + [ + PullRequestCommentGithubEvent( + action=PullRequestCommentAction.created, + pr_id=0, + base_repo_namespace="base", + base_repo_name="", + base_ref="", + target_repo_namespace="foo", + target_repo_name="bar", + project_url="https://github.com/foo/bar", + actor="bar", + comment="", + comment_id=0, + ), + IssueCommentEvent( + action=IssueCommentAction.created, + issue_id=0, + repo_namespace="foo", + repo_name="bar", + target_repo="", + project_url="https://github.com/foo/bar", + actor="baz", + comment="", + comment_id=0, + ), + PullRequestCommentGithubEvent( + action=PullRequestCommentAction.created, + pr_id=0, + base_repo_namespace="foo", + base_repo_name="dwm", + base_ref="", + target_repo_namespace="fero", + target_repo_name="dwm.git", + project_url="https://github.com/fero/dwm", + actor="lojzo", + comment="", + comment_id=0, + ), + IssueCommentEvent( + action=IssueCommentAction.created, + issue_id=0, + repo_namespace="packit-service/src", + repo_name="glibc", + target_repo="", + project_url="https://gitlab.com/packit-service/src/glibc", + actor="lojzo", + comment="", + comment_id=0, + ), + PullRequestCommentGithubEvent( + action=PullRequestCommentAction.created, + pr_id=0, + base_repo_namespace="banned_namespace", + base_repo_name="", + base_ref="", + target_repo_namespace="banned_namespace_again", + target_repo_name="some_repo", + project_url="https://github.com/banned_namespace_again/some_repo", + actor="ljzo", + comment="", + comment_id=0, + ), + ], +) +def test_check_and_report_denied_project(allowlist, event): + gp = GitProject("", GitService(), "") + flexmock(Allowlist).should_receive("is_denied").and_return(False) + flexmock(Allowlist).should_receive("is_namespace_or_parent_denied").and_return(True) + mocked_pr_or_issue = flexmock(author=None) + if isinstance(event, IssueCommentEvent): + flexmock(gp).should_receive("get_issue").and_return(mocked_pr_or_issue) + else: + flexmock(gp).should_receive("get_pr").and_return(mocked_pr_or_issue) + mocked_pr_or_issue.should_receive("comment").with_args( + f"{Allowlist._strip_protocol_and_add_git(event.project_url)} or parent namespaces denied!" + ).once() + + ServiceConfig.get_service_config().admins = {"admin"} + assert ( + allowlist.check_and_report( + event, + gp, + job_configs=[], + ) + is False + ) + + @pytest.fixture() def events(request) -> Iterable[Tuple[AbstractGithubEvent, bool, Iterable[str]]]: """ @@ -512,7 +647,13 @@ def test_check_and_report( flexmock(EventData).should_receive("from_event_dict").and_return( flexmock(commit_sha="0000000", pr_id="0") ) - + actor_namespace = ( + f"{'github.com' if isinstance(event.project, GithubProject) else 'gitlab.com'}" + f"/{event.actor}" + ) + flexmock(DBAllowlist).should_receive("get_namespace").with_args( + actor_namespace + ).and_return() if isinstance(event, PullRequestGithubEvent) and not is_valid: notification_project_mock = flexmock() notification_project_mock.should_receive("get_issue_list").with_args( @@ -544,7 +685,7 @@ def test_check_and_report( ) flexmock(LocalProject).should_receive("checkout_pr").and_return(None) flexmock(StatusReporter).should_receive("report").with_args( - description="github.com/the-namespace not allowed!", + description=str, state=BaseCommitStatus.neutral, url="https://issue.url", check_names=[EXPECTED_TESTING_FARM_CHECK_NAME], @@ -570,6 +711,9 @@ def test_check_and_report( "fedora-rawhide-x86_64", } ) + flexmock(Allowlist).should_receive("is_namespace_or_parent_denied").and_return( + False + ) mock_model(allowlist_entries, resolved_through) assert ( @@ -582,6 +726,143 @@ def test_check_and_report( ) +def test_check_and_report_actor_denied_issue(allowlist): + event = IssueCommentEvent( + action=IssueCommentAction.created, + issue_id=0, + repo_namespace="foo", + repo_name="bar", + target_repo="", + project_url="https://github.com/foo/bar", + actor="bar", + comment="", + comment_id=0, + ) + issue = flexmock() + flexmock(issue).should_receive("comment").with_args( + "User namespace bar denied!" + ).once() + flexmock( + GithubProject, + create_check_run=lambda *args, **kwargs: None, + get_issue=lambda *args, **kwargs: issue, + ) + job_configs = [ + JobConfig( + type=JobType.tests, + trigger=JobConfigTriggerType.pull_request, + packages={ + "package": CommonPackageConfig( + _targets=["fedora-rawhide"], + ) + }, + ) + ] + + git_project = GithubProject("the-repo", GithubService(), "the-namespace") + flexmock(event, project=git_project).should_receive("get_dict").and_return(None) + flexmock(EventData).should_receive("from_event_dict").and_return( + flexmock(commit_sha="0000000", pr_id="0") + ) + flexmock(DBAllowlist).should_receive("get_namespace").with_args( + "github.com/bar" + ).and_return(flexmock(status=AllowlistStatus.denied)) + flexmock(CoprHelper).should_receive("get_valid_build_targets").and_return( + { + "fedora-rawhide-x86_64", + } + ) + + assert ( + allowlist.check_and_report( + event, + git_project, + job_configs=job_configs, + ) + is False + ) + + +def test_check_and_report_actor_pull_request(allowlist): + event = PullRequestGithubEvent( + action=PullRequestAction.opened, + pr_id=0, + base_repo_namespace="base", + base_repo_name="", + base_ref="", + target_repo_namespace="foo", + target_repo_name="bar", + project_url="https://github.com/foo/bar", + actor="bar", + commit_sha="", + ) + flexmock( + GithubProject, + create_check_run=lambda *args, **kwargs: None, + get_pr=lambda *args, **kwargs: flexmock( + source_project=flexmock(), author=None, comment=lambda *args, **kwargs: None + ), + ) + job_configs = [ + JobConfig( + type=JobType.tests, + trigger=JobConfigTriggerType.pull_request, + packages={ + "package": CommonPackageConfig( + _targets=["fedora-rawhide"], + ) + }, + ) + ] + flexmock(PullRequestGithubEvent).should_receive("get_packages_config").and_return( + flexmock(jobs=job_configs, get_package_config_for=lambda job_config: flexmock()) + ) + flexmock(PullRequestModel).should_receive("get_or_create").and_return( + flexmock( + job_config_trigger_type=JobConfigTriggerType.pull_request, + id=123, + job_trigger_model_type=JobTriggerModelType.pull_request, + ) + ) + + flexmock(JobTriggerModel).should_receive("get_or_create").with_args( + type=JobTriggerModelType.pull_request, trigger_id=123 + ).and_return(flexmock(id=2, type=JobTriggerModelType.pull_request)) + git_project = GithubProject("the-repo", GithubService(), "the-namespace") + flexmock(event, project=git_project).should_receive("get_dict").and_return(None) + flexmock(EventData).should_receive("from_event_dict").and_return( + flexmock(commit_sha="0000000", pr_id="0") + ) + flexmock(DBAllowlist).should_receive("get_namespace").with_args( + "github.com/bar" + ).and_return(flexmock(status=AllowlistStatus.denied)) + flexmock(LocalProject).should_receive("refresh_the_arguments").and_return(None) + flexmock(LocalProject).should_receive("checkout_pr").and_return(None) + flexmock(StatusReporter).should_receive("report").with_args( + description="User namespace denied!", + state=BaseCommitStatus.neutral, + url=None, + check_names=[EXPECTED_TESTING_FARM_CHECK_NAME], + markdown_content=DENIED_MSG, + links_to_external_services=None, + update_feedback_time=object, + ).once() + flexmock(CoprHelper).should_receive("get_valid_build_targets").and_return( + { + "fedora-rawhide-x86_64", + } + ) + + assert ( + allowlist.check_and_report( + event, + git_project, + job_configs=job_configs, + ) + is False + ) + + @pytest.mark.parametrize( "url, expected_url", [ diff --git a/tests_openshift/database/test_allowlist.py b/tests_openshift/database/test_allowlist.py index ccec8df70..ec6f03773 100644 --- a/tests_openshift/database/test_allowlist.py +++ b/tests_openshift/database/test_allowlist.py @@ -52,9 +52,9 @@ def test_get_namespace(clean_before_and_after, multiple_allowlist_entries): def test_get_namespaces_by_status(clean_before_and_after, multiple_allowlist_entries): - a = AllowlistModel.get_namespaces_by_status("waiting") + a = AllowlistModel.get_by_status("waiting") assert len(list(a)) == 2 - b = AllowlistModel.get_namespaces_by_status("approved_manually") + b = AllowlistModel.get_by_status("approved_manually") assert len(list(b)) == 2