kallithea Changeset - 9a23b444a7fe

Changeset - 9a23b444a7fe

Parent rev.

Child rev.

[Not reviewed]

default

0 4 0

Thomas De Schampheleire - 10 years ago 2015-06-14 13:48:15
thomas.de_schampheleire@alcatel-lucent.com

pullrequests: detect invalid reviewers and raise HTTPBadRequest

Normally, the creation of a pullrequest with invalid reviewers is not
possible because the list of reviewers is populated from a form element
that only shows valid reviewers.
However, if creating a pullrequest through an API call, invalid reviewers
can be specified but would not be detected. The reviewer would be encoded
in the database as 'NULL'/None, and opening such a pull request would cause
a server error.

Instead, detect invalid reviewers at pullrequest creation/update time and
raise HTTPBadRequest.

4 files changed with 149 insertions and 3 deletions:

kallithea/controllers/pullrequests.py

kallithea/lib/exceptions.py

kallithea/model/pull_request.py

kallithea/tests/functional/test_pullrequests.py

129

0 comments (0 inline, 0 general)

kallithea/controllers/pullrequests.py

➞

Show inline comments

@@ @@ -41,12 +41,13 @@ from kallithea.lib.compat import json @@
 from kallithea.lib.base import BaseRepoController, render
 from kallithea.lib.auth import LoginRequired, HasRepoPermissionAnyDecorator,\
     NotAnonymous
 from kallithea.lib.helpers import Page
 from kallithea.lib import helpers as h
 from kallithea.lib import diffs
 from kallithea.lib.exceptions import UserInvalidException
 from kallithea.lib.utils import action_logger, jsonify
 from kallithea.lib.vcs.utils import safe_str
 from kallithea.lib.vcs.exceptions import EmptyRepositoryError
 from kallithea.lib.diffs import LimitedDiffContainer
 from kallithea.model.db import  PullRequest, ChangesetStatus, ChangesetComment,\
     PullRequestReviewers
@@ @@ -359,12 +360,15 @@ class PullrequestsController(BaseRepoCon @@
                 self.authuser.user_id, org_repo_name, org_ref, other_repo_name,
                 other_ref, revisions, reviewers, title, description
+            )
             Session().commit()
             h.flash(_('Successfully opened new pull request'),
                     category='success')
         except UserInvalidException, u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         except Exception:
             h.flash(_('Error occurred while creating pull request'),
                     category='error')
             log.error(traceback.format_exc())
             return redirect(url('pullrequest_home', repo_name=repo_name))
@@ @@ -443,12 +447,15 @@ class PullrequestsController(BaseRepoCon @@
             pull_request = PullRequestModel().create(
                 self.authuser.user_id,
                 old_pull_request.org_repo.repo_name, new_org_ref,
                 old_pull_request.other_repo.repo_name, new_other_ref,
                 revisions, reviewers_ids, title, description
+            )
         except UserInvalidException, u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         except Exception:
             h.flash(_('Error occurred while creating pull request'),
                     category='error')
             log.error(traceback.format_exc())
             return redirect(old_pull_request.url())
@@ @@ -492,15 +499,18 @@ class PullrequestsController(BaseRepoCon @@
                                       _form['pullrequest_desc'],
                                       reviewers_ids)
         old_description = pull_request.description
         pull_request.title = _form['pullrequest_title']
         pull_request.description = _form['pullrequest_desc'].strip() or _('No description')
         PullRequestModel().mention_from_description(pull_request, old_description)
         PullRequestModel().update_reviewers(pull_request_id, reviewers_ids)
         try:
             PullRequestModel().mention_from_description(pull_request, old_description)
             PullRequestModel().update_reviewers(pull_request_id, reviewers_ids)
         except UserInvalidException, u:
             h.flash(_('Invalid reviewer "%s" specified') % u, category='error')
             raise HTTPBadRequest()
         Session().commit()
         h.flash(_('Pull request updated'), category='success')
         return redirect(pull_request.url())

kallithea/lib/exceptions.py

➞

Show inline comments

@@ @@ -96,12 +96,16 @@ class IMCCommitError(Exception): @@
 class UserCreationError(Exception):
     pass
 class UserInvalidException(Exception):
     pass
 class RepositoryCreationError(Exception):
     pass
 class HgsubversionImportError(Exception):
     pass

kallithea/model/pull_request.py

➞

Show inline comments

@@ @@ -29,12 +29,13 @@ import logging @@
 import datetime
 from pylons.i18n.translation import _
 from kallithea.model.meta import Session
 from kallithea.lib import helpers as h
 from kallithea.lib.exceptions import UserInvalidException
 from kallithea.model import BaseModel
 from kallithea.model.db import PullRequest, PullRequestReviewers, Notification,\
     ChangesetStatus, User
 from kallithea.model.notification import NotificationModel
 from kallithea.lib.utils2 import extract_mentioned_users, safe_unicode
@@ @@ -114,12 +115,14 @@ class PullRequestModel(BaseModel): @@
         return new
     def __add_reviewers(self, pr, reviewers, mention_recipients=None):
         #members
         for member in set(reviewers):
             _usr = self._get_user(member)
             if _usr is None:
                 raise UserInvalidException(member)
             reviewer = PullRequestReviewers(_usr, pr)
             Session().add(reviewer)
         revision_data = [(x.raw_id, x.message)
                          for x in map(pr.org_repo.get_changeset, pr.revisions)]

kallithea/tests/functional/test_pullrequests.py

➞

Show inline comments

 import re
 from kallithea.tests import *
 from kallithea.tests.fixture import Fixture
 from kallithea.model.meta import Session
 from kallithea.controllers.pullrequests import PullrequestsController
 from kallithea.lib.exceptions import UserInvalidException
 fixture = Fixture()
 class TestPullrequestsController(TestController):
     def test_index(self):
         self.log_user()
         response = self.app.get(url(controller='pullrequests', action='index',
                                     repo_name=HG_REPO))
     def test_create_trivial(self):
         self.log_user()
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
                                  {'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
+                                 }
+                                )
         self.assertEqual(response.status, '302 Found')
         response = response.follow()
         self.assertEqual(response.status, '200 OK')
         response.mustcontain('This pull request has already been merged to default.')
     def test_create_with_existing_reviewer(self):
         self.log_user()
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
                                  {'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': TEST_USER_ADMIN_LOGIN,
+                                 }
+                                )
         self.assertEqual(response.status, '302 Found')
         response = response.follow()
         self.assertEqual(response.status, '200 OK')
         response.mustcontain('This pull request has already been merged to default.')
     def test_create_with_invalid_reviewer(self):
         invalid_user_name = 'invalid_user'
         self.log_user()
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
+                                 {
                                   'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': invalid_user_name,
                                  },
                                  status=400)
         response.mustcontain('Invalid reviewer &#34;%s&#34; specified' % invalid_user_name)
     def test_update_with_invalid_reviewer(self):
         invalid_user_id = 99999
         self.log_user()
         # create a valid pull request
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
+                                 {
                                   'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
+                                 }
+                                )
         self.assertEqual(response.status, '302 Found')
         # location is of the form:
         # http://localhost/vcs_test_hg/pull-request/54/_/title
         m = re.search('/pull-request/(\d+)/', response.location)
         self.assertNotEqual(m, None)
         pull_request_id = m.group(1)
         # update it
         response = self.app.post(url(controller='pullrequests', action='post',
                                      repo_name=HG_REPO, pull_request_id=pull_request_id),
+                                 {
                                   'updaterev': 'default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': invalid_user_id,
                                  },
                                  status=400)
         response.mustcontain('Invalid reviewer &#34;%s&#34; specified' % invalid_user_id)
     def test_edit_with_invalid_reviewer(self):
         invalid_user_id = 99999
         self.log_user()
         # create a valid pull request
         response = self.app.post(url(controller='pullrequests', action='create',
                                      repo_name=HG_REPO),
+                                 {
                                   'org_repo': HG_REPO,
                                   'org_ref': 'branch:default:default',
                                   'other_repo': HG_REPO,
                                   'other_ref': 'branch:default:default',
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
+                                 }
+                                )
         self.assertEqual(response.status, '302 Found')
         # location is of the form:
         # http://localhost/vcs_test_hg/pull-request/54/_/title
         m = re.search('/pull-request/(\d+)/', response.location)
         self.assertNotEqual(m, None)
         pull_request_id = m.group(1)
         # edit it
         response = self.app.post(url(controller='pullrequests', action='post',
                                      repo_name=HG_REPO, pull_request_id=pull_request_id),
+                                 {
                                   'pullrequest_title': 'title',
                                   'pullrequest_desc': 'description',
                                   '_authentication_token': self.authentication_token(),
                                   'review_members': invalid_user_id,
                                  },
                                  status=400)
         response.mustcontain('Invalid reviewer &#34;%s&#34; specified' % invalid_user_id)
 class TestPullrequestsGetRepoRefs(TestController):
     def setUp(self):
         self.main = fixture.create_repo('main', repo_type='hg')
         Session.add(self.main)
         Session.commit()

0 comments (0 inline, 0 general)