Commit 3fc0564a authored by Sean McGivern's avatar Sean McGivern Committed by Stan Hu

Merge branch '41567-projectfix' into 'security-10-3'

check project access on MR create

See merge request gitlab/gitlabhq!2273

(cherry picked from commit 1fe2325d6ef2bced4c5e97b57691c894f38b2834)

43e85f49 check project access on MR create
parent 954a4457
module MergeRequests module MergeRequests
class CreateService < MergeRequests::BaseService class CreateService < MergeRequests::BaseService
def execute def execute
# @project is used to determine whether the user can set the merge request's set_projects!
# assignee, milestone and labels. Whether they can depends on their
# permissions on the target project.
source_project = @project
@project = Project.find(params[:target_project_id]) if params[:target_project_id]
merge_request = MergeRequest.new merge_request = MergeRequest.new
merge_request.target_project = @project merge_request.target_project = @project
merge_request.source_project = source_project merge_request.source_project = @source_project
merge_request.source_branch = params[:source_branch] merge_request.source_branch = params[:source_branch]
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
...@@ -58,5 +54,25 @@ module MergeRequests ...@@ -58,5 +54,25 @@ module MergeRequests
pipelines.order(id: :desc).first pipelines.order(id: :desc).first
end end
def set_projects!
# @project is used to determine whether the user can set the merge request's
# assignee, milestone and labels. Whether they can depends on their
# permissions on the target project.
@source_project = @project
@project = Project.find(params[:target_project_id]) if params[:target_project_id]
# make sure that source/target project ids are not in
# params so it can't be overridden later when updating attributes
# from params when applying quick actions
params.delete(:source_project_id)
params.delete(:target_project_id)
unless can?(current_user, :read_project, @source_project) &&
can?(current_user, :read_project, @project)
raise Gitlab::Access::AccessDeniedError
end
end
end end
end end
---
title: Check user authorization for source and target projects when creating a merge
request.
merge_request:
author:
type: security
...@@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do ...@@ -113,6 +113,7 @@ feature 'Cycle Analytics', :js do
context "as a guest" do context "as a guest" do
before do before do
project.add_developer(user)
project.add_guest(guest) project.add_guest(guest)
allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue]) allow_any_instance_of(Gitlab::ReferenceExtractor).to receive(:issues).and_return([issue])
......
...@@ -92,6 +92,10 @@ describe MicrosoftTeamsService do ...@@ -92,6 +92,10 @@ describe MicrosoftTeamsService do
service.hook_data(merge_request, 'open') service.hook_data(merge_request, 'open')
end end
before do
project.add_developer(user)
end
it "calls Microsoft Teams API" do it "calls Microsoft Teams API" do
chat_service.execute(merge_sample_data) chat_service.execute(merge_sample_data)
......
...@@ -754,16 +754,28 @@ describe API::MergeRequests do ...@@ -754,16 +754,28 @@ describe API::MergeRequests do
expect(response).to have_gitlab_http_status(400) expect(response).to have_gitlab_http_status(400)
end end
context 'when target_branch is specified' do context 'when target_branch and target_project_id is specified' do
let(:params) do
{ title: 'Test merge_request',
target_branch: 'master',
source_branch: 'markdown',
author: user2,
target_project_id: unrelated_project.id }
end
it 'returns 422 if targeting a different fork' do it 'returns 422 if targeting a different fork' do
post api("/projects/#{forked_project.id}/merge_requests", user2), unrelated_project.add_developer(user2)
title: 'Test merge_request',
target_branch: 'master', post api("/projects/#{forked_project.id}/merge_requests", user2), params
source_branch: 'markdown',
author: user2,
target_project_id: unrelated_project.id
expect(response).to have_gitlab_http_status(422) expect(response).to have_gitlab_http_status(422)
end end
it 'returns 403 if targeting a different fork which user can not access' do
post api("/projects/#{forked_project.id}/merge_requests", user2), params
expect(response).to have_gitlab_http_status(403)
end
end end
it "returns 201 when target_branch is specified and for the same project" do it "returns 201 when target_branch is specified and for the same project" do
......
...@@ -371,16 +371,28 @@ describe API::MergeRequests do ...@@ -371,16 +371,28 @@ describe API::MergeRequests do
expect(response).to have_gitlab_http_status(400) expect(response).to have_gitlab_http_status(400)
end end
context 'when target_branch is specified' do context 'when target_branch and target_project_id is specified' do
let(:params) do
{ title: 'Test merge_request',
target_branch: 'master',
source_branch: 'markdown',
author: user2,
target_project_id: unrelated_project.id }
end
it 'returns 422 if targeting a different fork' do it 'returns 422 if targeting a different fork' do
post v3_api("/projects/#{forked_project.id}/merge_requests", user2), unrelated_project.add_developer(user2)
title: 'Test merge_request',
target_branch: 'master', post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params
source_branch: 'markdown',
author: user2,
target_project_id: unrelated_project.id
expect(response).to have_gitlab_http_status(422) expect(response).to have_gitlab_http_status(422)
end end
it 'returns 403 if targeting a different fork which user can not access' do
post v3_api("/projects/#{forked_project.id}/merge_requests", user2), params
expect(response).to have_gitlab_http_status(403)
end
end end
it "returns 201 when target_branch is specified and for the same project" do it "returns 201 when target_branch is specified and for the same project" do
......
...@@ -263,5 +263,66 @@ describe MergeRequests::CreateService do ...@@ -263,5 +263,66 @@ describe MergeRequests::CreateService do
expect(issue_ids).to match_array([first_issue.id, second_issue.id]) expect(issue_ids).to match_array([first_issue.id, second_issue.id])
end end
end end
context 'when source and target projects are different' do
let(:target_project) { create(:project) }
let(:opts) do
{
title: 'Awesome merge_request',
source_branch: 'feature',
target_branch: 'master',
target_project_id: target_project.id
}
end
context 'when user can not access source project' do
before do
target_project.add_developer(assignee)
target_project.add_master(user)
end
it 'raises an error' do
expect { described_class.new(project, user, opts).execute }
.to raise_error Gitlab::Access::AccessDeniedError
end
end
context 'when user can not access target project' do
before do
target_project.add_developer(assignee)
target_project.add_master(user)
end
it 'raises an error' do
expect { described_class.new(project, user, opts).execute }
.to raise_error Gitlab::Access::AccessDeniedError
end
end
end
context 'when user sets source project id' do
let(:another_project) { create(:project) }
let(:opts) do
{
title: 'Awesome merge_request',
source_branch: 'feature',
target_branch: 'master',
source_project_id: another_project.id
}
end
before do
project.add_developer(assignee)
project.add_master(user)
end
it 'ignores source_project_id' do
merge_request = described_class.new(project, user, opts).execute
expect(merge_request.source_project_id).to eq(project.id)
end
end
end end
end end
...@@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do ...@@ -57,6 +57,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do
@issue = issue_service.execute @issue = issue_service.execute
@issues_sample_data = issue_service.hook_data(@issue, 'open') @issues_sample_data = issue_service.hook_data(@issue, 'open')
project.add_developer(user)
opts = { opts = {
title: 'Awesome merge_request', title: 'Awesome merge_request',
description: 'please fix', description: 'please fix',
......
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