Merge branch 'fix-events-finder-incomplete-11-1' into 'security-11-1'

[11.1] Redact events shown in the events API

See merge request gitlab/gitlabhq!2520

7 files changed, 235 insertions, 6 deletions

diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb
index 8676925a540..eb8af63eeb9 100644
--- a/app/finders/events_finder.rb
+++ b/app/finders/events_finder.rb
@@ -10,6 +10,7 @@ class EventsFinder
   # Arguments:
   #   source - which user or project to looks for events on
   #   current_user - only return events for projects visible to this user
+  #                  WARNING: does not consider project feature visibility!
   #   params:
   #     action: string
   #     target_type: string
diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb
index 74776b2ed1f..1bedfc2eef8 100644
--- a/app/finders/user_recent_events_finder.rb
+++ b/app/finders/user_recent_events_finder.rb
@@ -1,6 +1,7 @@
 # Get user activity feed for projects common for a user and a logged in user
 #
 # - current_user: The user viewing the events
+#                 WARNING: does not consider project feature visibility!
 # - user: The user for which to load the events
 # - params:
 #   - offset: The page of events to return
diff --git a/app/models/event.rb b/app/models/event.rb
index ac0b1c7b27c..059aca6cc20 100644
--- a/app/models/event.rb
+++ b/app/models/event.rb
@@ -145,21 +145,31 @@ class Event < ActiveRecord::Base
     end
   end
 
+  # rubocop:disable Metrics/CyclomaticComplexity
+  # rubocop:disable Metrics/PerceivedComplexity
   def visible_to_user?(user = nil)
     if push? || commit_note?
       Ability.allowed?(user, :download_code, project)
     elsif membership_changed?
-      true
+      Ability.allowed?(user, :read_project, project)
     elsif created_project?
-      true
+      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?
+      Ability.allowed?(user, :read_personal_snippet, note_target)
+    elsif project_snippet_note?
+      Ability.allowed?(user, :read_project_snippet, note_target)
+    elsif milestone?
+      Ability.allowed?(user, :read_milestone, project)
     else
-      milestone?
+      false
     end
   end
+  # rubocop:enable Metrics/PerceivedComplexity
+  # rubocop:enable Metrics/CyclomaticComplexity
 
   def project_name
     if project
@@ -301,6 +311,10 @@ class Event < ActiveRecord::Base
     note? && target && target.for_snippet?
   end
 
+  def personal_snippet_note?
+    note? && target && target.for_personal_snippet?
+  end
+
   def note_target
     target.noteable
   end
diff --git a/changelogs/unreleased/fix-events-finder-incomplete.yml b/changelogs/unreleased/fix-events-finder-incomplete.yml
new file mode 100644
index 00000000000..f3a4e421d33
--- /dev/null
+++ b/changelogs/unreleased/fix-events-finder-incomplete.yml
@@ -0,0 +1,5 @@
+---
+title: Redact confidential events in the API
+merge_request:
+author:
+type: security
diff --git a/lib/api/events.rb b/lib/api/events.rb
index fc4ba5a3188..357ca83ddab 100644
--- a/lib/api/events.rb
+++ b/lib/api/events.rb
@@ -15,11 +15,26 @@ module API
                         desc: 'Return events sorted in ascending and descending order'
       end
 
-      def present_events(events)
+      RedactedEvent = OpenStruct.new(target_title: 'Confidential event').freeze
+
+      def redact_events(events)
+        events.map do |event|
+          if event.visible_to_user?(current_user)
+            event
+          else
+            RedactedEvent
+          end
+        end
+      end
+
+      def present_events(events, redact: true)
         events = events.reorder(created_at: params[:sort])
                  .with_associations
 
-        present paginate(events), with: Entities::Event
+        events = paginate(events)
+        events = redact_events(events) if redact
+
+        present events, with: Entities::Event
       end
     end
 
@@ -38,7 +53,8 @@ module API
 
         events = EventsFinder.new(params.merge(source: current_user, current_user: current_user)).execute.preload(:author, :target)
 
-        present_events(events)
+        # Since we're viewing our own events, redaction is unnecessary
+        present_events(events, redact: false)
       end
     end
 
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
index c1eac4fa489..81748681528 100644
--- a/spec/models/event_spec.rb
+++ b/spec/models/event_spec.rb
@@ -148,9 +148,14 @@ describe Event do
     let(:admin) { create(:admin) }
     let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) }
     let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) }
+    let(:project_snippet) { create(:project_snippet, :public, project: project, author: author) }
+    let(:personal_snippet) { create(:personal_snippet, :public, author: author) }
     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(:note_on_project_snippet) { create(:note_on_project_snippet, author: author, noteable: project_snippet, project: project) }
+    let(:note_on_personal_snippet) { create(:note_on_personal_snippet, author: author, noteable: personal_snippet, project: nil) }
+    let(:milestone_on_project) { create(:milestone, project: project) }
     let(:event) { described_class.new(project: project, target: target, author_id: author.id) }
 
     before do
@@ -268,6 +273,125 @@ describe Event do
         end
       end
     end
+
+    context 'milestone event' do
+      let(:target) { milestone_on_project }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(member)).to be_truthy
+        expect(event.visible_to_user?(guest)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on public project with private issue tracker and merge requests' do
+        let(:project) { create(:project, :public, :issues_private, :merge_requests_private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private project' do
+        let(:project) { create(:project, :private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+    end
+
+    context 'project snippet note event' do
+      let(:target) { note_on_project_snippet }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(author)).to be_truthy
+        expect(event.visible_to_user?(member)).to be_truthy
+        expect(event.visible_to_user?(guest)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on public project with private snippets' do
+        let(:project) { create(:project, :public, :snippets_private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+
+          # Normally, we'd expect the author of a comment to be able to view it.
+          # However, this doesn't seem to be the case for comments on snippets.
+          expect(event.visible_to_user?(author)).to be_falsy
+
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private project' do
+        let(:project) { create(:project, :private) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+
+          # Normally, we'd expect the author of a comment to be able to view it.
+          # However, this doesn't seem to be the case for comments on snippets.
+          expect(event.visible_to_user?(author)).to be_falsy
+
+          expect(event.visible_to_user?(member)).to be_truthy
+          expect(event.visible_to_user?(guest)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+    end
+
+    context 'personal snippet note event' do
+      let(:target) { note_on_personal_snippet }
+
+      it do
+        expect(event.visible_to_user?(nil)).to be_truthy
+        expect(event.visible_to_user?(non_member)).to be_truthy
+        expect(event.visible_to_user?(author)).to be_truthy
+        expect(event.visible_to_user?(admin)).to be_truthy
+      end
+
+      context 'on internal snippet' do
+        let(:personal_snippet) { create(:personal_snippet, :internal, author: author) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_truthy
+          expect(event.visible_to_user?(author)).to be_truthy
+          expect(event.visible_to_user?(admin)).to be_truthy
+        end
+      end
+
+      context 'on private snippet' do
+        let(:personal_snippet) { create(:personal_snippet, :private, author: author) }
+
+        it do
+          expect(event.visible_to_user?(nil)).to be_falsy
+          expect(event.visible_to_user?(non_member)).to be_falsy
+          expect(event.visible_to_user?(author)).to be_truthy
+
+          # It is very unexpected that a private personal snippet is not visible
+          # to an instance administrator. This should be fixed in the future.
+          expect(event.visible_to_user?(admin)).to be_falsy
+        end
+      end
+    end
   end
 
   describe '.limit_recent' do
diff --git a/spec/requests/api/redacted_events_spec.rb b/spec/requests/api/redacted_events_spec.rb
new file mode 100644
index 00000000000..086dd3df9ba
--- /dev/null
+++ b/spec/requests/api/redacted_events_spec.rb
@@ -0,0 +1,68 @@
+require 'spec_helper'
+
+describe 'Redacted events in API::Events' do
+  shared_examples 'private events are redacted' do
+    it 'redacts events the user does not have access to' do
+      expect_any_instance_of(Event).to receive(:visible_to_user?).and_call_original
+
+      get api(path), user
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response).to contain_exactly(
+        'project_id' => nil,
+        'action_name' => nil,
+        'target_id' => nil,
+        'target_iid' => nil,
+        'target_type' => nil,
+        'author_id' => nil,
+        'target_title' => 'Confidential event',
+        'created_at' => nil,
+        'author_username' => nil
+      )
+    end
+  end
+
+  describe '/users/:id/events' do
+    let(:project) { create(:project, :public) }
+    let(:path) { "/users/#{project.owner.id}/events" }
+    let(:issue) { create(:issue, :confidential, project: project) }
+
+    before do
+      EventCreateService.new.open_issue(issue, issue.author)
+    end
+
+    context 'unauthenticated user views another user with private events' do
+      let(:user) { nil }
+
+      include_examples 'private events are redacted'
+    end
+
+    context 'authenticated user without access views another user with private events' do
+      let(:user) { create(:user) }
+
+      include_examples 'private events are redacted'
+    end
+  end
+
+  describe '/projects/:id/events' do
+    let(:project) { create(:project, :public) }
+    let(:path) { "/projects/#{project.id}/events" }
+    let(:issue) { create(:issue, :confidential, project: project) }
+
+    before do
+      EventCreateService.new.open_issue(issue, issue.author)
+    end
+
+    context 'unauthenticated user views public project' do
+      let(:user) { nil }
+
+      include_examples 'private events are redacted'
+    end
+
+    context 'authenticated user without access views public project' do
+      let(:user) { create(:user) }
+
+      include_examples 'private events are redacted'
+    end
+  end
+end