From 850bbd528b1741cf2d8ab7fae0333dd1eb47bdb9 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 1/9] issues api: deduplicate of getting repository Replace copy paste with function call --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index f1397a5..9edb8c4 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -26,6 +26,28 @@ from pagure.api import ( ) +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 exists or is disabled + :return: repository name + """ + repo = pagure.lib.get_project( + SESSION, repo_name, user=username, namespace=namespace) + + if repo is None: + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ENOPROJECT) + + if not repo.settings.get('issue_tracker', True): + raise pagure.exceptions.APIError( + 404, error_code=APIERROR.ETRACKERDISABLED) + + return repo + + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -96,17 +118,8 @@ def api_new_issue(repo, username=None, namespace=None): } """ - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if flask.g.token.project and repo != flask.g.token.project: raise pagure.exceptions.APIError( @@ -288,16 +301,7 @@ def api_view_issues(repo, username=None, namespace=None): } """ - - repo = pagure.lib.get_project( - SESSION, repo, user=username, namespace=namespace) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) assignee = flask.request.args.get('assignee', None) author = flask.request.args.get('author', None) @@ -454,15 +458,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): if str(comments).lower() in ['0', 'False']: comments = False - repo = pagure.lib.get_project( - SESSION, repo, user=username, namespace=namespace) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) issue_id = issue_uid = None try: @@ -541,15 +537,7 @@ def api_view_issue_comment( """ # noqa - repo = pagure.lib.get_project( - SESSION, repo, user=username, namespace=namespace) - - if repo is None: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOPROJECT) - - if not repo.settings.get('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) issue_id = issue_uid = None try: @@ -637,17 +625,9 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): } """ - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if repo != flask.g.token.project: @@ -768,17 +748,8 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): } """ - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if repo != flask.g.token.project: @@ -887,16 +858,8 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): } """ - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if flask.g.token.project and repo != flask.g.token.project: @@ -986,16 +949,8 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): } """ - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if repo != flask.g.token.project: @@ -1103,16 +1058,8 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): } """ # noqa - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if repo != flask.g.token.project: @@ -1205,17 +1152,8 @@ def api_update_custom_field( } """ # noqa - repo = pagure.lib.get_project( - 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('issue_tracker', True): - raise pagure.exceptions.APIError( - 404, error_code=APIERROR.ETRACKERDISABLED) + repo = _get_repo(repo, username, namespace) if api_authenticated(): if repo != flask.g.token.project: From 3646a69d374317b6c4f9aacd434e6dfbf11b5f89 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 2/9] issues api: deduplicate checking of token Instead of copy paste a function can be used --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 9edb8c4..471e623 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -48,6 +48,17 @@ def _get_repo(repo_name, username=None, namespace=None): return repo +def _check_token(repo): + """Check if token is valid for the repo + :param repo: repository name + :raises pagure.exceptions.APIError: when token is not valid for repo + """ + if api_authenticated(): + if repo != flask.g.token.project: + raise pagure.exceptions.APIError( + 401, error_code=APIERROR.EINVALIDTOK) + + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -472,10 +483,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): if issue is None or issue.project != repo: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -551,10 +559,7 @@ def api_view_issue_comment( if issue is None or issue.project != repo: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) if issue.private and not is_repo_committer(issue.project) \ and (not api_authenticated() or @@ -629,10 +634,7 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): repo = _get_repo(repo, username, namespace) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) @@ -751,10 +753,7 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): output = {} repo = _get_repo(repo, username, namespace) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) @@ -952,10 +951,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): output = {} repo = _get_repo(repo, username, namespace) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) @@ -1061,10 +1057,7 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): output = {} repo = _get_repo(repo, username, namespace) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) @@ -1155,10 +1148,7 @@ def api_update_custom_field( output = {} repo = _get_repo(repo, username, namespace) - if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo) issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) From c45eac984538703059dfe15681abc6b01eb48ca9 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 3/9] issues API: performace: fail early with invalid token Fail early with invalid token and don't waste precious database operations --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 471e623..0540ba8 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -313,6 +313,7 @@ def api_view_issues(repo, username=None, namespace=None): """ repo = _get_repo(repo, username, namespace) + _check_token(repo) assignee = flask.request.args.get('assignee', None) author = flask.request.args.get('author', None) @@ -350,9 +351,6 @@ def api_view_issues(repo, username=None, namespace=None): private = False # If user is authenticated, show him/her his/her private tickets if api_authenticated(): - if repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) private = flask.g.fas_user.username # If user is repo committer, show all tickets included the private ones if is_repo_committer(repo): @@ -470,6 +468,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): comments = False repo = _get_repo(repo, username, namespace) + _check_token(repo) issue_id = issue_uid = None try: @@ -483,8 +482,6 @@ def api_view_issue(repo, issueid, username=None, namespace=None): if issue is None or issue.project != repo: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) - _check_token(repo) - if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or not issue.user.user == flask.g.fas_user.username): @@ -546,6 +543,7 @@ def api_view_issue_comment( """ # noqa repo = _get_repo(repo, username, namespace) + _check_token(repo) issue_id = issue_uid = None try: @@ -559,8 +557,6 @@ def api_view_issue_comment( if issue is None or issue.project != repo: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) - _check_token(repo) - if issue.private and not is_repo_committer(issue.project) \ and (not api_authenticated() or not issue.user.user == flask.g.fas_user.username): From aac5b57a26c0bfef569c4c04df334344e8e6f339 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 4/9] issues api: deduplicate getting issues This copy paste can be replaced by function --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 0540ba8..e56918d 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -59,6 +59,22 @@ def _check_token(repo): 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.search_issues(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 + + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -476,11 +492,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): except (ValueError, TypeError): issue_uid = issueid - issue = pagure.lib.search_issues( - SESSION, repo, issueid=issue_id, issueuid=issue_uid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issue_id, issueuid=issue_uid) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -551,11 +563,7 @@ def api_view_issue_comment( except (ValueError, TypeError): issue_uid = issueid - issue = pagure.lib.search_issues( - SESSION, repo, issueid=issue_id, issueuid=issue_uid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issue_id, issueuid=issue_uid) if issue.private and not is_repo_committer(issue.project) \ and (not api_authenticated() or @@ -632,10 +640,7 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): _check_token(repo) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -751,10 +756,7 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): _check_token(repo) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -861,10 +863,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): raise pagure.exceptions.APIError( 401, error_code=APIERROR.EINVALIDTOK) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -949,10 +948,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): _check_token(repo) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_committer(repo) \ and (not api_authenticated() or @@ -1055,10 +1051,7 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): _check_token(repo) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_admin(repo) \ and (not api_authenticated() or @@ -1146,10 +1139,7 @@ def api_update_custom_field( _check_token(repo) - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid) - - if issue is None or issue.project != repo: - raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) + issue = _get_issue(repo, issueid) if issue.private and not is_repo_admin(repo) \ and (not api_authenticated() or From 2ed3b42e42c31ac6593c036002f68813e0f00384 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 5/9] unify argument of is_repo_commiter Various sources of repo has been used: variable repo and issue.project IMO is safer to use issue.project in case that accidentaly an issue is taken from different repo. --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index e56918d..6b15f78 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -67,7 +67,8 @@ def _get_issue(repo, issueid, issueuid=None): :raises pagure.exceptions.APIError: when issues doesn't exists :return: issue """ - issue = pagure.lib.search_issues(SESSION, repo, issueid=issueid, issueuid=issueuid) + issue = pagure.lib.search_issues( + SESSION, repo, issueid=issueid, issueuid=issueuid) if issue is None or issue.project != repo: raise pagure.exceptions.APIError(404, error_code=APIERROR.ENOISSUE) @@ -494,7 +495,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): issue = _get_issue(repo, issue_id, issueuid=issue_uid) - if issue.private and not is_repo_committer(repo) \ + 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( @@ -642,7 +643,7 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): issue = _get_issue(repo, issueid) - if issue.private and not is_repo_committer(repo) \ + 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( @@ -758,7 +759,7 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): issue = _get_issue(repo, issueid) - if issue.private and not is_repo_committer(repo) \ + 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( @@ -865,7 +866,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): issue = _get_issue(repo, issueid) - if issue.private and not is_repo_committer(repo) \ + 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( @@ -950,7 +951,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): issue = _get_issue(repo, issueid) - if issue.private and not is_repo_committer(repo) \ + 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( From 8c3d8a0bb49e8ff8fa054b48ef2ce0a0cd49db9d Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 6/9] issues API: deduplicate issue access check --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 6b15f78..9de7d2d 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -76,6 +76,23 @@ def _get_issue(repo, issueid, issueuid=None): return issue +def _check_issue_access_repo_commiter(issue): + """Check if user can access issue. Must be repo commiter + 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) + + @API.route('//new_issue', methods=['POST']) @API.route('///new_issue', methods=['POST']) @API.route('/fork///new_issue', methods=['POST']) @@ -494,12 +511,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): issue_uid = issueid issue = _get_issue(repo, issue_id, issueuid=issue_uid) - - 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) + _check_issue_access_repo_commiter(issue) jsonout = flask.jsonify( issue.to_json(public=True, with_comments=comments)) @@ -565,12 +577,7 @@ def api_view_issue_comment( issue_uid = issueid issue = _get_issue(repo, issue_id, issueuid=issue_uid) - - 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) + _check_issue_access_repo_commiter(issue) comment = pagure.lib.get_issue_comment(SESSION, issue.uid, commentid) if not comment: @@ -642,12 +649,8 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): _check_token(repo) issue = _get_issue(repo, issueid) + _check_issue_access_repo_commiter(issue) - 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) status = pagure.lib.get_issue_statuses(SESSION) form = pagure.forms.StatusForm( @@ -758,12 +761,7 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): _check_token(repo) issue = _get_issue(repo, issueid) - - 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) + _check_issue_access_repo_commiter(issue) form = pagure.forms.MilestoneForm( milestones=repo.milestones.keys(), @@ -865,12 +863,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): 401, error_code=APIERROR.EINVALIDTOK) issue = _get_issue(repo, issueid) - - 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) + _check_issue_access_repo_commiter(issue) form = pagure.forms.CommentForm(csrf_enabled=False) if form.validate_on_submit(): @@ -950,12 +943,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): _check_token(repo) issue = _get_issue(repo, issueid) - - 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) + _check_issue_access_repo_commiter(issue) form = pagure.forms.AssignIssueForm(csrf_enabled=False) if form.validate_on_submit(): From ac2f71b9618afb9afd15d18dd687bc07d705d2f2 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 7/9] issues API: require repo_commiter not repo_admin privileges repo_admin requirement is too strong limitation because issues may be updated using different API endpoints just with repo_commiter permission. --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 9de7d2d..f03f050 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -1041,12 +1041,7 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): _check_token(repo) issue = _get_issue(repo, issueid) - - if issue.private and not is_repo_admin(repo) \ - and (not api_authenticated() or - not issue.user.user == flask.g.fas_user.username): - raise pagure.exceptions.APIError( - 403, error_code=APIERROR.EISSUENOTALLOWED) + _check_issue_access_repo_commiter(issue) form = pagure.forms.SubscribtionForm(csrf_enabled=False) if form.validate_on_submit(): @@ -1129,12 +1124,7 @@ def api_update_custom_field( _check_token(repo) issue = _get_issue(repo, issueid) - - if issue.private and not is_repo_admin(repo) \ - and (not api_authenticated() or - not issue.user.user == flask.g.fas_user.username): - raise pagure.exceptions.APIError( - 403, error_code=APIERROR.EISSUENOTALLOWED) + _check_issue_access_repo_commiter(issue) fields = {k.name: k for k in repo.issue_keys} if field not in fields: From c00ccc205d32f11dd5a67eefd43433a0dd4d2da1 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 8/9] issues API: update _check_token to support non-project tokens Add option to diferentiate between project token and non-project token --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index f03f050..2d79705 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -48,13 +48,18 @@ def _get_repo(repo_name, username=None, namespace=None): return repo -def _check_token(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 repo != flask.g.token.project: + if ( + (project_token or flask.g.token.project) and + repo != flask.g.token.project + ): raise pagure.exceptions.APIError( 401, error_code=APIERROR.EINVALIDTOK) @@ -856,11 +861,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): """ output = {} repo = _get_repo(repo, username, namespace) - - if api_authenticated(): - if flask.g.token.project and repo != flask.g.token.project: - raise pagure.exceptions.APIError( - 401, error_code=APIERROR.EINVALIDTOK) + _check_token(repo, project_token=False) issue = _get_issue(repo, issueid) _check_issue_access_repo_commiter(issue) From 213dd9b7602f539a6194f2e7c6d154692a2d0564 Mon Sep 17 00:00:00 2001 From: Martin Basti Date: Mar 29 2017 20:08:44 +0000 Subject: [PATCH 9/9] issues API: extract issue tracker check to separate function _check_issue_tracker now checks if tracker is enabled --- diff --git a/pagure/api/issue.py b/pagure/api/issue.py index 2d79705..8324180 100644 --- a/pagure/api/issue.py +++ b/pagure/api/issue.py @@ -41,12 +41,18 @@ def _get_repo(repo_name, username=None, namespace=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 + """ if not repo.settings.get('issue_tracker', True): raise pagure.exceptions.APIError( 404, error_code=APIERROR.ETRACKERDISABLED) - return repo - def _check_token(repo, project_token=True): """Check if token is valid for the repo @@ -170,6 +176,7 @@ def api_new_issue(repo, username=None, namespace=None): """ output = {} repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) if flask.g.token.project and repo != flask.g.token.project: raise pagure.exceptions.APIError( @@ -352,6 +359,7 @@ def api_view_issues(repo, username=None, namespace=None): """ repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) _check_token(repo) assignee = flask.request.args.get('assignee', None) @@ -507,6 +515,7 @@ def api_view_issue(repo, issueid, username=None, namespace=None): comments = False repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) _check_token(repo) issue_id = issue_uid = None @@ -573,6 +582,7 @@ def api_view_issue_comment( """ # noqa repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) _check_token(repo) issue_id = issue_uid = None @@ -650,7 +660,7 @@ def api_change_status_issue(repo, issueid, username=None, namespace=None): output = {} repo = _get_repo(repo, username, namespace) - + _check_issue_tracker(repo) _check_token(repo) issue = _get_issue(repo, issueid) @@ -762,7 +772,7 @@ def api_change_milestone_issue(repo, issueid, username=None, namespace=None): """ output = {} repo = _get_repo(repo, username, namespace) - + _check_issue_tracker(repo) _check_token(repo) issue = _get_issue(repo, issueid) @@ -861,6 +871,7 @@ def api_comment_issue(repo, issueid, username=None, namespace=None): """ output = {} repo = _get_repo(repo, username, namespace) + _check_issue_tracker(repo) _check_token(repo, project_token=False) issue = _get_issue(repo, issueid) @@ -940,7 +951,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None): """ output = {} repo = _get_repo(repo, username, namespace) - + _check_issue_tracker(repo) _check_token(repo) issue = _get_issue(repo, issueid) @@ -1038,7 +1049,7 @@ def api_subscribe_issue(repo, issueid, username=None, namespace=None): """ # noqa output = {} repo = _get_repo(repo, username, namespace) - + _check_issue_tracker(repo) _check_token(repo) issue = _get_issue(repo, issueid) @@ -1121,7 +1132,7 @@ def api_update_custom_field( """ # noqa output = {} repo = _get_repo(repo, username, namespace) - + _check_issue_tracker(repo) _check_token(repo) issue = _get_issue(repo, issueid)