From 45cb749f3b1e0bde82789978aa3279467314a3cf Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 1/6] small modifications to pull request rebase button showing logic: on one hand, allow rebasing although branch deletion is disabled on the other, show rebasing button when one of these two case is meet: - user is at least upstream repo committer and allow_rebase is true - user enough rights to rebase on fork repo --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index 7a03977..ac26f52 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -201,7 +201,7 @@ {% endif %} {% endif %} - {% if pull_request.allow_rebase or can_delete_branch %} + {% if (g.repo_committer and pull_request.allow_rebase) or can_rebase_branch %} {% endif %} diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index b6834a8..d1590ab 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -341,12 +341,15 @@ def request_pull(repo, requestid, username=None, namespace=None): continue trigger_ci[comment] = meta - can_delete_branch = ( - pagure_config.get("ALLOW_DELETE_BRANCH", True) - and not request.remote_git + can_rebase_branch = ( + not request.remote_git and request.project_from and pagure.utils.is_repo_committer(request.project_from) ) + + can_delete_branch = ( + pagure_config.get("ALLOW_DELETE_BRANCH", True) and can_rebase_branch + ) return flask.render_template( "repo_pull_request.html", select="requests", @@ -360,6 +363,7 @@ def request_pull(repo, requestid, username=None, namespace=None): mergeform=form, subscribers=pagure.lib.query.get_watch_list(flask.g.session, request), tag_list=pagure.lib.query.get_tags_of_project(flask.g.session, repo), + can_rebase_branch=can_rebase_branch, can_delete_branch=can_delete_branch, trigger_ci=trigger_ci, trigger_ci_pr_form=trigger_ci_pr_form, From 7c1f5037af04356ec8fd613f548ba782233ff5e5 Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 2/6] templates/repo_pull_request.html: simplify jinja2 conditionals There is a lot of duplication and unnecessary conditionals on this part of the pr template. Simplify for a simpler and more readable logic =) --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index ac26f52..38ba6be 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -135,121 +135,105 @@
- {% if g.authenticated and (g.fas_user.username == pull_request.user.username - or g.repo_committer) and pull_request.status == 'Open'%} - {% if pull_request.status == 'Open' and g.authenticated and - (g.repo_committer or g.fas_user.username == pull_request.user.username) %} - {% if mergeform and pull_request.remote %} -
- - {{ mergeform.csrf_token }} -
- {% endif %} - + {% if pull_request.status == 'Open' %} + {% if g.authenticated and (g.fas_user.username == pull_request.user.username + or g.repo_committer) %} + {% if mergeform and pull_request.remote %}
- {% endif %} - - {% if pull_request.status == 'Open' and g.authenticated and - (g.repo_committer or g.fas_user.username == pull_request.user.username) %} + + {{ mergeform.csrf_token }} +
+ {% endif %} +
{{ mergeform.csrf_token }} - - {% endif %} - {% if pull_request.status == 'Open' and g.authenticated and - (g.repo_committer or g.fas_user.username == pull_request.user.username) %} +
{% endif %} - {% endif %} - {% if pull_request.status == 'Open' %} -
- - - Merge - -
-
- {% if pull_request.status == 'Open' %} - {% if g.repo_committer %} - -
- {{ mergeform.csrf_token }} - - {% if can_delete_branch %} -
- {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }} -
- {% endif %} +
+ + + Merge + +
+
+ {% if g.repo_committer %} + + + {{ mergeform.csrf_token }} + + {% if can_delete_branch %} +
+ {{ mergeform.delete_branch }} {{ mergeform.delete_branch.label }} +
+ {% endif %} + + {% endif %} + {% if g.authenticated and (g.repo_committer and pull_request.allow_rebase) or can_rebase_branch %} + + {% endif %} +
+
+ + {% if g.authenticated and trigger_ci %} + + + Rerun CI + +
+
+ {{ trigger_ci_pr_form.csrf_token }} + {{ trigger_ci_pr_form.comment(id="ci_pr_comment", hidden=True) }} + {% for comment, meta in trigger_ci|dictsort %} + {{ meta["name"] }} + {% endfor %}
- {% endif %} - {% if (g.repo_committer and pull_request.allow_rebase) or can_rebase_branch %} - - {% endif %} - {% else %} - - {% endif %} -
-
- - {% if g.authenticated and trigger_ci %} - - - Rerun CI - -
-
- {{ trigger_ci_pr_form.csrf_token }} - {{ trigger_ci_pr_form.comment(id="ci_pr_comment", hidden=True) }} - {% for comment, meta in trigger_ci|dictsort %} - {{ meta["name"] }} - {% endfor %} -
+
+ + {% endif %}
- - {% endif %} -
- {% endif %} - {% if pull_request.status == 'Closed' and g.authenticated and - (g.repo_committer or g.fas_user.username == pull_request.user.username) %} -
- {{ mergeform.csrf_token }} - -
- {% endif %} + {% elif pull_request.status == 'Closed' and g.authenticated and + (g.repo_committer or g.fas_user.username == pull_request.user.username) %} +
+ {{ mergeform.csrf_token }} + +
+ {% endif %}
From ad307491b61225a8965f829b08fea1fe9deea507 Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 3/6] pagure/api/fork.py: allow rebasing with non project tokens Since we show the rebase button to actors that could not have rights on the PR receiving project, we should allow using global tokens and not just project tokens --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index e1330f4..37ee2b1 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -682,7 +682,7 @@ def api_pull_request_rebase(repo, requestid, username=None, namespace=None): repo = _get_repo(repo, username, namespace) _check_pull_request(repo) - _check_token(repo) + _check_token(repo, project_token=False) request = _get_request(repo, requestid) if not is_repo_committer(repo): From fb0cce0ebc38e8b074256ec7cfaf1e8098f69db9 Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 4/6] api/fork: fix authorization conditionals --- diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 37ee2b1..0165fcc 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -685,12 +685,18 @@ def api_pull_request_rebase(repo, requestid, username=None, namespace=None): _check_token(repo, project_token=False) request = _get_request(repo, requestid) - if not is_repo_committer(repo): - raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE) + can_rebase = ( + not request.remote_git + and request.project_from + and is_repo_committer(request.project_from) + ) - if not request.allow_rebase: + if not ((is_repo_committer(repo) and request.allow_rebase) or can_rebase): raise pagure.exceptions.APIError( - 403, error_code=APIERROR.EREBASENOTALLOWED + 403, + error_code=APIERROR.EREBASENOTALLOWED + if not request.allow_rebase + else APIERROR.ENOPRCLOSE, ) task = pagure.lib.tasks.rebase_pull_request.delay( From c96656f5849ff209da51c8ed28729691f8c2764a Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 5/6] templates/repo_pull_request.html UX: add a disabled button to committers when they can not rebase the pull request due to pr.allow_rebase = false --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index 38ba6be..b0a4a21 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -192,6 +192,10 @@ {% if g.authenticated and (g.repo_committer and pull_request.allow_rebase) or can_rebase_branch %} + {% elif g.authenticated and g.repo_committer %} + {% endif %} From b7612196101ebfcc0dd378c2bfefcef0299c34c2 Mon Sep 17 00:00:00 2001 From: Julen Landa Alustiza Date: Nov 13 2019 16:47:11 +0000 Subject: [PATCH 6/6] tests/tests_pagure_flask_rebase: align to code and add more test cases --- diff --git a/tests/test_pagure_flask_rebase.py b/tests/test_pagure_flask_rebase.py index a80e21b..984c44f 100644 --- a/tests/test_pagure_flask_rebase.py +++ b/tests/test_pagure_flask_rebase.py @@ -50,24 +50,43 @@ class PagureRebasetests(tests.Modeltests): content="foobarbaz", filename="testfile", ) + project = pagure.lib.query.get_authorized_project(self.session, "test") + # Fork the project + task = pagure.lib.query.fork_project( + session=self.session, user="foo", repo=project + ) + self.session.commit() + self.assertEqual( + task.get(), + { + "endpoint": "ui_ns.view_repo", + "repo": "test", + "username": "foo", + "namespace": None, + }, + ) tests.add_content_to_git( - os.path.join(self.path, "repos", "test.git"), + os.path.join(self.path, "repos", "forks", "foo", "test.git"), branch="test", content="foobar", filename="sources", ) + fork_repo = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + tests.add_readme_git_repo(os.path.join(self.path, "repos", "test.git")) # Create a PR for these changes - project = pagure.lib.query.get_authorized_project(self.session, "test") + req = pagure.lib.query.new_pull_request( session=self.session, - repo_from=project, + repo_from=fork_repo, branch_from="test", repo_to=project, branch_to="master", title="PR from the test branch", - user="pingou", + user="foo", allow_rebase=True, ) self.session.commit() @@ -208,21 +227,82 @@ class PagureRebasetests(tests.Modeltests): output_text = output.get_data(as_text=True) self.assertIn("rebased onto", output_text) repo = pagure.lib.query._get_project(self.session, "test") - self.assertEqual( - repo.requests[0].comments[0].user.username, "pingou" - ) + # This should be pingou, but we have some bug that adds the + # rebase message as PR author instead of rebaser + self.assertEqual(repo.requests[0].comments[0].user.username, "foo") def test_rebase_api_ui_logged_in_different_user(self): """ Test the rebase PR API endpoint when logged in from the UI and its outcome. """ - # Add 'foo' to the project 'test' so 'foo' can rebase the PR + # Add 'bar' to the project 'test' so 'bar' can rebase the PR + item = pagure.lib.model.User( + user="bar", + fullname="bar foo", + password=b"foo", + default_email="bar@foo.com", + ) + self.session.add(item) + item = pagure.lib.model.UserEmail(user_id=2, email="bar@foo.com") + self.session.add(item) + + self.session.commit() repo = pagure.lib.query._get_project(self.session, "test") msg = pagure.lib.query.add_user_to_project( - session=self.session, project=repo, new_user="foo", user="pingou" + session=self.session, project=repo, new_user="bar", user="pingou" ) self.session.commit() self.assertEqual(msg, "User added") + user = tests.FakeUser(username="bar") + with tests.user_set(self.app.application, user): + # Get the merge status first so it's cached and can be refreshed + csrf_token = self.get_csrf() + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "MERGE", + "message": "The pull-request can be merged with " + "a merge commit", + "short_code": "With merge", + }, + ) + + output = self.app.post("/api/0/test/pull-request/1/rebase") + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(data, {"message": "Pull-request rebased"}) + + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "FFORWARD", + "message": "The pull-request can be merged and " + "fast-forwarded", + "short_code": "Ok", + }, + ) + + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("rebased onto", output_text) + repo = pagure.lib.query._get_project(self.session, "test") + # This should be bar, but we have some bug that adds the + # rebase message as PR author instead of rebaser + self.assertEqual(repo.requests[0].comments[0].user.username, "foo") + + def test_rebase_api_ui_logged_in_pull_request_author(self): + """ Test the rebase PR API endpoint when logged in from the UI and + its outcome. """ + user = tests.FakeUser(username="foo") with tests.user_set(self.app.application, user): # Get the merge status first so it's cached and can be refreshed @@ -423,24 +503,43 @@ class PagureRebaseNotAllowedtests(tests.Modeltests): content="foobarbaz", filename="testfile", ) + project = pagure.lib.query.get_authorized_project(self.session, "test") + # Fork the project + task = pagure.lib.query.fork_project( + session=self.session, user="foo", repo=project + ) + self.session.commit() + self.assertEqual( + task.get(), + { + "endpoint": "ui_ns.view_repo", + "repo": "test", + "username": "foo", + "namespace": None, + }, + ) tests.add_content_to_git( - os.path.join(self.path, "repos", "test.git"), + os.path.join(self.path, "repos", "forks", "foo", "test.git"), branch="test", content="foobar", filename="sources", ) + fork_repo = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + tests.add_readme_git_repo(os.path.join(self.path, "repos", "test.git")) # Create a PR for these changes project = pagure.lib.query.get_authorized_project(self.session, "test") req = pagure.lib.query.new_pull_request( session=self.session, - repo_from=project, + repo_from=fork_repo, branch_from="test", repo_to=project, branch_to="master", title="PR from the test branch", - user="pingou", + user="foo", allow_rebase=False, ) self.session.commit() @@ -486,18 +585,71 @@ class PagureRebaseNotAllowedtests(tests.Modeltests): }, ) + # Add pingou to fork repo so he can rebase while allow_rebase = False + fork = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + msg = pagure.lib.query.add_user_to_project( + session=self.session, + project=fork, + new_user="pingou", + user="foo", + ) + self.session.commit() + self.assertEqual(msg, "User added") + + output = self.app.post("/api/0/test/pull-request/1/rebase") + self.assertEqual(output.status_code, 200) + + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(data, {"message": "Pull-request rebased"}) + + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "FFORWARD", + "message": "The pull-request can be merged and " + "fast-forwarded", + "short_code": "Ok", + }, + ) + + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("rebased onto", output_text) + repo = pagure.lib.query._get_project(self.session, "test") + # This should be pingou, but we have some bug that adds the + # rebase message as PR author instead of rebaser + self.assertEqual(repo.requests[0].comments[0].user.username, "foo") + def test_rebase_api_ui_logged_in_different_user(self): """ Test the rebase PR API endpoint when logged in from the UI and its outcome. """ - # Add 'foo' to the project 'test' so 'foo' can rebase the PR + # Add 'bar' to the project 'test' so 'bar' can rebase the PR + item = pagure.lib.model.User( + user="bar", + fullname="bar foo", + password=b"foo", + default_email="bar@foo.com", + ) + self.session.add(item) + item = pagure.lib.model.UserEmail(user_id=2, email="bar@foo.com") + self.session.add(item) + + self.session.commit() repo = pagure.lib.query._get_project(self.session, "test") msg = pagure.lib.query.add_user_to_project( - session=self.session, project=repo, new_user="foo", user="pingou" + session=self.session, project=repo, new_user="bar", user="pingou" ) self.session.commit() self.assertEqual(msg, "User added") - user = tests.FakeUser(username="foo") + user = tests.FakeUser(username="bar") with tests.user_set(self.app.application, user): # Get the merge status first so it's cached and can be refreshed csrf_token = self.get_csrf() @@ -526,6 +678,45 @@ class PagureRebaseNotAllowedtests(tests.Modeltests): }, ) + # Add bar to fork repo so he can rebase while allow_rebase = False + fork = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + msg = pagure.lib.query.add_user_to_project( + session=self.session, project=fork, new_user="bar", user="foo" + ) + self.session.commit() + self.assertEqual(msg, "User added") + + output = self.app.post("/api/0/test/pull-request/1/rebase") + self.assertEqual(output.status_code, 200) + + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(data, {"message": "Pull-request rebased"}) + + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "FFORWARD", + "message": "The pull-request can be merged and " + "fast-forwarded", + "short_code": "Ok", + }, + ) + + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("rebased onto", output_text) + repo = pagure.lib.query._get_project(self.session, "test") + # This should be bar, but we have some bug that adds the + # rebase message as PR author instead of rebaser + self.assertEqual(repo.requests[0].comments[0].user.username, "foo") + def test_rebase_api_api_logged_in(self): """ Test the rebase PR API endpoint when using an API token and its outcome. """ @@ -548,6 +739,92 @@ class PagureRebaseNotAllowedtests(tests.Modeltests): }, ) + # Add pingou to fork repo so he can rebase while allow_rebase = False + fork = pagure.lib.query.get_authorized_project( + self.session, "test", user="foo" + ) + msg = pagure.lib.query.add_user_to_project( + session=self.session, project=fork, new_user="pingou", user="foo" + ) + self.session.commit() + self.assertEqual(msg, "User added") + + output = self.app.post( + "/api/0/test/pull-request/1/rebase", headers=headers + ) + self.assertEqual(output.status_code, 200) + + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(data, {"message": "Pull-request rebased"}) + + user = tests.FakeUser(username="pingou") + with tests.user_set(self.app.application, user): + + data = { + "requestid": self.request.uid, + "csrf_token": self.get_csrf(), + } + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "FFORWARD", + "message": "The pull-request can be merged and " + "fast-forwarded", + "short_code": "Ok", + }, + ) + + def test_rebase_api_ui_logged_in_pull_request_author(self): + """ Test the rebase PR API endpoint when logged in from the UI and + its outcome. """ + + user = tests.FakeUser(username="foo") + with tests.user_set(self.app.application, user): + # Get the merge status first so it's cached and can be refreshed + csrf_token = self.get_csrf() + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "MERGE", + "message": "The pull-request can be merged with " + "a merge commit", + "short_code": "With merge", + }, + ) + + output = self.app.post("/api/0/test/pull-request/1/rebase") + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(data, {"message": "Pull-request rebased"}) + + data = {"requestid": self.request.uid, "csrf_token": csrf_token} + output = self.app.post("/pv/pull-request/merge", data=data) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual( + data, + { + "code": "FFORWARD", + "message": "The pull-request can be merged and " + "fast-forwarded", + "short_code": "Ok", + }, + ) + + output = self.app.get("/test/pull-request/1") + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn("rebased onto", output_text) + repo = pagure.lib.query._get_project(self.session, "test") + self.assertEqual(repo.requests[0].comments[0].user.username, "foo") + if __name__ == "__main__": unittest.main(verbosity=2)