Merge branch '207469-confidential-note-to-policy' into 'master'

Take confidential attr into account in NotePolicy

See merge request gitlab-org/gitlab!26766

app/models/discussion.rb

@@ -14,6 +14,7 @@ class Discussion
             :author,
             :noteable,
             :commit_id,
+            :confidential?,
             :for_commit?,
             :for_merge_request?,
             :noteable_ability_name,

app/models/note.rb

@@ -320,6 +320,13 @@ class Note < ApplicationRecord
     super(noteable_type.to_s.classify.constantize.base_class.to_s)
   end
 
+  def noteable_assignee_or_author?(user)
+    return false unless user
+    return noteable.assignee_or_author?(user) if [MergeRequest, Issue].include?(noteable.class)
+
+    noteable.author_id == user.id
+  end
+
   def special_role=(role)
     raise "Role is undefined, #{role} not found in #{SpecialRole.values}" unless SpecialRole.value?(role)
 
@@ -337,7 +344,7 @@ class Note < ApplicationRecord
   end
 
   def confidential?
-    noteable.try(:confidential?)
+    confidential || noteable.try(:confidential?)
   end
 
   def editable?

app/policies/note_policy.rb

 # frozen_string_literal: true
 
 class NotePolicy < BasePolicy
+  include Gitlab::Utils::StrongMemoize
+
   delegate { @subject.resource_parent }
   delegate { @subject.noteable if DeclarativePolicy.has_policy?(@subject.noteable) }
 
@@ -13,6 +15,12 @@ class NotePolicy < BasePolicy
 
   condition(:is_visible) { @subject.system_note_with_references_visible_for?(@user) }
 
+  condition(:confidential, scope: :subject) { @subject.confidential? }
+
+  condition(:can_read_confidential) do
+    access_level >= Gitlab::Access::REPORTER || @subject.noteable_assignee_or_author?(@user)
+  end
+
   rule { ~editable }.prevent :admin_note
 
   # If user can't read the issue/MR/etc then they should not be allowed to do anything to their own notes
@@ -39,4 +47,37 @@ class NotePolicy < BasePolicy
   rule { is_noteable_author }.policy do
     enable :resolve_note
   end
+
+  rule { confidential & ~can_read_confidential }.policy do
+    prevent :read_note
+    prevent :admin_note
+    prevent :resolve_note
+    prevent :award_emoji
+  end
+
+  def parent_namespace
+    strong_memoize(:parent_namespace) do
+      next if @subject.is_a?(PersonalSnippet)
+      next @subject.noteable.group if @subject.noteable&.is_a?(Epic)
+
+      @subject.project
+    end
+  end
+
+  def access_level
+    return -1 if @user.nil?
+    return -1 unless parent_namespace
+
+    lookup_access_level!
+  end
+
+  def lookup_access_level!
+    return ::Gitlab::Access::REPORTER if alert_bot?
+
+    if parent_namespace.is_a?(Project)
+      parent_namespace.team.max_member_access(@user.id)
+    else
+      parent_namespace.max_member_access_for_user(@user)
+    end
+  end
 end

ee/spec/policies/note_policy_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe NotePolicy do
+  describe '#rules' do
+    let(:reporter) { create(:user) }
+    let(:developer) { create(:user) }
+    let(:maintainer) { create(:user) }
+    let(:guest) { create(:user) }
+    let(:non_member) { create(:user) }
+    let(:author) { create(:user) }
+    let(:assignee) { create(:user) }
+
+    let(:group) { create(:group) }
+    let(:epic) { create(:epic, group: group, author: author) }
+    let(:note) { create(:note, :on_epic, noteable: epic) }
+
+    before do
+      stub_licensed_features(epics: true)
+
+      group.add_reporter(reporter)
+      group.add_developer(developer)
+      group.add_maintainer(maintainer)
+      group.add_guest(guest)
+    end
+
+    def permissions(user)
+      described_class.new(user, note)
+    end
+
+    shared_examples_for 'private notes' do
+      it 'does not allow non members to read notes' do
+        expect(permissions(non_member)).to be_disallowed(:read_note, :admin_note)
+      end
+
+      it 'allows reporter to read notes' do
+        expect(permissions(reporter)).to be_allowed(:read_note)
+        expect(permissions(reporter)).to be_disallowed(:admin_note)
+      end
+
+      it 'allows developer to read notes' do
+        expect(permissions(developer)).to be_allowed(:read_note)
+        expect(permissions(developer)).to be_disallowed(:admin_note)
+      end
+
+      it 'allows maintainers to read notes and admin them' do
+        expect(permissions(maintainer)).to be_allowed(:read_note, :admin_note)
+      end
+    end
+
+    context 'for epics in a public group' do
+      context 'with non-confidential notes' do
+        let(:note) { create(:note, :on_epic, noteable: epic) }
+
+        it 'allows non members to read notes' do
+          expect(permissions(non_member)).to be_allowed(:read_note)
+          expect(permissions(non_member)).to be_disallowed(:admin_note)
+        end
+
+        it 'allows guests only to read notes' do
+          expect(permissions(guest)).to be_allowed(:read_note)
+          expect(permissions(guest)).to be_disallowed(:admin_note)
+        end
+
+        it 'allows reporters only to read notes' do
+          expect(permissions(reporter)).to be_allowed(:read_note)
+          expect(permissions(reporter)).to be_disallowed(:admin_note)
+        end
+
+        it 'allows developers only to read notes' do
+          expect(permissions(developer)).to be_allowed(:read_note)
+          expect(permissions(developer)).to be_disallowed(:admin_note)
+        end
+
+        it 'allows maintainers to read notes and admin them' do
+          expect(permissions(maintainer)).to be_allowed(:read_note, :admin_note)
+        end
+
+        it 'allows noteable author to read notes' do
+          expect(permissions(author)).to be_allowed(:read_note)
+          expect(permissions(author)).to be_disallowed(:admin_note)
+        end
+      end
+
+      context 'with confidential notes' do
+        let(:note) { create(:note, :confidential, :on_epic, noteable: epic) }
+
+        it_behaves_like 'private notes'
+
+        it 'does not allow guests to read confidential notes and replies' do
+          expect(permissions(guest)).to be_disallowed(:read_note, :admin_note)
+        end
+
+        it 'allows noteable author to read all notes' do
+          expect(permissions(author)).to be_allowed(:read_note)
+          expect(permissions(author)).to be_disallowed(:admin_note)
+        end
+      end
+    end
+
+    context 'for epics in a private group' do
+      before do
+        group.update(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it_behaves_like 'private notes'
+
+      it 'does not allow guests to read notes' do
+        expect(permissions(guest)).to be_allowed(:read_note)
+        expect(permissions(guest)).to be_disallowed(:admin_note)
+      end
+    end
+  end
+end

spec/factories/notes.rb

@@ -164,6 +164,10 @@ FactoryBot.define do
       attachment { fixture_file_upload("spec/fixtures/git-cheat-sheet.pdf", "application/pdf") }
     end
 
+    trait :confidential do
+      confidential { true }
+    end
+
     transient do
       in_reply_to { nil }
     end

spec/factories/snippets.rb

@@ -48,6 +48,7 @@ FactoryBot.define do
     trait :secret do
       visibility_level { Snippet::PUBLIC }
       secret { true }
+      project { nil }
     end
   end
 end

spec/models/note_spec.rb

@@ -270,18 +270,35 @@ describe Note do
     end
   end
 
-  describe "confidential?" do
-    it "delegates to noteable" do
-      issue_note = build(:note, :on_issue)
-      confidential_note = build(:note, noteable: create(:issue, confidential: true))
+  describe '#confidential?' do
+    context 'when note is not confidential' do
+      it 'is true when a noteable is confidential' do
+        issue = create(:issue, :confidential)
+        note = build(:note, noteable: issue, project: issue.project)
 
-      expect(issue_note.confidential?).to be_falsy
-      expect(confidential_note.confidential?).to be_truthy
+        expect(note.confidential?).to be_truthy
+      end
+
+      it 'is false when a noteable is not confidential' do
+        issue = create(:issue, confidential: false)
+        note = build(:note, noteable: issue, project: issue.project)
+
+        expect(note.confidential?).to be_falsy
+      end
+
+      it "is falsey when noteable can't be confidential" do
+        commit_note = build(:note_on_commit)
+
+        expect(commit_note.confidential?).to be_falsy
+      end
     end
+    context 'when note is confidential' do
+      it 'is true even when a noteable is not confidential' do
+        issue = create(:issue, confidential: false)
+        note = build(:note, :confidential, noteable: issue, project: issue.project)
 
-    it "is falsey when noteable can't be confidential" do
-      commit_note = build(:note_on_commit)
-      expect(commit_note.confidential?).to be_falsy
+        expect(note.confidential?).to be_truthy
+      end
     end
   end
 
@@ -1230,5 +1247,69 @@ describe Note do
         expect(notes.second.id).to eq(note2.id)
       end
     end
+
+    describe '#noteable_assignee_or_author' do
+      let(:user) { create(:user) }
+      let(:noteable) { create(:issue) }
+      let(:note) { create(:note, project: noteable.project, noteable: noteable) }
+
+      subject { note.noteable_assignee_or_author?(user) }
+
+      shared_examples 'assignee check' do
+        context 'when the provided user is one of the assignees' do
+          before do
+            note.noteable.update(assignees: [user, create(:user)])
+          end
+
+          it 'returns true' do
+            expect(subject).to be_truthy
+          end
+        end
+      end
+
+      shared_examples 'author check' do
+        context 'when the provided user is the author' do
+          before do
+            note.noteable.update(author: user)
+          end
+
+          it 'returns true' do
+            expect(subject).to be_truthy
+          end
+        end
+
+        context 'when the provided user is neither author nor assignee' do
+          it 'returns true' do
+            expect(subject).to be_falsey
+          end
+        end
+      end
+
+      context 'when user is nil' do
+        let(:user) { nil }
+
+        it 'returns false' do
+          expect(subject).to be_falsey
+        end
+      end
+
+      context 'when noteable is an issue' do
+        it_behaves_like 'author check'
+        it_behaves_like 'assignee check'
+      end
+
+      context 'when noteable is a merge request' do
+        let(:noteable) { create(:merge_request) }
+
+        it_behaves_like 'author check'
+        it_behaves_like 'assignee check'
+      end
+
+      context 'when noteable is a snippet' do
+        let(:noteable) { create(:personal_snippet) }
+
+        it_behaves_like 'author check'
+      end
+    end
   end
 end

spec/policies/note_policy_spec.rb

@@ -238,6 +238,101 @@ describe NotePolicy do
           end
         end
       end
+
+      context 'with confidential notes' do
+        def permissions(user, note)
+          described_class.new(user, note)
+        end
+
+        let(:reporter) { create(:user) }
+        let(:developer) { create(:user) }
+        let(:maintainer) { create(:user) }
+        let(:guest) { create(:user) }
+        let(:non_member) { create(:user) }
+        let(:author) { create(:user) }
+        let(:assignee) { create(:user) }
+
+        before do
+          project.add_reporter(reporter)
+          project.add_developer(developer)
+          project.add_maintainer(maintainer)
+          project.add_guest(guest)
+        end
+
+        shared_examples_for 'confidential notes permissions' do
+          it 'does not allow non members to read confidential notes and replies' do
+            expect(permissions(non_member, confidential_note)).to be_disallowed(:read_note, :admin_note, :resolve_note, :award_emoji)
+          end
+
+          it 'does not allow guests to read confidential notes and replies' do
+            expect(permissions(guest, confidential_note)).to be_disallowed(:read_note, :admin_note, :resolve_note, :award_emoji)
+          end
+
+          it 'allows reporter to read all notes but not resolve and admin them' do
+            expect(permissions(reporter, confidential_note)).to be_allowed(:read_note, :award_emoji)
+            expect(permissions(reporter, confidential_note)).to be_disallowed(:admin_note, :resolve_note)
+          end
+
+          it 'allows developer to read and resolve all notes' do
+            expect(permissions(developer, confidential_note)).to be_allowed(:read_note, :award_emoji, :resolve_note)
+            expect(permissions(developer, confidential_note)).to be_disallowed(:admin_note)
+          end
+
+          it 'allows maintainers to read all notes and admin them' do
+            expect(permissions(maintainer, confidential_note)).to be_allowed(:read_note, :admin_note, :resolve_note, :award_emoji)
+          end
+
+          it 'allows noteable author to read and resolve all notes' do
+            expect(permissions(author, confidential_note)).to be_allowed(:read_note, :resolve_note, :award_emoji)
+            expect(permissions(author, confidential_note)).to be_disallowed(:admin_note)
+          end
+        end
+
+        context 'for issues' do
+          let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) }
+          let(:confidential_note) { create(:note, :confidential, project: project, noteable: issue) }
+
+          it_behaves_like 'confidential notes permissions'
+
+          it 'allows noteable assignees to read all notes' do
+            expect(permissions(assignee, confidential_note)).to be_allowed(:read_note, :award_emoji)
+            expect(permissions(assignee, confidential_note)).to be_disallowed(:admin_note, :resolve_note)
+          end
+        end
+
+        context 'for merge requests' do
+          let(:merge_request) { create(:merge_request, source_project: project, author: author, assignees: [assignee]) }
+          let(:confidential_note) { create(:note, :confidential, project: project, noteable: merge_request) }
+
+          it_behaves_like 'confidential notes permissions'
+
+          it 'allows noteable assignees to read all notes' do
+            expect(permissions(assignee, confidential_note)).to be_allowed(:read_note, :award_emoji)
+            expect(permissions(assignee, confidential_note)).to be_disallowed(:admin_note, :resolve_note)
+          end
+        end
+
+        context 'for project snippets' do
+          let(:project_snippet) { create(:project_snippet, project: project, author: author) }
+          let(:confidential_note) { create(:note, :confidential, project: project, noteable: project_snippet) }
+
+          it_behaves_like 'confidential notes permissions'
+        end
+
+        context 'for personal snippets' do
+          let(:personal_snippet) { create(:personal_snippet, author: author) }
+          let(:confidential_note) { create(:note, :confidential, project: nil, noteable: personal_snippet) }
+
+          it 'allows snippet author to read and resolve all notes' do
+            expect(permissions(author, confidential_note)).to be_allowed(:read_note, :resolve_note, :award_emoji)
+            expect(permissions(author, confidential_note)).to be_disallowed(:admin_note)
+          end
+
+          it 'does not allow maintainers to read confidential notes and replies' do
+            expect(permissions(maintainer, confidential_note)).to be_disallowed(:read_note, :admin_note, :resolve_note, :award_emoji)
+          end
+        end
+      end
     end
   end
 end