Merge branch 'ajk-198120-private-design-comments' into 'master'

Ensure events on designs are visible to the appropriate users

Closes #198120

See merge request gitlab-org/gitlab!26708

app/models/event.rb

@@ -135,29 +135,11 @@ class Event < ApplicationRecord
     super(presenter_class: ::EventPresenter)
   end
 
-  # rubocop:disable Metrics/CyclomaticComplexity
-  # rubocop:disable Metrics/PerceivedComplexity
   def visible_to_user?(user = nil)
-    if push_action? || commit_note?
-      Ability.allowed?(user, :download_code, project)
-    elsif membership_changed?
-      Ability.allowed?(user, :read_project, project)
-    elsif created_project_action?
-      Ability.allowed?(user, :read_project, project)
-    elsif issue? || issue_note?
-      Ability.allowed?(user, :read_issue, note? ? note_target : target)
-    elsif merge_request? || merge_request_note?
-      Ability.allowed?(user, :read_merge_request, note? ? note_target : target)
-    elsif personal_snippet_note? || project_snippet_note?
-      Ability.allowed?(user, :read_snippet, note_target)
-    elsif milestone?
-      Ability.allowed?(user, :read_milestone, project)
-    else
-      false # No other event types are visible
-    end
+    return false unless capability.present?
+
+    Ability.allowed?(user, capability, permission_object)
   end
-  # rubocop:enable Metrics/PerceivedComplexity
-  # rubocop:enable Metrics/CyclomaticComplexity
 
   def resource_parent
     project || group
@@ -364,8 +346,38 @@ class Event < ApplicationRecord
     Event._to_partial_path
   end
 
+  protected
+
+  def capability
+    @capability ||= begin
+                      if push_action? || commit_note?
+                        :download_code
+                      elsif membership_changed? || created_project_action?
+                        :read_project
+                      elsif issue? || issue_note?
+                        :read_issue
+                      elsif merge_request? || merge_request_note?
+                        :read_merge_request
+                      elsif personal_snippet_note? || project_snippet_note?
+                        :read_snippet
+                      elsif milestone?
+                        :read_milestone
+                      end
+                    end
+  end
+
   private
 
+  def permission_object
+    if note?
+      note_target
+    elsif target_id.present?
+      target
+    else
+      project
+    end
+  end
+
   def push_action_name
     if new_ref?
       "pushed new"

ee/app/models/ee/event.rb

@@ -18,15 +18,21 @@ module EE
       scope :epics, -> { where(target_type: 'Epic') }
     end
 
-    override :visible_to_user?
-    def visible_to_user?(user = nil)
-      if epic?
-        Ability.allowed?(user, :read_epic, target)
-      elsif epic_note?
-        Ability.allowed?(user, :read_epic, note_target)
-      else
-        super
-      end
+    override :capability
+    def capability
+      @capability ||= begin
+                        if epic? || epic_note?
+                          :read_epic
+                        elsif design_note?
+                          :read_design
+                        else
+                          super
+                        end
+                      end
+    end
+
+    def design_note?
+      note? && note.for_design?
     end
 
     def epic_note?

ee/changelogs/unreleased/ajk-198120-private-design-comments.yml

+---
+title: Ensure design events are correctly visible
+merge_request: 26708
+author:
+type: fixed

ee/spec/factories/events.rb

@@ -9,5 +9,15 @@ FactoryBot.modify do
       action { Event::CREATED }
       project { nil }
     end
+
+    trait :for_design do
+      transient do
+        design { create(:design, issue: create(:issue, project: project)) }
+        note { create(:note, project: project, noteable: design) }
+      end
+
+      action { Event::COMMENTED }
+      target { note }
+    end
   end
 end

ee/spec/factories/notes.rb

@@ -8,7 +8,17 @@ FactoryBot.modify do
     end
 
     trait :on_design do
-      noteable { create(:design, :with_file, issue: create(:issue, project: project)) }
+      transient do
+        issue { create(:issue, project: project) }
+      end
+      noteable { create(:design, :with_file, issue: issue) }
+
+      after(:build) do |note|
+        next if note.project == note.noteable.project
+
+        # note validations require consistency between these two objects
+        note.project = note.noteable.project
+      end
     end
 
     trait :with_review do

ee/spec/mailers/notify_spec.rb

@@ -77,7 +77,6 @@ describe Notify do
     let_it_be(:note) do
       create(:diff_note_on_design,
          noteable: design,
-         project: design.project,
          note: "Hello #{recipient.to_reference}")
     end
 

ee/spec/models/design_management/design_spec.rb

@@ -398,20 +398,20 @@ describe DesignManagement::Design do
     # Note: Cache invalidation tests are in `design_user_notes_count_service_spec.rb`
 
     it 'returns a count of user-generated notes' do
-      create(:diff_note_on_design, noteable: design, project: design.project)
+      create(:diff_note_on_design, noteable: design)
 
       is_expected.to eq(1)
     end
 
     it 'does not count notes on other designs' do
       second_design = create(:design, :with_file)
-      create(:diff_note_on_design, noteable: second_design, project: second_design.project)
+      create(:diff_note_on_design, noteable: second_design)
 
       is_expected.to eq(0)
     end
 
     it 'does not count system notes' do
-      create(:diff_note_on_design, system: true, noteable: design, project: design.project)
+      create(:diff_note_on_design, system: true, noteable: design)
 
       is_expected.to eq(0)
     end

ee/spec/models/ee/diff_note_spec.rb

@@ -12,7 +12,7 @@ describe DiffNote do
 
   context 'diff files' do
     let(:design) { create(:design, :with_file, versions_count: 2) }
-    let(:diff_note) { create(:diff_note_on_design, noteable: design, project: design.project) }
+    let(:diff_note) { create(:diff_note_on_design, noteable: design) }
 
     describe '#latest_diff_file' do
       it 'does not return a diff file' do

ee/spec/models/ee/event_spec.rb

@@ -7,33 +7,107 @@ describe Event do
     let_it_be(:non_member) { create(:user) }
     let_it_be(:member) { create(:user) }
     let_it_be(:guest) { create(:user) }
+    let_it_be(:reporter) { create(:user) }
     let_it_be(:author) { create(:author) }
     let_it_be(:admin) { create(:admin) }
+    let_it_be(:project) { create(:project) }
+
+    let(:users) { [non_member, member, reporter, guest, author, admin] }
+
     let(:epic) { create(:epic, group: group, author: author) }
     let(:note_on_epic) { create(:note, :on_epic, noteable: epic) }
     let(:event) { described_class.new(group: group, target: target, author: author) }
 
     before do
       stub_licensed_features(epics: true)
-      group.add_developer(member)
-      group.add_guest(guest)
+
+      project.add_developer(member)
+      project.add_guest(guest)
+      project.add_reporter(reporter)
+
+      if defined?(group)
+        group.add_developer(member)
+        group.add_guest(guest)
+      end
+    end
+
+    RSpec::Matchers.define :be_visible_to do |user|
+      match do |event|
+        event.visible_to_user?(user)
+      end
+
+      failure_message do |event|
+        "expected that #{event} should be visible to #{user}"
+      end
+
+      failure_message_when_negated do |event|
+        "expected that #{event} would not be visible to #{user}"
+      end
+    end
+
+    RSpec::Matchers.define :have_access_to do |event|
+      match do |user|
+        event.visible_to_user?(user)
+      end
+
+      failure_message do |user|
+        "expected that #{event} should be visible to #{user}"
+      end
+
+      failure_message_when_negated do |user|
+        "expected that #{event} would not be visible to #{user}"
+      end
     end
 
     shared_examples 'visible to group members only' do
-      it 'is not visible to other users' do
-        expect(event.visible_to_user?(non_member)).to eq false
-        expect(event.visible_to_user?(member)).to eq true
-        expect(event.visible_to_user?(guest)).to eq true
-        expect(event.visible_to_user?(admin)).to eq true
+      it 'is not visible to other users', :aggregate_failures do
+        expect(event).not_to be_visible_to(non_member)
+        expect(event).not_to be_visible_to(author)
+
+        expect(event).to be_visible_to(member)
+        expect(event).to be_visible_to(guest)
+        expect(event).to be_visible_to(admin)
       end
     end
 
     shared_examples 'visible to everybody' do
-      it 'is visible to other users' do
-        expect(event.visible_to_user?(non_member)).to eq true
-        expect(event.visible_to_user?(member)).to eq true
-        expect(event.visible_to_user?(guest)).to eq true
-        expect(event.visible_to_user?(admin)).to eq true
+      it 'is visible to other users', :aggregate_failures do
+        expect(users).to all(have_access_to(event))
+      end
+    end
+
+    context 'design event' do
+      include DesignManagementTestHelpers
+
+      before do
+        enable_design_management
+      end
+
+      it_behaves_like 'visible to group members only' do
+        let(:event) { create(:event, :for_design, project: project) }
+      end
+
+      context 'the event refers to a design on a confidential issue' do
+        let(:project) { create(:project, :public) }
+        let(:issue) { create(:issue, :confidential, project: project) }
+        let(:note) { create(:note, :on_design, issue: issue) }
+        let(:event) { create(:event, project: project, target: note) }
+
+        let(:assignees) do
+          create_list(:user, 3).each { |user| issue.assignees << user }
+        end
+
+        it 'visible to group reporters, the issue author, and assignees', :aggregate_failures do
+          expect(event).not_to be_visible_to(non_member)
+          expect(event).not_to be_visible_to(guest)
+
+          expect(event).to be_visible_to(reporter)
+          expect(event).to be_visible_to(member)
+          expect(event).to be_visible_to(admin)
+          expect(event).to be_visible_to(issue.author)
+
+          expect(assignees).to all(have_access_to(event))
+        end
       end
     end
 

ee/spec/requests/api/graphql/project/issue/designs/designs_spec.rb

@@ -318,7 +318,7 @@ describe 'Getting designs related to an issue' do
     end
 
     describe 'a design with note annotations' do
-      let_it_be(:note) { create(:diff_note_on_design, noteable: design, project: design.project) }
+      let_it_be(:note) { create(:diff_note_on_design, noteable: design) }
 
       let(:design_query) do
         <<~NODE

ee/spec/services/design_management/design_user_notes_count_service_spec.rb

@@ -11,7 +11,7 @@ describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_
 
   describe '#count' do
     it 'returns the count of notes' do
-      create_list(:diff_note_on_design, 3, noteable: design, project: design.project)
+      create_list(:diff_note_on_design, 3, noteable: design)
 
       expect(subject.count).to eq(3)
     end
@@ -33,7 +33,7 @@ describe DesignManagement::DesignUserNotesCountService, :use_clean_rails_memory_
     end
 
     it 'changes when a note is destroyed' do
-      note = create(:diff_note_on_design, noteable: design, project: design.project)
+      note = create(:diff_note_on_design, noteable: design)
 
       expect do
         Notes::DestroyService.new(note.project, note.author).execute(note)

ee/spec/services/ee/notes/post_process_service_spec.rb

@@ -11,7 +11,7 @@ describe Notes::PostProcessService do
       subject { described_class.new(note).execute }
 
       def create_note(in_reply_to: nil)
-        create(:diff_note_on_design, noteable: noteable, project: noteable.project, in_reply_to: in_reply_to)
+        create(:diff_note_on_design, noteable: noteable, in_reply_to: in_reply_to)
       end
 
       context 'when the note is the start of a new discussion' do

ee/spec/services/ee/notification_service_spec.rb

@@ -19,7 +19,6 @@ describe EE::NotificationService, :mailer do
     let_it_be(:note) do
       create(:diff_note_on_design,
          noteable: design,
-         project: project,
          note: "Hello #{dev.to_reference}, G'day #{stranger.to_reference}")
     end
 

ee/spec/services/ee/system_notes/design_management_service_spec.rb

@@ -132,7 +132,7 @@ describe EE::SystemNotes::DesignManagementService do
     let(:design) { create(:design, :with_file, issue: issue) }
     let(:author) { create(:user) }
     let(:discussion_note) do
-      create(:diff_note_on_design, noteable: design, author: author, project: project)
+      create(:diff_note_on_design, noteable: design, author: author)
     end
     let(:action) { 'designs_discussion_added' }
 

ee/spec/services/todo_service_spec.rb

@@ -341,7 +341,6 @@ describe TodoService do
 
     let(:note) do
       build(:diff_note_on_design,
-             project: project,
              noteable: design,
              author: author,
              note: "Hey #{john_doe.to_reference}")