Commit 28acd2b0 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Hide confidential events in ruby

We're filtering the events using `Event#visible_to_user?`.

At most we're loading 100 events at once.

Pagination is also dealt with in the finder, but the resulting array
is wrapped in a `Kaminari.paginate_array` so the API's pagination
helpers keep working. We're passing the total count into that
paginatable array, which would include confidential events. But we're
not disclosing anything.
parent 75262862
......@@ -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
......
......@@ -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
---
title: Hide confidential events in the API
merge_request: 23746
author:
type: other
......@@ -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
......@@ -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
......@@ -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 }
......
......@@ -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
......
......@@ -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)
......
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
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment