From b092924c4afacfc161ec35f91f48a17fc94190d7 Mon Sep 17 00:00:00 2001
From: Adam Hegyi <ahegyi@gitlab.com>
Date: Wed, 4 Dec 2019 16:35:46 +0100
Subject: [PATCH] Enforce permission check when counting events
- Call `visible_to_user?` when counting events
- Caching `visible_to_user?` to reduce the number of queries
- Add ee factory for events to create valid group event
---
app/controllers/dashboard_controller.rb | 3 +-
app/controllers/groups_controller.rb | 7 ++--
app/controllers/projects_controller.rb | 3 +-
app/presenters/event_presenter.rb | 12 ++++++
...nforce-permissions-for-event-filter-ee.yml | 5 +++
.../controllers/ee/groups_controller_spec.rb | 6 +--
ee/spec/factories/events.rb | 13 ++++++
spec/controllers/dashboard_controller_spec.rb | 41 +++++++++++++++++++
spec/controllers/groups_controller_spec.rb | 37 ++++++++++++++---
spec/controllers/projects_controller_spec.rb | 40 ++++++++++++++++++
10 files changed, 153 insertions(+), 14 deletions(-)
create mode 100644 changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml
create mode 100644 ee/spec/factories/events.rb
diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb
index 1a97b39d3ae1..1668cf004f80 100644
--- a/app/controllers/dashboard_controller.rb
+++ b/app/controllers/dashboard_controller.rb
@@ -19,7 +19,7 @@ def activity
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 @@ def load_events
@events = EventCollection
.new(projects, offset: params[:offset].to_i, filter: event_filter)
.to_a
+ .map(&:present)
Events::RenderService.new(current_user).execute(@events)
end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 0953ca963175..958dc27984f4 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -91,7 +91,7 @@ def activity
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 @@ def load_events
.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)
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index bf05defbc2e6..9d0901881d3e 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -118,7 +118,7 @@ def activity
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 @@ def load_events
@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
diff --git a/app/presenters/event_presenter.rb b/app/presenters/event_presenter.rb
index f31d362d5fa7..5657e0b96bce 100644
--- a/app/presenters/event_presenter.rb
+++ b/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
diff --git a/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml b/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml
new file mode 100644
index 000000000000..7d74d6108f87
--- /dev/null
+++ b/changelogs/unreleased/security-enforce-permissions-for-event-filter-ee.yml
@@ -0,0 +1,5 @@
+---
+title: Enforce permission check when counting activity events
+merge_request:
+author:
+type: security
diff --git a/ee/spec/controllers/ee/groups_controller_spec.rb b/ee/spec/controllers/ee/groups_controller_spec.rb
index 939920a0fb4e..4dc696a155c5 100644
--- a/ee/spec/controllers/ee/groups_controller_spec.rb
+++ b/ee/spec/controllers/ee/groups_controller_spec.rb
@@ -15,9 +15,9 @@
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
diff --git a/ee/spec/factories/events.rb b/ee/spec/factories/events.rb
new file mode 100644
index 000000000000..9c6f15ed47e3
--- /dev/null
+++ b/ee/spec/factories/events.rb
@@ -0,0 +1,13 @@
+# 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
diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb
index a733c3ecaa1f..305419efe969 100644
--- a/spec/controllers/dashboard_controller_spec.rb
+++ b/spec/controllers/dashboard_controller_spec.rb
@@ -23,6 +23,47 @@
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
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 2ed2b3192988..ddfd2b424e76 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -47,7 +47,7 @@
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 '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 @@
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
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 9ae1277de26b..3870ef5d947d 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -64,6 +64,46 @@
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
--
GitLab