diff options
author | drew cimino <dcimino@gitlab.com> | 2019-06-28 10:40:34 -0400 |
---|---|---|
committer | drew cimino <dcimino@gitlab.com> | 2019-07-05 11:24:33 -0400 |
commit | 019caa8de59f0ca701d4f099a4068605b17e3b93 (patch) | |
tree | 75d68bf840dd0de0261595ddf4b64b2740de6740 | |
parent | 2ad75a4f96c4d377e18788966e7eefee4d78b6d2 (diff) | |
download | gitlab-ce-019caa8de59f0ca701d4f099a4068605b17e3b93.tar.gz |
Use MergeRequest#source_project as permissions reference for MergeRequest#all_pipelines
MergeRequest#all_pipelines fetches Ci::Pipeline records from the source
project, so we should specifically check that project for permissions.
This was already happening for intra-project merge requests, but in the
event that the target and source projects both have private builds, we
should ensure that the project permissions are respected.
4 files changed, 102 insertions, 6 deletions
diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index dcc272aecff..006731c0e66 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -45,7 +45,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont def set_pipeline_variables @pipelines = - if can?(current_user, :read_pipeline, @project) + if can?(current_user, :read_pipeline, @merge_request.source_project) @merge_request.all_pipelines else Ci::Pipeline.none diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7ee8e0ea8f8..7f87fc3792a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -82,7 +82,8 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo end def pipelines - @pipelines = @merge_request.all_pipelines.page(params[:page]).per(30) + set_pipeline_variables + @pipelines = @pipelines.page(params[:page]).per(30) Gitlab::PollingInterval.set_header(response, interval: 10_000) diff --git a/changelogs/unreleased/security-mr-pipeline-permissions.yml b/changelogs/unreleased/security-mr-pipeline-permissions.yml new file mode 100644 index 00000000000..a317c93228c --- /dev/null +++ b/changelogs/unreleased/security-mr-pipeline-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Use source project as permissions reference for MergeRequestsController#pipelines +merge_request: +author: +type: security diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 0eca663a683..64a89205e2f 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -623,10 +623,100 @@ describe Projects::MergeRequestsController do format: :json end - it 'responds with serialized pipelines' do - expect(json_response['pipelines']).not_to be_empty - expect(json_response['count']['all']).to eq 1 - expect(response).to include_pagination_headers + context 'with "enabled" builds on a public project' do + let(:project) { create(:project, :repository, :public) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with private builds on a public project' do + let(:project) { create(:project, :repository, :public, :builds_private) } + + context 'for a project owner' do + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for an unassociated user' do + let(:user) { create :user } + + it 'responds with no pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'from a project fork' do + let(:fork_user) { create :user } + let(:forked_project) { fork_project(project, fork_user, repository: true) } # Forked project carries over :builds_private + let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: forked_project) } + + context 'with private builds' do + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_empty + expect(json_response['count']['all']).to eq(0) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + + context 'with public builds' do + let(:forked_project) do + fork_project(project, fork_user, repository: true).tap do |new_project| + new_project.project_feature.update(builds_access_level: ProjectFeature::ENABLED) + end + end + + context 'for the target project member' do + it 'does not respond with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + + context 'for the source project member' do + let(:user) { fork_user } + + it 'responds with serialized pipelines' do + expect(json_response['pipelines']).to be_present + expect(json_response['count']['all']).to eq(1) + expect(response).to include_pagination_headers + end + end + end + end end end |