Merge branch 'fix-comments-on-confidential-issues-show-activity-feed-for-non-members' into 'master'

Comments on confidential issues doesn't show in activity feed to non-members

Closes #14568 

See merge request !3375

5 files changed, 64 insertions, 31 deletions

diff --git a/CHANGELOG b/CHANGELOG
index d9be95defd1..5d9f4961ef5 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,9 @@ v 8.7.0 (unreleased)
   - Make HTTP(s) label consistent on clone bar (Stan Hu)
   - Fix avatar stretching by providing a cropping feature
 
+v 8.6.2 (unreleased)
+  - Comments on confidential issues don't show up in activity feed to non-members
+
 v 8.6.1
   - Add option to reload the schema before restoring a database backup. !2807
   - Display navigation controls on mobile. !3214
diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb
index a67a6b208e2..d3e5e3aa8b9 100644
--- a/app/helpers/events_helper.rb
+++ b/app/helpers/events_helper.rb
@@ -194,7 +194,7 @@ module EventsHelper
   end
 
   def event_to_atom(xml, event)
-    if event.proper?(current_user)
+    if event.visible_to_user?(current_user)
       xml.entry do
         event_link = event_feed_url(event)
         event_title = event_feed_title(event)
diff --git a/app/models/event.rb b/app/models/event.rb
index a5cfeaf388e..12183524b79 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -73,15 +73,15 @@ class Event < ActiveRecord::Base
     end
   end
 
-  def proper?(user = nil)
+  def visible_to_user?(user = nil)
     if push?
       true
     elsif membership_changed?
       true
     elsif created_project?
       true
-    elsif issue?
-      Ability.abilities.allowed?(user, :read_issue, issue)
+    elsif issue? || issue_note?
+      Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target)
     else
       ((merge_request? || note?) && target) || milestone?
     end
@@ -298,6 +298,10 @@ class Event < ActiveRecord::Base
     target.noteable_type == "Commit"
   end
 
+  def issue_note?
+    note? && target && target.noteable_type == "Issue"
+  end
+
   def note_project_snippet?
     target.noteable_type == "Snippet"
   end
diff --git a/app/views/events/_event.html.haml b/app/views/events/_event.html.haml
index 2d9d9dd6342..42c2764e7e2 100644
--- a/app/views/events/_event.html.haml
+++ b/app/views/events/_event.html.haml
@@ -1,4 +1,4 @@
-- if event.proper?(current_user)
+- if event.visible_to_user?(current_user)
   .event-item{class: "#{event.body? ? "event-block" : "event-inline" }"}
     .event-item-timestamp
       #{time_ago_with_tooltip(event.created_at)}
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index 5fe44246738..89909c2bcd7 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -59,44 +59,70 @@ describe Event, models: true do
     end
 
     it { expect(@event.push?).to be_truthy }
-    it { expect(@event.proper?).to be_truthy }
+    it { expect(@event.visible_to_user?).to be_truthy }
     it { expect(@event.tag?).to be_falsey }
     it { expect(@event.branch_name).to eq("master") }
     it { expect(@event.author).to eq(@user) }
   end
 
-  describe '#proper?' do
-    context 'issue event' do
-      let(:project) { create(:empty_project, :public) }
-      let(:non_member) { create(:user) }
-      let(:member)  { create(:user) }
-      let(:author) { create(:author) }
-      let(:assignee) { create(:user) }
-      let(:admin) { create(:admin) }
-      let(:event) { Event.new(project: project, action: Event::CREATED, target: issue, author_id: author.id) }
-
-      before do
-        project.team << [member, :developer]
-      end
+  describe '#visible_to_user?' do
+    let(:project) { create(:empty_project, :public) }
+    let(:non_member) { create(:user) }
+    let(:member)  { create(:user) }
+    let(:author) { create(:author) }
+    let(:assignee) { create(:user) }
+    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_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) }
 
+    before do
+      project.team << [member, :developer]
+    end
+
+    context 'issue event' do
       context 'for non confidential issues' do
-        let(:issue) { create(:issue, project: project, author: author, assignee: assignee) }
+        let(:target) { issue }
 
-        it { expect(event.proper?(non_member)).to eq true }
-        it { expect(event.proper?(author)).to eq true }
-        it { expect(event.proper?(assignee)).to eq true }
-        it { expect(event.proper?(member)).to eq true }
-        it { expect(event.proper?(admin)).to eq true }
+        it { expect(event.visible_to_user?(non_member)).to eq true }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
       end
 
       context 'for confidential issues' do
-        let(:issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+        let(:target) { confidential_issue }
+
+        it { expect(event.visible_to_user?(non_member)).to eq false }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
+      end
+    end
+
+    context 'note event' do
+      context 'on non confidential issues' do
+        let(:target) { note_on_issue }
+
+        it { expect(event.visible_to_user?(non_member)).to eq true }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
+      end
+
+      context 'on confidential issues' do
+        let(:target) { note_on_confidential_issue }
 
-        it { expect(event.proper?(non_member)).to eq false }
-        it { expect(event.proper?(author)).to eq true }
-        it { expect(event.proper?(assignee)).to eq true }
-        it { expect(event.proper?(member)).to eq true }
-        it { expect(event.proper?(admin)).to eq true }
+        it { expect(event.visible_to_user?(non_member)).to eq false }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
       end
     end
   end