diff --git a/changelogs/unreleased/secure-disallow-read-user-scope-to-read-project-events.yml b/changelogs/unreleased/secure-disallow-read-user-scope-to-read-project-events.yml new file mode 100644 index 00000000000..4a91bfa8827 --- /dev/null +++ b/changelogs/unreleased/secure-disallow-read-user-scope-to-read-project-events.yml @@ -0,0 +1,5 @@ +--- +title: Allow to see project events only with api scope token +merge_request: +author: +type: security diff --git a/lib/api/api.rb b/lib/api/api.rb index bf8ddba6f0d..a572cca24e9 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -134,6 +134,7 @@ module API mount ::API::Pipelines mount ::API::PipelineSchedules mount ::API::ProjectClusters + mount ::API::ProjectEvents mount ::API::ProjectExport mount ::API::ProjectImport mount ::API::ProjectHooks diff --git a/lib/api/events.rb b/lib/api/events.rb index b98aa9f31e1..e4c017fab42 100644 --- a/lib/api/events.rb +++ b/lib/api/events.rb @@ -4,34 +4,11 @@ module API class Events < Grape::API include PaginationParams include APIGuard + helpers ::API::Helpers::EventsHelpers - helpers do - params :event_filter_params do - optional :action, type: String, values: Event.actions, desc: 'Event action to filter on' - optional :target_type, type: String, values: Event.target_types, desc: 'Event target type to filter on' - optional :before, type: Date, desc: 'Include only events created before this date' - optional :after, type: Date, desc: 'Include only events created after this date' - end - - params :sort_params do - optional :sort, type: String, values: %w[asc desc], default: 'desc', - desc: 'Return events sorted in ascending and descending order' - end - - def present_events(events) - events = paginate(events) - - present events, with: Entities::Event - end - - def find_events(source) - EventsFinder.new(params.merge(source: source, current_user: current_user, with_associations: true)).execute - end - end + allow_access_with_scope :read_user, if: -> (request) { request.get? } resource :events do - allow_access_with_scope :read_user, if: -> (request) { request.get? } - desc "List currently authenticated user's events" do detail 'This feature was introduced in GitLab 9.3.' success Entities::Event @@ -55,8 +32,6 @@ module API requires :id, type: String, desc: 'The ID or Username of the user' end resource :users do - allow_access_with_scope :read_user, if: -> (request) { request.get? } - desc 'Get the contribution events of a specified user' do detail 'This feature was introduced in GitLab 8.13.' success Entities::Event @@ -76,25 +51,5 @@ module API present_events(events) end end - - params do - requires :id, type: String, desc: 'The ID of a project' - end - resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do - desc "List a Project's visible events" do - success Entities::Event - end - params do - use :pagination - use :event_filter_params - use :sort_params - end - - get ":id/events" do - events = find_events(user_project) - - present_events(events) - end - end end end diff --git a/lib/api/helpers/events_helpers.rb b/lib/api/helpers/events_helpers.rb new file mode 100644 index 00000000000..bf3b76bb92d --- /dev/null +++ b/lib/api/helpers/events_helpers.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module API + module Helpers + module EventsHelpers + extend Grape::API::Helpers + + params :event_filter_params do + optional :action, type: String, values: Event.actions, desc: 'Event action to filter on' + optional :target_type, type: String, values: Event.target_types, desc: 'Event target type to filter on' + optional :before, type: Date, desc: 'Include only events created before this date' + optional :after, type: Date, desc: 'Include only events created after this date' + end + + params :sort_params do + optional :sort, type: String, values: %w[asc desc], default: 'desc', + desc: 'Return events sorted in ascending and descending order' + end + + def present_events(events) + events = paginate(events) + + present events, with: Entities::Event + end + + def find_events(source) + EventsFinder.new(params.merge(source: source, current_user: current_user, with_associations: true)).execute + end + end + end +end diff --git a/lib/api/project_events.rb b/lib/api/project_events.rb new file mode 100644 index 00000000000..734311e1142 --- /dev/null +++ b/lib/api/project_events.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module API + class ProjectEvents < Grape::API + include PaginationParams + include APIGuard + helpers ::API::Helpers::EventsHelpers + + params do + requires :id, type: String, desc: 'The ID of a project' + end + resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do + desc "List a Project's visible events" do + success Entities::Event + end + params do + use :pagination + use :event_filter_params + use :sort_params + end + + get ":id/events" do + events = find_events(user_project) + + present_events(events) + end + end + end +end diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb index 065b16c6221..018691e8099 100644 --- a/spec/requests/api/events_spec.rb +++ b/spec/requests/api/events_spec.rb @@ -164,139 +164,4 @@ describe API::Events do expect(json_response['message']).to eq('404 User Not Found') end end - - describe 'GET /projects/:id/events' do - context 'when unauthenticated ' do - it 'returns 404 for private project' do - get api("/projects/#{private_project.id}/events") - - expect(response).to have_gitlab_http_status(404) - end - - it 'returns 200 status for a public project' do - public_project = create(:project, :public) - - get api("/projects/#{public_project.id}/events") - - expect(response).to have_gitlab_http_status(200) - 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), params: { 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), params: { 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) - - expect(response).to have_gitlab_http_status(404) - end - end - - context 'when authenticated' do - it 'returns project events' do - get api("/projects/#{private_project.id}/events?action=closed&target_type=issue&after=2016-12-1&before=2016-12-31", user) - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(json_response).to be_an Array - expect(json_response.size).to eq(1) - end - - it 'returns 404 if project does not exist' do - get api("/projects/1234/events", user) - - expect(response).to have_gitlab_http_status(404) - end - end - - context 'when exists some events' do - let(:merge_request1) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } - let(:merge_request2) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } - - before do - create_event(merge_request1) - end - - it 'avoids N+1 queries' do - control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do - get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request } - end.count - - create_event(merge_request2) - - expect do - get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request } - end.not_to exceed_all_query_limit(control_count) - - expect(response).to have_gitlab_http_status(200) - expect(response).to include_pagination_headers - expect(json_response.size).to eq(2) - expect(json_response.map { |r| r['target_id'] }).to match_array([merge_request1.id, merge_request2.id]) - end - - def create_event(target) - create(:event, project: private_project, author: user, target: target) - end - end - end end diff --git a/spec/requests/api/project_events_spec.rb b/spec/requests/api/project_events_spec.rb new file mode 100644 index 00000000000..43df9993eb9 --- /dev/null +++ b/spec/requests/api/project_events_spec.rb @@ -0,0 +1,156 @@ +require 'spec_helper' + +describe API::ProjectEvents do + include ApiHelpers + + let(:user) { create(:user) } + let(:non_member) { create(:user) } + let(:private_project) { create(:project, :private, creator_id: user.id, namespace: user.namespace) } + let(:closed_issue) { create(:closed_issue, project: private_project, author: user) } + let!(:closed_issue_event) { create(:event, project: private_project, author: user, target: closed_issue, action: Event::CLOSED, created_at: Date.new(2016, 12, 30)) } + + describe 'GET /projects/:id/events' do + context 'when unauthenticated ' do + it 'returns 404 for private project' do + get api("/projects/#{private_project.id}/events") + + expect(response).to have_gitlab_http_status(404) + end + + it 'returns 200 status for a public project' do + public_project = create(:project, :public) + + get api("/projects/#{public_project.id}/events") + + expect(response).to have_gitlab_http_status(200) + 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), params: { 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), params: { 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) + + expect(response).to have_gitlab_http_status(404) + end + end + + context 'when authenticated' do + it 'returns project events' do + get api("/projects/#{private_project.id}/events?action=closed&target_type=issue&after=2016-12-1&before=2016-12-31", user) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response.size).to eq(1) + end + + it 'returns 404 if project does not exist' do + get api("/projects/1234/events", user) + + expect(response).to have_gitlab_http_status(404) + end + + context 'when the requesting token does not have "api" scope' do + let(:token) { create(:personal_access_token, scopes: ['read_user'], user: user) } + + it 'returns a "403" response' do + get api("/projects/#{private_project.id}/events", personal_access_token: token) + + expect(response).to have_gitlab_http_status(403) + end + end + end + + context 'when exists some events' do + let(:merge_request1) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } + let(:merge_request2) { create(:merge_request, :closed, author: user, assignees: [user], source_project: private_project, title: 'Test') } + + before do + create_event(merge_request1) + end + + it 'avoids N+1 queries' do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do + get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request } + end.count + + create_event(merge_request2) + + expect do + get api("/projects/#{private_project.id}/events", user), params: { target_type: :merge_request } + end.not_to exceed_all_query_limit(control_count) + + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response.size).to eq(2) + expect(json_response.map { |r| r['target_id'] }).to match_array([merge_request1.id, merge_request2.id]) + end + + def create_event(target) + create(:event, project: private_project, author: user, target: target) + end + end + end +end