From a99a426ce05121d543db709457d531ded90dc143 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 03 2019 10:34:07 +0000 Subject: [PATCH 1/3] Drop two early return statements that by-pass the check for PR only Basically, we were returning True before we had a chance to check if the project allowed to direct commits vs enforcing a PR only workflow. RCM and releng are not concerned by this though. Fixes https://pagure.io/pagure/issue/4248 Signed-off-by: Pierre-Yves Chibon --- diff --git a/dist_git_auth.py b/dist_git_auth.py index 9625f8f..4fde76e 100644 --- a/dist_git_auth.py +++ b/dist_git_auth.py @@ -235,7 +235,6 @@ class DistGitAuth(GitAuthHelper): return False elif is_supported is True: self.debug("Branch %s is supported" % refname) - return is_committer else: self.debug("No supported status available") @@ -246,14 +245,16 @@ class DistGitAuth(GitAuthHelper): return False # Block second level blacklists - for entry in self.unspecified_blacklist: - if entry.match(refname): - self.info("Unspecified ref %s is blocked" % refname) - return False + if not is_supported: + # if a branch is not supported, check if the user is allowed + # to push to/create it. + for entry in self.unspecified_blacklist: + if entry.match(refname): + self.info("Unspecified ref %s is blocked" % refname) + return False # For unspecified refs, they can push if they're a committer self.debug("Unspecified branch push") - return is_committer # This is outside of the strongly protected namespaces if repotype == "main": From 240b1d66963734b566e06c8a10621060b32e2ea9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 03 2019 10:46:32 +0000 Subject: [PATCH 2/3] Expand on the documentation in the code Signed-off-by: Pierre-Yves Chibon --- diff --git a/dist_git_auth.py b/dist_git_auth.py index 4fde76e..168cfb2 100644 --- a/dist_git_auth.py +++ b/dist_git_auth.py @@ -199,20 +199,26 @@ class DistGitAuth(GitAuthHelper): self.debug("SIG memberships: %s" % user_sigs) self.debug("RCM: %s" % is_rcm) + # Quick reminder about the protected_namespace: git.centos.org is + # hosting in one pagure instance a "regular" git forge as well as + # a dist-git instance. So they need to protect the dist-git + # namespaces via the dist-git specific checks while the other + # namespaces go via the regular checks. + # We have data, start the actual ACL checking if ( repotype == "main" and not project.is_fork and project.namespace in self.protected_namespaces ): - # In the protected namespace, we want to make sure we don't - # trample on blacklisted content. + # In the protected namespace, we want to make sure that we block + # blacklisted branches. for entry in self.blacklists: if entry.match(refname): self.info("Ref %s is blocked" % refname) return False - # Allow RCM push + # Allow RCM/releng to push regardless if is_rcm: for refre in self.rcm_branches: if refre.match(refname): @@ -228,7 +234,8 @@ class DistGitAuth(GitAuthHelper): self.debug("SIG push") return True - # For Fedora, allow supported branches + # For Fedora, allow supported branches, these are the active + # branches in PDC is_supported = self.is_supported_branch(project, refname) if is_supported is False: self.info("Branch %s is unsupported" % refname) @@ -238,16 +245,17 @@ class DistGitAuth(GitAuthHelper): else: self.debug("No supported status available") + # This allows to block anything that is not allowed, so no + # random branch creation. if self.block_unspecified: self.info( "Access to namespace %s is restricted" % project.namespace ) return False - # Block second level blacklists + # For branches that are not explicitely active in PDC, check + # if the user is allowed to create/push to them. if not is_supported: - # if a branch is not supported, check if the user is allowed - # to push to/create it. for entry in self.unspecified_blacklist: if entry.match(refname): self.info("Unspecified ref %s is blocked" % refname) @@ -256,7 +264,8 @@ class DistGitAuth(GitAuthHelper): # For unspecified refs, they can push if they're a committer self.debug("Unspecified branch push") - # This is outside of the strongly protected namespaces + # This is applicable to all namespace, protected or not + if repotype == "main": # If this project has PRs only on, or PRs are globally enforced and # this is not a fork, only allow pushing if this is a PR merge. @@ -267,9 +276,9 @@ class DistGitAuth(GitAuthHelper): self.info("A pull request is required for this branch") return False - # This is an unprotected namespace, let's allow committers + # Allow committers to commit if is_committer: - self.debug("Committer push to unprotected") + self.debug("Committer push") return True # If all else fails, deny diff --git a/dist_git_auth_tests.py b/dist_git_auth_tests.py index 7092a34..fee227d 100644 --- a/dist_git_auth_tests.py +++ b/dist_git_auth_tests.py @@ -288,7 +288,7 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): ) ) - self.expect_info_msg("Committer push to unprotected") + self.expect_info_msg("Committer push") def test_unprotected_non_committer(self): project = self.create_namespaced_project('unprotected', 'test') @@ -329,7 +329,7 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): ) ) - self.expect_info_msg("Committer push to unprotected") + self.expect_info_msg("Committer push") def test_unprotected_pr_required_no_pr(self): project = self.create_namespaced_project('unprotected', 'test') @@ -371,8 +371,8 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): ) ) - self.expect_info_msg("Committer push to unprotected") - + self.expect_info_msg("Committer push") + def test_unprotected_pr_required_repo_pr_only_no_pr(self): settings = {"pull_request_access_only": True} project = self.create_namespaced_project('unprotected', 'test', settings=settings) @@ -393,7 +393,7 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): ) self.expect_info_msg("A pull request is required for this branch") - + def test_unprotected_pr_required_repo_pr_only(self): settings = {"pull_request_access_only": True} project = self.create_namespaced_project( @@ -416,7 +416,8 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): ) ) - self.expect_info_msg("Committer push to unprotected") + self.expect_info_msg("Committer push") + class DistGitAuthTestsFedora(DistGitAuthTests): dga_config = { From 25686b8675e8a4d888eef5d53c6e61845c88eaba Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 03 2019 11:39:11 +0000 Subject: [PATCH 3/3] Allow RCM/releng to by-pass a PR-only workflow The RCM_GROUP and the EXTERNAL_COMMITTER configuration keys will need to be in synced though. Signed-off-by: Pierre-Yves Chibon --- diff --git a/dist_git_auth.py b/dist_git_auth.py index 168cfb2..591b2f4 100644 --- a/dist_git_auth.py +++ b/dist_git_auth.py @@ -266,9 +266,10 @@ class DistGitAuth(GitAuthHelper): # This is applicable to all namespace, protected or not - if repotype == "main": + if repotype == "main" and not is_rcm: # If this project has PRs only on, or PRs are globally enforced and # this is not a fork, only allow pushing if this is a PR merge. + # However, RCM/releng is allowed to by-pass the PR only model pr_only = project.settings.get("pull_request_access_only", False) if ( pr_only or (self.global_pr_only and not project.is_fork) diff --git a/dist_git_auth_tests.py b/dist_git_auth_tests.py index fee227d..e52b99a 100644 --- a/dist_git_auth_tests.py +++ b/dist_git_auth_tests.py @@ -85,6 +85,7 @@ class DistGitAuthTests(tests.Modeltests): super(DistGitAuthTests, self).setUp() pagure.config.config['ACL_DEBUG'] = True + pagure.config.config['EXTERNAL_COMMITTER'] = {"relenggroup": {}} pagure.config.config.update(self.dga_config) self.dga = dist_git_auth.DistGitAuth() @@ -352,6 +353,27 @@ class DistGitAuthTestsGeneric(DistGitAuthTests): self.expect_info_msg("A pull request is required for this branch") + def test_unprotected_pr_required_no_pr_cvsadmin(self): + project = self.create_namespaced_project('unprotected', 'test') + self.dga.global_pr_only = True + + self.assertTrue( + self.dga.check_acl( + self.session, + project=project, + username="releng", + refname="refs/heads/mywip", + pull_request=None, + repodir=None, + repotype='main', + revfrom=None, + revto=None, + is_internal=False, + ) + ) + + self.expect_info_msg("Committer push") + def test_unprotected_pr_required_requests(self): project = self.create_namespaced_project('unprotected', 'test') self.dga.global_pr_only = True