Commit a04c4ae3 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Merge branch 'fix-events-finder-incomplete' into 'master'

[master] Redact events shown in the events API

See merge request gitlab/gitlabhq!2514
parents e5d3a75a 32930ecd
...@@ -12,6 +12,7 @@ class EventsFinder ...@@ -12,6 +12,7 @@ class EventsFinder
# Arguments: # Arguments:
# source - which user or project to looks for events on # source - which user or project to looks for events on
# current_user - only return events for projects visible to this user # current_user - only return events for projects visible to this user
# WARNING: does not consider project feature visibility!
# params: # params:
# action: string # action: string
# target_type: string # target_type: string
......
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
# Get user activity feed for projects common for a user and a logged in user # Get user activity feed for projects common for a user and a logged in user
# #
# - current_user: The user viewing the events # - current_user: The user viewing the events
# WARNING: does not consider project feature visibility!
# - user: The user for which to load the events # - user: The user for which to load the events
# - params: # - params:
# - offset: The page of events to return # - offset: The page of events to return
......
...@@ -148,6 +148,8 @@ class Event < ActiveRecord::Base ...@@ -148,6 +148,8 @@ class Event < ActiveRecord::Base
end end
end end
# rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/PerceivedComplexity
def visible_to_user?(user = nil) def visible_to_user?(user = nil)
if push? || commit_note? if push? || commit_note?
Ability.allowed?(user, :download_code, project) Ability.allowed?(user, :download_code, project)
...@@ -159,12 +161,18 @@ class Event < ActiveRecord::Base ...@@ -159,12 +161,18 @@ class Event < ActiveRecord::Base
Ability.allowed?(user, :read_issue, note? ? note_target : target) Ability.allowed?(user, :read_issue, note? ? note_target : target)
elsif merge_request? || merge_request_note? elsif merge_request? || merge_request_note?
Ability.allowed?(user, :read_merge_request, note? ? note_target : target) 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? elsif milestone?
Ability.allowed?(user, :read_project, project) Ability.allowed?(user, :read_milestone, project)
else else
false # No other event types are visible false # No other event types are visible
end end
end end
# rubocop:enable Metrics/PerceivedComplexity
# rubocop:enable Metrics/CyclomaticComplexity
def project_name def project_name
if project if project
...@@ -306,6 +314,10 @@ class Event < ActiveRecord::Base ...@@ -306,6 +314,10 @@ class Event < ActiveRecord::Base
note? && target && target.for_snippet? note? && target && target.for_snippet?
end end
def personal_snippet_note?
note? && target && target.for_personal_snippet?
end
def note_target def note_target
target.noteable target.noteable
end end
......
---
title: Redact confidential events in the API
merge_request:
author:
type: security
...@@ -16,12 +16,27 @@ module API ...@@ -16,12 +16,27 @@ module API
desc: 'Return events sorted in ascending and descending order' desc: 'Return events sorted in ascending and descending order'
end 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 # rubocop: disable CodeReuse/ActiveRecord
def present_events(events) def present_events(events, redact: true)
events = events.reorder(created_at: params[:sort]) events = events.reorder(created_at: params[:sort])
.with_associations .with_associations
present paginate(events), with: Entities::Event events = paginate(events)
events = redact_events(events) if redact
present events, with: Entities::Event
end end
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
end end
...@@ -44,7 +59,8 @@ module API ...@@ -44,7 +59,8 @@ module API
events = EventsFinder.new(params.merge(source: current_user, current_user: current_user)).execute.preload(:author, :target) 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
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
end end
......
...@@ -148,9 +148,14 @@ describe Event do ...@@ -148,9 +148,14 @@ describe Event do
let(:admin) { create(:admin) } let(:admin) { create(:admin) }
let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) } let(:issue) { create(:issue, project: project, author: author, assignees: [assignee]) }
let(:confidential_issue) { create(:issue, :confidential, 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_commit) { create(:note_on_commit, project: project) }
let(:note_on_issue) { create(:note_on_issue, noteable: issue, 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_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) } let(:event) { described_class.new(project: project, target: target, author_id: author.id) }
before do before do
...@@ -268,6 +273,125 @@ describe Event do ...@@ -268,6 +273,125 @@ describe Event do
end end
end 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 end
describe '.limit_recent' do describe '.limit_recent' do
......
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