Use note access check though its policy

Usage of note.visible_for?(user) for read(view) permission check
is not complete. This change replaces that check with an actual
note policy check for actuall permission checks.

app/controllers/concerns/issuable_actions.rb

@@ -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)
 

app/controllers/concerns/notes_actions.rb

@@ -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?

app/models/discussion.rb

@@ -20,6 +20,7 @@ class Discussion
             :to_ability_name,
             :editable?,
             :visible_for?,
+            :resource_parent,
 
             to: :first_note
 

app/models/note.rb

@@ -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 `visible_for?`
+  def readable_by?(user)
+    # note_policy accounts for #visible_for?(user) check when granting read access
+    Ability.allowed?(user, :read_note, self)
   end
 
   def award_emoji?
@@ -504,6 +502,16 @@ class Note < ApplicationRecord
     noteable.user_mentions.where(note: self)
   end
 
+  def cross_reference_not_visible_for?(user)
+    cross_reference? && !all_referenced_mentionables_allowed?(user)
+  end
+
+  # Using this method for checking `read` access on a note may result in incorrect permission granting
+  # check `spec/models/note_spec.rb` and `ee/spec/models/note_spec.rb` for corresponding specs
+  def visible_for?(user)
+    !cross_reference_not_visible_for?(user) && system_note_viewable_by?(user)
+  end
+
   private
 
   # Using this method followed by a call to `save` may result in ActiveRecord::RecordNotUnique exception

app/policies/note_policy.rb

 # 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 }

app/policies/personal_snippet_policy.rb

@@ -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
 

app/views/shared/notes/_note.html.haml

 - return unless note.author
-- return if note.cross_reference_not_visible_for?(current_user)
+- return if !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)

ee/lib/api/v3/github.rb

@@ -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
 

ee/spec/models/note_spec.rb

@@ -16,8 +16,9 @@ describe Note do
     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,48 +28,50 @@ 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
-      end
+        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
-      end
+        it 'returns visible but not readable for a non-member user' do
+          expect(note.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.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
-      end
+        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

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.select { |n| n.visible_for?(current_user) }
+        notes.select { |n| n.readable_by?(current_user) }
       end
       # rubocop: enable CodeReuse/ActiveRecord
     end

lib/api/helpers/notes_helpers.rb

@@ -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

lib/api/notes.rb

@@ -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

spec/models/note_spec.rb

@@ -288,8 +288,14 @@ describe Note do
   describe "#visible_for?" do
     using RSpec::Parameterized::TableSyntax
 
-    let_it_be(:note) { create(:note) }
-    let_it_be(:user) { create(:user) }
+    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(:note) { create(:note, project: project) }
 
     where(:cross_reference_visible, :system_note_viewable, :result) do
       true  | true  | false
@@ -299,14 +305,39 @@ describe Note do
 
     with_them do
       it "returns expected result" do
-        expect(note).to receive(:cross_reference_not_visible_for?).and_return(cross_reference_visible)
+        expect(note).to receive(:cross_reference_not_visible_for?).twice.and_return(cross_reference_visible)
 
         unless cross_reference_visible
-          expect(note).to receive(:system_note_viewable_by?)
-            .with(user).and_return(system_note_viewable)
+          expect(note).to receive(:system_note_viewable_by?).twice
+                            .with(user).and_return(system_note_viewable)
         end
 
         expect(note.visible_for?(user)).to eq result
+        expect(note.readable_by?(user)).to eq result
+      end
+    end
+
+    context 'when project is public' do
+      it_behaves_like 'users with note access' do
+        let(:users) { [reporter, maintainer, guest, non_member, nil] }
+      end
+    end
+
+    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
+
+      it 'returns visible but not readable for non-member user' do
+        expect(note.visible_for?(non_member)).to be_truthy
+        expect(note.readable_by?(non_member)).to be_falsy
+      end
+
+      it 'returns visible but not readable for a nil user' do
+        expect(note.visible_for?(nil)).to be_truthy
+        expect(note.readable_by?(nil)).to be_falsy
       end
     end
   end

spec/support/shared_examples/models/note_access_check_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'users with note access' do
+  it 'returns true' do
+    users.each do |user|
+      expect(note.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.visible_for?(user)).to be_falsy
+      expect(note.readable_by?(user)).to be_falsy
+    end
+  end
+end