diff options
author | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-09-25 19:25:40 +0300 |
---|---|---|
committer | Bob Van Landuyt <bob@vanlanduyt.co> | 2019-10-23 11:29:07 +0300 |
commit | b271eb42861c8067fc640a83a957742184d1221c (patch) | |
tree | bace1610d92f28a7bb5cec88042f0c3a93a723da /app | |
parent | 1425a56c75beecaa289ad59587d636f8f469509e (diff) |
Only assign merge params when allowed
When a user updates a merge request coming from a fork, they should
not be able to set `force_remove_source_branch` if they cannot push
code to the source project.
Otherwise developers of the target project could remove the source
branch of the source project by setting this flag through the API.
Diffstat (limited to 'app')
-rw-r--r-- | app/models/merge_request.rb | 8 | ||||
-rw-r--r-- | app/services/auto_merge/base_service.rb | 7 | ||||
-rw-r--r-- | app/services/concerns/merge_requests/assigns_merge_params.rb | 24 | ||||
-rw-r--r-- | app/services/merge_requests/base_service.rb | 14 | ||||
-rw-r--r-- | app/services/merge_requests/build_service.rb | 3 | ||||
-rw-r--r-- | app/services/merge_requests/create_service.rb | 1 | ||||
-rw-r--r-- | app/services/merge_requests/update_service.rb | 4 |
7 files changed, 52 insertions, 9 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 7cdaa3e3ca7..32741046f39 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -69,6 +69,14 @@ class MergeRequest < ApplicationRecord has_many :merge_request_assignees has_many :assignees, class_name: "User", through: :merge_request_assignees + KNOWN_MERGE_PARAMS = [ + :auto_merge_strategy, + :should_remove_source_branch, + :force_remove_source_branch, + :commit_message, + :squash_commit_message, + :sha + ].freeze serialize :merge_params, Hash # rubocop:disable Cop/ActiveRecordSerialize after_create :ensure_merge_request_diff diff --git a/app/services/auto_merge/base_service.rb b/app/services/auto_merge/base_service.rb index e06659a39cd..e08b4ac2260 100644 --- a/app/services/auto_merge/base_service.rb +++ b/app/services/auto_merge/base_service.rb @@ -3,12 +3,13 @@ module AutoMerge class BaseService < ::BaseService include Gitlab::Utils::StrongMemoize + include MergeRequests::AssignsMergeParams def execute(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) + merge_request.auto_merge_enabled = true merge_request.merge_user = current_user - merge_request.auto_merge_strategy = strategy return :failed unless merge_request.save @@ -21,7 +22,7 @@ module AutoMerge end def update(merge_request) - merge_request.merge_params.merge!(params) + assign_allowed_merge_params(merge_request, params.merge(auto_merge_strategy: strategy)) return :failed unless merge_request.save diff --git a/app/services/concerns/merge_requests/assigns_merge_params.rb b/app/services/concerns/merge_requests/assigns_merge_params.rb new file mode 100644 index 00000000000..bd870d9a1e7 --- /dev/null +++ b/app/services/concerns/merge_requests/assigns_merge_params.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module MergeRequests + module AssignsMergeParams + def self.included(klass) + raise "#{self} can not be included in #{klass} without implementing #current_user" unless klass.method_defined?(:current_user) + end + + def assign_allowed_merge_params(merge_request, merge_params) + known_merge_params = merge_params.to_h.with_indifferent_access.slice(*MergeRequest::KNOWN_MERGE_PARAMS) + + # Not checking `MergeRequest#can_remove_source_branch` as that includes + # other checks that aren't needed here. + known_merge_params.delete(:force_remove_source_branch) unless current_user.can?(:push_code, merge_request.source_project) + + merge_request.merge_params.merge!(known_merge_params) + + # Delete the known params now that they're assigned, so we don't try to + # assign them through an `#assign_attributes` later. + # They could be coming in as strings or symbols + merge_params.to_h.with_indifferent_access.except!(*MergeRequest::KNOWN_MERGE_PARAMS) + end + end +end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7d4227e4a41..aacc3d6831e 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,6 +2,8 @@ module MergeRequests class BaseService < ::IssuableBaseService + include MergeRequests::AssignsMergeParams + def create_note(merge_request, state = merge_request.state) SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, state, nil) end @@ -29,6 +31,18 @@ module MergeRequests private + def create(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + + def update(merge_request) + self.params = assign_allowed_merge_params(merge_request, params) + + super + end + def handle_wip_event(merge_request) if wip_event = params.delete(:wip_event) # We update the title that is provided in the params or we use the mr title diff --git a/app/services/merge_requests/build_service.rb b/app/services/merge_requests/build_service.rb index 214f145d09b..bf4da01723b 100644 --- a/app/services/merge_requests/build_service.rb +++ b/app/services/merge_requests/build_service.rb @@ -10,13 +10,14 @@ module MergeRequests # TODO: this should handle all quick actions that don't have side effects # https://gitlab.com/gitlab-org/gitlab-foss/issues/53658 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) # Assign the projects first so we can use policies for `filter_params` merge_request.author = current_user merge_request.source_project = find_source_project merge_request.target_project = find_target_project + self.params = assign_allowed_merge_params(merge_request, params) + filter_params(merge_request) # merge_request.assign_attributes(...) below is a Rails diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb index 1c730232abb..9a37a0330fc 100644 --- a/app/services/merge_requests/create_service.rb +++ b/app/services/merge_requests/create_service.rb @@ -9,7 +9,6 @@ module MergeRequests merge_request.target_project = @project merge_request.source_project = @source_project merge_request.source_branch = params[:source_branch] - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) create(merge_request) end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index ae678d4c036..7c9abb12b6e 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,10 +16,6 @@ module MergeRequests params.delete(:force_remove_source_branch) end - if params.has_key?(:force_remove_source_branch) - merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch) - end - handle_wip_event(merge_request) update_task_event(merge_request) || update(merge_request) end |