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

Create AuthorizedBuildService for user creation

See merge request gitlab-org/gitlab!61520

app/services/resource_access_tokens/create_service.rb

@@ -48,7 +48,7 @@ module ResourceAccessTokens
       # since someone like a project maintainer does not inherently have the ability
       # to create a new user in the system.
 
-      Users::CreateService.new(current_user, default_user_params).execute(skip_authorization: true)
+      ::Users::AuthorizedCreateService.new(current_user, default_user_params).execute
     end
 
     def delete_failed_user(user)

app/services/users/authorized_build_service.rb

+# frozen_string_literal: true
+
+module Users
+  class AuthorizedBuildService < BuildService
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    override :validate_access!
+    def validate_access!
+      # no-op
+    end
+
+    def signup_params
+      super + [:skip_confirmation]
+    end
+  end
+end

app/services/users/authorized_create_service.rb

+# frozen_string_literal: true
+
+module Users
+  class AuthorizedCreateService < CreateService
+    extend ::Gitlab::Utils::Override
+
+    private
+
+    override :build_class
+    def build_class
+      Users::AuthorizedBuildService
+    end
+  end
+end

app/services/users/build_service.rb

@@ -12,9 +12,7 @@ module Users
       @identity_params = params.slice(*identity_attributes)
     end
 
-    def execute(skip_authorization: false)
-      @skip_authorization = skip_authorization
-
+    def execute
       build_user
       build_identity
       update_canonical_email
@@ -24,7 +22,7 @@ module Users
 
     private
 
-    attr_reader :skip_authorization, :identity_params, :user_params, :user
+    attr_reader :identity_params, :user_params, :user
 
     def identity_attributes
       [:extern_uid, :provider]
@@ -97,7 +95,6 @@ module Users
     end
 
     def validate_access!
-      return if skip_authorization
       return if can_create_user?
 
       raise Gitlab::Access::AccessDeniedError
@@ -108,18 +105,11 @@ module Users
     end
 
     def build_user_params_for_non_admin
-      allowed_signup_params = signup_params
-      allowed_signup_params << :skip_confirmation if allow_caller_to_request_skip_confirmation?
-
-      @user_params = params.slice(*allowed_signup_params)
+      @user_params = params.slice(*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 allow_caller_to_request_skip_confirmation?
-      skip_authorization
-    end
-
     def assign_skip_confirmation_from_settings?
       user_params[:skip_confirmation].nil?
     end

app/services/users/create_service.rb

@@ -9,8 +9,8 @@ module Users
       @params = params.dup
     end
 
-    def execute(skip_authorization: false)
-      user = Users::BuildService.new(current_user, params).execute(skip_authorization: skip_authorization)
+    def execute
+      user = build_class.new(current_user, params).execute
       reset_token = user.generate_reset_token if user.recently_sent_password_reset?
 
       after_create_hook(user, reset_token) if user.save
@@ -23,6 +23,11 @@ module Users
     def after_create_hook(user, reset_token)
       notify_new_user(user, reset_token)
     end
+
+    def build_class
+      # overridden by inheriting classes
+      Users::BuildService
+    end
   end
 end
 

app/services/users/registrations_build_service.rb

@@ -6,9 +6,8 @@ module Users
 
     private
 
-    override :allow_caller_to_request_skip_confirmation?
-    def allow_caller_to_request_skip_confirmation?
-      true
+    def signup_params
+      super + [:skip_confirmation]
     end
 
     override :assign_skip_confirmation_from_settings?

ee/app/controllers/trial_registrations_controller.rb

@@ -35,6 +35,6 @@ class TrialRegistrationsController < RegistrationsController
   end
 
   def resource
-    @resource ||= Users::BuildService.new(current_user, sign_up_params).execute(skip_authorization: true)
+    @resource ||= Users::AuthorizedBuildService.new(current_user, sign_up_params).execute
   end
 end

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

@@ -18,7 +18,7 @@ module EE
       end
 
       override :execute
-      def execute(skip_authorization: false)
+      def execute
         super
 
         build_smartcard_identity if ::Gitlab::Auth::Smartcard.enabled?

ee/lib/ee/gitlab/scim/provisioning_service.rb

@@ -69,7 +69,7 @@ module EE
         end
 
         def build_user
-          ::Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
+          ::Users::AuthorizedBuildService.new(nil, user_params).execute
         end
 
         def build_scim_identity

ee/lib/gitlab/auth/smartcard/certificate.rb

@@ -35,7 +35,7 @@ module Gitlab
             skip_confirmation:          true
           }
 
-          Users::CreateService.new(nil, user_params).execute(skip_authorization: true)
+          Users::AuthorizedCreateService.new(nil, user_params).execute
         end
 
         def create_smartcard_identity_for(user)

ee/lib/gitlab/auth/smartcard/ldap_certificate.rb

@@ -43,7 +43,7 @@ module Gitlab
             skip_confirmation:          true
           }
 
-          Users::CreateService.new(nil, user_params).execute(skip_authorization: true)
+          Users::AuthorizedCreateService.new(nil, user_params).execute
         end
 
         def create_ldap_certificate_identity_for(user)

ee/spec/lib/gitlab/auth/smartcard/certificate_spec.rb

@@ -119,12 +119,12 @@ RSpec.describe Gitlab::Auth::Smartcard::Certificate do
                         certificate_subject:  subject_dn,
                         certificate_issuer: issuer_dn,
                         skip_confirmation: true }
-        expect(Users::BuildService).to(
+        expect(Users::AuthorizedBuildService).to(
           receive(:new)
             .with(nil, hash_including(user_params))
             .and_return(user_build_service))
         expect(user_build_service).to(
-          receive(:execute).with(skip_authorization: true).and_return(user))
+          receive(:execute).and_return(user))
 
         subject
       end

ee/spec/lib/gitlab/auth/smartcard/ldap_certificate_spec.rb

@@ -138,12 +138,12 @@ RSpec.describe Gitlab::Auth::Smartcard::LdapCertificate do
                         password_automatically_set: true,
                         skip_confirmation: true }
 
-        expect(Users::BuildService).to(
+        expect(Users::AuthorizedBuildService).to(
           receive(:new)
             .with(nil, hash_including(user_params))
             .and_return(user_build_service))
         expect(user_build_service).to(
-          receive(:execute).with(skip_authorization: true).and_return(user))
+          receive(:execute).and_return(user))
 
         subject
       end

ee/spec/services/ee/users/authorized_build_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::AuthorizedBuildService do
+  describe '#execute' do
+    context 'with non admin user' do
+      let(:non_admin) { create(:user) }
+
+      context 'when user signup cap is set' do
+        before do
+          allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(10)
+        end
+
+        it 'does not set the user state to blocked_pending_approval for non human users' do
+          params = {
+            name: 'Project Bot',
+            email: 'project_bot@example.com',
+            username: 'project_bot',
+            user_type: 'project_bot'
+          }
+
+          service = described_class.new(non_admin, params)
+          user = service.execute
+
+          expect(user).to be_active
+        end
+      end
+    end
+  end
+end

ee/spec/services/ee/users/build_service_spec.rb

@@ -8,31 +8,6 @@ RSpec.describe Users::BuildService do
       { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
     end
 
-    context 'with non admin user' do
-      let(:non_admin) { create(:user) }
-
-      context 'when user signup cap is set' do
-        before do
-          allow(Gitlab::CurrentSettings).to receive(:new_user_signups_cap).and_return(10)
-        end
-
-        it 'does not set the user state to blocked_pending_approval for non human users' do
-          params = {
-            name: 'Project Bot',
-            email: 'project_bot@example.com',
-            username: 'project_bot',
-            user_type: 'project_bot',
-            skip_confirmation: true
-          }
-
-          service = described_class.new(non_admin, params)
-          user = service.execute(skip_authorization: true)
-
-          expect(user).to be_active
-        end
-      end
-    end
-
     context 'with an admin user' do
       let_it_be(:admin_user) { create(:admin) }
 

lib/api/users.rb

@@ -241,7 +241,7 @@ module API
         authenticated_as_admin!
 
         params = declared_params(include_missing: false)
-        user = ::Users::CreateService.new(current_user, params).execute(skip_authorization: true)
+        user = ::Users::AuthorizedCreateService.new(current_user, params).execute
 
         if user.persisted?
           present user, with: Entities::UserWithAdmin, current_user: current_user

lib/gitlab/auth/o_auth/user.rb

@@ -208,7 +208,7 @@ module Gitlab
 
         def build_new_user(skip_confirmation: true)
           user_params = user_attributes.merge(skip_confirmation: skip_confirmation)
-          Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
+          Users::AuthorizedBuildService.new(nil, user_params).execute
         end
 
         def user_attributes

spec/services/users/authorized_build_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Users::AuthorizedBuildService do
+  describe '#execute' do
+    let_it_be(:current_user) { create(:user) }
+
+    let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
+
+    subject(:user) { described_class.new(current_user, params).execute }
+
+    it_behaves_like 'common user build items'
+    it_behaves_like 'current user not admin build items'
+  end
+end

spec/services/users/build_service_spec.rb

@@ -11,148 +11,19 @@ RSpec.describe Users::BuildService do
     let(:params) { build_stubbed(:user).slice(:first_name, :last_name, :username, :email, :password) }
     let(:service) { described_class.new(current_user, params) }
 
-    shared_examples_for 'common build items' do
-      it { is_expected.to be_valid }
-
-      it 'sets the created_by_id' do
-        expect(user.created_by_id).to eq(current_user&.id)
-      end
-
-      it 'calls UpdateCanonicalEmailService' do
-        expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
-
-        user
-      end
-
-      context 'when user_type is provided' do
-        context 'when project_bot' do
-          before do
-            params.merge!({ user_type: :project_bot })
-          end
-
-          it { expect(user.project_bot?).to be true }
-        end
-
-        context 'when not a project_bot' do
-          before do
-            params.merge!({ user_type: :alert_bot })
-          end
-
-          it { expect(user).to be_human }
-        end
-      end
-    end
-
-    shared_examples_for 'current user not admin' do
-      context 'with "user_default_external" application setting' do
-        where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
-          true  | nil   | 'fl@example.com'        | nil                     | true
-          true  | true  | 'fl@example.com'        | nil                     | true
-          true  | false | 'fl@example.com'        | nil                     | true # admin difference
-
-          true  | nil   | 'fl@example.com'        | ''                      | true
-          true  | true  | 'fl@example.com'        | ''                      | true
-          true  | false | 'fl@example.com'        | ''                      | true # admin difference
-
-          true  | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
-          true  | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-
-          true  | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
-          true  | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
-          true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
-
-          false | nil   | 'fl@example.com'        | nil                     | false
-          false | true  | 'fl@example.com'        | nil                     | false # admin difference
-          false | false | 'fl@example.com'        | nil                     | false
-
-          false | nil   | 'fl@example.com'        | ''                      | false
-          false | true  | 'fl@example.com'        | ''                      | false # admin difference
-          false | false | 'fl@example.com'        | ''                      | false
-
-          false | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
-          false | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
-
-          false | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
-          false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
-          false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
-        end
-
-        with_them do
-          before do
-            stub_application_setting(user_default_external: user_default_external)
-            stub_application_setting(user_default_internal_regex: user_default_internal_regex)
-
-            params.merge!({ external: external, email: email }.compact)
-          end
-
-          it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
-            expect(user.external).to eq(result)
-          end
-        end
-      end
-
-      context 'when "send_user_confirmation_email" application setting is true' do
-        before do
-          stub_application_setting(send_user_confirmation_email: true, signup_enabled?: true)
-        end
-
-        it 'does not confirm the user' do
-          expect(user).not_to be_confirmed
-        end
-      end
-
-      context 'when "send_user_confirmation_email" application setting is false' do
-        before do
-          stub_application_setting(send_user_confirmation_email: false, signup_enabled?: true)
-        end
-
-        it 'confirms the user' do
-          expect(user).to be_confirmed
-        end
-      end
-
-      context 'with allowed params' do
-        let(:params) do
-          {
-            email: 1,
-            name: 1,
-            password: 1,
-            password_automatically_set: 1,
-            username: 1,
-            user_type: 'project_bot'
-          }
-        end
-
-        it 'sets all allowed attributes' do
-          expect(User).to receive(:new).with(hash_including(params)).and_call_original
-
-          user
-        end
-      end
-    end
-
     context 'with nil current_user' do
       subject(:user) { service.execute }
 
-      it_behaves_like 'common build items'
-      it_behaves_like 'current user not admin'
+      it_behaves_like 'common user build items'
+      it_behaves_like 'current user not admin build items'
     end
 
     context 'with non admin current_user' do
       let_it_be(:current_user) { create(:user) }
 
-      let(:service) { described_class.new(current_user, params) }
-
-      subject(:user) { service.execute(skip_authorization: true) }
-
-      it 'raises AccessDeniedError exception when authorization is not skipped' do
-        expect { service.execute }.to raise_error Gitlab::Access::AccessDeniedError
+      it 'raises AccessDeniedError exception' do
+        expect { described_class.new(current_user, params).execute }.to raise_error Gitlab::Access::AccessDeniedError
       end
-
-      it_behaves_like 'common build items'
-      it_behaves_like 'current user not admin'
     end
 
     context 'with an admin current_user' do
@@ -163,7 +34,7 @@ RSpec.describe Users::BuildService do
 
       subject(:user) { service.execute }
 
-      it_behaves_like 'common build items'
+      it_behaves_like 'common user build items'
 
       context 'with allowed params' do
         let(:params) do

spec/support/shared_examples/services/users/build_service_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'common user build items' do
+  it { is_expected.to be_valid }
+
+  it 'sets the created_by_id' do
+    expect(user.created_by_id).to eq(current_user&.id)
+  end
+
+  it 'calls UpdateCanonicalEmailService' do
+    expect(Users::UpdateCanonicalEmailService).to receive(:new).and_call_original
+
+    user
+  end
+
+  context 'when user_type is provided' do
+    context 'when project_bot' do
+      before do
+        params.merge!({ user_type: :project_bot })
+      end
+
+      it { expect(user.project_bot?).to be true }
+    end
+
+    context 'when not a project_bot' do
+      before do
+        params.merge!({ user_type: :alert_bot })
+      end
+
+      it { expect(user).to be_human }
+    end
+  end
+end
+
+RSpec.shared_examples_for 'current user not admin build items' do
+  using RSpec::Parameterized::TableSyntax
+
+  context 'with "user_default_external" application setting' do
+    where(:user_default_external, :external, :email, :user_default_internal_regex, :result) do
+      true  | nil   | 'fl@example.com'        | nil                     | true
+      true  | true  | 'fl@example.com'        | nil                     | true
+      true  | false | 'fl@example.com'        | nil                     | true # admin difference
+
+      true  | nil   | 'fl@example.com'        | ''                      | true
+      true  | true  | 'fl@example.com'        | ''                      | true
+      true  | false | 'fl@example.com'        | ''                      | true # admin difference
+
+      true  | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+      true  | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
+      true  | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+
+      true  | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
+      true  | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true
+      true  | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | true # admin difference
+
+      false | nil   | 'fl@example.com'        | nil                     | false
+      false | true  | 'fl@example.com'        | nil                     | false # admin difference
+      false | false | 'fl@example.com'        | nil                     | false
+
+      false | nil   | 'fl@example.com'        | ''                      | false
+      false | true  | 'fl@example.com'        | ''                      | false # admin difference
+      false | false | 'fl@example.com'        | ''                      | false
+
+      false | nil   | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+      false | true  | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
+      false | false | 'fl@example.com'        | '^(?:(?!\.ext@).)*$\r?' | false
+
+      false | nil   | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
+      false | true  | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false # admin difference
+      false | false | 'tester.ext@domain.com' | '^(?:(?!\.ext@).)*$\r?' | false
+    end
+
+    with_them do
+      before do
+        stub_application_setting(user_default_external: user_default_external)
+        stub_application_setting(user_default_internal_regex: user_default_internal_regex)
+
+        params.merge!({ external: external, email: email }.compact)
+      end
+
+      it 'sets the value of Gitlab::CurrentSettings.user_default_external' do
+        expect(user.external).to eq(result)
+      end
+    end
+  end
+
+  context 'when "send_user_confirmation_email" application setting is true' do
+    before do
+      stub_application_setting(send_user_confirmation_email: true, signup_enabled?: true)
+    end
+
+    it 'does not confirm the user' do
+      expect(user).not_to be_confirmed
+    end
+  end
+
+  context 'when "send_user_confirmation_email" application setting is false' do
+    before do
+      stub_application_setting(send_user_confirmation_email: false, signup_enabled?: true)
+    end
+
+    it 'confirms the user' do
+      expect(user).to be_confirmed
+    end
+  end
+
+  context 'with allowed params' do
+    let(:params) do
+      {
+        email: 1,
+        name: 1,
+        password: 1,
+        password_automatically_set: 1,
+        username: 1,
+        user_type: 'project_bot'
+      }
+    end
+
+    it 'sets all allowed attributes' do
+      expect(User).to receive(:new).with(hash_including(params)).and_call_original
+
+      user
+    end
+  end
+end