PR#5457: Owner of a PR can update is own PR - pagure

pagure

#5457 Owner of a PR can update is own PR

Merged 2 years ago by ngompa. Opened 2 years ago by mmassari.

mmassari/pagure pr_owner_can_update_it into master

Allow author to update PR but not to change assignee

Maja Massarini • 2 years ago

c8ea202

Owner of a PR can update is own PR

Maja Massarini • 2 years ago

a3cd8f6

pagure/api/fork.py

file modified

+1 -1

		`@@ -502,7 +502,7 @@`
		`_check_token(repo, project_token=False)`

		`request = _get_request(repo, requestid)`
		`- _check_pull_request_access(request, assignee=True)`
		`+ _check_pull_request_access(request, assignee=True, allow_author=True)`

		`form = pagure.forms.RequestPullForm(meta={"csrf": False})`
		`if not form.validate_on_submit():`

pagure/api/utils.py

file modified

+7 -3

		`@@ -190,12 +190,14 @@`
		`)`


		`- def _check_pull_request_access(request, assignee=False):`
		`+ def _check_pull_request_access(request, assignee=False, allow_author=False):`
		`"""Check if user can access Pull-Request. Must be repo committer`
		`- or author to see private pull-requests.`
		`+ or author (if flag is true) to see private pull-requests.`
		`:param request: PullRequest object`
		`:param assignee: a boolean specifying whether to allow the assignee or not`
		`defaults to False`
		`+ :param allow_author: a boolean specifying whether the PR author should be`
		`+ allowed, defaults to False`
		`:raises pagure.exceptions.APIError: when access denied`
		`"""`
		`# Private PRs require commit access`
		`@@ -203,7 +205,9 @@`

		`error = False`
		`# Public tickets require ticket access`
		`- error = not is_repo_user(request.project)`
		`+ error = not is_repo_user(request.project) and not (`
		`+ allow_author and request.user.user == flask.g.fas_user.username`
		`+ )`

		`if assignee:`
		`if (`

tests/test_pagure_flask_api_fork_update.py

file modified

+234

		`@@ -764,5 +764,239 @@`
		`self.assertEqual(len(project.issues[0].related_prs), 1)`


		`+ class PagureFlaskApiForkUpdateNoCommitterTests(tests.SimplePagureTest):`
		`+ """Tests for the flask API of pagure for updating a PR`
		`+ when the PR owner is not a committer in the target project`
		`+ but he is the owner of the PR.`
		`+ """`
		`+`
		`+ maxDiff = None`
		`+`
		`+ @patch("pagure.lib.git.update_git", MagicMock(return_value=True))`
		`+ @patch("pagure.lib.notify.send_email", MagicMock(return_value=True))`
		`+ def setUp(self):`
		`+ """Set up the environnment, ran before every tests.`
		`+`
		`+ Pingou is the owner of the target project (test).`
		`+ Maja has a fork of the test project and creates`
		`+ a PR against Pingou parent project.`
		`+ She should be able to update her own PR.`
		`+ """`
		`+ super(PagureFlaskApiForkUpdateNoCommitterTests, self).setUp()`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.add_content_git_repo(`
		`+ os.path.join(self.path, "repos", "test.git")`
		`+ )`
		`+`
		`+ # Fork`
		`+ tests.create_user(self.session, "maja", "Maja M.", ["mm@f.com"])`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ task = pagure.lib.query.fork_project(`
		`+ session=self.session, user="maja", repo=project`
		`+ )`
		`+ self.session.commit()`
		`+ self.assertEqual(`
		`+ task.get(),`
		`+ {`
		`+ "endpoint": "ui_ns.view_repo",`
		`+ "repo": "test",`
		`+ "namespace": None,`
		`+ "username": "maja",`
		`+ },`
		`+ )`
		`+`
		`+ tests.add_readme_git_repo(`
		`+ os.path.join(self.path, "repos", "forks", "maja", "test.git")`
		`+ )`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ fork = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test", user="maja"`
		`+ )`
		`+`
		`+ tests.create_tokens(self.session)`
		`+ tests.create_tokens_acl(self.session)`
		`+`
		`+ req = pagure.lib.query.new_pull_request(`
		`+ session=self.session,`
		`+ repo_from=fork,`
		`+ branch_from="master",`
		`+ repo_to=project,`
		`+ branch_to="master",`
		`+ title="test pull-request",`
		`+ user="maja",`
		`+ )`
		`+ self.session.commit()`
		`+ self.assertEqual(req.id, 1)`
		`+ self.assertEqual(req.title, "test pull-request")`
		`+`
		`+ # Assert the PR is open`
		`+ self.session = pagure.lib.query.create_session(self.dbpath)`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+ self.assertEqual(project.requests[0].status, "Open")`
		`+ # Check how the PR renders in the API and the UI`
		`+ output = self.app.get("/api/0/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ def test_api_pull_request_updated_by_owner(self):`
		`+ """Owners of PRs can update their own PRs."""`
		`+`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+`
		`+ data = {`
		`+ "title": "edited test PR",`
		`+ "initial_comment": "Edited initial comment",`
		`+ }`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = "maja"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.post(`
		`+ "/api/0/test/pull-request/1", data=data, headers=headers`
		`+ )`
		`+`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ def test_api_pull_request_not_updated_by_other_user(self):`
		`+ """No repo committers or PR owners are allowed to update PR."""`
		`+`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+`
		`+ data = {`
		`+ "title": "edited test PR",`
		`+ "initial_comment": "Edited initial comment",`
		`+ }`
		`+`
		`+ tests.create_user(self.session, "other", "Another User", ["au@rh.com"])`
		`+ user = tests.FakeUser()`
		`+ user.username = "other"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.post(`
		`+ "/api/0/test/pull-request/1", data=data, headers=headers`
		`+ )`
		`+`
		`+ self.assertEqual(output.status_code, 403)`
		`+`
		`+`
		`+ class PagureFlaskApiForkUpdateNoCommitterTests(tests.SimplePagureTest):`
		`+ """Tests for the flask API of pagure for updating a PR`
		`+ when the PR owner is not a committer in the target project`
		`+ but he is the owner of the PR.`
		`+ """`
		`+`
		`+ maxDiff = None`
		`+`
		`+ @patch("pagure.lib.git.update_git", MagicMock(return_value=True))`
		`+ @patch("pagure.lib.notify.send_email", MagicMock(return_value=True))`
		`+ def setUp(self):`
		`+ """Set up the environnment, ran before every tests.`
		`+`
		`+ Pingou is the owner of the target project (test).`
		`+ Maja has a fork of the test project and creates`
		`+ a PR against Pingou parent project.`
		`+ She should be able to update her own PR.`
		`+ """`
		`+ super(PagureFlaskApiForkUpdateNoCommitterTests, self).setUp()`
		`+`
		`+ tests.create_projects(self.session)`
		`+ tests.add_content_git_repo(`
		`+ os.path.join(self.path, "repos", "test.git")`
		`+ )`
		`+`
		`+ # Fork`
		`+ tests.create_user(self.session, "maja", "Maja M.", ["mm@f.com"])`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ task = pagure.lib.query.fork_project(`
		`+ session=self.session, user="maja", repo=project`
		`+ )`
		`+ self.session.commit()`
		`+ self.assertEqual(`
		`+ task.get(),`
		`+ {`
		`+ "endpoint": "ui_ns.view_repo",`
		`+ "repo": "test",`
		`+ "namespace": None,`
		`+ "username": "maja",`
		`+ },`
		`+ )`
		`+`
		`+ tests.add_readme_git_repo(`
		`+ os.path.join(self.path, "repos", "forks", "maja", "test.git")`
		`+ )`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ fork = pagure.lib.query.get_authorized_project(`
		`+ self.session, "test", user="maja"`
		`+ )`
		`+`
		`+ tests.create_tokens(self.session)`
		`+ tests.create_tokens_acl(self.session)`
		`+`
		`+ req = pagure.lib.query.new_pull_request(`
		`+ session=self.session,`
		`+ repo_from=fork,`
		`+ branch_from="master",`
		`+ repo_to=project,`
		`+ branch_to="master",`
		`+ title="test pull-request",`
		`+ user="maja",`
		`+ )`
		`+ self.session.commit()`
		`+ self.assertEqual(req.id, 1)`
		`+ self.assertEqual(req.title, "test pull-request")`
		`+`
		`+ # Assert the PR is open`
		`+ self.session = pagure.lib.query.create_session(self.dbpath)`
		`+ project = pagure.lib.query.get_authorized_project(self.session, "test")`
		`+ self.assertEqual(len(project.requests), 1)`
		`+ self.assertEqual(project.requests[0].status, "Open")`
		`+ # Check how the PR renders in the API and the UI`
		`+ output = self.app.get("/api/0/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output = self.app.get("/test/pull-request/1")`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ def test_api_pull_request_updated_by_owner(self):`
		`+ """Owners of PRs can update their own PRs."""`
		`+`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+`
		`+ data = {`
		`+ "title": "edited test PR",`
		`+ "initial_comment": "Edited initial comment",`
		`+ }`
		`+`
		`+ user = tests.FakeUser()`
		`+ user.username = "maja"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.post(`
		`+ "/api/0/test/pull-request/1", data=data, headers=headers`
		`+ )`
		`+`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ def test_api_pull_request_not_updated_by_other_user(self):`
		`+ """No repo committers or PR owners are allowed to update PR."""`
		`+`
		`+ headers = {"Authorization": "token aaabbbcccddd"}`
		`+`
		`+ data = {`
		`+ "title": "edited test PR",`
		`+ "initial_comment": "Edited initial comment",`
		`+ }`
		`+`
		`+ tests.create_user(self.session, "other", "Another User", ["au@rh.com"])`
		`+ user = tests.FakeUser()`
		`+ user.username = "other"`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.post(`
		`+ "/api/0/test/pull-request/1", data=data, headers=headers`
		`+ )`
		`+`
		`+ self.assertEqual(output.status_code, 403)`
		`+`
		`+`
		`if __name__ == "__main__":`
		`unittest.main(verbosity=2)`

mmassari commented 2 years ago

me again :)

I changed default ACLs for a user token.
I don't get the EINVALIDTOK anymore but now I get EPRNOTALLOWED.
This PR should allow Packit to finally edit its own PRs.

Packit needs this change to be able to update PRs
it has already created.
We are implementing this feature:
https://github.com/packit/packit/issues/2182

Related with commit 133da07

mmassari commented 2 years ago

We realized that this change also affects api_pull_request_assign.
The PR owner will be now able to change the assignee.

Is it ok, or should I refactor it in a way that the two api calls api_pull_request_assign and api_pull_request_update use two different checks?

rebased onto 38b0100a4894de51963d57b132451b83596dc27e

2 years ago

wombelix commented 2 years ago

We realized that this change also affects api_pull_request_assign.
The PR owner will be now able to change the assignee.

Is it ok, or should I refactor it in a way that the two api calls api_pull_request_assign and api_pull_request_update use two different checks?

@mmassari good catch, I see what you mean. I think the PR shouldn't change the current behavior. So I suggest to refactor so that api_pull_request_assign isn't affected.