Add latest changes from gitlab-org/gitlab@14-10-stable-eev14.10.0-rc42

5 files changed, 161 insertions, 58 deletions

diff --git a/app/services/members/create_service.rb b/app/services/members/create_service.rb
index 758fa2e67f1..8f7b63c32c8 100644
--- a/app/services/members/create_service.rb
+++ b/app/services/members/create_service.rb
@@ -14,8 +14,9 @@ module Members
       super
 
       @errors = []
-      @invites = invites_from_params&.split(',')&.uniq&.flatten
+      @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,10 +42,13 @@ 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
-      params[:user_ids]
+      return params[:user_ids] if params[:user_ids].is_a?(Array)
+
+      params[:user_ids]&.to_s&.split(',')&.uniq&.flatten || []
     end
 
     def validate_invite_source!
@@ -74,33 +79,45 @@ 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
       end
     end
 
-    def add_error_for_member(member)
+    # overridden
+    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
@@ -114,16 +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?
-
-      valid_members = members.select { |member| member.valid? && member.member_task.valid? }
-      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 fcce32ead94..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
 
@@ -101,6 +137,8 @@ module Members
                 else
                   source.members.build(invite_email: user)
                 end
+
+      @member.blocking_refresh = args[:blocking_refresh]
     end
 
     # This method is used to find users that have been entered into the "Add members" field.
@@ -114,7 +152,7 @@ module Members
         User.find_by(id: user) # rubocop:todo CodeReuse/ActiveRecord
       else
         # must be an email or at least we'll consider it one
-        User.find_by_any_email(user) || user
+        source.users_by_emails([user])[user] || user
       end
     end
 
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 85acb720f0f..1bf209ab79d 100644
--- a/app/services/members/invite_service.rb
+++ b/app/services/members/invite_service.rb
@@ -7,6 +7,8 @@ module Members
     def initialize(*args)
       super
 
+      @invites += parsed_emails
+
       @errors = {}
     end
 
@@ -14,38 +16,63 @@ module Members
 
     alias_method :formatted_errors, :errors
 
-    def invites_from_params
-      params[:email]
+    def parsed_emails
+      # can't put this in the initializer since `invites_from_params` is called in super class
+      # and needs it
+      @parsed_emails ||= (formatted_param(params[:email]) || [])
+    end
+
+    def formatted_param(parameter)
+      parameter&.split(',')&.uniq&.flatten
     end
 
     def validate_invitable!
       super
 
+      return if params[:email].blank?
+
       # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails
       # ideally we wouldn't need this, but we can't really change the add_users method
-      valid, invalid = invites.partition { |email| Member.valid_email?(email) }
-      @invites = valid
+      invalid_emails.each { |email| errors[email] = s_('AddMember|Invite email is invalid') }
+    end
+
+    def invalid_emails
+      parsed_emails.each_with_object([]) do |email, invalid|
+        next if Member.valid_email?(email)
 
-      invalid.each { |email| errors[email] = s_('AddMember|Invite email is invalid') }
+        invalid << email
+        @invites.delete(email)
+      end
     end
 
     override :blank_invites_message
     def blank_invites_message
-      s_('AddMember|Emails cannot be blank')
+      s_('AddMember|Invites cannot be blank')
     end
 
     override :add_error_for_member
-    def add_error_for_member(member)
-      errors[invite_email(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
 
-    override :create_tasks_to_be_done
-    def create_tasks_to_be_done
-      # Only create task issues for existing users. Tasks for new users are created when they signup.
-    end
+    def invited_object(member)
+      return member.invite_email if member.invite_email
 
-    def invite_email(member)
-      member.invite_email || member.user.email
+      # There is a case where someone was invited by email, but the `user` record exists.
+      # The member record returned will not have an invite_email attribute defined since
+      # the CreatorService finds `user` record sometimes by email.
+      # At that point we lose the info of whether this invite was done by `user` or by email.
+      # Here we will give preference to check invites by user_id first.
+      # There is also a case where a user could be invited by their email and
+      # at the same time via the API in the same request.
+      # This would would mean the same user is invited as user_id and email.
+      # However, that isn't as likely from the UI at least since the token generator checks
+      # for that case and doesn't allow email being used if the user exists as a record already.
+      if member.user_id.to_s.in?(invites)
+        member.user.username
+      else
+        member.user.all_emails.detect { |email| email.in?(invites) }
+      end
     end
   end
 end
diff --git a/app/services/members/projects/creator_service.rb b/app/services/members/projects/creator_service.rb
index 4dba81acf73..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_a_new_owner?
+      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_a_new_owner?
+      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.owner? && member.new_record?
+        member.project.personal_namespace_holder?(member.user)
       end
     end
   end