[PATCH] pullrequests: detect invalid reviewers and raise HTTPBadRequest

Thomas De Schampheleire patrickdepinguin at gmail.com
Tue Jun 16 15:48:47 EDT 2015


# HG changeset patch
# User Thomas De Schampheleire <thomas.de_schampheleire at alcatel-lucent.com>
# Date 1434282495 -7200
#      Sun Jun 14 13:48:15 2015 +0200
# Node ID ad0534276417535724301a0c84ced272940456c1
# Parent  53d68f201e4602d3f2ccfcd27107d2ebea2deea2
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.

diff --git a/kallithea/controllers/pullrequests.py b/kallithea/controllers/pullrequests.py
--- a/kallithea/controllers/pullrequests.py
+++ b/kallithea/controllers/pullrequests.py
@@ -44,6 +44,7 @@ from kallithea.lib.auth import LoginRequ
 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
@@ -362,6 +363,9 @@ class PullrequestsController(BaseRepoCon
             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')
@@ -446,6 +450,9 @@ class PullrequestsController(BaseRepoCon
                 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')
@@ -495,9 +502,12 @@ class PullrequestsController(BaseRepoCon
         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')
diff --git a/kallithea/lib/exceptions.py b/kallithea/lib/exceptions.py
--- a/kallithea/lib/exceptions.py
+++ b/kallithea/lib/exceptions.py
@@ -99,6 +99,10 @@ class UserCreationError(Exception):
     pass
 
 
+class UserInvalidException(Exception):
+    pass
+
+
 class RepositoryCreationError(Exception):
     pass
 
diff --git a/kallithea/model/pull_request.py b/kallithea/model/pull_request.py
--- a/kallithea/model/pull_request.py
+++ b/kallithea/model/pull_request.py
@@ -32,6 +32,7 @@ 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
@@ -117,6 +118,8 @@ class PullRequestModel(BaseModel):
         #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)
 
diff --git a/kallithea/tests/functional/test_pullrequests.py b/kallithea/tests/functional/test_pullrequests.py
--- a/kallithea/tests/functional/test_pullrequests.py
+++ b/kallithea/tests/functional/test_pullrequests.py
@@ -1,8 +1,11 @@
+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()
 
@@ -13,6 +16,132 @@ class TestPullrequestsController(TestCon
         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 "%s" 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 "%s" 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 "%s" specified' % invalid_user_id)
+
 class TestPullrequestsGetRepoRefs(TestController):
 
     def setUp(self):


More information about the kallithea-general mailing list