PR#168: Restrict pushing new branches that are in "unspecified blacklist" to the releng user group - pagure-dist-git

pagure-dist-git

#168 Restrict pushing new branches that are in "unspecified blacklist" to the releng user group

Closed 5 months ago by dherrera. Opened 5 months ago by dherrera.

dherrera/pagure-dist-git restrict_push_reserved_branch into master

Restricting users creating branches for unspecified blacklisted names

Diego Herrera • 5 months ago

56515d7

Add test case for restricting users creating branches for unspecified blacklisted names

Diego Herrera • 5 months ago

fa45f63

Add repo check to ACL tests

Diego Herrera • 5 months ago

9ef613b

dist_git_auth.py

file modified

		`@@ -81,6 +81,11 @@`
		`cmd = ["ls-tree", branch, "--name-only", "--", "dead.package"]`
		`return read_git_output(cmd, abspath) != "dead.package"`

		`+ def branch_exists(self, branch, abspath):`
		`+ """Returns if the branch is already created"""`
		`+ cmd = ["show-ref", "refs/heads/" + branch]`
		`+ return len(read_git_output(cmd, abspath)) > 0`
		`+`
		`def is_supported_branch(self, project, refname, repodir):`
		`"""Returns whether a specific branch is currently supported for Fedora`

		`@@ -135,6 +140,10 @@`
		`else:`
		`return None`

		`+ if not self.branch_exists(refname, repodir):`
		`+ # If branch doesn't exist, return None`
		`+ return None`
		`+`
		`# Branch can be supported, but package can be retired,`
		`# in that case don't push`
		`active = self.is_not_retired_package(refname, repodir)`

pagure_distgit_tests/test_dist_git_auth.py

file modified

+130 -6

		`@@ -401,9 +401,9 @@`
		`"PR_ONLY": False,`
		`"ACL_BLOCK_UNSPECIFIED": False,`
		`"BLACKLIST_RES": ["refs/heads/c[0-9]+.*"],`
		`- "UNSPECIFIED_BLACKLIST_RES": ["refs/heads/f[0-9]+"],`
		`+ "UNSPECIFIED_BLACKLIST_RES": ["refs/heads/f[0-9]+", "refs/heads/epel[0-9]+"],`
		`"RCM_GROUP": "relenggroup",`
		`- "RCM_BRANCHES": ["refs/heads/f[0-9]+"],`
		`+ "RCM_BRANCHES": ["refs/heads/f[0-9]+", "refs/heads/epel[0-9]+"],`
		`"ACL_PROTECTED_NAMESPACES": ["rpms", "modules", "container"],`
		`"PDC_URL": "invalid://",`
		`"BODHI_URL": "invalid://",`
		`@@ -505,6 +505,10 @@`

		`@patch("dist_git_auth.requests")`
		`def test_protected_supported_branch_committer(self, mock_requests):`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ tests.add_content_git_repo(repos[0], branch="f39")`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`project = self.create_namespaced_project("rpms", "test")`
		`res = Mock()`
		`res.ok = True`
		`@@ -544,7 +548,7 @@`
		`username="pingou",`
		`refname="refs/heads/f39",`
		`pull_request=None,`
		`- repodir=None,`
		`+ repodir=repodir,`
		`repotype="main",`
		`revfrom=None,`
		`revto=None,`
		`@@ -557,6 +561,10 @@`
		`@patch("dist_git_auth.requests")`
		`def test_protected_supported_branch_non_committer(self, mock_requests):`
		`project = self.create_namespaced_project("rpms", "test")`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ tests.add_content_git_repo(repos[0], branch="f39")`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`res = Mock()`
		`res.ok = True`
		`res.json.return_value = {`
		`@@ -595,7 +603,7 @@`
		`username="foo",`
		`refname="refs/heads/f39",`
		`pull_request=None,`
		`- repodir=None,`
		`+ repodir=repodir,`
		`repotype="main",`
		`revfrom=None,`
		`revto=None,`
		`@@ -608,6 +616,10 @@`
		`@patch("dist_git_auth.requests")`
		`def test_protected_supported_branch_epel_minor(self, mock_requests):`
		`project = self.create_namespaced_project("rpms", "test")`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ tests.add_content_git_repo(repos[0], branch="epel10.1")`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`res = Mock()`
		`res.ok = True`
		`res.json.return_value = {`
		`@@ -646,7 +658,7 @@`
		`username="foo",`
		`refname="refs/heads/epel10.1",`
		`pull_request=None,`
		`- repodir=None,`
		`+ repodir=repodir,`
		`repotype="main",`
		`revfrom=None,`
		`revto=None,`
		`@@ -659,6 +671,10 @@`
		`@patch("dist_git_auth.requests")`
		`def test_protected_supported_branch_epel(self, mock_requests):`
		`project = self.create_namespaced_project("rpms", "test")`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ tests.add_content_git_repo(repos[0], branch="epel10")`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`res = Mock()`
		`res.ok = True`
		`res.json.return_value = {`
		`@@ -697,7 +713,7 @@`
		`username="foo",`
		`refname="refs/heads/epel10",`
		`pull_request=None,`
		`- repodir=None,`
		`+ repodir=repodir,`
		`repotype="main",`
		`revfrom=None,`
		`revto=None,`
		`@@ -735,6 +751,114 @@`

		`self.expect_info_msg("Unspecified ref refs/heads/f28 is blocked")`

		`+ @patch("dist_git_auth.requests")`
		`+ def test_protected_create_unspecified_blacklisted_branch_fail(self, mock_requests):`
		`+ project = self.create_namespaced_project("rpms", "test")`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`+ res = Mock()`
		`+ res.ok = True`
		`+ res.json.return_value = {`
		`+ "pages": 1,`
		`+ "releases": [`
		`+ {`
		`+ "name": "EPEL-10.0",`
		`+ "long_name": "Fedora EPEL 10.0",`
		`+ "version": "10.0",`
		`+ "id_prefix": "FEDORA-EPEL",`
		`+ "branch": "epel10",`
		`+ "dist_tag": "epel10.0",`
		`+ "stable_tag": "epel10.0",`
		`+ "testing_tag": "epel10.0-testing",`
		`+ "candidate_tag": "epel10.0-testing-candidate",`
		`+ "pending_signing_tag": "epel10.0-signing-pending",`
		`+ "pending_testing_tag": "epel10.0-testing-pending",`
		`+ "pending_stable_tag": "epel10.0-pending",`
		`+ "override_tag": "epel10.0-override",`
		`+ "mail_template": "fedora_epel_legacy_errata_template",`
		`+ "state": "current",`
		`+ "composed_by_bodhi": True,`
		`+ "create_automatic_updates": False,`
		`+ "package_manager": "unspecified",`
		`+ "testing_repository": None,`
		`+ "eol": None,`
		`+ },`
		`+ ],`
		`+ }`
		`+`
		`+ mock_requests.get.return_value = res`
		`+`
		`+ self.assertFalse(`
		`+ self.dga.check_acl(`
		`+ self.session,`
		`+ project=project,`
		`+ username="pingou",`
		`+ refname="refs/heads/epel10",`
		`+ pull_request=None,`
		`+ repodir=repodir,`
		`+ repotype="main",`
		`+ revfrom=None,`
		`+ revto=None,`
		`+ is_internal=False,`
		`+ )`
		`+ )`
		`+`
		`+ self.expect_info_msg("Unspecified ref refs/heads/epel10 is blocked")`
		`+`
		`+ @patch("dist_git_auth.requests")`
		`+ def test_protected_create_unspecified_blacklisted_branch_success(self, mock_requests):`
		`+ project = self.create_namespaced_project("rpms", "test")`
		`+ repodir = os.path.join(self.path, "repos")`
		`+ repos = tests.create_projects_git(repodir, bare=True)`
		`+ repodir = os.path.join(self.path, "repos", "test.git")`
		`+ res = Mock()`
		`+ res.ok = True`
		`+ res.json.return_value = {`
		`+ "pages": 1,`
		`+ "releases": [`
		`+ {`
		`+ "name": "EPEL-10.0",`
		`+ "long_name": "Fedora EPEL 10.0",`
		`+ "version": "10.0",`
		`+ "id_prefix": "FEDORA-EPEL",`
		`+ "branch": "epel10",`
		`+ "dist_tag": "epel10.0",`
		`+ "stable_tag": "epel10.0",`
		`+ "testing_tag": "epel10.0-testing",`
		`+ "candidate_tag": "epel10.0-testing-candidate",`
		`+ "pending_signing_tag": "epel10.0-signing-pending",`
		`+ "pending_testing_tag": "epel10.0-testing-pending",`
		`+ "pending_stable_tag": "epel10.0-pending",`
		`+ "override_tag": "epel10.0-override",`
		`+ "mail_template": "fedora_epel_legacy_errata_template",`
		`+ "state": "current",`
		`+ "composed_by_bodhi": True,`
		`+ "create_automatic_updates": False,`
		`+ "package_manager": "unspecified",`
		`+ "testing_repository": None,`
		`+ "eol": None,`
		`+ },`
		`+ ],`
		`+ }`
		`+`
		`+ mock_requests.get.return_value = res`
		`+`
		`+ self.assertTrue(`
		`+ self.dga.check_acl(`
		`+ self.session,`
		`+ project=project,`
		`+ username="releng",`
		`+ refname="refs/heads/epel10",`
		`+ pull_request=None,`
		`+ repodir=repodir,`
		`+ repotype="main",`
		`+ revfrom=None,`
		`+ revto=None,`
		`+ is_internal=False,`
		`+ )`
		`+ )`
		`+`
		`def test_is_not_retired_package(self):`
		`projects = tests.create_projects_git(os.path.join(self.path, "repos"), bare=True)`
		`tests.add_content_git_repo(projects[0])`

dherrera commented 5 months ago

This PR is a proposal to fix releng#12457.

The problem is that package maintainers should create new EPEL10 branches through the package-request process, but currently users can push those branches directly through distgit. Since EPEL branch names are matched in the Unspecified Blacklist regex array, this PR would make it for them to be filtered when creating new branches with those names.

carlwgeorge commented 5 months ago

Wouldn't this change break the fedpkg request-branch --no-git-branch workflow? In that scenario we want to allow pushing to a branch that doesn't exist yet.

dherrera commented 5 months ago

Wouldn't this change break the fedpkg request-branch --no-git-branch workflow? In that scenario we want to allow pushing to a branch that doesn't exist yet.

Yes, it would disable people from doing that :/

Even if this fixes the problem, it's way too overkill to disable that usecase for this purpse... I'll just close it for now.

Pull-Request has been closed by dherrera

5 months ago