From a3cd8f60bc7f7c8f29f1ce0f0737c2778bb839d2 Mon Sep 17 00:00:00 2001 From: Maja Massarini Date: Apr 19 2024 13:30:50 +0000 Subject: [PATCH 1/2] Owner of a PR can update is own PR 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 133da07764314a73452487603f6f509b05f96204 --- diff --git a/pagure/api/utils.py b/pagure/api/utils.py index 2df505a..c2a9e33 100644 --- a/pagure/api/utils.py +++ b/pagure/api/utils.py @@ -203,7 +203,9 @@ def _check_pull_request_access(request, assignee=False): error = False # Public tickets require ticket access - error = not is_repo_user(request.project) + error = not is_repo_user(request.project) and not ( + request.user.user == flask.g.fas_user.username + ) if assignee: if ( diff --git a/tests/test_pagure_flask_api_fork_update.py b/tests/test_pagure_flask_api_fork_update.py index 4ffbbc6..77c57f4 100644 --- a/tests/test_pagure_flask_api_fork_update.py +++ b/tests/test_pagure_flask_api_fork_update.py @@ -764,5 +764,239 @@ class PagureFlaskApiForkUpdatetests(tests.SimplePagureTest): 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) From c8ea20215862e8ea8611fec66e1a386aff91c65c Mon Sep 17 00:00:00 2001 From: Maja Massarini Date: Apr 19 2024 13:30:50 +0000 Subject: [PATCH 2/2] Allow author to update PR but not to change assignee --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index cfffecc..f8bd223 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -502,7 +502,7 @@ def api_pull_request_update(repo, requestid, username=None, namespace=None): _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(): diff --git a/pagure/api/utils.py b/pagure/api/utils.py index c2a9e33..924f03e 100644 --- a/pagure/api/utils.py +++ b/pagure/api/utils.py @@ -190,12 +190,14 @@ def _check_private_issue_access(issue): ) -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 @@ -204,7 +206,7 @@ def _check_pull_request_access(request, assignee=False): error = False # Public tickets require ticket access error = not is_repo_user(request.project) and not ( - request.user.user == flask.g.fas_user.username + allow_author and request.user.user == flask.g.fas_user.username ) if assignee: