Merge branch 'security-enforce-permissions-for-event-filter-ee' into 'master'

Enforce permission check when counting events

See merge request gitlab-org/security/gitlab!51

app/controllers/dashboard_controller.rb

@@ -19,7 +19,7 @@ class DashboardController < Dashboard::ApplicationController
 
       format.json do
         load_events
-        pager_json("events/_events", @events.count)
+        pager_json('events/_events', @events.count { |event| event.visible_to_user?(current_user) })
       end
     end
   end
@@ -37,6 +37,7 @@ class DashboardController < Dashboard::ApplicationController
     @events = EventCollection
       .new(projects, offset: params[:offset].to_i, filter: event_filter)
       .to_a
+      .map(&:present)
 
     Events::RenderService.new(current_user).execute(@events)
   end

app/controllers/groups_controller.rb

@@ -91,7 +91,7 @@ class GroupsController < Groups::ApplicationController
 
       format.json do
         load_events
-        pager_json("events/_events", @events.count)
+        pager_json("events/_events", @events.count { |event| event.visible_to_user?(current_user) })
       end
     end
   end
@@ -209,8 +209,9 @@ class GroupsController < Groups::ApplicationController
                  .includes(:namespace)
 
     @events = EventCollection
-                .new(projects, offset: params[:offset].to_i, filter: event_filter, groups: groups)
-                .to_a
+      .new(projects, offset: params[:offset].to_i, filter: event_filter, groups: groups)
+      .to_a
+      .map(&:present)
 
     Events::RenderService
       .new(current_user)

app/controllers/projects_controller.rb

@@ -118,7 +118,7 @@ class ProjectsController < Projects::ApplicationController
       format.html
       format.json do
         load_events
-        pager_json('events/_events', @events.count)
+        pager_json('events/_events', @events.count { |event| event.visible_to_user?(current_user) })
       end
     end
   end
@@ -343,6 +343,7 @@ class ProjectsController < Projects::ApplicationController
     @events = EventCollection
       .new(projects, offset: params[:offset].to_i, filter: event_filter)
       .to_a
+      .map(&:present)
 
     Events::RenderService.new(current_user).execute(@events, atom_request: request.format.atom?)
   end

app/presenters/event_presenter.rb

@@ -3,6 +3,18 @@
 class EventPresenter < Gitlab::View::Presenter::Delegated
   presents :event
 
+  def initialize(subject, **attributes)
+    super
+
+    @visible_to_user_cache = ActiveSupport::Cache::MemoryStore.new
+  end
+
+  # Caching `visible_to_user?` method in the presenter beause it might be called multiple times.
+  def visible_to_user?(user = nil)
+    @visible_to_user_cache.fetch(user&.id) { super(user) }
+  end
+
+  # implement cache here
   def resource_parent_name
     resource_parent&.full_name || ''
   end

changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml

+---
+title: Enforce permission check when counting activity events
+merge_request:
+author:
+type: security

ee/spec/controllers/ee/groups_controller_spec.rb

@@ -15,9 +15,9 @@ describe GroupsController do
     render_views
 
     let_it_be(:event1) { create(:event, project: project) }
-    let_it_be(:event2) { create(:event, project: nil, group: group) }
-    let_it_be(:event3) { create(:event, project: nil, group: subgroup) }
-    let_it_be(:event4) { create(:event, project: nil, group: subgroup2) }
+    let_it_be(:event2) { create(:event, :epic_create_event, group: group) }
+    let_it_be(:event3) { create(:event, :epic_create_event, group: subgroup) }
+    let_it_be(:event4) { create(:event, :epic_create_event, group: subgroup2) }
 
     context 'when authorized' do
       before do

ee/spec/factories/events.rb

+# frozen_string_literal: true
+
+FactoryBot.modify do
+  factory :event do
+    trait :epic_create_event do
+      group
+      author(factory: :user)
+      target(factory: :epic)
+      action { Event::CREATED }
+      project { nil }
+    end
+  end
+end

spec/controllers/dashboard_controller_spec.rb

@@ -23,6 +23,47 @@ describe DashboardController do
     end
   end
 
+  describe "GET activity as JSON" do
+    render_views
+
+    let(:user) { create(:user) }
+    let(:project) { create(:project, :public, issues_access_level: ProjectFeature::PRIVATE) }
+
+    before do
+      create(:event, :created, project: project, target: create(:issue))
+
+      sign_in(user)
+
+      request.cookies[:event_filter] = 'all'
+    end
+
+    context 'when user has permission to see the event' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns count' do
+        get :activity, params: { format: :json }
+
+        expect(json_response['count']).to eq(1)
+      end
+    end
+
+    context 'when user has no permission to see the event' do
+      it 'filters out invisible event' do
+        get :activity, params: { format: :json }
+
+        expect(json_response['html']).to include(_('No activities found'))
+      end
+
+      it 'filters out invisible event when calculating the count' do
+        get :activity, params: { format: :json }
+
+        expect(json_response['count']).to eq(0)
+      end
+    end
+  end
+
   it_behaves_like 'authenticates sessionless user', :issues, :atom, author_id: User.first
   it_behaves_like 'authenticates sessionless user', :issues_calendar, :ics
 

spec/controllers/groups_controller_spec.rb

@@ -47,7 +47,7 @@ describe GroupsController do
 
       it 'assigns events for all the projects in the group', :sidekiq_might_not_need_inline do
         subject
-        expect(assigns(:events)).to contain_exactly(event)
+        expect(assigns(:events).map(&:id)).to contain_exactly(event.id)
       end
     end
   end
@@ -119,12 +119,12 @@ describe GroupsController do
   describe 'GET #activity' do
     render_views
 
-    before do
-      sign_in(user)
-      project
-    end
-
     context 'as json' do
+      before do
+        sign_in(user)
+        project
+      end
+
       it 'includes events from all projects in group and subgroups', :sidekiq_might_not_need_inline do
         2.times do
           project = create(:project, group: group)
@@ -141,6 +141,31 @@ describe GroupsController do
         expect(assigns(:projects).limit_value).to be_nil
       end
     end
+
+    context 'when user has no permission to see the event' do
+      let(:user) { create(:user) }
+      let(:group) { create(:group) }
+      let(:project) { create(:project, group: group) }
+
+      let(:project_with_restricted_access) do
+        create(:project, :public, issues_access_level: ProjectFeature::PRIVATE, group: group)
+      end
+
+      before do
+        create(:event, project: project)
+        create(:event, :created, project: project_with_restricted_access, target: create(:issue))
+
+        group.add_guest(user)
+
+        sign_in(user)
+      end
+
+      it 'filters out invisible event' do
+        get :activity, params: { id: group.to_param }, format: :json
+
+        expect(json_response['count']).to eq(1)
+      end
+    end
   end
 
   describe 'POST #create' do

spec/controllers/projects_controller_spec.rb

@@ -64,6 +64,46 @@ describe ProjectsController do
     end
   end
 
+  describe "GET #activity as JSON" do
+    render_views
+
+    let(:project) { create(:project, :public, issues_access_level: ProjectFeature::PRIVATE) }
+
+    before do
+      create(:event, :created, project: project, target: create(:issue))
+
+      sign_in(user)
+
+      request.cookies[:event_filter] = 'all'
+    end
+
+    context 'when user has permission to see the event' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns count' do
+        get :activity, params: { namespace_id: project.namespace, id: project, format: :json }
+
+        expect(json_response['count']).to eq(1)
+      end
+    end
+
+    context 'when user has no permission to see the event' do
+      it 'filters out invisible event' do
+        get :activity, params: { namespace_id: project.namespace, id: project, format: :json }
+
+        expect(json_response['html']).to eq("\n")
+      end
+
+      it 'filters out invisible event when calculating the count' do
+        get :activity, params: { namespace_id: project.namespace, id: project, format: :json }
+
+        expect(json_response['count']).to eq(0)
+      end
+    end
+  end
+
   describe "GET show" do
     context "user not project member" do
       before do