Commit fc83718a authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Merge branch '32639-use-note-policy-checks' into 'master'

Use note access check though its policy

See merge request gitlab-org/gitlab!25153
parents 14baa888 35ab5377
......@@ -137,7 +137,7 @@ module IssuableActions
end
notes = prepare_notes_for_rendering(notes)
notes = notes.select { |n| n.visible_for?(current_user) }
notes = notes.select { |n| n.readable_by?(current_user) }
discussions = Discussion.build_collection(notes, issuable)
......
......@@ -29,7 +29,7 @@ module NotesActions
end
notes = prepare_notes_for_rendering(notes)
notes = notes.select { |n| n.visible_for?(current_user) }
notes = notes.select { |n| n.readable_by?(current_user) }
notes_json[:notes] =
if use_note_serializer?
......
......@@ -19,7 +19,8 @@ class Discussion
:noteable_ability_name,
:to_ability_name,
:editable?,
:visible_for?,
:system_note_with_references_visible_for?,
:resource_parent,
to: :first_note
......
......@@ -223,7 +223,7 @@ class Note < ApplicationRecord
end
# rubocop: disable CodeReuse/ServiceClass
def cross_reference?
def system_note_with_references?
return unless system?
if force_cross_reference_regex_check?
......@@ -339,12 +339,10 @@ class Note < ApplicationRecord
super
end
def cross_reference_not_visible_for?(user)
cross_reference? && !all_referenced_mentionables_allowed?(user)
end
def visible_for?(user)
!cross_reference_not_visible_for?(user) && system_note_viewable_by?(user)
# This method is to be used for checking read permissions on a note instead of `system_note_with_references_visible_for?`
def readable_by?(user)
# note_policy accounts for #system_note_with_references_visible_for?(user) check when granting read access
Ability.allowed?(user, :read_note, self)
end
def award_emoji?
......@@ -504,6 +502,10 @@ class Note < ApplicationRecord
noteable.user_mentions.where(note: self)
end
def system_note_with_references_visible_for?(user)
(!system_note_with_references? || all_referenced_mentionables_allowed?(user)) && system_note_viewable_by?(user)
end
private
# Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception
......
# frozen_string_literal: true
class NotePolicy < BasePolicy
delegate { @subject.project }
delegate { @subject.resource_parent }
delegate { @subject.noteable if DeclarativePolicy.has_policy?(@subject.noteable) }
condition(:is_author) { @user && @subject.author == @user }
......@@ -11,7 +11,7 @@ class NotePolicy < BasePolicy
condition(:can_read_noteable) { can?(:"read_#{@subject.noteable_ability_name}") }
condition(:is_visible) { @subject.visible_for?(@user) }
condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) }
rule { ~editable }.prevent :admin_note
......
......@@ -7,6 +7,7 @@ class PersonalSnippetPolicy < BasePolicy
rule { public_snippet }.policy do
enable :read_snippet
enable :read_note
enable :create_note
end
......@@ -14,11 +15,13 @@ class PersonalSnippetPolicy < BasePolicy
enable :read_snippet
enable :update_snippet
enable :admin_snippet
enable :read_note
enable :create_note
end
rule { internal_snippet & ~external_user }.policy do
enable :read_snippet
enable :read_note
enable :create_note
end
......
......@@ -283,7 +283,7 @@ class NotificationService
return true unless note.noteable_type.present?
# ignore gitlab service messages
return true if note.cross_reference? && note.system?
return true if note.system_note_with_references?
send_new_note_notifications(note)
end
......
- return unless note.author
- return if note.cross_reference_not_visible_for?(current_user)
- return unless note.readable_by?(current_user)
- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false)
- note_editable = can?(current_user, :admin_note, note)
......
......@@ -275,12 +275,12 @@ module EE
def to_reference(from = nil, full: false)
reference = "#{self.class.reference_prefix}#{iid}"
return reference unless (cross_reference?(from) && !group.projects.include?(from)) || full
return reference unless (cross_referenced?(from) && !group.projects.include?(from)) || full
"#{group.full_path}#{reference}"
end
def cross_reference?(from)
def cross_referenced?(from)
from && from != group
end
......
......@@ -70,8 +70,8 @@ module EE
noteable&.after_note_destroyed(self)
end
override :visible_for?
def visible_for?(user)
override :system_note_with_references_visible_for?
def system_note_with_references_visible_for?(user)
return false unless super
return true unless system_note_for_epic? && created_before_noteable?
......@@ -82,7 +82,7 @@ module EE
private
def system_note_for_epic?
for_epic? && system?
system? && for_epic?
end
def created_before_noteable?
......
......@@ -87,7 +87,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.select { |n| n.visible_for?(current_user) }
notes.select { |n| n.readable_by?(current_user) }
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -39,7 +39,7 @@ describe Groups::Epics::NotesController do
context 'with cross-reference system note that is not visible to the current user', :request_store do
it "does not return any note" do
expect_any_instance_of(Note).to receive(:cross_reference_not_visible_for?).and_return(true)
expect_any_instance_of(Note).to receive(:readable_by?).and_return(false)
get :index, params: request_params
......
......@@ -11,13 +11,14 @@ describe Note do
let(:set_mentionable_text) { ->(txt) { subject.note = txt } }
end
describe '#visible_for?' do
describe '#readable_by?' 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(:non_member) { create(:user) }
let(:group) { create(:group) }
let(:group) { create(:group, :public) }
let(:epic) { create(:epic, group: group, author: owner, created_at: 1.day.ago) }
before do
......@@ -27,53 +28,55 @@ describe Note do
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
it_behaves_like 'users with note access' do
let(:users) { [owner, reporter, maintainer, guest, non_member, nil] }
end
it 'returns true for a reporter' do
expect(note.visible_for?(reporter)).to be_truthy
end
context 'when group is private' do
let(:group) { create(:group, :private) }
it 'returns true for a maintainer' do
expect(note.visible_for?(maintainer)).to be_truthy
it_behaves_like 'users with note access' do
let(:users) { [owner, reporter, maintainer, guest] }
end
it 'returns true for a guest user' do
expect(note.visible_for?(guest)).to be_truthy
it 'returns visible but not readable for a non-member user' do
expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
expect(note.readable_by?(non_member)).to be_falsy
end
it 'returns true for a nil user' do
expect(note.visible_for?(nil)).to be_truthy
it 'returns visible but not readable for a nil user' do
expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
expect(note.readable_by?(nil)).to be_falsy
end
end
end
context 'when note is older than epic' do
let(:older_note) { create(:system_note, noteable: epic, created_at: 2.days.ago) }
let(: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
it_behaves_like 'users with note access' do
let(:users) { [owner, reporter, maintainer] }
end
it 'returns true for a reporter' do
expect(older_note.visible_for?(reporter)).to be_truthy
it_behaves_like 'users without note access' do
let(:users) { [guest, non_member, nil] }
end
it 'returns true for a maintainer' do
expect(older_note.visible_for?(maintainer)).to be_truthy
end
context 'when group is private' do
let(:group) { create(:group, :private) }
it 'returns false for a guest user' do
expect(older_note.visible_for?(guest)).to be_falsy
it_behaves_like 'users with note access' do
let(:users) { [owner, reporter, maintainer] }
end
it 'returns false for a nil user' do
expect(older_note.visible_for?(nil)).to be_falsy
it_behaves_like 'users without note access' do
let(:users) { [guest, non_member, nil] }
end
end
end
end
describe '#cross_reference?' do
describe '#system_note_with_references?' do
[:relate_epic, :unrelate_epic].each do |type|
it "delegates #{type} system note to the cross-reference regex" do
note = create(:note, :system)
......@@ -81,7 +84,7 @@ describe Note do
expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
note.cross_reference?
note.system_note_with_references?
end
end
end
......
......@@ -230,7 +230,7 @@ module API
.fresh
# Without RendersActions#prepare_notes_for_rendering,
# Note#cross_reference_not_visible_for? will attempt to render
# Note#system_note_with_references_visible_for? will attempt to render
# Markdown references mentioned in the note to see whether they
# should be redacted. For notes that reference a commit, this
# would also incur a Gitaly call to verify the commit exists.
......@@ -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.select { |n| n.visible_for?(current_user) }
notes.select { |n| n.readable_by?(current_user) }
end
# rubocop: enable CodeReuse/ActiveRecord
end
......
......@@ -62,7 +62,7 @@ module API
def get_note(noteable, note_id)
note = noteable.notes.with_metadata.find(note_id)
can_read_note = note.visible_for?(current_user)
can_read_note = note.readable_by?(current_user)
if can_read_note
present note, with: Entities::Note
......
......@@ -45,7 +45,7 @@ module API
# array returned, but this is really a edge-case.
notes = paginate(raw_notes)
notes = prepare_notes_for_rendering(notes)
notes = notes.select { |note| note.visible_for?(current_user) }
notes = notes.select { |note| note.readable_by?(current_user) }
present notes, with: Entities::Note
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -18,7 +18,7 @@ module Banzai
issuables = extractor.extract([doc])
issuables.each do |node, issuable|
next if !can_read_cross_project? && cross_reference?(issuable)
next if !can_read_cross_project? && cross_referenced?(issuable)
if VISIBLE_STATES.include?(issuable.state) && issuable_reference?(node.inner_html, issuable)
state = moved_issue?(issuable) ? s_("IssuableStatus|moved") : issuable.state
......@@ -39,7 +39,7 @@ module Banzai
CGI.unescapeHTML(text) == issuable.reference_link_text(project || group)
end
def cross_reference?(issuable)
def cross_referenced?(issuable)
return true if issuable.project != project
return true if issuable.respond_to?(:group) && issuable.group != group
......
......@@ -108,7 +108,7 @@ describe Snippets::NotesController do
sign_in(user)
expect_any_instance_of(Note).to receive(:cross_reference_not_visible_for?).and_return(true)
expect_any_instance_of(Note).to receive(:readable_by?).and_return(false)
end
it "does not return any note" do
......
......@@ -285,28 +285,37 @@ describe Note do
end
end
describe "#visible_for?" do
using RSpec::Parameterized::TableSyntax
describe "#system_note_with_references_visible_for?" do
let(:project) { create(:project, :public) }
let(:user) { create(:user) }
let(:guest) { create(:project_member, :guest, project: project, user: create(:user)).user }
let(:reporter) { create(:project_member, :reporter, project: project, user: create(:user)).user }
let(:maintainer) { create(:project_member, :maintainer, project: project, user: create(:user)).user }
let(:non_member) { create(:user) }
let_it_be(:note) { create(:note) }
let_it_be(:user) { create(:user) }
let(:note) { create(:note, project: project) }
where(:cross_reference_visible, :system_note_viewable, :result) do
true | true | false
false | true | true
false | false | false
context 'when project is public' do
it_behaves_like 'users with note access' do
let(:users) { [reporter, maintainer, guest, non_member, nil] }
end
end
with_them do
it "returns expected result" do
expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible)
context 'when group is private' do
let(:project) { create(:project, :private) }
it_behaves_like 'users with note access' do
let(:users) { [reporter, maintainer, guest] }
end
unless cross_reference_visible
expect(note).to receive(:system_note_viewable_by?)
.with(user).and_return(system_note_viewable)
it 'returns visible but not readable for non-member user' do
expect(note.system_note_with_references_visible_for?(non_member)).to be_truthy
expect(note.readable_by?(non_member)).to be_falsy
end
expect(note.visible_for?(user)).to eq result
it 'returns visible but not readable for a nil user' do
expect(note.system_note_with_references_visible_for?(nil)).to be_truthy
expect(note.readable_by?(nil)).to be_falsy
end
end
end
......@@ -349,7 +358,7 @@ describe Note do
end
end
describe "cross_reference_not_visible_for?" do
describe "system_note_with_references_visible_for?" do
let_it_be(:private_user) { create(:user) }
let_it_be(:private_project) { create(:project, namespace: private_user.namespace) { |p| p.add_maintainer(private_user) } }
let_it_be(:private_issue) { create(:issue, project: private_project) }
......@@ -359,11 +368,11 @@ describe Note do
shared_examples "checks references" do
it "returns true" do
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end
it "returns false" do
expect(note.cross_reference_not_visible_for?(private_user)).to be_falsy
expect(note.system_note_with_references_visible_for?(private_user)).to be_truthy
end
it "returns false if user visible reference count set" do
......@@ -371,14 +380,14 @@ describe Note do
note.total_reference_count = 1
expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_falsy
expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_truthy
end
it "returns true if ref count is 0" do
note.user_visible_reference_count = 0
expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end
end
......@@ -423,16 +432,16 @@ describe Note do
note.total_reference_count = 2
expect(note).not_to receive(:reference_mentionables)
expect(note.cross_reference_not_visible_for?(ext_issue.author)).to be_truthy
expect(note.system_note_with_references_visible_for?(ext_issue.author)).to be_falsy
end
end
end
describe '#cross_reference?' do
describe '#system_note_with_references?' do
it 'falsey for user-generated notes' do
note = create(:note, system: false)
expect(note.cross_reference?).to be_falsy
expect(note.system_note_with_references?).to be_falsy
end
context 'when the note might contain cross references' do
......@@ -443,7 +452,7 @@ describe Note do
it 'delegates to the cross-reference regex' do
expect(note).to receive(:matches_cross_reference_regex?).and_return(false)
note.cross_reference?
note.system_note_with_references?
end
end
end
......@@ -453,8 +462,8 @@ describe Note do
let(:label_note) { build(:note, note: 'added ~2323232323', system: true) }
it 'scan for a `mentioned in` prefix' do
expect(commit_note.cross_reference?).to be_truthy
expect(label_note.cross_reference?).to be_falsy
expect(commit_note.system_note_with_references?).to be_truthy
expect(label_note.system_note_with_references?).to be_falsy
end
end
......@@ -468,7 +477,7 @@ describe Note do
it 'delegates to the system note service' do
expect(SystemNotes::IssuablesService).to receive(:cross_reference?).with(note.note)
note.cross_reference?
note.system_note_with_references?
end
end
......@@ -480,7 +489,7 @@ describe Note do
it 'delegates to the cross-reference regex' do
expect(note).to receive(:matches_cross_reference_regex?)
note.cross_reference?
note.system_note_with_references?
end
end
......@@ -489,13 +498,13 @@ describe Note do
it_behaves_like 'system_note_metadata includes note action'
it { expect(note.cross_reference?).to be_falsy }
it { expect(note.system_note_with_references?).to be_falsy }
context 'with cross reference label note' do
let(:label) { create(:label, project: issue.project)}
let(:note) { create(:system_note, note: "added #{label.to_reference} label", noteable: issue, project: issue.project) }
it { expect(note.cross_reference?).to be_truthy }
it { expect(note.system_note_with_references?).to be_truthy }
end
end
......@@ -504,13 +513,13 @@ describe Note do
it_behaves_like 'system_note_metadata includes note action'
it { expect(note.cross_reference?).to be_falsy }
it { expect(note.system_note_with_references?).to be_falsy }
context 'with cross reference milestone note' do
let(:milestone) { create(:milestone, project: issue.project)}
let(:note) { create(:system_note, note: "added #{milestone.to_reference} milestone", noteable: issue, project: issue.project) }
it { expect(note.cross_reference?).to be_truthy }
it { expect(note.system_note_with_references?).to be_truthy }
end
end
end
......
# frozen_string_literal: true
shared_examples 'users with note access' do
it 'returns true' do
users.each do |user|
expect(note.system_note_with_references_visible_for?(user)).to be_truthy
expect(note.readable_by?(user)).to be_truthy
end
end
end
shared_examples 'users without note access' do
it 'returns false' do
users.each do |user|
expect(note.system_note_with_references_visible_for?(user)).to be_falsy
expect(note.readable_by?(user)).to be_falsy
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment