PR#124: Add support for the collaborator access level/push - pagure-dist-git

pagure-dist-git

#124 Add support for the collaborator access level/push

Merged 3 years ago by nphilipp. Opened 3 years ago by pingou.

collaborator_access into master

Add support for the collaborator access level/push

Pierre-Yves Chibon • 3 years ago

2e491c9

dist_git_auth.py

file modified

+7 -5

		`@@ -36,7 +36,7 @@`
		`from pagure.lib.query import get_user`
		`from pagure.lib.git import is_forced_push`
		`from pagure.lib.git_auth import GitAuthHelper, _read_file`
		`- from pagure.utils import is_repo_committer`
		`+ from pagure.utils import is_repo_collaborator`

		`_log = logging.getLogger("pagure_auth")`

		`@@ -180,8 +180,10 @@`
		`user = get_user(session, username)`
		`usergroups = set(user.groups)`

		`- # Determine whether the user is a committer`
		`- is_committer = is_repo_committer(project, username, session)`
		`+ # Determine whether the user is a committer/collaborator, ie: may commit`
		`+ may_commit = is_repo_collaborator(`
		`+ project, refname, username, session`
		`+ )`

		`# Determine whether the user is a SIG member`
		`user_sigs = self.supported_sigs & usergroups`
		`@@ -199,7 +201,7 @@`
		`self.debug("Blacklists: %s" % self.blacklists)`
		`self.debug("User: %s" % user)`
		`self.debug("User groups: %s" % usergroups)`
		`- self.debug("Committer: %s" % is_committer)`
		`+ self.debug("Committer: %s" % may_commit)`
		`self.debug("SIG memberships: %s" % user_sigs)`
		`self.debug("RCM: %s" % is_rcm)`
		`self.debug("By-pass PR-only: %s" % bool(bypass_pr_only))`
		`@@ -283,7 +285,7 @@`
		`return False`

		`# Allow committers to commit`
		`- if is_committer:`
		`+ if may_commit:`
		`self.debug("Committer push")`
nphilipp commented 3 years ago Shouldn't the debugging messages here and above reflect the true meaning? E.g. "May commit:" above and "Push by committer/collaborator"? Not sure how important clarity is for debugging output like this.
		`return True`

pagure_distgit_tests/test_dist_git_auth.py

file modified

+90 -2

		`@@ -7,10 +7,13 @@`
		`import pagure.lib.model`

		`try:`
		`- from pagure.lib import _get_project as get_project`
		`+ from pagure.lib import _get_project as get_project, add_user_to_project`
		`except ImportError:`
		`# From pagure 5.2, code has been moved to pagure.lib.query`
		`- from pagure.lib.query import _get_project as get_project`
		`+ from pagure.lib.query import (`
		`+ _get_project as get_project,`
		`+ add_user_to_project,`
		`+ )`

		`import tests`

		`@@ -600,6 +603,91 @@`
		`self.expect_info_msg("Unspecified branch push")`


		`+ class DistGitAuthTestsFedoraCommitterAccess(DistGitAuthTests):`
		`+ dga_config = {`
		`+ "PR_ONLY": False,`
		`+ "ACL_BLOCK_UNSPECIFIED": False,`
		`+ "BLACKLIST_RES": ["refs/heads/c[0-9]+.*"],`
		`+ "UNSPECIFIED_BLACKLIST_RES": ["refs/heads/f[0-9]+"],`
		`+ "RCM_GROUP": "relenggroup",`
		`+ "RCM_BRANCHES": ["refs/heads/f[0-9]+"],`
		`+ "ACL_PROTECTED_NAMESPACES": ["rpms", "modules", "container"],`
		`+ "PDC_URL": "invalid://",`
		`+ }`
		`+`
		`+ def setUp(self):`
		`+ super(DistGitAuthTestsFedoraCommitterAccess, self).setUp()`
		`+`
		`+ project = self.create_namespaced_project("rpms", "test")`
		`+`
		`+ msg = add_user_to_project(`
		`+ session=self.session,`
		`+ project=project,`
		`+ new_user="foo",`
		`+ user="pingou",`
		`+ access="collaborator",`
		`+ branches="epel*",`
		`+ )`
		`+ self.session.commit()`
		`+ self.assertEqual(msg, "User added")`
		`+`
		`+ @patch("dist_git_auth.requests")`
		`+ def test_protected_unspecified_branch_collaborator_invalid_branch(`
		`+ self, mock_requests`
		`+ ):`
		`+ project = get_project(self.session, name="test", namespace="rpms")`
		`+ res = Mock()`
		`+ res.ok = True`
		`+ res.json.return_value = {"results": []}`
		`+ mock_requests.get.return_value = res`
		`+`
		`+ self.assertFalse(`
		`+ self.dga.check_acl(`
		`+ self.session,`
		`+ project=project,`
		`+ username="foo",`
		`+ refname="refs/heads/mywip",`
		`+ pull_request=None,`
		`+ repodir=None,`
		`+ repotype="main",`
		`+ revfrom=None,`
		`+ revto=None,`
		`+ is_internal=False,`
		`+ )`
		`+ )`
		`+`
		`+ self.expect_info_msg("Committer: False")`
		`+ self.expect_info_msg("Fall-through deny")`
		`+`
		`+ @patch("dist_git_auth.requests")`
		`+ def test_protected_unspecified_branch_collaborator_valid_branch(`
		`+ self, mock_requests`
		`+ ):`
		`+ project = get_project(self.session, name="test", namespace="rpms")`
		`+ res = Mock()`
		`+ res.ok = True`
		`+ res.json.return_value = {"results": []}`
		`+ mock_requests.get.return_value = res`
		`+`
		`+ self.assertTrue(`
		`+ self.dga.check_acl(`
		`+ self.session,`
		`+ project=project,`
		`+ username="foo",`
		`+ refname="refs/heads/epel8",`
		`+ pull_request=None,`
		`+ repodir=None,`
		`+ repotype="main",`
		`+ revfrom=None,`
		`+ revto=None,`
		`+ is_internal=False,`
		`+ )`
		`+ )`
		`+`
		`+ self.expect_info_msg("Committer: True")`
		`+ self.expect_info_msg("Committer push")`
		`+`
		`+`
		`class DistGitAuthTestsCentOS(DistGitAuthTests):`
		`dga_config = {`
		`"PR_ONLY": False,`

pingou commented 3 years ago

Until this commit we only allowed project committer to push to the
project on dist-git while pagure has gained a few months back a new
access level: collaborator which allow to give some user access to only
certain branches.

With this commit we're turning on this feature on dist-git as well.

Fixes https://pagure.io/releng/issue/9834

Signed-off-by: Pierre-Yves Chibon pingou@pingoured.fr

Metadata Update from @nphilipp:
- Request assigned

3 years ago

nphilipp commented on line 17 of dist_git_auth.py 3 years ago

For consistency, maybe rename is_committer to is_collaborator or may_commit?

nphilipp commented 3 years ago

Looks good to me!

rebased onto 2e491c9

3 years ago

nphilipp commented on line 38 of dist_git_auth.py 3 years ago

Shouldn't the debugging messages here and above reflect the true meaning? E.g. "May commit:" above and "Push by committer/collaborator"? Not sure how important clarity is for debugging output like this.

nphilipp commented 3 years ago

As discussed on IRC, the exact wording of debug logs is not terribly important, more that you can follow what happened and why with the logs. Merging.

Pull-Request has been merged by nphilipp

3 years ago