From 6fdb42b924c402b94c783afb960de7016a277fc1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 27 2019 12:24:12 +0000 Subject: [PATCH 1/4] Move utility methods to used in different places of the API to a module These methods are used by both the api.issue and api.fork modules and are close enough that we want to keep them in a near proximity. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index 8a0eccd..e3a94b9 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -79,6 +79,7 @@ class APIERROR(enum.Enum): ) ENOISSUE = "Issue not found" EISSUENOTALLOWED = "You are not allowed to view this issue" + EPRNOTALLOWED = "You are not allowed to view this pull-request" EPULLREQUESTSDISABLED = ( "Pull-Request have been deactivated for this " "project" ) diff --git a/pagure/api/fork.py b/pagure/api/fork.py index e0b09c3..66da001 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -31,11 +31,13 @@ from pagure.api import ( get_per_page, ) from pagure.config import config as pagure_config -from pagure.utils import ( - authenticated, - is_repo_committer, - is_true, - api_authenticated, +from pagure.utils import is_repo_committer, is_true +from pagure.api.utils import ( + _get_repo, + _check_token, + _get_request, + _check_pull_request, + _check_pull_request_access, ) @@ -146,17 +148,8 @@ def api_pull_request_views(repo, username=None, namespace=None): """ - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) status = flask.request.args.get("status", True) assignee = flask.request.args.get("assignee", None) @@ -280,9 +273,8 @@ def api_pull_request_by_uid_view(uid): } """ - request = pagure.lib.query.get_request_by_uid(flask.g.session, uid) - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + + request = _get_request(requestuid=uid) # we don't really need the repo, but we need to make sure # that we're allowed to access it @@ -378,24 +370,9 @@ def api_pull_request_view(repo, requestid, username=None, namespace=None): """ - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + request = _get_request(repo, requestid) jsonout = flask.jsonify(request.to_json(public=True, api=True)) return jsonout @@ -452,27 +429,10 @@ def api_pull_request_merge(repo, requestid, username=None, namespace=None): """ # noqa output = {} - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if flask.g.token.project and repo != flask.g.token.project: - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _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) @@ -561,29 +521,10 @@ def api_pull_request_rebase(repo, requestid, username=None, namespace=None): """ # noqa output = {} - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if ( - api_authenticated() and flask.g.token and repo != flask.g.token.project - ) or not authenticated(): - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo) + _get_request(repo, requestid) if not is_repo_committer(repo): raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE) @@ -648,27 +589,10 @@ def api_pull_request_close(repo, requestid, username=None, namespace=None): """ # noqa output = {} - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if repo != flask.g.token.project: - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo) + request = _get_request(repo, requestid) if not is_repo_committer(repo): raise pagure.exceptions.APIError(403, error_code=APIERROR.ENOPRCLOSE) @@ -758,29 +682,13 @@ def api_pull_request_add_comment( } """ # noqa - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) output = {} - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if flask.g.token.project and repo != flask.g.token.project: - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo, project_token=False) + request = _get_request(repo, requestid) form = pagure.forms.AddPullRequestCommentForm(csrf_enabled=False) if form.validate_on_submit(): @@ -947,29 +855,13 @@ def api_pull_request_add_flag(repo, requestid, username=None, namespace=None): } """ # noqa - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) output = {} - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if flask.g.token.project and repo != flask.g.token.project: - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo, project_token=False) + request = _get_request(repo, requestid) if "status" in get_request_data(): form = pagure.forms.AddPullRequestFlagForm(csrf_enabled=False) @@ -1104,26 +996,12 @@ def api_pull_request_get_flag(repo, requestid, username=None, namespace=None): } """ # noqa - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) output = {} - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + request = _get_request(repo, requestid) output = {"flags": []} @@ -1192,34 +1070,12 @@ def api_subscribe_pull_request(repo, requestid, username=None, namespace=None): """ # noqa - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - output = {} - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - if ( - api_authenticated() - and flask.g.token - and flask.g.token.project - and repo != flask.g.token.project - ) or not authenticated(): - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo) + request = _get_request(repo, requestid) form = pagure.forms.SubscribtionForm(csrf_enabled=False) if form.validate_on_submit(): @@ -1350,15 +1206,9 @@ def api_pull_request_create(repo, username=None, namespace=None): """ - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if flask.g.token.project and repo != flask.g.token.project: - raise pagure.exceptions.APIError(401, error_code=APIERROR.EINVALIDTOK) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + _check_token(repo) form = pagure.forms.RequestPullForm(csrf_enabled=False) if not form.validate_on_submit(): @@ -1511,24 +1361,9 @@ def api_pull_request_diffstats(repo, requestid, username=None, namespace=None): """ # noqa - repo = get_authorized_api_project( - flask.g.session, repo, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get("pull_requests", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.EPULLREQUESTSDISABLED - ) - - request = pagure.lib.query.search_pull_requests( - flask.g.session, project_id=repo.id, requestid=requestid - ) - - if not request: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + repo = _get_repo(repo, username, namespace) + _check_pull_request(repo) + request = _get_request(repo, requestid) repopath = None parentpath = pagure.utils.get_repo_path(request.project) diff --git a/pagure/api/issue.py b/pagure/api/issue.py index c0707e4..d94456e 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -25,7 +25,6 @@ from pagure.api import ( api_login_required, api_login_optional, APIERROR, - get_authorized_api_project, get_request_data, get_page, get_per_page, @@ -34,142 +33,21 @@ from pagure.config import config as pagure_config from pagure.utils import ( api_authenticated, is_repo_committer, - is_repo_user, urlpattern, is_true, ) - +from pagure.api.utils import ( + _get_repo, + _check_token, + _get_issue, + _check_issue_tracker, + _check_ticket_access, + _check_private_issue_access, +) _log = logging.getLogger(__name__) -def _get_repo(repo_name, username=None, namespace=None): - """Check if repository exists and get repository name - :param repo_name: name of repository - :param username: - :param namespace: - :raises pagure.exceptions.APIError: when repository doesn't exist or - is disabled - :return: repository name - """ - repo = get_authorized_api_project( - flask.g.session, repo_name, user=username, namespace=namespace - ) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - return repo - - -def _check_issue_tracker(repo): - """Check if issue tracker is enabled for repository - :param repo: repository - :raises pagure.exceptions.APIError: when issue tracker is disabled - """ - ticket_namespaces = pagure_config.get("ENABLE_TICKETS_NAMESPACE") - if ( - ticket_namespaces - and repo.namespace - and repo.namespace not in ticket_namespaces - ) or not repo.settings.get("issue_tracker", True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED - ) - - # forbid all POST requests if the issue tracker is made read-only - if flask.request.method == "POST" and repo.settings.get( - "issue_tracker_read_only", False - ): - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.ETRACKERREADONLY - ) - - -def _check_token(repo, project_token=True): - """Check if token is valid for the repo - :param repo: repository name - :param project_token: set True when project token is required, - otherwise any token can be used - :raises pagure.exceptions.APIError: when token is not valid for repo - """ - if api_authenticated(): - # if there is a project associated with the token, check it - # if there is no project associated, check if it is required - if ( - flask.g.token.project is not None and repo != flask.g.token.project - ) or (flask.g.token.project is None and project_token): - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK - ) - - -def _get_issue(repo, issueid, issueuid=None): - """Get issue and check permissions - :param repo: repository name - :param issueid: issue ID - :param issueuid: issue Unique ID - :raises pagure.exceptions.APIError: when issues doesn't exists - :return: issue - """ - issue = pagure.lib.query.search_issues( - flask.g.session, repo, issueid=issueid, issueuid=issueuid - ) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) - - return issue - - -def _check_private_issue_access(issue): - """Check if user can access issue. Must be repo committer - or author to see private issues. - :param issue: issue object - :raises pagure.exceptions.APIError: when access denied - """ - if ( - issue.private - and not is_repo_committer(issue.project) - and ( - not api_authenticated() - or not issue.user.user == flask.g.fas_user.username - ) - ): - raise pagure.exceptions.APIError( - 403, error_code=APIERROR.EISSUENOTALLOWED - ) - - -def _check_ticket_access(issue, assignee=False, open_access=False): - """Check if user can access issue. Must be repo committer - or author to see private issues. - :param issue: issue object - :param assignee: a boolean specifying whether to allow the assignee or not - defaults to False - :raises pagure.exceptions.APIError: when access denied - """ - # Private tickets require commit access - _check_private_issue_access(issue) - - error = False - if not open_access: - # Public tickets require ticket access - error = not is_repo_user(issue.project) - - if assignee: - if ( - issue.assignee is not None - and issue.assignee.user == flask.g.fas_user.username - ): - error = False - - if error: - raise pagure.exceptions.APIError( - 403, error_code=APIERROR.EISSUENOTALLOWED - ) - - def _check_link_custom_field(field, links): """Check if the value provided in the link custom field is a link. diff --git a/pagure/api/utils.py b/pagure/api/utils.py new file mode 100644 index 0000000..8b87edd --- /dev/null +++ b/pagure/api/utils.py @@ -0,0 +1,235 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2015-2019 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import print_function, unicode_literals, absolute_import + +import flask +import logging + + +import pagure.exceptions +import pagure.lib.query + +from pagure.config import config as pagure_config +from pagure.api import APIERROR, get_authorized_api_project + +from pagure.utils import api_authenticated, is_repo_committer, is_repo_user + + +_log = logging.getLogger(__name__) + + +def _get_repo(repo_name, username=None, namespace=None): + """Check if repository exists and get repository name + :param repo_name: name of repository + :param username: + :param namespace: + :raises pagure.exceptions.APIError: when repository doesn't exist or + is disabled + :return: repository name + """ + repo = get_authorized_api_project( + flask.g.session, repo_name, user=username, namespace=namespace + ) + + if repo is None: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) + + return repo + + +def _check_token(repo, project_token=True): + """Check if token is valid for the repo + :param repo: repository name + :param project_token: set True when project token is required, + otherwise any token can be used + :raises pagure.exceptions.APIError: when token is not valid for repo + """ + if api_authenticated(): + # if there is a project associated with the token, check it + # if there is no project associated, check if it is required + if ( + flask.g.token.project is not None and repo != flask.g.token.project + ) or (flask.g.token.project is None and project_token): + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.EINVALIDTOK + ) + + +def _get_issue(repo, issueid, issueuid=None): + """Get issue and check permissions + :param repo: repository name + :param issueid: issue ID + :param issueuid: issue Unique ID + :raises pagure.exceptions.APIError: when issues doesn't exists + :return: issue + """ + issue = pagure.lib.query.search_issues( + flask.g.session, repo, issueid=issueid, issueuid=issueuid + ) + + if issue is None or issue.project != repo: + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + + return issue + + +def _get_request(repo=None, requestid=None, requestuid=None): + """Get pull-request if it exists + :param repo: repository name + :param requestid: pull-request ID + :param requestuid: pull-request Unique ID + :raises pagure.exceptions.APIError: when pull-request doesn't exists + :return: issue + """ + request = None + if repo and requestid: + request = pagure.lib.query.search_pull_requests( + flask.g.session, project_id=repo.id, requestid=requestid + ) + elif requestuid: + request = pagure.lib.query.get_request_by_uid( + flask.g.session, requestuid + ) + + if not request or (repo and request.project != repo): + raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOREQ) + + return request + + +def _check_issue_tracker(repo): + """Check if issue tracker is enabled for repository + :param repo: repository + :raises pagure.exceptions.APIError: when issue tracker is disabled + """ + ticket_namespaces = pagure_config.get("ENABLE_TICKETS_NAMESPACE") + if ( + ticket_namespaces + and repo.namespace + and repo.namespace not in ticket_namespaces + ) or not repo.settings.get("issue_tracker", True): + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ETRACKERDISABLED + ) + + # forbid all POST requests if the issue tracker is made read-only + if flask.request.method == "POST" and repo.settings.get( + "issue_tracker_read_only", False + ): + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.ETRACKERREADONLY + ) + + +def _check_pull_request(repo): + """Check if pull-requests are enabled for repository + :param repo: repository + :raises pagure.exceptions.APIError: when issue tracker is disabled + """ + if not repo.settings.get("pull_requests", True): + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.EPULLREQUESTSDISABLED + ) + + +def _check_ticket_access(issue, assignee=False, open_access=False): + """Check if user can access issue. Must be repo committer + or author to see private issues. + :param issue: issue object + :param assignee: a boolean specifying whether to allow the assignee or not + defaults to False + :raises pagure.exceptions.APIError: when access denied + """ + # Private tickets require commit access + _check_private_issue_access(issue) + + error = False + if not open_access: + # Public tickets require ticket access + error = not is_repo_user(issue.project) + + if assignee: + if ( + issue.assignee is not None + and issue.assignee.user == flask.g.fas_user.username + ): + error = False + + if error: + raise pagure.exceptions.APIError( + 403, error_code=APIERROR.EISSUENOTALLOWED + ) + + +def _check_private_issue_access(issue): + """Check if user can access issue. Must be repo committer + or author to see private issues. + :param issue: issue object + :raises pagure.exceptions.APIError: when access denied + """ + if ( + issue.private + and not is_repo_committer(issue.project) + and ( + not api_authenticated() + or not issue.user.user == flask.g.fas_user.username + ) + ): + raise pagure.exceptions.APIError( + 403, error_code=APIERROR.EISSUENOTALLOWED + ) + + +def _check_pull_request_access(request, assignee=False): + """Check if user can access Pull-Request. Must be repo committer + or author to see private pull-requests. + :param request: PullRequest object + :param assignee: a boolean specifying whether to allow the assignee or not + defaults to False + :raises pagure.exceptions.APIError: when access denied + """ + # Private PRs require commit access + _check_private_pull_request_access(request) + + error = False + # Public tickets require ticket access + error = not is_repo_user(request.project) + + if assignee: + if ( + request.assignee is not None + and request.assignee.user == flask.g.fas_user.username + ): + error = False + + if error: + raise pagure.exceptions.APIError( + 403, error_code=APIERROR.EPRNOTALLOWED + ) + + +def _check_private_pull_request_access(request): + """Check if user can access PR. Must be repo committer + or author to see private PR. + :param request: PullRequest object + :raises pagure.exceptions.APIError: when access denied + """ + if ( + request.private + and not is_repo_committer(request.project) + and ( + not api_authenticated() + or not request.user.user == flask.g.fas_user.username + ) + ): + raise pagure.exceptions.APIError( + 403, error_code=APIERROR.EPRNOTALLOWED + ) diff --git a/tests/test_pagure_flask_api.py b/tests/test_pagure_flask_api.py index 3732f12..301e3f0 100644 --- a/tests/test_pagure_flask_api.py +++ b/tests/test_pagure_flask_api.py @@ -236,22 +236,45 @@ class PagureFlaskApitests(tests.SimplePagureTest): output = self.app.get('/api/0/-/error_codes') self.assertEqual(output.status_code, 200) data = json.loads(output.get_data(as_text=True)) - self.assertEqual(len(data), 34) + self.assertEqual(len(data), 35) self.assertEqual( sorted(data.keys()), [ - u'EDATETIME', u'EDBERROR', u'EGITERROR', - u'EINVALIDISSUEFIELD', u'EINVALIDISSUEFIELD_LINK', - u'EINVALIDPERPAGEVALUE', u'EINVALIDPRIORITY', u'EINVALIDREQ', - u'EINVALIDTOK', u'EISSUENOTALLOWED', - u'EMODIFYPROJECTNOTALLOWED', u'ENEWPROJECTDISABLED', - u'ENOCODE', u'ENOCOMMENT', u'ENOCOMMIT', u'ENOGROUP', - u'ENOISSUE', u'ENOPRCLOSE', u'ENOPROJECT', u'ENOPROJECTS', - u'ENOPRSTATS', u'ENOREQ', u'ENOSIGNEDOFF', u'ENOTASSIGNED', - u'ENOTASSIGNEE', u'ENOTHIGHENOUGH', u'ENOTMAINADMIN', - u'ENOUSER', u'EPRCONFLICTS', u'EPRSCORE', - u'EPULLREQUESTSDISABLED', u'ETIMESTAMP', u'ETRACKERDISABLED', - u'ETRACKERREADONLY' + 'EDATETIME', + 'EDBERROR', + 'EGITERROR', + 'EINVALIDISSUEFIELD', + 'EINVALIDISSUEFIELD_LINK', + 'EINVALIDPERPAGEVALUE', + 'EINVALIDPRIORITY', + 'EINVALIDREQ', + 'EINVALIDTOK', + 'EISSUENOTALLOWED', + 'EMODIFYPROJECTNOTALLOWED', + 'ENEWPROJECTDISABLED', + 'ENOCODE', + 'ENOCOMMENT', + 'ENOCOMMIT', + 'ENOGROUP', + 'ENOISSUE', + 'ENOPRCLOSE', + 'ENOPROJECT', + 'ENOPROJECTS', + 'ENOPRSTATS', + 'ENOREQ', + 'ENOSIGNEDOFF', + 'ENOTASSIGNED', + 'ENOTASSIGNEE', + 'ENOTHIGHENOUGH', + 'ENOTMAINADMIN', + 'ENOUSER', + 'EPRCONFLICTS', + 'EPRNOTALLOWED', + 'EPRSCORE', + 'EPULLREQUESTSDISABLED', + 'ETIMESTAMP', + 'ETRACKERDISABLED', + 'ETRACKERREADONLY', ] ) From bc397c7846611e6323bb17d7ba9f016fc1cb1368 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 27 2019 14:54:32 +0000 Subject: [PATCH 2/4] Add a new API endpoint to assign pull-request to someone Adjust also the message returned when a PR assignee is reset. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/api/__init__.py b/pagure/api/__init__.py index e3a94b9..7630b9c 100644 --- a/pagure/api/__init__.py +++ b/pagure/api/__init__.py @@ -566,6 +566,7 @@ def api(): fork.api_pull_request_add_comment ) api_pull_request_add_flag_doc = load_doc(fork.api_pull_request_add_flag) + api_pull_request_assign_doc = load_doc(fork.api_pull_request_assign) api_version_doc = load_doc(api_version) api_whoami_doc = load_doc(api_whoami) @@ -631,6 +632,7 @@ def api(): api_pull_request_close_doc, api_pull_request_add_comment_doc, api_pull_request_add_flag_doc, + api_pull_request_assign_doc, ], users=[ api_users_doc, diff --git a/pagure/api/fork.py b/pagure/api/fork.py index 66da001..4dea521 100644 --- a/pagure/api/fork.py +++ b/pagure/api/fork.py @@ -1439,3 +1439,98 @@ def api_pull_request_diffstats(repo, requestid, username=None, namespace=None): jsonout = flask.jsonify(output) return jsonout + + +@API.route("//pull-request//assign", methods=["POST"]) +@API.route( + "///pull-request//assign", methods=["POST"] +) +@API.route( + "/fork///pull-request//assign", + methods=["POST"], +) +@API.route( + "/fork////pull-request//assign", + methods=["POST"], +) +@api_login_required(acls=["pull_request_assign", "pull_request_update"]) +@api_method +def api_pull_request_assign(repo, requestid, username=None, namespace=None): + """ + Assign a pull-request + --------------------- + Assign a pull-request to someone. + + :: + + POST /api/0//pull-request//assign + POST /api/0///pull-request//assign + + :: + + POST /api/0/fork///pull-request//assign + POST /api/0/fork////pull-request//assign + + Input + ^^^^^ + + +--------------+----------+---------------+---------------------------+ + | Key | Type | Optionality | Description | + +==============+==========+===============+===========================+ + | ``assignee`` | string | Mandatory | | The username of the user| + | | | | to assign the PR to. | + +--------------+----------+---------------+---------------------------+ + + Sample response + ^^^^^^^^^^^^^^^ + + :: + + { + "message": "pull-request assigned" + } + + """ # noqa + output = {} + repo = _get_repo(repo, username, namespace) + + _check_pull_request(repo) + _check_token(repo) + + request = _get_request(repo, requestid) + _check_pull_request_access(request, assignee=True) + + form = pagure.forms.AssignIssueForm(csrf_enabled=False) + if form.validate_on_submit(): + assignee = form.assignee.data or None + # Create our metadata comment object + try: + # New comment + message = pagure.lib.query.add_pull_request_assignee( + flask.g.session, + request=request, + assignee=assignee, + user=flask.g.fas_user.username, + ) + flask.g.session.commit() + if message: + pagure.lib.query.add_metadata_update_notif( + session=flask.g.session, + obj=request, + messages=message, + user=flask.g.fas_user.username, + ) + output["message"] = message + else: + output["message"] = "Nothing to change" + except pagure.exceptions.PagureException as err: # pragma: no cover + raise pagure.exceptions.APIError( + 400, error_code=APIERROR.ENOCODE, error=str(err) + ) + except SQLAlchemyError as err: # pragma: no cover + flask.g.session.rollback() + _log.exception(err) + raise pagure.exceptions.APIError(400, error_code=APIERROR.EDBERROR) + + jsonout = flask.jsonify(output) + return jsonout diff --git a/pagure/default_config.py b/pagure/default_config.py index 2c777cd..b43d5ce 100644 --- a/pagure/default_config.py +++ b/pagure/default_config.py @@ -345,6 +345,10 @@ ACLS = { "pull_request_subscribe": ( "Subscribe the user with this token to a pull-request" ), + "pull_request_assign": "Assign someone to a pull-request", + "pull_request_update": ( + "Update a pull-request (title, description, assignee...)" + ), "update_watch_status": "Update the watch status on a project", "pull_request_rebase": "Rebase a pull-request", } diff --git a/pagure/lib/query.py b/pagure/lib/query.py index 078aeac..1327637 100644 --- a/pagure/lib/query.py +++ b/pagure/lib/query.py @@ -646,7 +646,7 @@ def add_pull_request_assignee(session, request, assignee, user): ), ) - return "Request reset" + return "Request assignee reset" elif assignee is None and request.assignee is None: return diff --git a/tests/test_pagure_flask_api_fork_assign.py b/tests/test_pagure_flask_api_fork_assign.py new file mode 100644 index 0000000..eeb80c4 --- /dev/null +++ b/tests/test_pagure_flask_api_fork_assign.py @@ -0,0 +1,265 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2019 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import unicode_literals, absolute_import + +import arrow +import copy +import datetime +import unittest +import shutil +import sys +import time +import os + +import flask +import json +import munch +from mock import patch, MagicMock +from sqlalchemy.exc import SQLAlchemyError + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.lib.query +import tests + + +class PagureFlaskApiForkAssigntests(tests.SimplePagureTest): + """ Tests for the flask API of pagure for assigning a 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. """ + super(PagureFlaskApiForkAssigntests, self).setUp() + + tests.create_projects(self.session) + tests.add_content_git_repo( + os.path.join(self.path, "repos", "test.git")) + + # Fork + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + task = pagure.lib.query.fork_project( + session=self.session, + user='pingou', + repo=project, + ) + self.session.commit() + self.assertEqual( + task.get(), + {'endpoint': 'ui_ns.view_repo', + 'repo': 'test', + 'namespace': None, + 'username': 'pingou'}) + + tests.add_readme_git_repo( + os.path.join(self.path, "repos", "forks", "pingou", "test.git")) + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + fork = pagure.lib.query.get_authorized_project( + self.session, + 'test', + user='pingou', + ) + + 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='pingou', + ) + 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_assign_pr_invalid_project_namespace(self): + """ Test api_pull_request_assign method when the project doesn't exist. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Valid token, wrong project + output = self.app.post( + '/api/0/somenamespace/test3/pull-request/1/assign', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'error': 'Invalid or expired token. Please visit ' + 'http://localhost.localdomain/settings#api-keys to get or renew your ' + 'API token.', + 'error_code': 'EINVALIDTOK'} + + ) + + def test_api_assign_pr_invalid_project(self): + """ Test api_pull_request_assign method when the project doesn't exist. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Invalid project + output = self.app.post('/api/0/foo/pull-request/1/assign', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + { + "error": "Project not found", + "error_code": "ENOPROJECT", + } + ) + + def test_api_assign_pr_invalid_project_token(self): + """ Test api_pull_request_assign method when the token doesn't correspond + to the project. + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Valid token, wrong project + output = self.app.post('/api/0/test2/pull-request/1/assign', headers=headers) + self.assertEqual(output.status_code, 401) + data = json.loads(output.get_data(as_text=True)) + self.assertEqual(sorted(data.keys()), ['error', 'error_code']) + self.assertEqual( + pagure.api.APIERROR.EINVALIDTOK.value, data['error']) + self.assertEqual( + pagure.api.APIERROR.EINVALIDTOK.name, data['error_code']) + + def test_api_assign_pr_invalid_pr(self): + """ Test api_pull_request_assign method when asking for an invalid PR + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # No input + output = self.app.post('/api/0/test/pull-request/404/assign', headers=headers) + self.assertEqual(output.status_code, 404) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'error': 'Pull-Request not found', 'error_code': 'ENOREQ'} + ) + + def test_api_assign_pr_no_input(self): + """ Test api_pull_request_assign method when no input is specified + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + # No input + output = self.app.post('/api/0/test/pull-request/1/assign', headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Nothing to change'} + ) + + def test_api_assign_pr_assigned(self): + """ Test api_pull_request_assign method when with valid input + """ + + headers = {'Authorization': 'token aaabbbcccddd'} + + data = { + 'assignee': 'pingou', + } + + # Valid request + output = self.app.post( + '/api/0/test/pull-request/1/assign', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Request assigned'} + ) + + def test_api_assign_pr_unassigned(self): + """ Test api_pull_request_assign method when unassigning + """ + self.test_api_assign_pr_assigned() + + headers = {'Authorization': 'token aaabbbcccddd'} + data = {} + + # Un-assign + output = self.app.post( + '/api/0/test/pull-request/1/assign', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Request assignee reset'} + ) + + def test_api_assign_pr_unassigned_twice(self): + """ Test api_pull_request_assign method when unassigning + """ + self.test_api_assign_pr_unassigned() + headers = {'Authorization': 'token aaabbbcccddd'} + data = {'assignee': None} + + # Un-assign + output = self.app.post( + '/api/0/test/pull-request/1/assign', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Nothing to change'} + ) + + def test_api_assign_pr_unassigned_empty_string(self): + """ Test api_pull_request_assign method when unassigning with an + empty string + """ + self.test_api_assign_pr_assigned() + + headers = {'Authorization': 'token aaabbbcccddd'} + + # Un-assign + data = {'assignee': ''} + output = self.app.post( + '/api/0/test/pull-request/1/assign', data=data, headers=headers) + self.assertEqual(output.status_code, 200) + data = json.loads(output.get_data(as_text=True)) + self.assertDictEqual( + data, + {'message': 'Request assignee reset'} + ) + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 84a264f..d0a613d 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2998,7 +2998,7 @@ class PagureLibtests(tests.Modeltests): assignee=None, user='foo', ) - self.assertEqual(msg, 'Request reset') + self.assertEqual(msg, 'Request assignee reset') # Try resetting again msg = pagure.lib.query.add_pull_request_assignee( @@ -5534,6 +5534,7 @@ foo bar 'issue_update_custom_fields', 'issue_update_milestone', 'modify_project', + 'pull_request_assign', 'pull_request_close', 'pull_request_comment', 'pull_request_create', @@ -5541,6 +5542,7 @@ foo bar 'pull_request_merge', 'pull_request_rebase', 'pull_request_subscribe', + 'pull_request_update', 'update_watch_status', ] ) From 627fb17a4b1d9db4bb98796a970878d5ea8db18e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 27 2019 14:54:32 +0000 Subject: [PATCH 3/4] Add a button to take/drop a pull-request (assignee field) This button behaves in the same way as we have for ticket. Fixes https://pagure.io/pagure/issue/4084 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/repo_pull_request.html b/pagure/templates/repo_pull_request.html index 8f544aa..6c57da1 100644 --- a/pagure/templates/repo_pull_request.html +++ b/pagure/templates/repo_pull_request.html @@ -509,13 +509,39 @@ {% endif %}
- -
+ +
+
{% if pull_request.assignee.username %} -
{{pull_request.assignee.username| avatar(size=24) | safe}} {{ pull_request.assignee.username }}
+
{{pull_request.assignee.username| avatar(size=24) | safe}} + + {{ pull_request.assignee.username }} + + {% if g.authenticated and (pull_request.assignee.username == g.fas_user.username) %} + — + Drop + + {% endif %} +
{% else %} - None +
+ None + {% if g.authenticated and (g.repo_user or g.fas_user.username == pull_request.user.user) and pull_request.status|lower == 'open' + and (not pull_request.assignee or pull_request.assignee.username != g.fas_user.username) + and not repo.settings.get('pull_request_tracker_read_only', False) %} + — Take + {% endif %} +
{% endif %} +
@@ -523,7 +549,7 @@ g.repo_user or g.fas_user.username == pull_request.user.user) %}