From e2c0b83843133b24ce98f5ec387879754aa0a481 Mon Sep 17 00:00:00 2001 From: Laura Barcziova Date: Sat, 9 Mar 2024 16:54:56 +0100 Subject: [PATCH 1/2] Extract checking of dist-git account aliases to separate class So it can be reused for Bodhi checks as well. --- packit_service/constants.py | 5 -- packit_service/worker/checker/distgit.py | 84 ++++------------------- packit_service/worker/checker/helper.py | 86 ++++++++++++++++++++++++ tests/unit/test_checkers.py | 33 ++------- 4 files changed, 105 insertions(+), 103 deletions(-) create mode 100644 packit_service/worker/checker/helper.py diff --git a/packit_service/constants.py b/packit_service/constants.py index 8a5ebb32e..83ea7a574 100644 --- a/packit_service/constants.py +++ b/packit_service/constants.py @@ -279,8 +279,3 @@ def from_number(number: int): "logs_url", ) } - - -class KojiAllowedAccountsAlias(Enum): - all_admins = "all_admins" - all_committers = "all_committers" diff --git a/packit_service/worker/checker/distgit.py b/packit_service/worker/checker/distgit.py index 7061aba8c..8efb86904 100644 --- a/packit_service/worker/checker/distgit.py +++ b/packit_service/worker/checker/distgit.py @@ -4,11 +4,11 @@ import logging import re -from ogr.abstract import AccessLevel from packit.config.aliases import get_branches -from packit_service.constants import MSG_GET_IN_TOUCH, KojiAllowedAccountsAlias +from packit_service.constants import MSG_GET_IN_TOUCH from packit_service.utils import pr_labels_match_configuration from packit_service.worker.checker.abstract import Checker, ActorChecker +from packit_service.worker.checker.helper import DistgitAccountsChecker from packit_service.worker.events import ( PushPagureEvent, IssueCommentEvent, @@ -41,70 +41,6 @@ def pre_check(self) -> bool: class PermissionOnDistgit(Checker, GetPagurePullRequestMixin): - @staticmethod - def is_koji_allowed_accounts_alias(value: str) -> bool: - return any(value == alias.value for alias in KojiAllowedAccountsAlias) - - def check_allowed_accounts( - self, accounts_list: list[str], account_to_check: str - ) -> bool: - """ - Check whether the account_to_check matches one of the values in accounts_list - (considering the groups and aliases). - """ - logger.info(f"Checking {account_to_check} in list of accounts: {accounts_list}") - - direct_account_names = [ - value - for value in accounts_list - if not self.is_koji_allowed_accounts_alias(value) - and not value.startswith("@") - ] - - # check the direct account names to prevent unneeded API interactions - if account_to_check in direct_account_names: - return True - - all_accounts = set() - - for value in accounts_list: - if self.is_koji_allowed_accounts_alias(value): - all_accounts.update(self.expand_maintainer_alias(value)) - elif value.startswith("@"): - try: - # remove @ - group_name = value[1:] - group = self.project.service.get_group(group_name) - all_accounts.update(group.members) - except Exception as ex: - logger.debug( - f"Exception while getting the members of group {value}: {ex!r}" - ) - continue - else: - all_accounts.add(value) - - logger.debug(f"Expanded accounts list: {all_accounts}") - return account_to_check in all_accounts - - def expand_maintainer_alias(self, alias: str) -> set[str]: - """ - Expand the 'all_admins' and 'all_committers' aliases to users. - """ - # see AccessLevel mapping - # https://github.com/packit/ogr/blob/d183a6c6459231c2a60bacd6b827502c92a130ef/ogr/abstract.py#L1079 - # all_admins -> Pagure "admin" and "maintainer" access - # all_committers -> on top of that "commit" access - access_levels = [AccessLevel.maintain] - - if alias == KojiAllowedAccountsAlias.all_committers.value: - access_levels.extend([AccessLevel.admin, AccessLevel.push]) - - accounts = self.project.get_users_with_given_access(access_levels) - - logger.debug(f"Expanded {alias}: {accounts}") - return accounts - def pre_check(self) -> bool: if self.data.event_type in (PushPagureEvent.__name__,): if self.data.git_ref not in ( @@ -123,9 +59,11 @@ def pre_check(self) -> bool: if self.pull_request: pr_author = self.get_pr_author() logger.debug(f"PR author: {pr_author}") - if not self.check_allowed_accounts( - self.job_config.allowed_pr_authors, pr_author - ): + if not DistgitAccountsChecker( + self.project, + accounts_list=self.job_config.allowed_pr_authors, + account_to_check=pr_author, + ).check_allowed_accounts(): logger.info( f"Push event {self.data.identifier} with corresponding PR created by" f" {pr_author} that is not allowed in project " @@ -138,9 +76,11 @@ def pre_check(self) -> bool: ) committer = self.data.event_dict["committer"] logger.debug(f"Committer: {committer}") - if not self.check_allowed_accounts( - self.job_config.allowed_committers, committer - ): + if not DistgitAccountsChecker( + self.project, + accounts_list=self.job_config.allowed_committers, + account_to_check=committer, + ).check_allowed_accounts(): logger.info( f"Push event {self.data.identifier} done by " f"{committer} that is not allowed in project " diff --git a/packit_service/worker/checker/helper.py b/packit_service/worker/checker/helper.py new file mode 100644 index 000000000..65e89c76a --- /dev/null +++ b/packit_service/worker/checker/helper.py @@ -0,0 +1,86 @@ +# Copyright Contributors to the Packit project. +# SPDX-License-Identifier: MIT +import logging +from enum import Enum + +from ogr.abstract import GitProject, AccessLevel + +logger = logging.getLogger(__name__) + + +class DistgitAllowedAccountsAlias(Enum): + all_admins = "all_admins" + all_committers = "all_committers" + + +class DistgitAccountsChecker: + def __init__( + self, project: GitProject, accounts_list: list[str], account_to_check: str + ): + self.project = project + self.accounts_list = accounts_list + self.account_to_check = account_to_check + + @staticmethod + def is_distgit_allowed_accounts_alias(value: str) -> bool: + return any(value == alias.value for alias in DistgitAllowedAccountsAlias) + + def check_allowed_accounts(self) -> bool: + """ + Check whether the account_to_check matches one of the values in accounts_list + (considering the groups and aliases). + """ + logger.info( + f"Checking {self.account_to_check} in list of accounts: {self.accounts_list}" + ) + + direct_account_names = [ + value + for value in self.accounts_list + if not self.is_distgit_allowed_accounts_alias(value) + and not value.startswith("@") + ] + + # check the direct account names to prevent unneeded API interactions + if self.account_to_check in direct_account_names: + return True + + all_accounts = set() + + for value in self.accounts_list: + if self.is_distgit_allowed_accounts_alias(value): + all_accounts.update(self.expand_maintainer_alias(value)) + elif value.startswith("@"): + try: + # remove @ + group_name = value[1:] + group = self.project.service.get_group(group_name) + all_accounts.update(group.members) + except Exception as ex: + logger.debug( + f"Exception while getting the members of group {value}: {ex!r}" + ) + continue + else: + all_accounts.add(value) + + logger.debug(f"Expanded accounts list: {all_accounts}") + return self.account_to_check in all_accounts + + def expand_maintainer_alias(self, alias: str) -> set[str]: + """ + Expand the 'all_admins' and 'all_committers' aliases to users. + """ + # see AccessLevel mapping + # https://github.com/packit/ogr/blob/d183a6c6459231c2a60bacd6b827502c92a130ef/ogr/abstract.py#L1079 + # all_admins -> Pagure "admin" and "maintainer" access + # all_committers -> on top of that "commit" access + access_levels = [AccessLevel.maintain] + + if alias == DistgitAllowedAccountsAlias.all_committers.value: + access_levels.extend([AccessLevel.admin, AccessLevel.push]) + + accounts = self.project.get_users_with_given_access(access_levels) + + logger.debug(f"Expanded {alias}: {accounts}") + return accounts diff --git a/tests/unit/test_checkers.py b/tests/unit/test_checkers.py index f61861dd1..b8fac4101 100644 --- a/tests/unit/test_checkers.py +++ b/tests/unit/test_checkers.py @@ -16,9 +16,9 @@ JobConfig, PackageConfig, ) - from packit.config.commands import TestCommandConfig from packit.config.requirements import RequirementsConfig, LabelRequirementsConfig +from packit.copr_helper import CoprHelper from packit_service.config import ServiceConfig from packit_service.models import CoprBuildTargetModel from packit_service.worker.checker.bodhi import IsKojiBuildOwnerMatchingConfiguration @@ -28,9 +28,9 @@ ) from packit_service.worker.checker.distgit import ( IsUpstreamTagMatchingConfig, - PermissionOnDistgit, LabelsOnDistgitPR, ) +from packit_service.worker.checker.helper import DistgitAccountsChecker from packit_service.worker.checker.koji import ( IsJobConfigTriggerMatching as IsJobConfigTriggerMatchingKoji, ) @@ -60,8 +60,6 @@ from packit_service.worker.helpers.build.koji_build import KojiBuildJobHelper from packit_service.worker.mixin import ConfigFromEventMixin -from packit.copr_helper import CoprHelper - def construct_dict(event, action=None, git_ref="random-non-configured-branch"): return { @@ -965,25 +963,6 @@ def test_koji_check_allowed_accounts( allowed_pr_authors, should_pass, ): - jobs = [ - JobConfig( - type=JobType.koji_build, - trigger=JobConfigTriggerType.commit, - packages={ - "package": CommonPackageConfig( - dist_git_branches=["f36"], - allowed_pr_authors=allowed_pr_authors, - ) - }, - ), - ] - - package_config = PackageConfig( - jobs=jobs, - packages={"package": CommonPackageConfig()}, - ) - job_config = jobs[0] - flexmock(PagureProject).should_receive("get_users_with_given_access").with_args( [AccessLevel.maintain] ).and_return({"admin-1"}) @@ -991,10 +970,12 @@ def test_koji_check_allowed_accounts( flexmock(members={"group-account-1"}) ) - checker = PermissionOnDistgit( - package_config, job_config, distgit_push_event.get_dict() + assert ( + DistgitAccountsChecker( + distgit_push_event.project, allowed_pr_authors, account + ).check_allowed_accounts() + == should_pass ) - assert checker.check_allowed_accounts(allowed_pr_authors, account) == should_pass @pytest.mark.parametrize( From aaa1e1dc76ce005d03df4ebd542c7515d065dc4a Mon Sep 17 00:00:00 2001 From: Laura Barcziova Date: Sat, 9 Mar 2024 16:55:31 +0100 Subject: [PATCH 2/2] Support dist-git account aliases for allowed_builders The same way as for allowed_pr_authors and allowed_committers. --- packit_service/worker/checker/bodhi.py | 14 +++++++---- tests/unit/test_checkers.py | 33 +++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/packit_service/worker/checker/bodhi.py b/packit_service/worker/checker/bodhi.py index b442f31a5..bd9575a82 100644 --- a/packit_service/worker/checker/bodhi.py +++ b/packit_service/worker/checker/bodhi.py @@ -11,6 +11,7 @@ ActorChecker, Checker, ) +from packit_service.worker.checker.helper import DistgitAccountsChecker from packit_service.worker.handlers.mixin import ( GetKojiBuildData, GetKojiBuildDataFromKojiBuildEventMixin, @@ -74,11 +75,16 @@ def pre_check(self) -> bool: """Check if the build submitter matches the configuration""" if self.data.event_type in (KojiBuildEvent.__name__,): - if (owner := self.koji_build_event.owner) not in ( - configured_builders := self.job_config.allowed_builders - ): + owner = self.koji_build_event.owner + configured_builders = self.job_config.allowed_builders + + if not DistgitAccountsChecker( + self.project, + accounts_list=configured_builders, + account_to_check=owner, + ).check_allowed_accounts(): logger.info( - f"Owner of the build ({owner}) does not match the" + f"Owner of the build ({owner}) does not match the " f"configuration: {configured_builders}" ) return False diff --git a/tests/unit/test_checkers.py b/tests/unit/test_checkers.py index b8fac4101..171216ff4 100644 --- a/tests/unit/test_checkers.py +++ b/tests/unit/test_checkers.py @@ -2,7 +2,6 @@ # SPDX-License-Identifier: MIT import pytest - from flexmock import flexmock from ogr import PagureService @@ -1079,3 +1078,35 @@ def test_allowed_builders_for_bodhi( package_config, job_config, koji_build_completed_event.get_dict() ) assert checker.pre_check() == should_pass + + +def test_allowed_builders_for_bodhi_alias( + koji_build_completed_event, +): + koji_build_completed_event.owner = "owner" + jobs = [ + JobConfig( + type=JobType.bodhi_update, + trigger=JobConfigTriggerType.commit, + packages={ + "package": CommonPackageConfig( + dist_git_branches=["f36"], allowed_builders=["all_admins"] + ) + }, + ), + ] + + flexmock(PagureProject).should_receive("get_users_with_given_access").and_return( + ["owner"] + ) + + package_config = PackageConfig( + jobs=jobs, + packages={"package": CommonPackageConfig()}, + ) + job_config = jobs[0] + + checker = IsKojiBuildOwnerMatchingConfiguration( + package_config, job_config, koji_build_completed_event.get_dict() + ) + assert checker.pre_check()