Merge branch 'bvl-hide-confidential-events-take2' into 'master'

Hide confidential events in ruby

Closes #51791

See merge request gitlab-org/gitlab-ce!23746

app/finders/concerns/finder_with_cross_project_access.rb

@@ -5,7 +5,8 @@
 #
 # This module depends on the finder implementing the following methods:
 #
-# - `#execute` should return an `ActiveRecord::Relation`
+# - `#execute` should return an `ActiveRecord::Relation` or the `model` needs to
+#              be defined in the call to `requires_cross_project_access`.
 # - `#current_user` the user that requires access (or nil)
 module FinderWithCrossProjectAccess
   extend ActiveSupport::Concern
@@ -13,20 +14,35 @@ module FinderWithCrossProjectAccess
 
   prepended do
     extend Gitlab::CrossProjectAccess::ClassMethods
+
+    cattr_accessor :finder_model
+
+    def self.requires_cross_project_access(*args)
+      super
+
+      self.finder_model = extract_model_from_arguments(args)
+    end
+
+    private
+
+    def self.extract_model_from_arguments(args)
+      args.detect { |argument| argument.is_a?(Hash) && argument[:model] }
+        &.fetch(:model)
+    end
   end
 
   override :execute
   def execute(*args)
     check = Gitlab::CrossProjectAccess.find_check(self)
-    original = super
+    original = -> { super }
 
-    return original unless check
-    return original if should_skip_cross_project_check || can_read_cross_project?
+    return original.call unless check
+    return original.call if should_skip_cross_project_check || can_read_cross_project?
 
     if check.should_run?(self)
-      original.model.none
+      finder_model&.none || original.call.model.none
     else
-      original
+      original.call
     end
   end
 
@@ -48,8 +64,6 @@ module FinderWithCrossProjectAccess
     skip_cross_project_check { super }
   end
 
-  private
-
   attr_accessor :should_skip_cross_project_check
 
   def skip_cross_project_check

app/finders/events_finder.rb

@@ -3,22 +3,27 @@
 class EventsFinder
   prepend FinderMethods
   prepend FinderWithCrossProjectAccess
+
+  MAX_PER_PAGE = 100
+
   attr_reader :source, :params, :current_user
 
-  requires_cross_project_access unless: -> { source.is_a?(Project) }
+  requires_cross_project_access unless: -> { source.is_a?(Project) }, model: Event
 
   # Used to filter Events
   #
   # 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
   #     before: datetime
   #     after: datetime
-  #
+  #     per_page: integer (max. 100)
+  #     page: integer
+  #     with_associations: boolean
+  #     sort: 'asc' or 'desc'
   def initialize(params = {})
     @source = params.delete(:source)
     @current_user = params.delete(:current_user)
@@ -33,15 +38,18 @@ class EventsFinder
     events = by_target_type(events)
     events = by_created_at_before(events)
     events = by_created_at_after(events)
+    events = sort(events)
+
+    events = events.with_associations if params[:with_associations]
 
-    events
+    paginated_filtered_by_user_visibility(events)
   end
 
   private
 
   # rubocop: disable CodeReuse/ActiveRecord
   def by_current_user_access(events)
-    events.merge(ProjectsFinder.new(current_user: current_user).execute) # rubocop: disable CodeReuse/Finder
+    events.merge(Project.public_or_visible_to_user(current_user))
       .joins(:project)
   end
   # rubocop: enable CodeReuse/ActiveRecord
@@ -77,4 +85,31 @@ class EventsFinder
     events.where('events.created_at > ?', params[:after].end_of_day)
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def sort(events)
+    return events unless params[:sort]
+
+    if params[:sort] == 'asc'
+      events.order_id_asc
+    else
+      events.order_id_desc
+    end
+  end
+
+  def paginated_filtered_by_user_visibility(events)
+    limited_events = events.page(page).per(per_page)
+    visible_events = limited_events.select { |event| event.visible_to_user?(current_user) }
+
+    Kaminari.paginate_array(visible_events, total_count: events.count)
+  end
+
+  def per_page
+    return MAX_PER_PAGE unless params[:per_page]
+
+    [params[:per_page], MAX_PER_PAGE].min
+  end
+
+  def page
+    params[:page] || 1
+  end
 end

changelogs/unreleased/bvl-hide-confidential-events-take2.yml

+---
+title: Hide confidential events in the API
+merge_request: 23746
+author:
+type: other

lib/api/events.rb

@@ -18,29 +18,15 @@ module API
                         desc: 'Return events sorted in ascending and descending order'
       end
 
-      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
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def present_events(events, redact: true)
-        events = events.reorder(created_at: params[:sort])
-                 .with_associations
-
+      def present_events(events)
         events = paginate(events)
-        events = redact_events(events) if redact
 
         present events, with: Entities::Event
       end
-      # rubocop: enable CodeReuse/ActiveRecord
+
+      def find_events(source)
+        EventsFinder.new(params.merge(source: source, current_user: current_user, with_associations: true)).execute
+      end
     end
 
     resource :events do
@@ -55,16 +41,14 @@ module API
         use :event_filter_params
         use :sort_params
       end
-      # rubocop: disable CodeReuse/ActiveRecord
+
       get do
         authenticate!
 
-        events = EventsFinder.new(params.merge(source: current_user, current_user: current_user)).execute.preload(:author, :target)
+        events = find_events(current_user)
 
-        # Since we're viewing our own events, redaction is unnecessary
-        present_events(events, redact: false)
+        present_events(events)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
 
     params do
@@ -82,16 +66,15 @@ module API
         use :event_filter_params
         use :sort_params
       end
-      # rubocop: disable CodeReuse/ActiveRecord
+
       get ':id/events' do
         user = find_user(params[:id])
         not_found!('User') unless user
 
-        events = EventsFinder.new(params.merge(source: user, current_user: current_user)).execute.preload(:author, :target)
+        events = find_events(user)
 
         present_events(events)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
 
     params do
@@ -106,13 +89,12 @@ module API
         use :event_filter_params
         use :sort_params
       end
-      # rubocop: disable CodeReuse/ActiveRecord
+
       get ":id/events" do
-        events = EventsFinder.new(params.merge(source: user_project, current_user: current_user)).execute.preload(:author, :target)
+        events = find_events(user_project)
 
         present_events(events)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

spec/finders/concerns/finder_with_cross_project_access_spec.rb

@@ -115,4 +115,20 @@ describe FinderWithCrossProjectAccess do
       expect(finder.execute).to include(result)
     end
   end
+
+  context 'when specifying a model' do
+    let(:finder_class) do
+      Class.new do
+        prepend FinderWithCrossProjectAccess
+
+        requires_cross_project_access model: Project
+      end
+    end
+
+    context '.finder_model' do
+      it 'is set correctly' do
+        expect(finder_class.finder_model).to eq(Project)
+      end
+    end
+  end
 end

spec/finders/events_finder_spec.rb

@@ -14,6 +14,10 @@ describe EventsFinder do
   let!(:closed_issue_event2) { create(:event, project: project1, author: user, target: closed_issue, action: Event::CLOSED, created_at: Date.new(2016, 2, 2)) }
   let!(:opened_merge_request_event2) { create(:event, project: project2, author: user, target: opened_merge_request, action: Event::CREATED, created_at: Date.new(2017, 2, 2)) }
 
+  let(:public_project) { create(:project, :public, creator_id: user.id, namespace: user.namespace) }
+  let(:confidential_issue) { create(:closed_issue, confidential: true, project: public_project, author: user) }
+  let!(:confidential_event) { create(:event, project: public_project, author: user, target: confidential_issue, action: Event::CLOSED) }
+
   context 'when targeting a user' do
     it 'returns events between specified dates filtered on action and type' do
       events = described_class.new(source: user, current_user: user, action: 'created', target_type: 'merge_request', after: Date.new(2017, 1, 1), before: Date.new(2017, 2, 1)).execute
@@ -27,6 +31,19 @@ describe EventsFinder do
       expect(events).not_to include(opened_merge_request_event)
     end
 
+    it 'does not include events on confidential issues the user does not have access to' do
+      events = described_class.new(source: user, current_user: other_user).execute
+
+      expect(events).not_to include(confidential_event)
+    end
+
+    it 'includes confidential events user has access to' do
+      public_project.add_developer(other_user)
+      events = described_class.new(source: user, current_user: other_user).execute
+
+      expect(events).to include(confidential_event)
+    end
+
     it 'returns nothing when the current user cannot read cross project' do
       expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
 

spec/finders/user_recent_events_finder_spec.rb

@@ -29,8 +29,9 @@ describe UserRecentEventsFinder do
     end
 
     it 'does not include the events if the user cannot read cross project' do
-      expect(Ability).to receive(:allowed?).and_call_original
+      allow(Ability).to receive(:allowed?).and_call_original
       expect(Ability).to receive(:allowed?).with(current_user, :read_cross_project) { false }
+
       expect(finder.execute).to be_empty
     end
   end

spec/requests/api/events_spec.rb

@@ -182,6 +182,68 @@ describe API::Events do
       end
     end
 
+    context 'with inaccessible events' do
+      let(:public_project) { create(:project, :public, creator_id: user.id, namespace: user.namespace) }
+      let(:confidential_issue) { create(:closed_issue, confidential: true, project: public_project, author: user) }
+      let!(:confidential_event) { create(:event, project: public_project, author: user, target: confidential_issue, action: Event::CLOSED) }
+      let(:public_issue) { create(:closed_issue, project: public_project, author: user) }
+      let!(:public_event) { create(:event, project: public_project, author: user, target: public_issue, action: Event::CLOSED) }
+
+      it 'returns only accessible events' do
+        get api("/projects/#{public_project.id}/events", non_member)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response.size).to eq(1)
+      end
+
+      it 'returns all events when the user has access' do
+        get api("/projects/#{public_project.id}/events", user)
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response.size).to eq(2)
+      end
+    end
+
+    context 'pagination' do
+      let(:public_project) { create(:project, :public) }
+
+      before do
+        create(:event,
+               project: public_project,
+               target: create(:issue, project: public_project, title: 'Issue 1'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-10'))
+        create(:event,
+               project: public_project,
+               target: create(:issue, confidential: true, project: public_project, title: 'Confidential event'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-11'))
+        create(:event,
+               project: public_project,
+               target: create(:issue, project: public_project, title: 'Issue 2'),
+               action: Event::CLOSED,
+               created_at: Date.parse('2018-12-12'))
+      end
+
+      it 'correctly returns the second page without inaccessible events' do
+        get api("/projects/#{public_project.id}/events", user), per_page: 2, page: 2
+
+        titles = json_response.map { |event| event['target_title'] }
+
+        expect(titles.first).to eq('Issue 1')
+        expect(titles).not_to include('Confidential event')
+      end
+
+      it 'correctly returns the first page without inaccessible events' do
+        get api("/projects/#{public_project.id}/events", user), per_page: 2, page: 1
+
+        titles = json_response.map { |event| event['target_title'] }
+
+        expect(titles.first).to eq('Issue 2')
+        expect(titles).not_to include('Confidential event')
+      end
+    end
+
     context 'when not permitted to read' do
       it 'returns 404' do
         get api("/projects/#{private_project.id}/events", non_member)

spec/requests/api/redacted_events_spec.rb

-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