Support dist-git account aliases for Bodhi's allowed_builders (#2368)

Support dist-git account aliases for Bodhi's allowed_builders

Related to #2312
RELEASE NOTES BEGIN
N/A
RELEASE NOTES END

Reviewed-by: František Lachman <[email protected]>
Reviewed-by: Maja Massarini

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"

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

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 "

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

tests/unit/test_checkers.py

@@ -2,7 +2,6 @@
 # SPDX-License-Identifier: MIT
 
 import pytest
-
 from flexmock import flexmock
 
 from ogr import PagureService
@@ -16,9 +15,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 +27,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 +59,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,36 +962,19 @@ 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"})
     flexmock(PagureService).should_receive("get_group").with_args("copr").and_return(
         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(
@@ -1098,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()