PR#4675: Fix checking if the user is a committer of the repo the PR originates from - pagure

pagure

#4675 Fix checking if the user is a committer of the repo the PR originates from

Merged 4 years ago by pingou. Opened 4 years ago by pingou.

fix_committer_check into master

Fix checking if the user is a committer of the repo the PR originates from

Pierre-Yves Chibon • 4 years ago

78d1ae2

pagure/ui/fork.py

file modified

+15 -6

		`@@ -341,11 +341,13 @@`
		`continue`
		`trigger_ci[comment] = meta`

		`- can_rebase_branch = (`
		`- not request.remote_git`
		`- and request.project_from`
		`- and pagure.utils.is_repo_committer(request.project_from)`
		`- )`
		`+ committer = False`
		`+ if request.project_from:`
		`+ committer = pagure.utils.is_repo_committer(request.project_from)`
		`+ else:`
		`+ committer = pagure.utils.is_repo_committer(request.project)`
		`+`
		`+ can_rebase_branch = not request.remote_git and committer`

		`can_delete_branch = (`
		`pagure_config.get("ALLOW_DELETE_BRANCH", True) and can_rebase_branch`
		`@@ -1177,7 +1179,14 @@`
		`requestid=requestid,`
		`)`
		`)`
		`- if not pagure.utils.is_repo_committer(request.project_from):`
		`+`
		`+ committer = False`
		`+ if request.project_from:`
		`+ committer = pagure.utils.is_repo_committer(request.project_from)`
		`+ else:`
		`+ committer = pagure.utils.is_repo_committer(request.project)`
		`+`
		`+ if not committer:`
		`flask.flash(`
		`"You do not have permissions to delete the branch in the "`
		`"source repo",`

tests/test_pagure_flask_ui_fork.py

file modified

+164

		`@@ -301,6 +301,170 @@`
		`)`

		`@patch("pagure.lib.notify.send_email")`
		`+ def test_request_pull_delete_branch_button_no_auth(self, send_email):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(`
		`+ os.path.join(self.path, "requests"), bare=True`
		`+ )`
		`+`
		`+ set_up_git_repo(`
		`+ self.session, self.path, new_project=None, branch_from="feature"`
		`+ )`
		`+`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+`
		`+ # View the pull-request`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ "<title>PR#1: PR from the feature branch - test\n - "`
		`+ "Pagure</title>",`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'title="View file as of 2a552b">sources</a>', output_text`
		`+ )`
		`+ # Un-authenticated user cannot see this checkbox`
		`+ self.assertNotIn(`
		`+ '<input id="delete_branch" name="delete_branch" type="checkbox" '`
		`+ 'value="y"> <label for="delete_branch">Delete branch after '`
		`+ "merging</label>",`
		`+ output_text,`
		`+ )`
		`+`
		`+ @patch("pagure.lib.notify.send_email")`
		`+ def test_request_pull_delete_branch_button(self, send_email):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(`
		`+ os.path.join(self.path, "requests"), bare=True`
		`+ )`
		`+`
		`+ set_up_git_repo(`
		`+ self.session, self.path, new_project=None, branch_from="feature"`
		`+ )`
		`+`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+`
		`+ # View the pull-request`
		`+ user = tests.FakeUser()`
		`+ user.username = "pingou"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ "<title>PR#1: PR from the feature branch - test\n - "`
		`+ "Pagure</title>",`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'title="View file as of 2a552b">sources</a>', output_text`
		`+ )`
		`+ self.assertIn(`
		`+ '<input id="delete_branch" name="delete_branch" type="checkbox" '`
		`+ 'value="y"> <label for="delete_branch">Delete branch after '`
		`+ "merging</label>",`
		`+ output_text,`
		`+ )`
		`+`
		`+ @patch("pagure.lib.notify.send_email")`
		`+ def test_request_pull_delete_branch_button_no_project_from(`
		`+ self, send_email`
		`+ ):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(`
		`+ os.path.join(self.path, "requests"), bare=True`
		`+ )`
		`+`
		`+ set_up_git_repo(`
		`+ self.session, self.path, new_project=None, branch_from="feature"`
		`+ )`
		`+`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+ project.requests[0].project_from = None`
		`+ self.session.add(project.requests[0])`
		`+ self.session.commit()`
		`+`
		`+ # View the pull-request`
		`+ user = tests.FakeUser()`
		`+ user.username = "pingou"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ "<title>PR#1: PR from the feature branch - test\n - "`
		`+ "Pagure</title>",`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'title="View file as of 2a552b">sources</a>', output_text`
		`+ )`
		`+ self.assertIn(`
		`+ '<input id="delete_branch" name="delete_branch" type="checkbox" '`
		`+ 'value="y"> <label for="delete_branch">Delete branch after '`
		`+ "merging</label>",`
		`+ output_text,`
		`+ )`
		`+`
		`+ @patch("pagure.lib.notify.send_email")`
		`+ def test_request_pull_delete_branch_button_no_project_from_no_acl(`
		`+ self, send_email`
		`+ ):`
		`+ """ Test the request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.create_projects_git(`
		`+ os.path.join(self.path, "requests"), bare=True`
		`+ )`
		`+`
		`+ set_up_git_repo(`
		`+ self.session, self.path, new_project=None, branch_from="feature"`
		`+ )`
		`+`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+ project.requests[0].project_from = None`
		`+ self.session.add(project.requests[0])`
		`+ self.session.commit()`
		`+`
		`+ # View the pull-request`
		`+ user = tests.FakeUser()`
		`+ user.username = "foo"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ "<title>PR#1: PR from the feature branch - test\n - "`
		`+ "Pagure</title>",`
		`+ output_text,`
		`+ )`
		`+ self.assertIn(`
		`+ 'title="View file as of 2a552b">sources</a>', output_text`
		`+ )`
		`+ self.assertNotIn(`
		`+ '<input id="delete_branch" name="delete_branch" type="checkbox" '`
		`+ 'value="y"> <label for="delete_branch">Delete branch after '`
		`+ "merging</label>",`
		`+ output_text,`
		`+ )`
		`+`
		`+ @patch("pagure.lib.notify.send_email")`
		`def test_task_update_request_pull(self, send_email):`
		`""" Test the task update_pull_request endpoint. """`
		`send_email.return_value = True`

pingou commented 4 years ago

There are a few cases where if the PR originates from the same repo that it
targets the project_from attribute of the PullRequest object is
None.
Therefore, we need to check if the user is a committer of either the
project_from if there is one and otherwise the target repo.

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

pingou commented 4 years ago

This should superseed: https://pagure.io/pagure/pull-request/4295#

rebased onto 7ae4892c023cc1025d4d70e1ca39cc8d3627407b

4 years ago