diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-12-07 18:09:00 +0100 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-12-17 18:47:53 +0100 |
commit | 28acd2b087d5b80cd89354d58f937aed0f4928cb (patch) | |
tree | 0eda3c8ee7be722d51a390c750f1fd39dd88276b | |
parent | 75262862c434a98b9183a4a63f3ad86dec52b079 (diff) | |
download | gitlab-ce-28acd2b087d5b80cd89354d58f937aed0f4928cb.tar.gz |
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.
-rw-r--r-- | app/finders/concerns/finder_with_cross_project_access.rb | 30 | ||||
-rw-r--r-- | app/finders/events_finder.rb | 45 | ||||
-rw-r--r-- | changelogs/unreleased/bvl-hide-confidential-events-take2.yml | 5 | ||||
-rw-r--r-- | lib/api/events.rb | 42 | ||||
-rw-r--r-- | spec/finders/concerns/finder_with_cross_project_access_spec.rb | 16 | ||||
-rw-r--r-- | spec/finders/events_finder_spec.rb | 17 | ||||
-rw-r--r-- | spec/finders/user_recent_events_finder_spec.rb | 3 | ||||
-rw-r--r-- | spec/requests/api/events_spec.rb | 62 | ||||
-rw-r--r-- | spec/requests/api/redacted_events_spec.rb | 68 |
9 files changed, 176 insertions, 112 deletions
diff --git a/app/finders/concerns/finder_with_cross_project_access.rb b/app/finders/concerns/finder_with_cross_project_access.rb index 220f62bcc7f..06ebb286086 100644 --- a/app/finders/concerns/finder_with_cross_project_access.rb +++ b/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 diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index 8df01f1dad9..234b7090fd9 100644 --- a/app/finders/events_finder.rb +++ b/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 diff --git a/changelogs/unreleased/bvl-hide-confidential-events-take2.yml b/changelogs/unreleased/bvl-hide-confidential-events-take2.yml new file mode 100644 index 00000000000..a5abd496a9d --- /dev/null +++ b/changelogs/unreleased/bvl-hide-confidential-events-take2.yml @@ -0,0 +1,5 @@ +--- +title: Hide confidential events in the API +merge_request: 23746 +author: +type: other diff --git a/lib/api/events.rb b/lib/api/events.rb index 44dae57770d..b98aa9f31e1 100644 --- a/lib/api/events.rb +++ b/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 diff --git a/spec/finders/concerns/finder_with_cross_project_access_spec.rb b/spec/finders/concerns/finder_with_cross_project_access_spec.rb index 1ff65a8101b..f29acb521a8 100644 --- a/spec/finders/concerns/finder_with_cross_project_access_spec.rb +++ b/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 diff --git a/spec/finders/events_finder_spec.rb b/spec/finders/events_finder_spec.rb index 62968e83292..3bce46cc4d1 100644 --- a/spec/finders/events_finder_spec.rb +++ b/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 } diff --git a/spec/finders/user_recent_events_finder_spec.rb b/spec/finders/user_recent_events_finder_spec.rb index c5fcd68eb4c..5ebceeb7586 100644 --- a/spec/finders/user_recent_events_finder_spec.rb +++ b/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 diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb index 573eebe314d..c4e3ac14441 100644 --- a/spec/requests/api/events_spec.rb +++ b/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) diff --git a/spec/requests/api/redacted_events_spec.rb b/spec/requests/api/redacted_events_spec.rb deleted file mode 100644 index 086dd3df9ba..00000000000 --- a/spec/requests/api/redacted_events_spec.rb +++ /dev/null @@ -1,68 +0,0 @@ -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 |