Support aliases and groups in allowed_* config options for Koji (#2320)

Support aliases and groups in allowed_* config options for Koji

Introduce aliases all_admins and all_committers and also support groups in allowed_pr_authors and allowed_committers config options
Fixes packit/packit#2088
Requires packit/ogr#834
TODO:

 Write new tests or update the old ones to cover new functionality (needs packit/ogr#834)
 Update or write new documentation in packit/packit.dev. (packit/packit.dev#814)


RELEASE NOTES BEGIN
allowed_pr_authors and allowed_committers now allow specifying groups and also aliases all_admins and all_committers (corresponding to the access to the repository).
RELEASE NOTES END

Reviewed-by: Maja Massarini
Reviewed-by: Laura Barcziová
Reviewed-by: Nikola Forró

Author: softwarefactory-project-zuul[bot]

packit_service/constants.py

@@ -289,3 +289,8 @@ def from_number(number: int):
     "of the `pull_from_upstream` job. We believe this adjustment will simplify the onboarding "
     "process and enhance the overall user experience. "
 )
+
+
+class KojiAllowedAccountsAlias(Enum):
+    all_admins = "all_admins"
+    all_committers = "all_committers"

packit_service/worker/checker/distgit.py

@@ -4,8 +4,9 @@
 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
+from packit_service.constants import MSG_GET_IN_TOUCH, KojiAllowedAccountsAlias
 from packit_service.worker.checker.abstract import Checker, ActorChecker
 from packit_service.worker.events import (
     PushPagureEvent,
@@ -43,6 +44,70 @@ def contains_specfile_change(self):
             return False
         return True
 
+    @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 (
@@ -71,7 +136,9 @@ def pre_check(self) -> bool:
 
                 pr_author = self.get_pr_author()
                 logger.debug(f"PR author: {pr_author}")
-                if pr_author not in self.job_config.allowed_pr_authors:
+                if not self.check_allowed_accounts(
+                    self.job_config.allowed_pr_authors, pr_author
+                ):
                     logger.info(
                         f"Push event {self.data.identifier} with corresponding PR created by"
                         f" {pr_author} that is not allowed in project "
@@ -81,7 +148,9 @@ def pre_check(self) -> bool:
             else:
                 committer = self.data.event_dict["committer"]
                 logger.debug(f"Committer: {committer}")
-                if committer not in self.job_config.allowed_committers:
+                if not self.check_allowed_accounts(
+                    self.job_config.allowed_committers, committer
+                ):
                     logger.info(
                         f"Push event {self.data.identifier} done by "
                         f"{committer} that is not allowed in project "

tests/unit/test_checkers.py

@@ -5,12 +5,16 @@
 
 from flexmock import flexmock
 
+from ogr import PagureService
+from ogr.abstract import AccessLevel
+from ogr.services.pagure import PagureProject
 from packit.config import (
     CommonPackageConfig,
     JobType,
     JobConfigTriggerType,
     JobConfigView,
     JobConfig,
+    PackageConfig,
 )
 
 from packit.config.commands import TestCommandConfig
@@ -20,7 +24,10 @@
     IsJobConfigTriggerMatching as IsJobConfigTriggerMatchingCopr,
     IsPackageMatchingJobView,
 )
-from packit_service.worker.checker.distgit import IsUpstreamTagMatchingConfig
+from packit_service.worker.checker.distgit import (
+    IsUpstreamTagMatchingConfig,
+    PermissionOnDistgit,
+)
 from packit_service.worker.checker.koji import (
     IsJobConfigTriggerMatching as IsJobConfigTriggerMatchingKoji,
 )
@@ -937,3 +944,51 @@ def test_sync_release_matching_tag(upstream_tag_include, upstream_tag_exclude, r
     )
 
     assert checker.pre_check() == result
+
+
+@pytest.mark.parametrize(
+    "account, allowed_pr_authors, should_pass",
+    (
+        ("direct-account", ["all_admins", "direct-account"], True),
+        ("admin-1", ["all_admins"], True),
+        ("admin-2", ["all_admins"], False),
+        ("group-account-1", ["all_admins", "@copr"], True),
+        ("group-account-2", ["all_admins", "@copr"], False),
+    ),
+)
+def test_koji_check_allowed_accounts(
+    distgit_push_event,
+    account,
+    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 checker.check_allowed_accounts(allowed_pr_authors, account) == should_pass