Add latest changes from gitlab-org/gitlab@master

5 files changed, 113 insertions, 43 deletions

diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 88afea92d26..8f7b63c32c8 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -16,6 +16,7 @@ module Members
       @errors = []
       @invites = invites_from_params
       @source = params[:source]
+      @tasks_to_be_done_members = []
     end
 
     def execute
@@ -25,6 +26,7 @@ module Members
       validate_invitable!
 
       add_members
+      create_tasks_to_be_done
       enqueue_onboarding_progress_action
 
       publish_event!
@@ -40,7 +42,8 @@ module Members
 
     private
 
-    attr_reader :source, :errors, :invites, :member_created_namespace_id, :members
+    attr_reader :source, :errors, :invites, :member_created_namespace_id, :members,
+                :tasks_to_be_done_members, :member_created_member_task_id
 
     def invites_from_params
       return params[:user_ids] if params[:user_ids].is_a?(Array)
@@ -76,13 +79,15 @@ module Members
       )
 
       members.each { |member| process_result(member) }
-
-      create_tasks_to_be_done
     end
 
     def process_result(member)
-      if member.invalid?
-        add_error_for_member(member)
+      existing_errors = member.errors.full_messages
+
+      # calling invalid? clears any errors that were added outside of the
+      # rails validation process
+      if member.invalid? || existing_errors.present?
+        add_error_for_member(member, existing_errors)
       else
         after_execute(member: member)
         @member_created_namespace_id ||= member.namespace_id
@@ -90,20 +95,29 @@ module Members
     end
 
     # overridden
-    def add_error_for_member(member)
+    def add_error_for_member(member, existing_errors)
       prefix = "#{member.user.username}: " if member.user.present?
 
-      errors << "#{prefix}#{member.errors.full_messages.to_sentence}"
+      errors << "#{prefix}#{all_member_errors(member, existing_errors).to_sentence}"
+    end
+
+    def all_member_errors(member, existing_errors)
+      existing_errors.concat(member.errors.full_messages).uniq
     end
 
     def after_execute(member:)
       super
 
+      build_tasks_to_be_done_members(member)
       track_invite_source(member)
     end
 
     def track_invite_source(member)
-      Gitlab::Tracking.event(self.class.name, 'create_member', label: invite_source, property: tracking_property(member), user: current_user)
+      Gitlab::Tracking.event(self.class.name,
+                             'create_member',
+                             label: invite_source,
+                             property: tracking_property(member),
+                             user: current_user)
     end
 
     def invite_source
@@ -117,17 +131,28 @@ module Members
       member.invite? ? 'net_new_user' : 'existing_user'
     end
 
-    def create_tasks_to_be_done
-      return if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank?
-
-      # Only create task issues for existing users. Tasks for new users are created when they signup.
-      valid_members = members.select { |member| member.valid? && member.member_task.valid? && member.user.present? }
-      return unless valid_members.present?
+    def build_tasks_to_be_done_members(member)
+      return unless tasks_to_be_done?(member)
 
+      @tasks_to_be_done_members << member
       # We can take the first `member_task` here, since all tasks will have the same attributes needed
       # for the `TasksToBeDone::CreateWorker`, ie. `project` and `tasks_to_be_done`.
-      member_task = valid_members[0].member_task
-      TasksToBeDone::CreateWorker.perform_async(member_task.id, current_user.id, valid_members.map(&:user_id))
+      @member_created_member_task_id ||= member.member_task.id
+    end
+
+    def tasks_to_be_done?(member)
+      return false if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank?
+
+      # Only create task issues for existing users. Tasks for new users are created when they signup.
+      member.member_task&.valid? && member.user.present?
+    end
+
+    def create_tasks_to_be_done
+      return unless member_created_member_task_id # signal if there is any work to be done here
+
+      TasksToBeDone::CreateWorker.perform_async(member_created_member_task_id,
+                                                current_user.id,
+                                                tasks_to_be_done_members.map(&:user_id))
     end
 
     def user_limit
diff --git a/app/services/members/creator_service.rb b/app/services/members/creator_service.rb
index 96f88a7d627..321658ac9c5 100644
--- a/app/services/members/creator_service.rb
+++ b/app/services/members/creator_service.rb
@@ -4,15 +4,13 @@ module Members
   # This class serves as more of an app-wide way we add/create members
   # All roads to add members should take this path.
   class CreatorService
-    include Gitlab::Experiment::Dsl
-
     class << self
       def parsed_access_level(access_level)
         access_levels.fetch(access_level) { access_level.to_i }
       end
 
       def access_levels
-        raise NotImplementedError
+        Gitlab::Access.sym_options_with_owner
       end
     end
 
@@ -25,7 +23,7 @@ module Members
 
     def execute
       find_or_build_member
-      update_member
+      commit_member
       create_member_task
 
       member
@@ -33,23 +31,39 @@ module Members
 
     private
 
+    delegate :new_record?, to: :member
     attr_reader :source, :user, :access_level, :member, :args
 
-    def update_member
-      return unless can_update_member?
-
+    def assign_member_attributes
       member.attributes = member_attributes
+    end
 
-      if member.request?
-        approve_request
+    def commit_member
+      if can_commit_member?
+        assign_member_attributes
+        commit_changes
       else
-        member.save
+        add_commit_error
       end
     end
 
-    def can_update_member?
+    def can_commit_member?
       # There is no current user for bulk actions, in which case anything is allowed
-      !current_user # inheriting classes will add more logic
+      return true if skip_authorization?
+
+      if new_record?
+        can_create_new_member?
+      else
+        can_update_existing_member?
+      end
+    end
+
+    def can_create_new_member?
+      raise NotImplementedError
+    end
+
+    def can_update_existing_member?
+      raise NotImplementedError
     end
 
     # Populates the attributes of a member.
@@ -64,6 +78,14 @@ module Members
       }
     end
 
+    def commit_changes
+      if member.request?
+        approve_request
+      else
+        member.save
+      end
+    end
+
     def create_member_task
       return unless member.persisted?
       return if member_task_attributes.value?(nil)
@@ -93,6 +115,20 @@ module Members
       args[:current_user]
     end
 
+    def skip_authorization?
+      !current_user
+    end
+
+    def add_commit_error
+      msg = if new_record?
+              _('not authorized to create member')
+            else
+              _('not authorized to update member')
+            end
+
+      member.errors.add(:base, msg)
+    end
+
     def find_or_build_member
       @user = parse_user_param
 
diff --git a/app/services/members/groups/creator_service.rb b/app/services/members/groups/creator_service.rb
index df4d3f59d3b..a6f0daa99aa 100644
--- a/app/services/members/groups/creator_service.rb
+++ b/app/services/members/groups/creator_service.rb
@@ -3,14 +3,14 @@
 module Members
   module Groups
     class CreatorService < Members::CreatorService
-      def self.access_levels
-        Gitlab::Access.sym_options_with_owner
-      end
-
       private
 
-      def can_update_member?
-        super || current_user.can?(:update_group_member, member)
+      def can_create_new_member?
+        current_user.can?(:admin_group_member, member.group)
+      end
+
+      def can_update_existing_member?
+        current_user.can?(:update_group_member, member)
       end
     end
   end
diff --git a/app/services/members/invite_service.rb b/app/services/members/invite_service.rb
index a5af17d4491..1bf209ab79d 100644
--- a/app/services/members/invite_service.rb
+++ b/app/services/members/invite_service.rb
@@ -51,8 +51,8 @@ module Members
     end
 
     override :add_error_for_member
-    def add_error_for_member(member)
-      errors[invited_object(member)] = member.errors.full_messages.to_sentence
+    def add_error_for_member(member, existing_errors)
+      errors[invited_object(member)] = all_member_errors(member, existing_errors).to_sentence
     end
 
     def invited_object(member)
diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb
index 7d62e2705bc..d92fe60c54a 100644
--- a/app/services/members/projects/creator_service.rb
+++ b/app/services/members/projects/creator_service.rb
@@ -3,19 +3,28 @@
 module Members
   module Projects
     class CreatorService < Members::CreatorService
-      def self.access_levels
-        Gitlab::Access.sym_options_with_owner
-      end
-
       private
 
-      def can_update_member?
-        super || current_user.can?(:update_project_member, member) || adding_the_creator_as_owner_in_a_personal_project?
+      def can_create_new_member?
+        # order is important here!
+        # The `admin_project_member` check has side-effects that causes projects not be created if this area is hit
+        # during project creation.
+        # Call that triggers is current_user.can?(:admin_project_member, member.project)
+        # I tracked back to base_policy.rb admin check and specifically in
+        # Gitlab::Auth::CurrentUserMode.new(@user).admin_mode? call.
+        # This calls user.admin? and that specific call causes issues with project creation in
+        # spec/requests/api/projects_spec.rb specs and others, mostly around project creation.
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/358931 for investigation
+        adding_the_creator_as_owner_in_a_personal_project? || current_user.can?(:admin_project_member, member.project)
+      end
+
+      def can_update_existing_member?
+        current_user.can?(:update_project_member, member)
       end
 
       def adding_the_creator_as_owner_in_a_personal_project?
         # this condition is reached during testing setup a lot due to use of `.add_user`
-        member.project.personal_namespace_holder?(member.user) && member.new_record?
+        member.project.personal_namespace_holder?(member.user)
       end
     end
   end