security: make sure the admin of repo can only delete comments which are from the same repo.

- fixes IDOR issue
- protects against other people comment deletion by repo admins.

rhodecode/controllers/changeset.py

@@ -40,7 +40,7 @@ from rhodecode.lib.compat import OrderedDict
 from rhodecode.lib.exceptions import StatusChangeOnClosedPullRequestError
 import rhodecode.lib.helpers as h
 from rhodecode.lib.utils import jsonify
-from rhodecode.lib.utils2 import safe_unicode
+from rhodecode.lib.utils2 import safe_unicode, safe_int
 from rhodecode.lib.vcs.backends.base import EmptyCommit
 from rhodecode.lib.vcs.exceptions import (
     RepositoryError, CommitDoesNotExistError, NodeDoesNotExistError)
@@ -431,15 +431,19 @@ class ChangesetController(BaseRepoController):
     @auth.CSRFRequired()
     @jsonify
     def delete_comment(self, repo_name, comment_id):
-        comment = ChangesetComment.get(comment_id)
+        comment = ChangesetComment.get_or_404(safe_int(comment_id))
         if not comment:
             log.debug('Comment with id:%s not found, skipping', comment_id)
             # comment already deleted in another call probably
             return True
 
-        owner = (comment.author.user_id == c.rhodecode_user.user_id)
         is_repo_admin = h.HasRepoPermissionAny('repository.admin')(c.repo_name)
-        if h.HasPermissionAny('hg.admin')() or is_repo_admin or owner:
+        super_admin = h.HasPermissionAny('hg.admin')()
+        comment_owner = (comment.author.user_id == c.rhodecode_user.user_id)
+        is_repo_comment = comment.repo.repo_name == c.repo_name
+        comment_repo_admin = is_repo_admin and is_repo_comment
+
+        if super_admin or comment_owner or comment_repo_admin:
             CommentsModel().delete(comment=comment, user=c.rhodecode_user)
             Session().commit()
             return True

rhodecode/controllers/pullrequests.py

@@ -987,25 +987,30 @@ class PullrequestsController(BaseRepoController):
     @auth.CSRFRequired()
     @jsonify
     def delete_comment(self, repo_name, comment_id):
-        return self._delete_comment(comment_id)
+        comment = ChangesetComment.get_or_404(safe_int(comment_id))
+        if not comment:
+            log.debug('Comment with id:%s not found, skipping', comment_id)
+            # comment already deleted in another call probably
+            return True
 
-    def _delete_comment(self, comment_id):
-        comment_id = safe_int(comment_id)
-        co = ChangesetComment.get_or_404(comment_id)
-        if co.pull_request.is_closed():
+        if comment.pull_request.is_closed():
             # don't allow deleting comments on closed pull request
             raise HTTPForbidden()
 
-        is_owner = co.author.user_id == c.rhodecode_user.user_id
         is_repo_admin = h.HasRepoPermissionAny('repository.admin')(c.repo_name)
-        if h.HasPermissionAny('hg.admin')() or is_repo_admin or is_owner:
-            old_calculated_status = co.pull_request.calculated_review_status()
-            CommentsModel().delete(comment=co, user=c.rhodecode_user)
+        super_admin = h.HasPermissionAny('hg.admin')()
+        comment_owner = comment.author.user_id == c.rhodecode_user.user_id
+        is_repo_comment = comment.repo.repo_name == c.repo_name
+        comment_repo_admin = is_repo_admin and is_repo_comment
+
+        if super_admin or comment_owner or comment_repo_admin:
+            old_calculated_status = comment.pull_request.calculated_review_status()
+            CommentsModel().delete(comment=comment, user=c.rhodecode_user)
             Session().commit()
-            calculated_status = co.pull_request.calculated_review_status()
+            calculated_status = comment.pull_request.calculated_review_status()
             if old_calculated_status != calculated_status:
                 PullRequestModel()._trigger_pull_request_hook(
-                    co.pull_request, c.rhodecode_user, 'review_status_change')
+                    comment.pull_request, c.rhodecode_user, 'review_status_change')
             return True
         else:
             raise HTTPForbidden()

rhodecode/tests/functional/test_pullrequests.py

@@ -1060,6 +1060,17 @@ class TestPullrequestsControllerDelete(object):
         response.mustcontain('id="delete_pullrequest"')
         response.mustcontain(no=['Confirm to delete this pull request'])
 
+    def test_delete_comment_returns_404_if_comment_does_not_exist(
+            self, autologin_user, pr_util, user_admin):
+
+        pull_request = pr_util.create_pull_request(
+            author=user_admin.username, enable_notifications=False)
+
+        self.app.get(url(
+            controller='pullrequests', action='delete_comment',
+            repo_name=pull_request.target_repo.scm_instance().name,
+            comment_id=1024404), status=404)
+
 
 def assert_pull_request_status(pull_request, expected_status):
     status = ChangesetStatusModel().calculated_review_status(
@@ -1081,13 +1092,3 @@ def test_redirects_to_repo_summary_for_svn_repositories(backend_svn, app, action
 
     # URL adds leading slash and path doesn't have it
     assert redirected.request.path == summary_url
-
-
-def test_delete_comment_returns_404_if_comment_does_not_exist(pylonsapp):
-    # TODO: johbo: Global import not possible because models.forms blows up
-    from rhodecode.controllers.pullrequests import PullrequestsController
-    controller = PullrequestsController()
-    patcher = mock.patch(
-        'rhodecode.model.db.BaseModel.get', return_value=None)
-    with pytest.raises(HTTPNotFound), patcher:
-        controller._delete_comment(1)