Skip to content
Snippets Groups Projects
Commit 30e98613 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin :two: Committed by Doug Stull
Browse files

Fix participants detection for system notes

Contributes to #409006

**Problem**

Previously we implemented an optimization to improve the performance of
participants detection.

Merge request:
!98116

Instead of checking permissions for each element on the page, we started
checking permissons for issuable object (issue, merge request, epic).

However, this approach doesn't work well for system notes. System note
requires a permission check on Note object itself. Checking it on
issuable will skip the verification for system note content.

**Solution**

Fallback to verifying permissions for system notes on Note objects.

Changelog: fixed
parent 63b08fd1
No related branches found
No related tags found
1 merge request!120883Fix participants detection for system notes
......@@ -654,6 +654,7 @@ def draftless_title_changed(old_title)
def read_ability_for(participable_source)
return super if participable_source == self
return super if participable_source.is_a?(Note) && participable_source.system?
name = participable_source.try(:issuable_ability_name) || :read_issuable_participables
......
......@@ -8,39 +8,54 @@
describe '#resolve' do
let_it_be(:user) { create(:user) }
let_it_be(:guest) { create(:user) }
let_it_be(:project) { create(:project, :public) }
let_it_be(:project) do
create(:project, :public).tap do |r|
r.add_developer(user)
r.add_guest(guest)
end
end
let_it_be(:private_project) { create(:project, :private).tap { |r| r.add_developer(user) } }
let_it_be(:issue) { create(:issue, project: project) }
let_it_be(:private_issue) { create(:issue, project: private_project) }
let_it_be(:public_note_author) { create(:user) }
let_it_be(:public_reply_author) { create(:user) }
let_it_be(:internal_note_author) { create(:user) }
let_it_be(:internal_reply_author) { create(:user) }
let_it_be(:system_note_author) { create(:user) }
let_it_be(:internal_system_note_author) { create(:user) }
let_it_be(:public_note) { create(:note, project: project, noteable: issue, author: public_note_author) }
let_it_be(:internal_note) { create(:note, :confidential, project: project, noteable: issue, author: internal_note_author) }
let_it_be(:public_reply) { create(:note, noteable: issue, in_reply_to: public_note, project: project, author: public_reply_author) }
let_it_be(:internal_reply) { create(:note, :confidential, noteable: issue, in_reply_to: internal_note, project: project, author: internal_reply_author) }
let_it_be(:note_metadata2) { create(:system_note_metadata, note: public_note) }
let_it_be(:public_reply) do
create(:note, noteable: issue, in_reply_to: public_note, project: project, author: public_reply_author)
end
let_it_be(:issue_emoji) { create(:award_emoji, name: 'thumbsup', awardable: issue) }
let_it_be(:note_emoji1) { create(:award_emoji, name: 'thumbsup', awardable: public_note) }
let_it_be(:note_emoji2) { create(:award_emoji, name: 'thumbsup', awardable: internal_note) }
let_it_be(:note_emoji3) { create(:award_emoji, name: 'thumbsup', awardable: public_reply) }
let_it_be(:note_emoji4) { create(:award_emoji, name: 'thumbsup', awardable: internal_reply) }
let_it_be(:internal_reply) do
create(:note, :confidential, noteable: issue, in_reply_to: internal_note, project: project, author: internal_reply_author)
end
let_it_be(:issue_emoji_author) { issue_emoji.user }
let_it_be(:public_note_emoji_author) { note_emoji1.user }
let_it_be(:internal_note_emoji_author) { note_emoji2.user }
let_it_be(:public_reply_emoji_author) { note_emoji3.user }
let_it_be(:internal_reply_emoji_author) { note_emoji4.user }
let_it_be(:issue_emoji_author) { create(:award_emoji, name: 'thumbsup', awardable: issue).user }
let_it_be(:public_note_emoji_author) { create(:award_emoji, name: 'thumbsup', awardable: public_note).user }
let_it_be(:internal_note_emoji_author) { create(:award_emoji, name: 'thumbsup', awardable: internal_note).user }
let_it_be(:public_reply_emoji_author) { create(:award_emoji, name: 'thumbsup', awardable: public_reply).user }
let_it_be(:internal_reply_emoji_author) { create(:award_emoji, name: 'thumbsup', awardable: internal_reply).user }
subject(:resolved_items) { resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items }
subject(:resolved_items) do
resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items
end
before do
project.add_guest(guest)
project.add_developer(user)
before_all do
create(:system_note, project: project, noteable: issue, author: system_note_author)
create(
:system_note,
note: "mentioned in issue #{private_issue.to_reference(full: true)}",
project: project, noteable: issue, author: internal_system_note_author
)
create(:system_note_metadata, note: public_note)
end
context 'when current user is not set' do
......@@ -54,7 +69,8 @@
public_note_author,
public_note_emoji_author,
public_reply_author,
public_reply_emoji_author
public_reply_emoji_author,
system_note_author
]
)
end
......@@ -71,7 +87,8 @@
public_note_author,
public_note_emoji_author,
public_reply_author,
public_reply_emoji_author
public_reply_emoji_author,
system_note_author
]
)
end
......@@ -92,13 +109,17 @@
internal_note_emoji_author,
internal_reply_author,
public_reply_emoji_author,
internal_reply_emoji_author
internal_reply_emoji_author,
system_note_author,
internal_system_note_author
]
)
end
context 'N+1 queries' do
let(:query) { -> { resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items } }
let(:query) do
-> { resolve(described_class, args: {}, ctx: { current_user: current_user }, obj: issue)&.items }
end
before do
# warm-up
......
......@@ -10,13 +10,14 @@
allow(model).to receive(:participant_attrs).and_return([:bar])
end
shared_examples 'check for participables read ability' do |ability_name|
shared_examples 'check for participables read ability' do |ability_name, ability_source: nil|
it 'receives expected ability' do
instance = model.new
source = ability_source == :participable_source ? participable_source : instance
allow(instance).to receive(:bar).and_return(participable_source)
expect(Ability).to receive(:allowed?).with(anything, ability_name, instance)
expect(Ability).to receive(:allowed?).with(anything, ability_name, source)
expect(instance.visible_participants(user1)).to be_empty
end
......@@ -39,4 +40,10 @@
it_behaves_like 'check for participables read ability', :read_internal_note
end
context 'when source is a system note' do
let(:participable_source) { build(:system_note) }
it_behaves_like 'check for participables read ability', :read_note, ability_source: :participable_source
end
end
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment