#2180 WIP: Remove duplicated code from issues API
Merged 7 years ago by pingou. Opened 7 years ago by mbasti.
mbasti/pagure duplication-cleanup  into  master

file modified
+123 -218
@@ -26,6 +26,84 @@ 

  )

  

  

+ 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)

+ 

+     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)

+ 

+ 

+ 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 (

+             (project_token or flask.g.token.project) and

+             repo != flask.g.token.project

+         ):

+             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.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

+ 

+ 

+ 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('/<repo>/new_issue', methods=['POST'])

  @API.route('/<namespace>/<repo>/new_issue', methods=['POST'])

  @API.route('/fork/<username>/<repo>/new_issue', methods=['POST'])
@@ -96,17 +174,9 @@ 

          }

  

      """

-     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)

+     _check_issue_tracker(repo)

  

      if flask.g.token.project and repo != flask.g.token.project:

          raise pagure.exceptions.APIError(
@@ -288,16 +358,9 @@ 

          }

  

      """

- 

-     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)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

      assignee = flask.request.args.get('assignee', None)

      author = flask.request.args.get('author', None)
@@ -335,9 +398,6 @@ 

      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):
@@ -454,15 +514,9 @@ 

      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)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

      issue_id = issue_uid = None

      try:
@@ -470,22 +524,8 @@ 

      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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             raise pagure.exceptions.APIError(

-                 401, error_code=APIERROR.EINVALIDTOK)

- 

-     if issue.private and not is_repo_committer(repo) \

-             and (not api_authenticated() or

-                  not issue.user.user == flask.g.fas_user.username):

-         raise pagure.exceptions.APIError(

-             403, error_code=APIERROR.EISSUENOTALLOWED)

+     issue = _get_issue(repo, issue_id, issueuid=issue_uid)

+     _check_issue_access_repo_commiter(issue)

  

      jsonout = flask.jsonify(

          issue.to_json(public=True, with_comments=comments))
@@ -541,15 +581,9 @@ 

  

      """  # 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)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

      issue_id = issue_uid = None

      try:
@@ -557,22 +591,8 @@ 

      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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             raise pagure.exceptions.APIError(

-                 401, error_code=APIERROR.EINVALIDTOK)

- 

-     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)

+     issue = _get_issue(repo, issue_id, issueuid=issue_uid)

+     _check_issue_access_repo_commiter(issue)

  

      comment = pagure.lib.get_issue_comment(SESSION, issue.uid, commentid)

      if not comment:
@@ -637,33 +657,15 @@ 

          }

  

      """

-     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)

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

-     if not repo.settings.get('issue_tracker', True):

-         raise pagure.exceptions.APIError(

-             404, error_code=APIERROR.ETRACKERDISABLED)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             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)

- 

-     if issue.private and not is_repo_committer(repo) \

-             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(
@@ -768,33 +770,13 @@ 

          }

  

      """

-     repo = pagure.lib.get_project(

-         SESSION, repo, user=username, namespace=namespace)

- 

      output = {}

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

-     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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             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)

- 

-     if issue.private and not is_repo_committer(repo) \

-             and (not api_authenticated() or

-                  not issue.user.user == flask.g.fas_user.username):

-         raise pagure.exceptions.APIError(

-             403, error_code=APIERROR.EISSUENOTALLOWED)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

      form = pagure.forms.MilestoneForm(

          milestones=repo.milestones.keys(),
@@ -887,32 +869,13 @@ 

          }

  

      """

-     repo = pagure.lib.get_project(

-         SESSION, repo, user=username, namespace=namespace)

      output = {}

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo, project_token=False)

  

-     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)

- 

-     if api_authenticated():

-         if flask.g.token.project and repo != flask.g.token.project:

-             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)

- 

-     if issue.private and not is_repo_committer(repo) \

-             and (not api_authenticated() or

-                  not issue.user.user == flask.g.fas_user.username):

-         raise pagure.exceptions.APIError(

-             403, error_code=APIERROR.EISSUENOTALLOWED)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

      form = pagure.forms.CommentForm(csrf_enabled=False)

      if form.validate_on_submit():
@@ -986,32 +949,13 @@ 

          }

  

      """

-     repo = pagure.lib.get_project(

-         SESSION, repo, user=username, namespace=namespace)

      output = {}

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

-     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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             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)

- 

-     if issue.private and not is_repo_committer(repo) \

-             and (not api_authenticated() or

-                  not issue.user.user == flask.g.fas_user.username):

-         raise pagure.exceptions.APIError(

-             403, error_code=APIERROR.EISSUENOTALLOWED)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

      form = pagure.forms.AssignIssueForm(csrf_enabled=False)

      if form.validate_on_submit():
@@ -1103,32 +1047,13 @@ 

          }

  

      """  # noqa

-     repo = pagure.lib.get_project(

-         SESSION, repo, user=username, namespace=namespace)

      output = {}

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

-     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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             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)

- 

-     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)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

      form = pagure.forms.SubscribtionForm(csrf_enabled=False)

      if form.validate_on_submit():
@@ -1205,33 +1130,13 @@ 

          }

  

      """  # noqa

-     repo = pagure.lib.get_project(

-         SESSION, repo, user=username, namespace=namespace)

- 

      output = {}

+     repo = _get_repo(repo, username, namespace)

+     _check_issue_tracker(repo)

+     _check_token(repo)

  

-     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)

- 

-     if api_authenticated():

-         if repo != flask.g.token.project:

-             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)

- 

-     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)

+     issue = _get_issue(repo, issueid)

+     _check_issue_access_repo_commiter(issue)

  

      fields = {k.name: k for k in repo.issue_keys}

      if field not in fields:

Duplicated code is ugly, hard to maintain, error prone, unreadable and http://i0.kym-cdn.com/photos/images/newsfeed/001/209/105/7b2.jpeg

Why can't you just contribute to a project without making snarky comment? Seriously, this isn't fun when you're on the receiving end of it.

API endpoint requiring a token should have it declared with a decorator, so this is only to check that the token correspond to the expected project.

Here we enforce the presence of the API token :)

Missing _check_repo no?

This check seems to have been lost

This has been lost as well

The check is required, we have API token specific to some project and project-less API token

Nope (if you mean _check_token), It was moved to earlier place in "issues API: performace: fail early with invalid token", see line 183

It is part of _get_repo() should I extract it into separate function?

_get_repo() contains it

Maybe then I should create a _check_token(require_project_token=True) and cover both cases in that function

require_project_token=False in this case

Ok, should be there if api_authenticated() as in code later for non-project token?

Isn't then if api_authenticated() duplication when api must be authenticated already as enfoce decorators, or if decorator allows unathenticated querys should be check for repo really there for authenticated?

Thanks for explanation

Might make sense, because then we could move get_repo to api/init and re-use it in other API controller

Sounds doable, though I'd suggest a slightly shorter variable name, maybe project_token?

Well, thanks for the patch :)

rebased

7 years ago

Unresolved previously:

 pingou commented 2 days ago
API endpoint requiring a token should have it declared with a decorator, so this is only to check that the token correspond to the expected project.

 mbasti commented 2 days ago
Isn't then if api_authenticated() duplication when api must be authenticated already as enfoce decorators, or if decorator allows unathenticated querys should be check for repo really there for authenticated?

10 new commits added

  • NEEDINFO
  • issues API: extract issue tracker check to separate function
  • issues API: update _check_token to support non-project tokens
  • issues API: require repo_commiter not repo_admin privileges
  • issues API: deduplicate issue access check
  • unify argument of is_repo_commiter
  • issues api: deduplicate getting issues
  • issues API: performace: fail early with invalid token
  • issues api: deduplicate checking of token
  • issues api: deduplicate of getting repository
7 years ago

For this one no, we must be authenticated which should be ensured by @api_login_required

rebased

7 years ago

rebased

7 years ago

rebased

7 years ago

I'd say this is the final version

pretty please pagure-ci rebuild

Thanks pagure-ci :)

This is looking all good to me as well, let's merge!

Pull-Request has been merged by pingou

7 years ago

Well, thank you, this is a pretty awesome PR :)