Merge dev.gitlab.org@master into GitLab.com@master

11 files changed, 113 insertions, 15 deletions

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/error_tracking/list_projects_service.rb b/app/services/error_tracking/list_projects_service.rb
index 8d08f0cda94..92d4ef85ecf 100644
--- a/app/services/error_tracking/list_projects_service.rb
+++ b/app/services/error_tracking/list_projects_service.rb
@@ -32,7 +32,7 @@ module ErrorTracking
           project_slug: 'proj'
         )
 
-        setting.token = params[:token]
+        setting.token = token(setting)
         setting.enabled = true
       end
     end
@@ -40,5 +40,12 @@ module ErrorTracking
     def can_read?
       can?(current_user, :read_sentry_issue, project)
     end
+
+    def token(setting)
+      # Use param token if not masked, otherwise use database token
+      return params[:token] unless /\A\*+\z/.match?(params[:token])
+
+      setting.token
+    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 06ee25eac2a..456cc589477 100644
--- a/app/services/merge_requests/build_service.rb
+++ b/app/services/merge_requests/build_service.rb
@@ -24,6 +24,8 @@ module MergeRequests
           merge_request.source_project.remove_source_branch_after_merge?
         end
 
+      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
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index b56b2cf14e3..1709474a6c7 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -281,7 +281,7 @@ class NotificationService
   end
 
   def send_new_note_notifications(note)
-    notify_method = "note_#{note.to_ability_name}_email".to_sym
+    notify_method = "note_#{note.noteable_ability_name}_email".to_sym
 
     recipients = NotificationRecipientService.build_new_note_recipients(note)
     recipients.each do |recipient|
diff --git a/app/services/projects/operations/update_service.rb b/app/services/projects/operations/update_service.rb
index 64519501ff4..0ca89664304 100644
--- a/app/services/projects/operations/update_service.rb
+++ b/app/services/projects/operations/update_service.rb
@@ -36,15 +36,17 @@ module Projects
           organization_slug: settings.dig(:project, :organization_slug)
         )
 
-        {
+        params = {
           error_tracking_setting_attributes: {
             api_url: api_url,
-            token: settings[:token],
             enabled: settings[:enabled],
             project_name: settings.dig(:project, :name),
             organization_name: settings.dig(:project, :organization_name)
           }
         }
+        params[:error_tracking_setting_attributes][:token] = settings[:token] unless /\A\*+\z/.match?(settings[:token]) # Don't update token if we receive masked value
+
+        params
       end
 
       def grafana_integration_params
diff --git a/app/services/projects/participants_service.rb b/app/services/projects/participants_service.rb
index 7080f388e53..1cd81fe37c7 100644
--- a/app/services/projects/participants_service.rb
+++ b/app/services/projects/participants_service.rb
@@ -7,16 +7,69 @@ module Projects
     def execute(noteable)
       @noteable = noteable
 
-      participants = noteable_owner + participants_in_noteable + all_members + groups + project_members
+      participants =
+        noteable_owner +
+        participants_in_noteable +
+        all_members +
+        groups +
+        project_members
+
       participants.uniq
     end
 
     def project_members
-      @project_members ||= sorted(project.team.members)
+      @project_members ||= sorted(get_project_members)
+    end
+
+    def get_project_members
+      members = Member.from_union([project_members_through_ancestral_groups,
+                                   project_members_through_invited_groups,
+                                   individual_project_members])
+
+      User.id_in(members.select(:user_id))
     end
 
     def all_members
       [{ username: "all", name: "All Project and Group Members", count: project_members.count }]
     end
+
+    private
+
+    def project_members_through_invited_groups
+      groups_with_ancestors_ids = Gitlab::ObjectHierarchy
+        .new(visible_groups)
+        .base_and_ancestors
+        .pluck_primary_key
+
+      GroupMember
+        .active_without_invites_and_requests
+        .with_source_id(groups_with_ancestors_ids)
+    end
+
+    def visible_groups
+      visible_groups = project.invited_groups
+
+      unless project_owner?
+        visible_groups = visible_groups.public_or_visible_to_user(current_user)
+      end
+
+      visible_groups
+    end
+
+    def project_members_through_ancestral_groups
+      project.group.present? ? project.group.members_with_parents : Member.none
+    end
+
+    def individual_project_members
+      project.project_members
+    end
+
+    def project_owner?
+      if project.group.present?
+        project.group.owners.include?(current_user)
+      else
+        project.namespace.owner == current_user
+      end
+    end
   end
 end
diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb
index 4b3aca58dd7..525fc18b312 100644
--- a/app/services/projects/transfer_service.rb
+++ b/app/services/projects/transfer_service.rb
@@ -98,7 +98,7 @@ module Projects
       @new_namespace &&
         can?(current_user, :change_namespace, project) &&
         @new_namespace.id != project.namespace_id &&
-        current_user.can?(:create_projects, @new_namespace)
+        current_user.can?(:transfer_projects, @new_namespace)
     end
 
     def update_namespace_and_visibility(to_namespace)