From 9273ca68ea1e8f75ae29b073dc707042f3294809 Mon Sep 17 00:00:00 2001
From: Patrick Derichs <pderichs@gitlab.com>
Date: Tue, 20 Aug 2019 09:19:20 +0200
Subject: [PATCH] Filter out old system notes for epics
Add spec for note filtering on epics
---
app/controllers/concerns/issuable_actions.rb | 2 +-
app/controllers/concerns/notes_actions.rb | 2 +-
app/models/note.rb | 4 ++
ee/app/models/ee/note.rb | 21 ++++++++
...-api-reveals-historical-info-ee-master.yml | 5 ++
ee/lib/api/v3/github.rb | 2 +-
.../lib/ee/api/helpers/notes_helpers_spec.rb | 45 ++++++++++++++++
ee/spec/models/note_spec.rb | 54 +++++++++++++++++++
ee/spec/requests/api/notes_spec.rb | 32 +++++++++++
lib/api/discussions.rb | 2 +-
lib/api/helpers/notes_helpers.rb | 6 +--
lib/api/notes.rb | 2 +-
12 files changed, 169 insertions(+), 8 deletions(-)
create mode 100644 ee/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ee-master.yml
create mode 100644 ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index d920a31027c..bf5384d5ac8 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/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)
diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb
index d2a961efff7..80be7095ed3 100644
--- a/app/controllers/concerns/notes_actions.rb
+++ b/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?
diff --git a/app/models/note.rb b/app/models/note.rb
index 215e8883691..c31c514c160 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -331,6 +331,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
diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb
index acb153850e4..cbffbdc2b03 100644
--- a/ee/app/models/ee/note.rb
+++ b/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
diff --git a/ee/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ee-master.yml b/ee/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ee-master.yml
new file mode 100644
index 00000000000..c639098721e
--- /dev/null
+++ b/ee/changelogs/unreleased/security-epic-notes-api-reveals-historical-info-ee-master.yml
@@ -0,0 +1,5 @@
+---
+title: Filter out old system notes for epics in notes api endpoint response
+merge_request:
+author:
+type: security
diff --git a/ee/lib/api/v3/github.rb b/ee/lib/api/v3/github.rb
index 80c568067dd..6645249146d 100644
--- a/ee/lib/api/v3/github.rb
+++ b/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
diff --git a/ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb b/ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb
new file mode 100644
index 00000000000..b11c2231330
--- /dev/null
+++ b/ee/spec/lib/ee/api/helpers/notes_helpers_spec.rb
@@ -0,0 +1,45 @@
+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
diff --git a/ee/spec/models/note_spec.rb b/ee/spec/models/note_spec.rb
index 29c71ad584c..3bd9c4fb149 100644
--- a/ee/spec/models/note_spec.rb
+++ b/ee/spec/models/note_spec.rb
@@ -10,4 +10,58 @@ 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
+ 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
+ end
+ end
end
diff --git a/ee/spec/requests/api/notes_spec.rb b/ee/spec/requests/api/notes_spec.rb
index b85a4d50741..b87b27c563a 100644
--- a/ee/spec/requests/api/notes_spec.rb
+++ b/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
diff --git a/lib/api/discussions.rb b/lib/api/discussions.rb
index 6c1acc3963f..9125207167c 100644
--- a/lib/api/discussions.rb
+++ b/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
diff --git a/lib/api/helpers/notes_helpers.rb b/lib/api/helpers/notes_helpers.rb
index c938cb8a592..8adfac346f6 100644
--- a/lib/api/helpers/notes_helpers.rb
+++ b/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
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index 84563d66ee8..16fca9acccb 100644
--- a/lib/api/notes.rb
+++ b/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
--
2.30.9