Merge branch '23824-activity-page-does-not-show-commits-comments' into 'master'

Allow commit note to be visible if repo is visible

## What does this MR do?

It enforces the `:download_code` permission in `Event#visible_to_user?` for commit notes.

Closes #23824

See merge request !7504

4 files changed, 36 insertions, 5 deletions

diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb
index f8ded05c31a..6d8b8b5dfa6 100644
--- a/app/helpers/events_helper.rb
+++ b/app/helpers/events_helper.rb
@@ -80,7 +80,7 @@ module EventsHelper
     elsif event.merge_request?
       namespace_project_merge_request_url(event.project.namespace,
                                           event.project, event.merge_request)
-    elsif event.note? && event.commit_note?
+    elsif event.commit_note?
       namespace_project_commit_url(event.project.namespace, event.project,
                                    event.note_target)
     elsif event.note?
@@ -121,7 +121,7 @@ module EventsHelper
   end
 
   def event_note_target_path(event)
-    if event.note? && event.commit_note?
+    if event.commit_note?
       namespace_project_commit_path(event.project.namespace,
                                     event.project,
                                     event.note_target,
diff --git a/app/models/event.rb b/app/models/event.rb
index 511b989c63f..1d193de1d3d 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -61,7 +61,7 @@ class Event < ActiveRecord::Base
   end
 
   def visible_to_user?(user = nil)
-    if push?
+    if push? || commit_note?
       Ability.allowed?(user, :download_code, project)
     elsif membership_changed?
       true
@@ -276,7 +276,7 @@ class Event < ActiveRecord::Base
   end
 
   def commit_note?
-    target.for_commit?
+    note? && target && target.for_commit?
   end
 
   def issue_note?
@@ -288,7 +288,7 @@ class Event < ActiveRecord::Base
   end
 
   def project_snippet_note?
-    target.for_snippet?
+    note? && target && target.for_snippet?
   end
 
   def note_target
diff --git a/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml b/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml
new file mode 100644
index 00000000000..48f733f9c5e
--- /dev/null
+++ b/changelogs/unreleased/23824-activity-page-does-not-show-commits-comments.yml
@@ -0,0 +1,4 @@
+---
+title: Allow commit note to be visible if repo is visible
+merge_request: 
+author: 
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 7e82e50e139..ebe46efc5ae 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -67,6 +67,7 @@ describe Event, models: true do
     let(:admin) { create(:admin) }
     let(:issue) { create(:issue, project: project, author: author, assignee: assignee) }
     let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+    let(:note_on_commit) { create(:note_on_commit, project: project) }
     let(:note_on_issue) { create(:note_on_issue, noteable: issue, project: project) }
     let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project) }
     let(:event) { Event.new(project: project, target: target, author_id: author.id) }
@@ -76,6 +77,32 @@ describe Event, models: true do
       project.team << [guest, :guest]
     end
 
+    context 'commit note event' do
+      let(:target) { note_on_commit }
+
+      it do
+        aggregate_failures 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
+        end
+      end
+
+      context 'private project' do
+        let(:project) { create(:empty_project, :private) }
+
+        it do
+          aggregate_failures 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 false
+            expect(event.visible_to_user?(admin)).to eq true
+          end
+        end
+      end
+    end
+
     context 'issue event' do
       context 'for non confidential issues' do
         let(:target) { issue }