FIX: Ensure review actions are only performable on visible topics (#276)

It's rare for code-review topics to be private, and it's even rarer for the review status to be considered sensitive. But even so, it makes sense to ensure reviewers can only change review status of topics they can see.

Author: davidtaylorhq

app/controllers/discourse_code_review/code_review_controller.rb

@@ -93,7 +93,7 @@ def webhook
     end
 
     def skip
-      topic = Topic.find_by(id: params[:topic_id])
+      topic = find_reviewable_commit_topic
 
       State::CommitApproval.skip(topic, current_user)
 
@@ -103,7 +103,7 @@ def skip
     def followup
       raise Discourse::InvalidAccess if !SiteSetting.code_review_allow_manual_followup
 
-      topic = Topic.find_by(id: params[:topic_id])
+      topic = find_reviewable_commit_topic
 
       State::CommitApproval.followup(topic, current_user)
 
@@ -113,33 +113,15 @@ def followup
     def followed_up
       raise Discourse::InvalidAccess if !SiteSetting.code_review_allow_manual_followup
 
-      topic = Topic.find_by(id: params[:topic_id])
-
-      tags = topic.tags.pluck(:name)
-
-      if tags.include?(SiteSetting.code_review_followup_tag)
-        tags -= [SiteSetting.code_review_approved_tag, SiteSetting.code_review_followup_tag]
-
-        tags << SiteSetting.code_review_pending_tag
-
-        DiscourseTagging.tag_topic_by_names(topic, Guardian.new(current_user), tags)
-
-        topic.add_moderator_post(
-          current_user,
-          nil,
-          bump: false,
-          post_type: Post.types[:small_action],
-          action_code: "followed_up",
-        )
+      topic = find_reviewable_commit_topic
 
-        DiscourseEvent.trigger(:unassign_topic, topic, current_user)
-      end
+      State::CommitApproval.complete_followup(topic, current_user)
 
       render json: success_json
     end
 
     def approve
-      topic = Topic.find_by(id: params[:topic_id])
+      topic = find_reviewable_commit_topic
 
       if !SiteSetting.code_review_allow_self_approval && topic.user_id == current_user.id
         raise Discourse::InvalidAccess
@@ -174,6 +156,14 @@ def redirect
 
     protected
 
+    def find_reviewable_commit_topic
+      topic = Topic.find_by(id: params[:topic_id])
+      guardian.ensure_can_see!(topic)
+      raise Discourse::InvalidAccess if !topic.code_review_commit_topic
+
+      topic
+    end
+
     def render_next_topic(category_id)
       category_filter_sql = <<~SQL
         category_id NOT IN (

lib/discourse_code_review/state/commit_approval.rb

@@ -6,6 +6,8 @@ module DiscourseCodeReview::State::CommitApproval
 
   class << self
     def skip(topic, user)
+      ensure_can_skip!(topic, user)
+
       if SiteSetting.code_review_skip_duration_minutes > 0
         DiscourseCodeReview::SkippedCodeReview.upsert(
           {
@@ -21,6 +23,8 @@ def skip(topic, user)
     end
 
     def approve(topic, approvers, pr: nil, merged_by: nil)
+      approvers.each { |approver| ensure_can_approve!(topic, approver) }
+
       last_post = nil
       approvers.each { |approver| last_post = ensure_approved_post(topic, approver) }
 
@@ -38,14 +42,14 @@ def approve(topic, approvers, pr: nil, merged_by: nil)
     end
 
     def followup(topic, actor)
-      tags = topic.tags.pluck(:name)
+      tags = ensure_can_followup!(topic, actor)
 
       if !tags.include?(SiteSetting.code_review_followup_tag)
         tags -= [SiteSetting.code_review_approved_tag, SiteSetting.code_review_pending_tag]
 
         tags << SiteSetting.code_review_followup_tag
 
-        DiscourseTagging.tag_topic_by_names(topic, Guardian.new(actor), tags)
+        DiscourseTagging.tag_topic_by_names(topic, actor.guardian, tags)
 
         topic.add_moderator_post(
           actor,
@@ -61,6 +65,25 @@ def followup(topic, actor)
       end
     end
 
+    def complete_followup(topic, actor)
+      tags = ensure_can_complete_followup!(topic, actor)
+
+      tags -= [SiteSetting.code_review_approved_tag, SiteSetting.code_review_followup_tag]
+      tags << SiteSetting.code_review_pending_tag
+
+      DiscourseTagging.tag_topic_by_names(topic, actor.guardian, tags)
+
+      topic.add_moderator_post(
+        actor,
+        nil,
+        bump: false,
+        post_type: Post.types[:small_action],
+        action_code: "followed_up",
+      )
+
+      DiscourseEvent.trigger(:unassign_topic, topic, actor)
+    end
+
     def followed_up(followee_topic, follower_topic)
       last_post =
         followee_topic.add_moderator_post(
@@ -87,6 +110,58 @@ def followed_up(followee_topic, follower_topic)
 
     private
 
+    def ensure_can_skip!(topic, actor)
+      tags = ensure_can_review_commit_topic!(topic, actor)
+      raise Discourse::InvalidAccess if (tags & approvable_tags).empty?
+    end
+
+    def ensure_can_approve!(topic, actor)
+      tags = ensure_can_review_commit_topic!(topic, actor)
+      if !tags.include?(SiteSetting.code_review_approved_tag) && (tags & approvable_tags).empty?
+        raise Discourse::InvalidAccess
+      end
+
+      if !SiteSetting.code_review_allow_self_approval && topic.user_id == actor.id
+        raise Discourse::InvalidAccess
+      end
+    end
+
+    def ensure_can_followup!(topic, actor)
+      tags = ensure_can_review_commit_topic!(topic, actor)
+      if !tags.include?(SiteSetting.code_review_followup_tag) &&
+           (
+             tags & [SiteSetting.code_review_pending_tag, SiteSetting.code_review_approved_tag]
+           ).empty?
+        raise Discourse::InvalidAccess
+      end
+
+      tags
+    end
+
+    def ensure_can_complete_followup!(topic, actor)
+      tags = ensure_can_review_commit_topic!(topic, actor)
+      raise Discourse::InvalidAccess if actor.id != topic.user_id && !actor.staff?
+      raise Discourse::InvalidAccess if !tags.include?(SiteSetting.code_review_followup_tag)
+
+      tags
+    end
+
+    def ensure_can_review_commit_topic!(topic, actor, require_code_reviewer: true)
+      raise Discourse::InvalidAccess if !topic || !actor
+      if require_code_reviewer && !actor.admin? && !actor.can_review_code?
+        raise Discourse::InvalidAccess
+      end
+
+      actor.guardian.ensure_can_see!(topic)
+      raise Discourse::InvalidAccess if !topic.code_review_commit_topic
+
+      topic.tags.pluck(:name)
+    end
+
+    def approvable_tags
+      [SiteSetting.code_review_pending_tag, SiteSetting.code_review_followup_tag]
+    end
+
     def ensure_approved_post(topic, approver)
       DistributedMutex.synchronize("code-review:ensure-approved-post:#{topic.id}") do
         ActiveRecord::Base.transaction(requires_new: true) do
@@ -121,7 +196,7 @@ def transition_to_approved(topic)
 
           tags << SiteSetting.code_review_approved_tag
 
-          DiscourseTagging.tag_topic_by_names(topic, Guardian.new(Discourse.system_user), tags)
+          DiscourseTagging.tag_topic_by_names(topic, Discourse.system_user.guardian, tags)
           already_approved = false
         end
       end

spec/discourse_code_review/lib/state/commit_approval_spec.rb

@@ -2,12 +2,17 @@
 
 module DiscourseCodeReview
   describe State::CommitApproval do
-    fab!(:topic)
+    fab!(:pending_tag) { Fabricate(:tag, name: SiteSetting.code_review_pending_tag) }
+    fab!(:topic) { Fabricate(:topic, tags: [pending_tag]) }
     fab!(:pr) do
       DiscourseCodeReview::PullRequest.new(owner: "owner", name: "name", issue_number: 101)
     end
-    fab!(:approver, :user)
-    fab!(:merged_by, :user)
+    fab!(:approver, :admin)
+    fab!(:merged_by, :admin)
+
+    before do
+      DiscourseCodeReview::CommitTopic.create!(topic_id: topic.id, sha: SecureRandom.hex(20))
+    end
 
     describe "#ensure_pr_merge_info_post" do
       it "does not consider duplicate approvers" do
@@ -24,11 +29,48 @@ module DiscourseCodeReview
       end
     end
 
+    describe ".followup" do
+      fab!(:reviewer_group, :group)
+      fab!(:actor, :user)
+      fab!(:private_group, :group)
+
+      fab!(:private_category) { Fabricate(:private_category, group: private_group) }
+
+      fab!(:private_topic) do
+        Fabricate(:topic, category: private_category, tags: [pending_tag], user: Fabricate(:admin))
+      end
+
+      before do
+        SiteSetting.code_review_allowed_groups = reviewer_group.id.to_s
+        SiteSetting.tagging_enabled = true
+        reviewer_group.add(actor)
+        DiscourseCodeReview::CommitTopic.create!(
+          topic_id: private_topic.id,
+          sha: SecureRandom.hex(20),
+        )
+      end
+
+      it "requires the actor to see the topic" do
+        expect(actor.guardian.can_see?(private_topic)).to eq(false)
+
+        expect { State::CommitApproval.followup(private_topic, actor) }.to raise_error(
+          Discourse::InvalidAccess,
+        )
+        expect(private_topic.reload.tags.pluck(:name)).to contain_exactly(
+          SiteSetting.code_review_pending_tag,
+        )
+      end
+    end
+
     describe "creates notifications upon approval" do
       before { SiteSetting.code_review_enabled = true }
 
       it "doesn't consolidate notifications if they were created more than 6 hours ago" do
-        second_topic = Fabricate(:topic, user: topic.user)
+        second_topic = Fabricate(:topic, tags: [pending_tag], user: topic.user)
+        DiscourseCodeReview::CommitTopic.create!(
+          topic_id: second_topic.id,
+          sha: SecureRandom.hex(20),
+        )
         second_pr =
           DiscourseCodeReview::PullRequest.new(owner: "owner", name: "name", issue_number: 102)