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`.
This commit is contained in:
Bob Van Landuyt 2019-07-12 11:10:54 +02:00
parent 9c3dfd2085
commit 6c27c0d394
4 changed files with 83 additions and 8 deletions

View file

@ -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.compare_commits = []
merge_request.source_project = find_source_project merge_request.source_project = find_source_project
merge_request.target_project = find_target_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.target_branch = find_target_branch merge_request.target_branch = find_target_branch
merge_request.can_be_created = projects_and_branches_valid? 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
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
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

View file

@ -0,0 +1,5 @@
---
title: Filter merge request params on the new merge request page
merge_request:
author:
type: security

View file

@ -1,6 +1,8 @@
# 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
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

View file

@ -1,5 +1,4 @@
# 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
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
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