Merge branch 'security-filter-related-branches-from-activity-feed-12.5' into '12-5-stable'

Related Branches Visible to Guests in Issue Activity

See merge request gitlab/gitlabhq!3538

4 files changed, 121 insertions, 1 deletions

diff --git a/app/models/note.rb b/app/models/note.rb
index ce60413b8a0..493132e30cc 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -37,6 +37,10 @@ class Note < ApplicationRecord
 
   redact_field :note
 
+  TYPES_RESTRICTED_BY_ABILITY = {
+    branch: :download_code
+  }.freeze
+
   # Aliases to make application_helper#edited_time_ago_with_tooltip helper work properly with notes.
   # See https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/10392/diffs#note_28719102
   alias_attribute :last_edited_at, :updated_at
@@ -341,7 +345,7 @@ class Note < ApplicationRecord
   end
 
   def visible_for?(user)
-    !cross_reference_not_visible_for?(user)
+    !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user)
   end
 
   def award_emoji?
@@ -493,6 +497,15 @@ class Note < ApplicationRecord
 
   private
 
+  def system_note_viewable_by?(user)
+    return true unless system_note_metadata
+
+    restriction = TYPES_RESTRICTED_BY_ABILITY[system_note_metadata.action.to_sym]
+    return Ability.allowed?(user, restriction, project) if restriction
+
+    true
+  end
+
   def keep_around_commit
     project.repository.keep_around(self.commit_id)
   end
diff --git a/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml b/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml
new file mode 100644
index 00000000000..78d87ef37a5
--- /dev/null
+++ b/changelogs/unreleased/security-filter-related-branches-from-activity-feed.yml
@@ -0,0 +1,6 @@
+---
+title: Remove notes regarding Related Branches from Issue activity feeds for guest
+  users
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 8770a5ee303..29746bbd863 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -1435,6 +1435,43 @@ describe Projects::IssuesController do
           expect { get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid } }.not_to exceed_query_limit(control_count)
         end
       end
+
+      context 'private project' do
+        let!(:branch_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) }
+        let!(:commit_note) { create(:discussion_note_on_issue, :system, noteable: issue, project: project) }
+        let!(:branch_note_meta) { create(:system_note_metadata, note: branch_note, action: "branch") }
+        let!(:commit_note_meta) { create(:system_note_metadata, note: commit_note, action: "commit") }
+
+        context 'user is allowed access' do
+          before do
+            project.add_user(user, :maintainer)
+          end
+
+          it 'displays all available notes' do
+            get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
+
+            expect(json_response.length).to eq(3)
+          end
+        end
+
+        context 'user is a guest' do
+          let(:json_response_note_ids) do
+            json_response.collect { |discussion| discussion["notes"] }.flatten
+              .collect { |note| note["id"].to_i }
+          end
+
+          before do
+            project.add_guest(user)
+          end
+
+          it 'does not display notes w/type listed in TYPES_RESTRICTED_BY_ACCESS_LEVEL' do
+            get :discussions, params: { namespace_id: project.namespace, project_id: project, id: issue.iid }
+
+            expect(json_response.length).to eq(2)
+            expect(json_response_note_ids).not_to include(branch_note.id)
+          end
+        end
+      end
     end
   end
 
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 3ab88b52568..9c06cd049f5 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -285,6 +285,70 @@ describe Note do
     end
   end
 
+  describe "#visible_for?" do
+    using RSpec::Parameterized::TableSyntax
+
+    let_it_be(:note) { create(:note) }
+    let_it_be(:user) { create(:user) }
+
+    where(:cross_reference_visible, :system_note_viewable, :result) do
+      true  | true  | false
+      false | true  | true
+      false | false | false
+    end
+
+    with_them do
+      it "returns expected result" do
+        expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible)
+
+        unless cross_reference_visible
+          expect(note).to receive(:system_note_viewable_by?)
+            .with(user).and_return(system_note_viewable)
+        end
+
+        expect(note.visible_for?(user)).to eq result
+      end
+    end
+  end
+
+  describe "#system_note_viewable_by?(user)" do
+    let_it_be(:note) { create(:note) }
+    let_it_be(:user) { create(:user) }
+    let!(:metadata) { create(:system_note_metadata, note: note, action: "branch") }
+
+    context "when system_note_metadata is not present" do
+      it "returns true" do
+        expect(note).to receive(:system_note_metadata).and_return(nil)
+
+        expect(note.send(:system_note_viewable_by?, user)).to be_truthy
+      end
+    end
+
+    context "system_note_metadata isn't of type 'branch'" do
+      before do
+        metadata.action = "not_a_branch"
+      end
+
+      it "returns true" do
+        expect(note.send(:system_note_viewable_by?, user)).to be_truthy
+      end
+    end
+
+    context "user doesn't have :download_code ability" do
+      it "returns false" do
+        expect(note.send(:system_note_viewable_by?, user)).to be_falsey
+      end
+    end
+
+    context "user has the :download_code ability" do
+      it "returns true" do
+        expect(Ability).to receive(:allowed?).with(user, :download_code, note.project).and_return(true)
+
+        expect(note.send(:system_note_viewable_by?, user)).to be_truthy
+      end
+    end
+  end
+
   describe "cross_reference_not_visible_for?" do
     let(:private_user)    { create(:user) }
     let(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }