diff options
author | Bob Van Landuyt <bob@gitlab.com> | 2018-09-24 15:54:07 +0000 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2018-09-24 18:01:14 +0200 |
commit | 20d6be94c90a268a531280d6c4cab14b45439a08 (patch) | |
tree | 764953c5b44286ec08402dac6acd7b88820bb957 | |
parent | 5ddc012d457acda07189ff70ea210c47bf0c0402 (diff) | |
download | gitlab-ce-20d6be94c90a268a531280d6c4cab14b45439a08.tar.gz |
Merge branch 'fix-events-finder-incomplete-11-2' into 'security-11-2'
[11.2] Redact events shown in the events API
See merge request gitlab/gitlabhq!2519
-rw-r--r-- | app/finders/events_finder.rb | 1 | ||||
-rw-r--r-- | app/finders/user_recent_events_finder.rb | 1 | ||||
-rw-r--r-- | app/models/event.rb | 20 | ||||
-rw-r--r-- | changelogs/unreleased/fix-events-finder-incomplete.yml | 5 | ||||
-rw-r--r-- | lib/api/events.rb | 22 | ||||
-rw-r--r-- | spec/models/event_spec.rb | 124 | ||||
-rw-r--r-- | spec/requests/api/redacted_events_spec.rb | 68 |
7 files changed, 235 insertions, 6 deletions
diff --git a/app/finders/events_finder.rb b/app/finders/events_finder.rb index 8676925a540..eb8af63eeb9 100644 --- a/app/finders/events_finder.rb +++ b/app/finders/events_finder.rb @@ -10,6 +10,7 @@ class EventsFinder # 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 diff --git a/app/finders/user_recent_events_finder.rb b/app/finders/user_recent_events_finder.rb index 876f086a3ef..9a063a9a252 100644 --- a/app/finders/user_recent_events_finder.rb +++ b/app/finders/user_recent_events_finder.rb @@ -1,6 +1,7 @@ # Get user activity feed for projects common for a user and a logged in user # # - current_user: The user viewing the events +# WARNING: does not consider project feature visibility! # - user: The user for which to load the events # - params: # - offset: The page of events to return diff --git a/app/models/event.rb b/app/models/event.rb index ba28866e8e6..96baf523536 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -147,21 +147,31 @@ class Event < ActiveRecord::Base end end + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/PerceivedComplexity def visible_to_user?(user = nil) if push? || commit_note? Ability.allowed?(user, :download_code, project) elsif membership_changed? - true + Ability.allowed?(user, :read_project, project) elsif created_project? - true + Ability.allowed?(user, :read_project, project) elsif issue? || issue_note? Ability.allowed?(user, :read_issue, note? ? note_target : target) elsif merge_request? || merge_request_note? Ability.allowed?(user, :read_merge_request, note? ? note_target : target) + elsif personal_snippet_note? + Ability.allowed?(user, :read_personal_snippet, note_target) + elsif project_snippet_note? + Ability.allowed?(user, :read_project_snippet, note_target) + elsif milestone? + Ability.allowed?(user, :read_milestone, project) else - milestone? + false end end + # rubocop:enable Metrics/PerceivedComplexity + # rubocop:enable Metrics/CyclomaticComplexity def project_name if project @@ -303,6 +313,10 @@ class Event < ActiveRecord::Base note? && target && target.for_snippet? end + def personal_snippet_note? + note? && target && target.for_personal_snippet? + end + def note_target target.noteable end diff --git a/changelogs/unreleased/fix-events-finder-incomplete.yml b/changelogs/unreleased/fix-events-finder-incomplete.yml new file mode 100644 index 00000000000..f3a4e421d33 --- /dev/null +++ b/changelogs/unreleased/fix-events-finder-incomplete.yml @@ -0,0 +1,5 @@ +--- +title: Redact confidential events in the API +merge_request: +author: +type: security diff --git a/lib/api/events.rb b/lib/api/events.rb index fc4ba5a3188..357ca83ddab 100644 --- a/lib/api/events.rb +++ b/lib/api/events.rb @@ -15,11 +15,26 @@ module API desc: 'Return events sorted in ascending and descending order' end - def present_events(events) + 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 + + def present_events(events, redact: true) events = events.reorder(created_at: params[:sort]) .with_associations - present paginate(events), with: Entities::Event + events = paginate(events) + events = redact_events(events) if redact + + present events, with: Entities::Event end end @@ -38,7 +53,8 @@ module API events = EventsFinder.new(params.merge(source: current_user, current_user: current_user)).execute.preload(:author, :target) - present_events(events) + # Since we're viewing our own events, redaction is unnecessary + present_events(events, redact: false) end end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index c1eac4fa489..81748681528 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -148,9 +148,14 @@ describe Event do let(:admin) { create(:admin) } let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) } let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignees: [assignee]) } + let(:project_snippet) { create(:project_snippet, :public, project: project, author: author) } + let(:personal_snippet) { create(:personal_snippet, :public, author: author) } let(:note_on_commit) { create(:note_on_commit, project: project) } let(:note_on_issue) { create(:note_on_issue, noteable: issue, project: project) } let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project) } + let(:note_on_project_snippet) { create(:note_on_project_snippet, author: author, noteable: project_snippet, project: project) } + let(:note_on_personal_snippet) { create(:note_on_personal_snippet, author: author, noteable: personal_snippet, project: nil) } + let(:milestone_on_project) { create(:milestone, project: project) } let(:event) { described_class.new(project: project, target: target, author_id: author.id) } before do @@ -268,6 +273,125 @@ describe Event do end end end + + context 'milestone event' do + let(:target) { milestone_on_project } + + it do + expect(event.visible_to_user?(nil)).to be_truthy + expect(event.visible_to_user?(non_member)).to be_truthy + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + + context 'on public project with private issue tracker and merge requests' do + let(:project) { create(:project, :public, :issues_private, :merge_requests_private) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_falsy + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + end + + context 'on private project' do + let(:project) { create(:project, :private) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_falsy + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + end + end + + context 'project snippet note event' do + let(:target) { note_on_project_snippet } + + it do + expect(event.visible_to_user?(nil)).to be_truthy + expect(event.visible_to_user?(non_member)).to be_truthy + expect(event.visible_to_user?(author)).to be_truthy + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + + context 'on public project with private snippets' do + let(:project) { create(:project, :public, :snippets_private) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_falsy + + # Normally, we'd expect the author of a comment to be able to view it. + # However, this doesn't seem to be the case for comments on snippets. + expect(event.visible_to_user?(author)).to be_falsy + + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + end + + context 'on private project' do + let(:project) { create(:project, :private) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_falsy + + # Normally, we'd expect the author of a comment to be able to view it. + # However, this doesn't seem to be the case for comments on snippets. + expect(event.visible_to_user?(author)).to be_falsy + + expect(event.visible_to_user?(member)).to be_truthy + expect(event.visible_to_user?(guest)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + end + end + + context 'personal snippet note event' do + let(:target) { note_on_personal_snippet } + + it do + expect(event.visible_to_user?(nil)).to be_truthy + expect(event.visible_to_user?(non_member)).to be_truthy + expect(event.visible_to_user?(author)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + + context 'on internal snippet' do + let(:personal_snippet) { create(:personal_snippet, :internal, author: author) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_truthy + expect(event.visible_to_user?(author)).to be_truthy + expect(event.visible_to_user?(admin)).to be_truthy + end + end + + context 'on private snippet' do + let(:personal_snippet) { create(:personal_snippet, :private, author: author) } + + it do + expect(event.visible_to_user?(nil)).to be_falsy + expect(event.visible_to_user?(non_member)).to be_falsy + expect(event.visible_to_user?(author)).to be_truthy + + # It is very unexpected that a private personal snippet is not visible + # to an instance administrator. This should be fixed in the future. + expect(event.visible_to_user?(admin)).to be_falsy + end + end + end end describe '.limit_recent' do diff --git a/spec/requests/api/redacted_events_spec.rb b/spec/requests/api/redacted_events_spec.rb new file mode 100644 index 00000000000..086dd3df9ba --- /dev/null +++ b/spec/requests/api/redacted_events_spec.rb @@ -0,0 +1,68 @@ +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 |