Commit 8af7f42c authored by Bob Van Landuyt's avatar Bob Van Landuyt

Filter params in MR build service

Reusing the existing `IssuableBaseService#filter_params` which uses
the policies to determine what params a user can set, and which values
it can be set to.

This also removed the need for the seperate call to
`IssuableBaseService#ensure_milestone_available`.

The `Issues::BuildService` does not suffer from this because it limits
the params that are assignable to the `title`, `description` and
`milestone_id`.
parent 9dc82e81
...@@ -11,15 +11,18 @@ module MergeRequests ...@@ -11,15 +11,18 @@ module MergeRequests
# https://gitlab.com/gitlab-org/gitlab-ce/issues/53658 # https://gitlab.com/gitlab-org/gitlab-ce/issues/53658
merge_quick_actions_into_params!(merge_request, only: [:target_branch]) merge_quick_actions_into_params!(merge_request, only: [:target_branch])
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch) merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) if params.has_key?(:force_remove_source_branch)
merge_request.assign_attributes(params)
# Assign the projects first so we can use policies for `filter_params`
merge_request.author = current_user merge_request.author = current_user
merge_request.source_project = find_source_project
merge_request.target_project = find_target_project
filter_params(merge_request)
merge_request.assign_attributes(params.to_h.compact)
merge_request.compare_commits = [] merge_request.compare_commits = []
merge_request.source_project = find_source_project merge_request.target_branch = find_target_branch
merge_request.target_project = find_target_project merge_request.can_be_created = projects_and_branches_valid?
merge_request.target_branch = find_target_branch
merge_request.can_be_created = projects_and_branches_valid?
ensure_milestone_available(merge_request)
# compare branches only if branches are valid, otherwise # compare branches only if branches are valid, otherwise
# compare_branches may raise an error # compare_branches may raise an error
...@@ -50,12 +53,14 @@ module MergeRequests ...@@ -50,12 +53,14 @@ module MergeRequests
to: :merge_request to: :merge_request
def find_source_project def find_source_project
source_project = project_from_params(:source_project)
return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project) return source_project if source_project.present? && can?(current_user, :create_merge_request_from, source_project)
project project
end end
def find_target_project def find_target_project
target_project = project_from_params(:target_project)
return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project) return target_project if target_project.present? && can?(current_user, :create_merge_request_in, target_project)
target_project = project.default_merge_request_target target_project = project.default_merge_request_target
...@@ -65,6 +70,17 @@ module MergeRequests ...@@ -65,6 +70,17 @@ module MergeRequests
project project
end end
def project_from_params(param_name)
project_from_params = params.delete(param_name)
id_param_name = :"#{param_name}_id"
if project_from_params.nil? && params[id_param_name]
project_from_params = Project.find_by_id(params.delete(id_param_name))
end
project_from_params
end
def find_target_branch def find_target_branch
target_branch || target_project.default_branch target_branch || target_project.default_branch
end end
......
---
title: Filter merge request params on the new merge request page
merge_request:
author:
type: security
# frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
describe 'Merge Request > Tries to access private repo of public project' do describe 'Merge Request > User tries to access private project information through the new mr page' do
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
let(:private_project) do let(:private_project) do
create(:project, :public, :repository, create(:project, :public, :repository,
...@@ -33,5 +35,22 @@ describe 'Merge Request > Tries to access private repo of public project' do ...@@ -33,5 +35,22 @@ describe 'Merge Request > Tries to access private repo of public project' do
it "does not mention the project the user can't see the repo of" do it "does not mention the project the user can't see the repo of" do
expect(page).not_to have_content('nothing-to-see-here') expect(page).not_to have_content('nothing-to-see-here')
end end
context 'when the user enters label information from the private project in the querystring' do
let(:inaccessible_label) { create(:label, project: private_project) }
let(:mr_path) do
project_new_merge_request_path(
owned_project,
merge_request: {
label_ids: [inaccessible_label.id],
source_branch: 'feature'
}
)
end
it 'does not expose the label name' do
expect(page).not_to have_content(inaccessible_label.name)
end
end
end end
end end
# frozen_string_literal: true # frozen_string_literal: true
require 'spec_helper' require 'spec_helper'
describe MergeRequests::BuildService do describe MergeRequests::BuildService do
...@@ -225,6 +224,11 @@ describe MergeRequests::BuildService do ...@@ -225,6 +224,11 @@ describe MergeRequests::BuildService do
let(:label_ids) { [label2.id] } let(:label_ids) { [label2.id] }
let(:milestone_id) { milestone2.id } let(:milestone_id) { milestone2.id }
before do
# Guests are not able to assign labels or milestones to an issue
project.add_developer(user)
end
it 'assigns milestone_id and label_ids instead of issue labels and milestone' do it 'assigns milestone_id and label_ids instead of issue labels and milestone' do
expect(merge_request.milestone).to eq(milestone2) expect(merge_request.milestone).to eq(milestone2)
expect(merge_request.labels).to match_array([label2]) expect(merge_request.labels).to match_array([label2])
...@@ -479,4 +483,35 @@ describe MergeRequests::BuildService do ...@@ -479,4 +483,35 @@ describe MergeRequests::BuildService do
end end
end end
end end
context 'when assigning labels' do
let(:label_ids) { [create(:label, project: project).id] }
context 'for members with less than developer access' do
it 'is not allowed' do
expect(merge_request.label_ids).to be_empty
end
end
context 'for users allowed to assign labels' do
before do
project.add_developer(user)
end
context 'for labels in the project' do
it 'is allowed for developers' do
expect(merge_request.label_ids).to contain_exactly(*label_ids)
end
end
context 'for unrelated labels' do
let(:project_label) { create(:label, project: project) }
let(:label_ids) { [create(:label).id, project_label.id] }
it 'only assigns related labels' do
expect(merge_request.label_ids).to contain_exactly(project_label.id)
end
end
end
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