Merge branch 'security-epic-notes-api-reveals-historical-info-ee-master' into 'master'

Filter out old system notes for epics in notes api endpoint response

See merge request gitlab/gitlab-ee!1057

app/controllers/concerns/issuable_actions.rb

@@ -110,7 +110,7 @@ module IssuableActions
     end
 
     notes = prepare_notes_for_rendering(notes)
-    notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+    notes = notes.select { |n| n.visible_for?(current_user) }
 
     discussions = Discussion.build_collection(notes, issuable)
 

app/controllers/concerns/notes_actions.rb

@@ -29,7 +29,7 @@ module NotesActions
     end
 
     notes = prepare_notes_for_rendering(notes)
-    notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+    notes = notes.select { |n| n.visible_for?(current_user) }
 
     notes_json[:notes] =
       if use_note_serializer?

app/models/group.rb

@@ -365,6 +365,8 @@ class Group < Namespace
   end
 
   def max_member_access_for_user(user)
+    return GroupMember::NO_ACCESS unless user
+
     return GroupMember::OWNER if user.admin?
 
     members_with_parents

app/models/note.rb

@@ -332,6 +332,10 @@ class Note < ApplicationRecord
     cross_reference? && !all_referenced_mentionables_allowed?(user)
   end
 
+  def visible_for?(user)
+    !cross_reference_not_visible_for?(user)
+  end
+
   def award_emoji?
     can_be_award_emoji? && contains_emoji_only?
   end

ee/app/models/ee/note.rb

@@ -56,8 +56,29 @@ module EE
       for_epic? ? noteable.group : super
     end
 
+    override :visible_for?
+    def visible_for?(user)
+      return false unless super
+
+      return true unless system_note_for_epic? && created_before_noteable?
+
+      group_reporter?(user, noteable.group)
+    end
+
     private
 
+    def system_note_for_epic?
+      for_epic? && system?
+    end
+
+    def created_before_noteable?
+      created_at.to_i < noteable.created_at.to_i
+    end
+
+    def group_reporter?(user, group)
+      group.max_member_access_for_user(user) >= ::Gitlab::Access::REPORTER
+    end
+
     def banzai_context_params
       { group: noteable.group, label_url_method: :group_epics_url }
     end

ee/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ee-master.yml

+---
+title: Filter out old system notes for epics in notes api endpoint response
+merge_request:
+author:
+type: security

ee/lib/api/v3/github.rb

@@ -83,7 +83,7 @@ module API
           # They're not presented on Jira Dev Panel ATM. A comments count with a
           # redirect link is presented.
           notes = paginate(noteable.notes.user.reorder(nil))
-          notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          notes.select { |n| n.visible_for?(current_user) }
         end
         # rubocop: enable CodeReuse/ActiveRecord
 

ee/spec/features/groups/hooks/user_tests_hooks_spec.rb

 require "rails_helper"
 
 describe "User tests hooks", :js do
+  include StubRequests
+
   let!(:group) { create(:group) }
   let!(:hook) { create(:group_hook, group: group) }
   let!(:user) { create(:user) }
@@ -29,7 +31,7 @@ describe "User tests hooks", :js do
 
     context "when URL is invalid" do
       before do
-        stub_request(:post, hook.url).to_raise(SocketError.new("Failed to open"))
+        stub_full_request(hook.url, method: :post).to_raise(SocketError.new("Failed to open"))
 
         click_link("Test")
       end
@@ -51,7 +53,7 @@ describe "User tests hooks", :js do
   private
 
   def trigger_hook
-    stub_request(:post, hook.url).to_return(status: 200)
+    stub_full_request(hook.url, method: :post).to_return(status: 200)
 
     click_link("Test")
   end

ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb

+require 'spec_helper'
+
+describe 'NotesHelpers' do
+  describe '#find_noteable' do
+    let!(:group) { create(:group, :public) }
+    let!(:other_group) { create(:group, :public) }
+    let!(:project) { create(:project, :public, namespace: group) }
+    let!(:user) { create(:group_member, :owner, group: group, user: create(:user)).user }
+    let!(:epic) { create(:epic, author: user, group: group) }
+    let!(:parent_id) { group.id }
+    let!(:noteable_type) { Epic }
+
+    let(:klazz) do
+      klazz = Class.new do
+        def initialize(user)
+          @user = user
+        end
+
+        def current_user
+          @user
+        end
+
+        def can?(user, ability, noteable)
+          user == @user && ability == :read_epic
+        end
+      end
+
+      klazz.prepend(API::Helpers::NotesHelpers)
+    end
+
+    let(:subject) { klazz.new(user) }
+
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    it 'returns the expected epic' do
+      expect(subject.find_noteable(Group, parent_id, noteable_type, epic.id)).to eq(epic)
+    end
+
+    it 'raises not found exception when epic does not belong to group' do
+      expect { subject.find_noteable(Group, other_group.id, noteable_type, epic.id) }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+end

ee/spec/models/note_spec.rb

@@ -10,4 +10,66 @@ describe Note do
     let(:backref_text) { issue.gfm_reference }
     let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
   end
+
+  describe '#visible_for?' do
+    let(:owner) { create(:group_member, :owner, group: group, user: create(:user)).user }
+    let(:guest) { create(:group_member, :guest, group: group, user: create(:user)).user }
+    let(:reporter) { create(:group_member, :reporter, group: group, user: create(:user)).user }
+    let(:maintainer) { create(:group_member, :maintainer, group: group, user: create(:user)).user }
+
+    let(:group) { create(:group) }
+    let(:epic) { create(:epic, group: group, author: owner, created_at: 1.day.ago) }
+
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    context 'note created after epic' do
+      let(:note) { create(:system_note, noteable: epic, created_at: 1.minute.ago) }
+
+      it 'returns true for an owner' do
+        expect(note.visible_for?(owner)).to be_truthy
+      end
+
+      it 'returns true for a reporter' do
+        expect(note.visible_for?(reporter)).to be_truthy
+      end
+
+      it 'returns true for a maintainer' do
+        expect(note.visible_for?(maintainer)).to be_truthy
+      end
+
+      it 'returns true for a guest user' do
+        expect(note.visible_for?(guest)).to be_truthy
+      end
+
+      it 'returns true for a nil user' do
+        expect(note.visible_for?(nil)).to be_truthy
+      end
+    end
+
+    context 'when note is older than epic' do
+      let(:older_note) { create(:system_note, noteable: epic, created_at: 2.days.ago) }
+
+      it 'returns true for the owner' do
+        expect(older_note.visible_for?(owner)).to be_truthy
+      end
+
+      it 'returns true for a reporter' do
+        expect(older_note.visible_for?(reporter)).to be_truthy
+      end
+
+      it 'returns true for a maintainer' do
+        expect(older_note.visible_for?(maintainer)).to be_truthy
+      end
+
+      it 'returns false for a guest user' do
+        expect(older_note.visible_for?(guest)).to be_falsy
+      end
+
+      it 'returns false for a nil user' do
+        expect(older_note.visible_for?(nil)).to be_falsy
+      end
+    end
+  end
 end

ee/spec/requests/api/notes_spec.rb

@@ -26,5 +26,37 @@ describe API::Notes do
       let(:noteable) { epic }
       let(:note) { epic_note }
     end
+
+    context 'when issue was promoted to epic' do
+      let!(:promoted_issue_epic) { create(:epic, group: group, author: owner, created_at: 1.day.ago) }
+      let!(:owner) { create(:group_member, :owner, user: create(:user), group: group).user }
+      let!(:reporter) { create(:group_member, :reporter, user: create(:user), group: group).user }
+      let!(:guest) { create(:group_member, :guest, user: create(:user), group: group).user }
+      let!(:previous_note) { create(:note, :system, noteable: promoted_issue_epic, created_at: 2.days.ago) }
+      let!(:previous_note2) { create(:note, :system, noteable: promoted_issue_epic, created_at: 2.minutes.ago) }
+      let!(:epic_note) { create(:note, noteable: promoted_issue_epic, author: owner) }
+
+      context 'when user is reporter' do
+        it 'returns previous issue system notes' do
+          get api("/groups/#{group.id}/epics/#{promoted_issue_epic.id}/notes", reporter)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response.size).to eq(3)
+        end
+      end
+
+      context 'when user is guest' do
+        it 'does not return previous issue system notes' do
+          get api("/groups/#{group.id}/epics/#{promoted_issue_epic.id}/notes", guest)
+
+          expect(response).to have_gitlab_http_status(200)
+          expect(response).to include_pagination_headers
+          expect(json_response).to be_an Array
+          expect(json_response.size).to eq(2)
+        end
+      end
+    end
   end
 end

lib/api/discussions.rb

@@ -239,7 +239,7 @@ module API
         # because notes are redacted if they point to projects that
         # cannot be accessed by the user.
         notes = prepare_notes_for_rendering(notes)
-        notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+        notes.select { |n| n.visible_for?(current_user) }
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/api/helpers/notes_helpers.rb

@@ -12,7 +12,7 @@ module API
       end
 
       def update_note(noteable, note_id)
-        note = noteable.notes.find(params[:note_id])
+        note = noteable.notes.find(note_id)
 
         authorize! :admin_note, note
 
@@ -61,8 +61,8 @@ module API
       end
 
       def get_note(noteable, note_id)
-        note = noteable.notes.with_metadata.find(params[:note_id])
-        can_read_note = !note.cross_reference_not_visible_for?(current_user)
+        note = noteable.notes.with_metadata.find(note_id)
+        can_read_note = note.visible_for?(current_user)
 
         if can_read_note
           present note, with: Entities::Note

lib/api/notes.rb

@@ -42,7 +42,7 @@ module API
           # array returned, but this is really a edge-case.
           notes = paginate(raw_notes)
           notes = prepare_notes_for_rendering(notes)
-          notes = notes.reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          notes = notes.select { |note| note.visible_for?(current_user) }
           present notes, with: Entities::Note
         end
         # rubocop: enable CodeReuse/ActiveRecord

spec/factories/sequences.rb

@@ -7,7 +7,7 @@ FactoryBot.define do
   sequence(:email_alias) { |n| "user.alias#{n}@example.org" }
   sequence(:title) { |n| "My title #{n}" }
   sequence(:filename) { |n| "filename-#{n}.rb" }
-  sequence(:url) { |n| "http://example#{n}.org" }
+  sequence(:url) { |n| "http://example#{n}.test" }
   sequence(:label_title) { |n| "label#{n}" }
   sequence(:branch) { |n| "my-branch-#{n}" }
   sequence(:past_time) { |n| 4.hours.ago + (2 * n).seconds }

spec/models/ci/pipeline_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Ci::Pipeline, :mailer do
   include ProjectForksHelper
+  include StubRequests
 
   let(:user) { create(:user) }
   set(:project) { create(:project) }
@@ -2504,7 +2505,7 @@ describe Ci::Pipeline, :mailer do
       let(:enabled) { true }
 
       before do
-        WebMock.stub_request(:post, hook.url)
+        stub_full_request(hook.url, method: :post)
       end
 
       context 'with multiple builds' do
@@ -2558,7 +2559,7 @@ describe Ci::Pipeline, :mailer do
         end
 
         def have_requested_pipeline_hook(status)
-          have_requested(:post, hook.url).with do |req|
+          have_requested(:post, stubbed_hostname(hook.url)).with do |req|
             json_body = JSON.parse(req.body)
             json_body['object_attributes']['status'] == status &&
               json_body['builds'].length == 2

spec/services/web_hook_service_spec.rb

@@ -55,31 +55,38 @@ describe WebHookService do
   describe '#execute' do
     before do
       project.hooks << [project_hook]
-
-      WebMock.stub_request(:post, project_hook.url)
     end
 
     context 'when token is defined' do
       let(:project_hook) { create(:project_hook, :token) }
 
       it 'POSTs to the webhook URL' do
+        stub_full_request(project_hook.url, method: :post)
+
         service_instance.execute
-        expect(WebMock).to have_requested(:post, project_hook.url).with(
+
+        expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with(
           headers: headers.merge({ 'X-Gitlab-Token' => project_hook.token })
         ).once
       end
     end
 
     it 'POSTs to the webhook URL' do
+      stub_full_request(project_hook.url, method: :post)
+
       service_instance.execute
-      expect(WebMock).to have_requested(:post, project_hook.url).with(
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with(
         headers: headers
       ).once
     end
 
     it 'POSTs the data as JSON' do
+      stub_full_request(project_hook.url, method: :post)
+
       service_instance.execute
-      expect(WebMock).to have_requested(:post, project_hook.url).with(
+
+      expect(WebMock).to have_requested(:post, stubbed_hostname(project_hook.url)).with(
         headers: headers
       ).once
     end
@@ -115,7 +122,7 @@ describe WebHookService do
     end
 
     it 'catches exceptions' do
-      WebMock.stub_request(:post, project_hook.url).to_raise(StandardError.new('Some error'))
+      stub_full_request(project_hook.url, method: :post).to_raise(StandardError.new('Some error'))
 
       expect { service_instance.execute }.to raise_error(StandardError)
     end
@@ -125,20 +132,20 @@ describe WebHookService do
       exceptions.each do |exception_class|
         exception = exception_class.new('Exception message')
 
-        WebMock.stub_request(:post, project_hook.url).to_raise(exception)
+        stub_full_request(project_hook.url, method: :post).to_raise(exception)
         expect(service_instance.execute).to eq({ status: :error, message: exception.to_s })
         expect { service_instance.execute }.not_to raise_error
       end
     end
 
     it 'handles 200 status code' do
-      WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success')
+      stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success')
 
       expect(service_instance.execute).to include({ status: :success, http_status: 200, message: 'Success' })
     end
 
     it 'handles 2xx status codes' do
-      WebMock.stub_request(:post, project_hook.url).to_return(status: 201, body: 'Success')
+      stub_full_request(project_hook.url, method: :post).to_return(status: 201, body: 'Success')
 
       expect(service_instance.execute).to include({ status: :success, http_status: 201, message: 'Success' })
     end
@@ -148,7 +155,7 @@ describe WebHookService do
 
       context 'with success' do
         before do
-          WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: 'Success')
+          stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: 'Success')
           service_instance.execute
         end
 
@@ -165,7 +172,7 @@ describe WebHookService do
 
       context 'with exception' do
         before do
-          WebMock.stub_request(:post, project_hook.url).to_raise(SocketError.new('Some HTTP Post error'))
+          stub_full_request(project_hook.url, method: :post).to_raise(SocketError.new('Some HTTP Post error'))
           service_instance.execute
         end
 
@@ -182,7 +189,7 @@ describe WebHookService do
 
       context 'with unsafe response body' do
         before do
-          WebMock.stub_request(:post, project_hook.url).to_return(status: 200, body: "\xBB")
+          stub_full_request(project_hook.url, method: :post).to_return(status: 200, body: "\xBB")
           service_instance.execute
         end
 
@@ -202,7 +209,7 @@ describe WebHookService do
         let(:service_instance) { described_class.new(service_hook, data, 'service_hook') }
 
         before do
-          WebMock.stub_request(:post, service_hook.url).to_return(status: 200, body: 'Success')
+          stub_full_request(service_hook.url, method: :post).to_return(status: 200, body: 'Success')
         end
 
         it { expect { service_instance.execute }.not_to change(WebHookLog, :count) }