Merge branch '328806-improve-clarity-in-the-users-buildservice-2' into 'master'

Improve clarity in the Users::BuildService [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!61633

app/services/users/build_service.rb

@@ -5,7 +5,6 @@ module Users
     delegate :user_default_internal_regex_enabled?,
              :user_default_internal_regex_instance,
              to: :'Gitlab::CurrentSettings.current_application_settings'
-    attr_reader :identity_params
 
     def initialize(current_user, params = {})
       @current_user = current_user
@@ -16,43 +15,135 @@ module Users
     def execute(skip_authorization: false)
       @skip_authorization = skip_authorization
 
-      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_create_user?
+      build_user
+      build_identity
+      update_canonical_email
 
-      user_params = build_user_params
-      user = User.new(user_params)
+      user
+    end
+
+    private
+
+    attr_reader :skip_authorization, :identity_params, :user_params, :user
+
+    def identity_attributes
+      [:extern_uid, :provider]
+    end
+
+    def build_user
+      if admin?
+        admin_build_user
+      else
+        standard_build_user
+      end
+    end
+
+    def admin?
+      return false unless current_user
+
+      current_user.admin?
+    end
+
+    def admin_build_user
+      build_user_params_for_admin
+      init_user
+      password_reset
+    end
+
+    def standard_build_user
+      # current_user non admin or nil
+      validate_access!
+      build_user_params_for_non_admin
+      init_user
+    end
+
+    def build_user_params_for_admin
+      @user_params = params.slice(*admin_create_params)
+      @user_params.merge!(force_random_password: true, password_expires_at: nil) if params[:reset_password]
+    end
+
+    def init_user
+      assign_common_user_params
+
+      @user = User.new(user_params)
+    end
+
+    def assign_common_user_params
+      @user_params[:created_by_id] = current_user&.id
+      @user_params[:external] = user_external? if set_external_param?
 
-      if current_user&.admin?
+      @user_params.delete(:user_type) unless project_bot?
+    end
+
+    def set_external_param?
+      user_default_internal_regex_enabled? && !user_params.key?(:external)
+    end
+
+    def user_external?
+      user_default_internal_regex_instance.match(params[:email]).nil?
+    end
+
+    def project_bot?
+      user_params[:user_type]&.to_sym == :project_bot
+    end
+
+    def password_reset
       @reset_token = user.generate_reset_token if params[:reset_password]
 
       if user_params[:force_random_password]
         random_password = User.random_password
-          user.password = user.password_confirmation = random_password
+        @user.password = user.password_confirmation = random_password
       end
     end
 
-      build_identity(user)
+    def validate_access!
+      return if skip_authorization
+      return if can_create_user?
 
-      Users::UpdateCanonicalEmailService.new(user: user).execute
+      raise Gitlab::Access::AccessDeniedError
+    end
 
-      user
+    def can_create_user?
+      current_user.nil? && Gitlab::CurrentSettings.allow_signup?
     end
 
-    private
+    def build_user_params_for_non_admin
+      allowed_signup_params = signup_params
+      allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation?
 
-    attr_reader :skip_authorization
+      @user_params = params.slice(*allowed_signup_params)
+      @user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting if assign_skip_confirmation_from_settings?
+      @user_params[:name] = fallback_name if use_fallback_name?
+    end
 
-    def identity_attributes
-      [:extern_uid, :provider]
+    def allow_caller_to_request_skip_confirmation?
+      skip_authorization
+    end
+
+    def assign_skip_confirmation_from_settings?
+      user_params[:skip_confirmation].nil?
     end
 
-    def build_identity(user)
+    def skip_user_confirmation_email_from_setting
+      !Gitlab::CurrentSettings.send_user_confirmation_email
+    end
+
+    def use_fallback_name?
+      user_params[:name].blank? && fallback_name.present?
+    end
+
+    def fallback_name
+      "#{user_params[:first_name]} #{user_params[:last_name]}"
+    end
+
+    def build_identity
       return if identity_params.empty?
 
       user.identities.build(identity_params)
     end
 
-    def can_create_user?
-      (current_user.nil? && Gitlab::CurrentSettings.allow_signup?) || current_user&.admin?
+    def update_canonical_email
+      Users::UpdateCanonicalEmailService.new(user: user).execute
     end
 
     # Allowed params for creating a user (admins only)
@@ -96,69 +187,15 @@ module Users
     def signup_params
       [
         :email,
-        :password_automatically_set,
         :name,
-        :first_name,
-        :last_name,
         :password,
+        :password_automatically_set,
         :username,
-        :user_type
+        :user_type,
+        :first_name,
+        :last_name
       ]
     end
-
-    def build_user_params
-      if current_user&.admin?
-        user_params = params.slice(*admin_create_params)
-
-        if params[:reset_password]
-          user_params.merge!(force_random_password: true, password_expires_at: nil)
-        end
-      else
-        allowed_signup_params = signup_params
-        allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation?
-
-        user_params = params.slice(*allowed_signup_params)
-        if assign_skip_confirmation_from_settings?(user_params)
-          user_params[:skip_confirmation] = skip_user_confirmation_email_from_setting
-        end
-
-        fallback_name = "#{user_params[:first_name]} #{user_params[:last_name]}"
-
-        if user_params[:name].blank? && fallback_name.present?
-          user_params = user_params.merge(name: fallback_name)
-        end
-      end
-
-      user_params[:created_by_id] = current_user&.id
-
-      if user_default_internal_regex_enabled? && !user_params.key?(:external)
-        user_params[:external] = user_external?
-      end
-
-      user_params.delete(:user_type) unless project_bot?(user_params[:user_type])
-
-      user_params
-    end
-
-    def allow_caller_to_request_skip_confirmation?
-      skip_authorization
-    end
-
-    def assign_skip_confirmation_from_settings?(user_params)
-      user_params[:skip_confirmation].nil?
-    end
-
-    def skip_user_confirmation_email_from_setting
-      !Gitlab::CurrentSettings.send_user_confirmation_email
-    end
-
-    def user_external?
-      user_default_internal_regex_instance.match(params[:email]).nil?
-    end
-
-    def project_bot?(user_type)
-      user_type&.to_sym == :project_bot
-    end
   end
 end
 

app/services/users/registrations_build_service.rb

@@ -12,7 +12,7 @@ module Users
     end
 
     override :assign_skip_confirmation_from_settings?
-    def assign_skip_confirmation_from_settings?(user_params)
+    def assign_skip_confirmation_from_settings?
       user_params[:skip_confirmation].blank?
     end
   end

ee/app/services/ee/users/build_service.rb

@@ -19,10 +19,10 @@ module EE
 
       override :execute
       def execute(skip_authorization: false)
-        user = super
+        super
 
-        build_smartcard_identity(user, params) if ::Gitlab::Auth::Smartcard.enabled?
-        set_pending_approval_state(user)
+        build_smartcard_identity if ::Gitlab::Auth::Smartcard.enabled?
+        set_pending_approval_state
 
         user
       end
@@ -56,10 +56,10 @@ module EE
       end
 
       override :build_identity
-      def build_identity(user)
+      def build_identity
         return super unless params[:provider] == GROUP_SCIM_PROVIDER
 
-        build_scim_identity(user)
+        build_scim_identity
         identity_params[:provider] = GROUP_SAML_PROVIDER
 
         user.provisioned_by_group_id = params[:group_id]
@@ -87,24 +87,22 @@ module EE
         end
       end
 
-      def build_smartcard_identity(user, params)
+      def build_smartcard_identity
         smartcard_identity_attrs = params.slice(:certificate_subject, :certificate_issuer)
 
-        unless smartcard_identity_attrs.empty?
-          user.smartcard_identities.build(subject: params[:certificate_subject],
-                                          issuer: params[:certificate_issuer])
-        end
+        return if smartcard_identity_attrs.empty?
+
+        user.smartcard_identities.build(subject: params[:certificate_subject], issuer: params[:certificate_issuer])
       end
 
-      def build_scim_identity(user)
+      def build_scim_identity
         scim_identity_params = params.slice(*scim_identity_attributes)
 
         user.scim_identities.build(scim_identity_params.merge(active: true))
       end
 
-      def set_pending_approval_state(user)
+      def set_pending_approval_state
         return unless ::Gitlab::CurrentSettings.should_apply_user_signup_cap?
-
         return unless user.human?
 
         user.state = ::User::BLOCKED_PENDING_APPROVAL_STATE