diff --git a/app/models/ability.rb b/app/models/ability.rb
index a185448d5ea24dc8f41b268f7fa1de85fcc4671e..b15143c8c9c4b471cd60848113f5abdec9944763 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -26,6 +26,13 @@ def users_that_can_read_personal_snippet(users, snippet)
end
end
+ # A list of users that can read confidential notes in a project
+ def users_that_can_read_internal_notes(users, note_parent)
+ DeclarativePolicy.subject_scope do
+ users.select { |u| allowed?(u, :reporter_access, note_parent) }
+ end
+ end
+
# Returns an Array of Issues that can be read by the given user.
#
# issues - The issues to reduce down to those readable by the user.
diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb
index 99dbe464a7c8a947c1c0af933ec5a13003bf7964..9ee0fd1db1dee5779cb4005263d16502deb9d34c 100644
--- a/app/models/concerns/cache_markdown_field.rb
+++ b/app/models/concerns/cache_markdown_field.rb
@@ -172,7 +172,7 @@ def store_mentions!
refs = all_references(self.author)
references = {}
- references[:mentioned_users_ids] = refs.mentioned_user_ids.presence
+ references[:mentioned_users_ids] = mentioned_filtered_user_ids_for(refs)
references[:mentioned_groups_ids] = refs.mentioned_group_ids.presence
references[:mentioned_projects_ids] = refs.mentioned_project_ids.presence
@@ -185,6 +185,13 @@ def store_mentions!
true
end
+ # Overriden on objects that needs to filter
+ # mentioned users that cannot read them, for example,
+ # guest users that are referenced on a confidential note.
+ def mentioned_filtered_user_ids_for(refs)
+ refs.mentioned_user_ids.presence
+ end
+
def mentionable_attributes_changed?(changes = saved_changes)
return false unless is_a?(Mentionable)
diff --git a/app/models/concerns/participable.rb b/app/models/concerns/participable.rb
index 20743ebcb522d1dda6756b28f366f0403b976839..f59b5d1ecc8f4274802deedcf792b96b1d53623d 100644
--- a/app/models/concerns/participable.rb
+++ b/app/models/concerns/participable.rb
@@ -92,7 +92,13 @@ def filtered_participants_hash
end
def raw_participants(current_user = nil, verify_access: false)
- ext = Gitlab::ReferenceExtractor.new(project, current_user)
+ extractor = Gitlab::ReferenceExtractor.new(project, current_user)
+
+ # Used to extract references from confidential notes.
+ # Referenced users that cannot read confidential notes are
+ # later removed from participants array.
+ internal_notes_extractor = Gitlab::ReferenceExtractor.new(project, current_user)
+
participants = Set.new
process = [self]
@@ -107,6 +113,8 @@ def raw_participants(current_user = nil, verify_access: false)
source.class.participant_attrs.each do |attr|
if attr.respond_to?(:call)
+ ext = use_internal_notes_extractor_for?(source) ? internal_notes_extractor : extractor
+
source.instance_exec(current_user, ext, &attr)
else
process << source.__send__(attr) # rubocop:disable GitlabSecurity/PublicSend
@@ -121,7 +129,18 @@ def raw_participants(current_user = nil, verify_access: false)
end
end
- participants.merge(ext.users)
+ participants.merge(users_that_can_read_internal_notes(internal_notes_extractor))
+ participants.merge(extractor.users)
+ end
+
+ def use_internal_notes_extractor_for?(source)
+ source.is_a?(Note) && source.confidential?
+ end
+
+ def users_that_can_read_internal_notes(extractor)
+ return [] unless self.is_a?(Noteable) && self.try(:resource_parent)
+
+ Ability.users_that_can_read_internal_notes(extractor.users, self.resource_parent)
end
def source_visible_to_user?(source, user)
diff --git a/app/models/note.rb b/app/models/note.rb
index 41e45a8759f73bbc8bf093467dd918e1d4195c61..f2ddf0efe47cb93316bb4125680e25e77bcf90d6 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -665,6 +665,25 @@ def commands_changes
)
end
+ def mentioned_users(current_user = nil)
+ users = super
+
+ return users unless confidential?
+
+ Ability.users_that_can_read_internal_notes(users, resource_parent)
+ end
+
+ def mentioned_filtered_user_ids_for(references)
+ return super unless confidential?
+
+ user_ids = references.mentioned_user_ids.presence
+
+ return [] if user_ids.blank?
+
+ users = User.where(id: user_ids)
+ Ability.users_that_can_read_internal_notes(users, resource_parent).pluck(:id)
+ end
+
private
def system_note_viewable_by?(user)
diff --git a/ee/spec/models/concerns/ee/participable_spec.rb b/ee/spec/models/concerns/ee/participable_spec.rb
index b369893640969a710608d61654df631eeff995ad..79293578eecb0653d37acf277627f3b5445b761e 100644
--- a/ee/spec/models/concerns/ee/participable_spec.rb
+++ b/ee/spec/models/concerns/ee/participable_spec.rb
@@ -20,7 +20,7 @@
it 'returns the list of participants' do
expect(instance).to receive(:foo).and_return(user2)
expect(instance).to receive(:bar).and_return(user3)
- expect(instance).to receive(:group).and_return(group)
+ expect(instance).to receive(:group).thrice.and_return(group)
participants = instance.participants(user1)
expect(participants).to contain_exactly(user2, user3)
diff --git a/ee/spec/models/epic_spec.rb b/ee/spec/models/epic_spec.rb
index ae11d5e3c3c9e78fc23dda99100b7954574b7975..16ed27d4402305c14319da56d33d88c2783efc5a 100644
--- a/ee/spec/models/epic_spec.rb
+++ b/ee/spec/models/epic_spec.rb
@@ -1134,4 +1134,13 @@ def as_item(item)
end
end
end
+
+ describe '#participants' do
+ it_behaves_like 'issuable participants' do
+ let_it_be(:issuable_parent) { create(:group, :public) }
+ let_it_be_with_refind(:issuable) { create(:epic, group: issuable_parent) }
+
+ let(:params) { { noteable: issuable } }
+ end
+ end
end
diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb
index 5bd69ad9fad15ece90c4f2a2aa81f1f86dab0c34..422dd9a463bc8affd5ea23233b2c4913f48d0607 100644
--- a/spec/models/ability_spec.rb
+++ b/spec/models/ability_spec.rb
@@ -151,6 +151,38 @@ def users_for_snippet(snippet)
end
end
+ describe '.users_that_can_read_internal_note' do
+ shared_examples 'filtering users that can read internal note' do
+ let_it_be(:guest) { create(:user) }
+ let_it_be(:reporter) { create(:user) }
+
+ let(:users) { [reporter, guest] }
+
+ before do
+ parent.add_guest(guest)
+ parent.add_reporter(reporter)
+ end
+
+ it 'returns users that can read internal notes' do
+ result = described_class.users_that_can_read_internal_notes(users, parent)
+
+ expect(result).to match_array([reporter])
+ end
+ end
+
+ context 'for groups' do
+ it_behaves_like 'filtering users that can read internal note' do
+ let(:parent) { create(:group) }
+ end
+ end
+
+ context 'for projects' do
+ it_behaves_like 'filtering users that can read internal note' do
+ let(:parent) { create(:project) }
+ end
+ end
+ end
+
describe '.merge_requests_readable_by_user' do
context 'with an admin when admin mode is enabled', :enable_admin_mode do
it 'returns all merge requests' do
diff --git a/spec/models/concerns/participable_spec.rb b/spec/models/concerns/participable_spec.rb
index 99a3a0fb79a7db38005ad4419051bbd9431bc9ea..b92c7c52f0be5b1323853952f77d63d91de0e896 100644
--- a/spec/models/concerns/participable_spec.rb
+++ b/spec/models/concerns/participable_spec.rb
@@ -31,7 +31,7 @@
expect(instance).to receive(:foo).and_return(user2)
expect(instance).to receive(:bar).and_return(user3)
- expect(instance).to receive(:project).twice.and_return(project)
+ expect(instance).to receive(:project).thrice.and_return(project)
participants = instance.participants(user1)
@@ -66,7 +66,7 @@
expect(instance).to receive(:foo).and_return(other)
expect(other).to receive(:bar).and_return(user2)
- expect(instance).to receive(:project).twice.and_return(project)
+ expect(instance).to receive(:project).thrice.and_return(project)
expect(instance.participants(user1)).to eq([user2])
end
@@ -86,7 +86,7 @@
instance = model.new
- expect(instance).to receive(:project).twice.and_return(project)
+ expect(instance).to receive(:project).thrice.and_return(project)
instance.participants(user1)
@@ -138,7 +138,7 @@
allow(instance).to receive_message_chain(:model_name, :element) { 'class' }
expect(instance).to receive(:foo).and_return(user2)
expect(instance).to receive(:bar).and_return(user3)
- expect(instance).to receive(:project).twice.and_return(project)
+ expect(instance).to receive(:project).thrice.and_return(project)
participants = instance.visible_participants(user1)
@@ -159,7 +159,7 @@
allow(instance).to receive_message_chain(:model_name, :element) { 'class' }
allow(instance).to receive(:bar).and_return(user2)
- expect(instance).to receive(:project).twice.and_return(project)
+ expect(instance).to receive(:project).thrice.and_return(project)
expect(instance.visible_participants(user1)).to be_empty
end
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index d45a23a7ef8082107f0fb63ebbc75cfa5547790e..9f864afc2131d6ddd4534aaa1bd1096a7ff5c1c3 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -744,25 +744,11 @@
end
describe '#participants' do
- context 'using a public project' do
- let_it_be(:public_project) { create(:project, :public) }
- let_it_be(:issue) { create(:issue, project: public_project) }
-
- let!(:note1) do
- create(:note_on_issue, noteable: issue, project: public_project, note: 'a')
- end
-
- let!(:note2) do
- create(:note_on_issue, noteable: issue, project: public_project, note: 'b')
- end
-
- it 'includes the issue author' do
- expect(issue.participants).to include(issue.author)
- end
+ it_behaves_like 'issuable participants' do
+ let_it_be(:issuable_parent) { create(:project, :public) }
+ let_it_be_with_refind(:issuable) { create(:issue, project: issuable_parent) }
- it 'includes the authors of the notes' do
- expect(issue.participants).to include(note1.author, note2.author)
- end
+ let(:params) { { noteable: issuable, project: issuable_parent } }
end
context 'using a private project' do
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index 032f35cfc29af2e17de048546b6a2863b113754e..8e0f964d965364f49462f228925485d42d432d91 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -147,6 +147,34 @@
end
end
+ shared_examples 'participating by confidential note notification' do
+ context 'when user is mentioned on confidential note' do
+ let_it_be(:guest_1) { create(:user) }
+ let_it_be(:guest_2) { create(:user) }
+ let_it_be(:reporter) { create(:user) }
+
+ before do
+ issuable.resource_parent.add_guest(guest_1)
+ issuable.resource_parent.add_guest(guest_2)
+ issuable.resource_parent.add_reporter(reporter)
+ end
+
+ it 'only emails authorized users' do
+ confidential_note_text = "#{guest_1.to_reference} and #{guest_2.to_reference} and #{reporter.to_reference}"
+ note_text = "Mentions #{guest_2.to_reference}"
+ create(:note_on_issue, noteable: issuable, project_id: project.id, note: confidential_note_text, confidential: true)
+ create(:note_on_issue, noteable: issuable, project_id: project.id, note: note_text)
+ reset_delivered_emails!
+
+ notification_trigger
+
+ should_not_email(guest_1)
+ should_email(guest_2)
+ should_email(reporter)
+ end
+ end
+ end
+
shared_examples 'participating by assignee notification' do
it 'emails the participant' do
issuable.assignees << participant
@@ -736,6 +764,20 @@
let(:notification_target) { note }
let(:notification_trigger) { notification.new_note(note) }
end
+
+ context 'when note is confidential' do
+ let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned', confidential: true) }
+ let(:guest) { create(:user) }
+
+ it 'does not notify users that cannot read note' do
+ project.add_guest(guest)
+ reset_delivered_emails!
+
+ notification.new_note(note)
+
+ should_not_email(guest)
+ end
+ end
end
end
@@ -1376,6 +1418,11 @@
let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) }
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { issue }
let(:notification_trigger) { notification.reassigned_issue(issue, @u_disabled, [assignee]) }
@@ -1494,6 +1541,11 @@
let(:notification_target) { issue }
let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) }
end
+
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.removed_milestone_issue(issue, issue.author) }
+ end
end
context 'confidential issues' do
@@ -1616,6 +1668,11 @@
let(:notification_trigger) { notification.close_issue(issue, @u_disabled) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.close_issue(issue, @u_disabled) }
+ end
+
it 'adds "subscribed" reason to subscriber emails' do
user_1 = create(:user)
issue.subscribe(user_1)
@@ -1658,6 +1715,11 @@
let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) }
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { issue }
let(:notification_trigger) { notification.reopen_issue(issue, @u_disabled) }
@@ -1689,6 +1751,11 @@
let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) }
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { issue }
let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) }
@@ -1720,6 +1787,11 @@
let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) }
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { issue }
let(:notification_trigger) { notification.issue_cloned(issue, new_issue, @u_disabled) }
@@ -1765,6 +1837,11 @@
let(:notification_trigger) { notification.issue_due(issue) }
end
+ it_behaves_like 'participating by confidential note notification' do
+ let(:issuable) { issue }
+ let(:notification_trigger) { notification.issue_due(issue) }
+ end
+
it_behaves_like 'project emails are disabled' do
let(:notification_target) { issue }
let(:notification_trigger) { notification.issue_due(issue) }
diff --git a/spec/support/shared_examples/models/issuable_participants_shared_examples.rb b/spec/support/shared_examples/models/issuable_participants_shared_examples.rb
new file mode 100644
index 0000000000000000000000000000000000000000..c3eaae0ace2134e1ef36d732c83deb1208d4ade1
--- /dev/null
+++ b/spec/support/shared_examples/models/issuable_participants_shared_examples.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+RSpec.shared_examples 'issuable participants' do
+ context 'when resource parent is public' do
+ context 'and users are referenced on notes' do
+ let_it_be(:notes_author) { create(:user) }
+
+ let(:note_params) { params.merge(author: notes_author) }
+
+ before do
+ create(:note, note_params)
+ end
+
+ it 'includes the issue author' do
+ expect(issuable.participants).to include(issuable.author)
+ end
+
+ it 'includes the authors of the notes' do
+ expect(issuable.participants).to include(notes_author)
+ end
+
+ context 'and note is confidential' do
+ context 'and mentions users' do
+ let_it_be(:guest_1) { create(:user) }
+ let_it_be(:guest_2) { create(:user) }
+ let_it_be(:reporter) { create(:user) }
+
+ before do
+ issuable_parent.add_guest(guest_1)
+ issuable_parent.add_guest(guest_2)
+ issuable_parent.add_reporter(reporter)
+
+ confidential_note_params =
+ note_params.merge(
+ confidential: true,
+ note: "mentions #{guest_1.to_reference} and #{guest_2.to_reference} and #{reporter.to_reference}"
+ )
+
+ regular_note_params =
+ note_params.merge(note: "Mentions #{guest_2.to_reference}")
+
+ create(:note, confidential_note_params)
+ create(:note, regular_note_params)
+ end
+
+ it 'only includes users that can read the note as participants' do
+ expect(issuable.participants).to contain_exactly(issuable.author, notes_author, reporter, guest_2)
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/spec/support/shared_examples/models/mentionable_shared_examples.rb b/spec/support/shared_examples/models/mentionable_shared_examples.rb
index e23658d17749027a4f74669ba258ea1d3b705e17..f9612dd61beb625d6fcbeb33fdcb85e033a4ffab 100644
--- a/spec/support/shared_examples/models/mentionable_shared_examples.rb
+++ b/spec/support/shared_examples/models/mentionable_shared_examples.rb
@@ -260,6 +260,25 @@
expect(mentionable.referenced_projects(user)).to eq [mentionable.project].compact # epic.project is nil, and we want empty []
expect(mentionable.referenced_groups(user)).to eq [group]
end
+
+ if [:epic, :issue].include?(mentionable_type)
+ context 'and note is confidential' do
+ let_it_be(:guest) { create(:user) }
+
+ let(:note_desc) { "#{guest.to_reference} and #{user2.to_reference} and #{user.to_reference}" }
+
+ before do
+ note.resource_parent.add_reporter(user2)
+ note.resource_parent.add_guest(guest)
+ # Bypass :confidential update model validation for testing purposes
+ note.update_attribute(:confidential, true)
+ end
+
+ it 'returns only mentioned users that has permissions' do
+ expect(note.mentioned_users).to contain_exactly(user, user2)
+ end
+ end
+ end
end
end
@@ -294,6 +313,26 @@
end
end
+ if [:epic, :issue].include?(mentionable_type)
+ context 'and note is confidential' do
+ let_it_be(:guest) { create(:user) }
+
+ let(:note_desc) { "#{guest.to_reference} and #{mentioned_user.to_reference}" }
+
+ before do
+ note.resource_parent.add_reporter(mentioned_user)
+ note.resource_parent.add_guest(guest)
+ # Bypass :confidential update model validation for testing purposes
+ note.update_attribute(:confidential, true)
+ note.store_mentions!
+ end
+
+ it 'stores only mentioned users that has permissions' do
+ expect(mentionable.referenced_users).to contain_exactly(mentioned_user)
+ end
+ end
+ end
+
context 'when private projects and groups are mentioned' do
let(:mega_user) { create(:user) }
let(:private_project) { create(:project, :private) }