# HG changeset patch # User Thomas De Schampheleire # Date 2015-06-14 13:48:15 # Node ID 9a23b444a7fefe5b67e57a42d26343787d992874 # Parent 5fb4e6f884cec710b34736756d4eadaa53dd51d0 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):