Merge branch '27878-new-service-for-creating-user' into 'master'

Implement new service for creating user

Closes #27878

See merge request !9220

app/controllers/admin/users_controller.rb

@@ -95,18 +95,14 @@ class Admin::UsersController < Admin::ApplicationController
 
   def create
     opts = {
-      force_random_password: true,
-      password_expires_at: nil
+      reset_password: true,
+      skip_confirmation: true
     }
 
-    @user = User.new(user_params.merge(opts))
-    @user.created_by_id = current_user.id
-    @user.generate_password
-    @user.generate_reset_token
-    @user.skip_confirmation!
+    @user = Users::CreateService.new(current_user, user_params.merge(opts)).execute
 
     respond_to do |format|
-      if @user.save
+      if @user.persisted?
         format.html { redirect_to [:admin, @user], notice: 'User was successfully created.' }
         format.json { render json: @user, status: :created, location: @user }
       else

app/controllers/registrations_controller.rb

 class RegistrationsController < Devise::RegistrationsController
-  before_action :signup_enabled?
   include Recaptcha::Verify
 
   def new
@@ -21,6 +20,8 @@ class RegistrationsController < Devise::RegistrationsController
       flash.delete :recaptcha_error
       render action: 'new'
     end
+  rescue Gitlab::Access::AccessDeniedError
+    redirect_to(new_user_session_path)
   end
 
   def destroy
@@ -50,12 +51,6 @@ class RegistrationsController < Devise::RegistrationsController
 
   private
 
-  def signup_enabled?
-    unless current_application_settings.signup_enabled?
-      redirect_to(new_user_session_path)
-    end
-  end
-
   def sign_up_params
     params.require(:user).permit(:username, :email, :email_confirmation, :name, :password)
   end
@@ -65,7 +60,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   def resource
-    @resource ||= User.new(sign_up_params)
+    @resource ||= Users::CreateService.new(current_user, sign_up_params).build
   end
 
   def devise_mapping

app/models/user.rb

@@ -128,10 +128,9 @@ class User < ActiveRecord::Base
   validate :unique_email, if: ->(user) { user.email_changed? }
   validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
   validate :owns_public_email, if: ->(user) { user.public_email_changed? }
+  validate :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
   validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
 
-  before_validation :generate_password, on: :create
-  before_validation :signup_domain_valid?, on: :create, if: ->(user) { !user.created_by_id }
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: ->(user) { user.email_changed? }
   before_validation :set_public_email, if: ->(user) { user.public_email_changed? }
@@ -141,8 +140,6 @@ class User < ActiveRecord::Base
   before_save :ensure_external_user_rights
   after_save :ensure_namespace_correct
   after_initialize :set_projects_limit
-  before_create :check_confirmation_email
-  after_create :post_create_hook
   after_destroy :post_destroy_hook
 
   # User's Layout preference
@@ -386,10 +383,8 @@ class User < ActiveRecord::Base
     "#{self.class.reference_prefix}#{username}"
   end
 
-  def generate_password
-    if force_random_password
-      self.password = self.password_confirmation = Devise.friendly_token.first(Devise.password_length.min)
-    end
+  def skip_confirmation=(bool)
+    skip_confirmation! if bool
   end
 
   def generate_reset_token
@@ -401,10 +396,6 @@ class User < ActiveRecord::Base
     @reset_token
   end
 
-  def check_confirmation_email
-    skip_confirmation! unless current_application_settings.send_user_confirmation_email
-  end
-
   def recently_sent_password_reset?
     reset_password_sent_at.present? && reset_password_sent_at >= 1.minute.ago
   end
@@ -799,12 +790,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  def post_create_hook
-    log_info("User \"#{name}\" (#{email}) was created")
-    notification_service.new_user(self, @reset_token) if created_by_id
-    system_hook_service.execute_hooks_for(self, :create)
-  end
-
   def post_destroy_hook
     log_info("User \"#{name}\" (#{email})  was removed")
     system_hook_service.execute_hooks_for(self, :destroy)

app/services/users/create_service.rb

+module Users
+  # Service for creating a new user.
+  class CreateService < BaseService
+    def initialize(current_user, params = {})
+      @current_user = current_user
+      @params = params.dup
+    end
+
+    def build
+      raise Gitlab::Access::AccessDeniedError unless can_create_user?
+
+      user = User.new(build_user_params)
+
+      if current_user&.is_admin?
+        if params[:reset_password]
+          @reset_token = user.generate_reset_token
+          params[:force_random_password] = true
+        end
+
+        if params[:force_random_password]
+          random_password = Devise.friendly_token.first(Devise.password_length.min)
+          user.password = user.password_confirmation = random_password
+        end
+      end
+
+      identity_attrs = params.slice(:extern_uid, :provider)
+
+      if identity_attrs.any?
+        user.identities.build(identity_attrs)
+      end
+
+      user
+    end
+
+    def execute
+      user = build
+
+      if user.save
+        log_info("User \"#{user.name}\" (#{user.email}) was created")
+        notification_service.new_user(user, @reset_token) if @reset_token
+        system_hook_service.execute_hooks_for(user, :create)
+      end
+
+      user
+    end
+
+    private
+
+    def can_create_user?
+      (current_user.nil? && current_application_settings.signup_enabled?) || current_user&.is_admin?
+    end
+
+    # Allowed params for creating a user (admins only)
+    def admin_create_params
+      [
+        :access_level,
+        :admin,
+        :avatar,
+        :bio,
+        :can_create_group,
+        :color_scheme_id,
+        :email,
+        :external,
+        :force_random_password,
+        :hide_no_password,
+        :hide_no_ssh_key,
+        :key_id,
+        :linkedin,
+        :name,
+        :password,
+        :password_expires_at,
+        :projects_limit,
+        :remember_me,
+        :skip_confirmation,
+        :skype,
+        :theme_id,
+        :twitter,
+        :username,
+        :website_url
+      ]
+    end
+
+    # Allowed params for user signup
+    def signup_params
+      [
+        :email,
+        :email_confirmation,
+        :name,
+        :password,
+        :username
+      ]
+    end
+
+    def build_user_params
+      if current_user&.is_admin?
+        user_params = params.slice(*admin_create_params)
+        user_params[:created_by_id] = current_user.id
+
+        if params[:reset_password]
+          user_params.merge!(force_random_password: true, password_expires_at: nil)
+        end
+      else
+        user_params = params.slice(*signup_params)
+        user_params[:skip_confirmation] = !current_application_settings.send_user_confirmation_email
+      end
+
+      user_params
+    end
+  end
+end

changelogs/unreleased/27878-new-service-for-creating-user.yml

+---
+title: Implement user create service
+merge_request: 9220
+author: George Andrinopoulos

doc/api/v3_to_v4.md

@@ -80,3 +80,4 @@ Below are the changes made between V3 and V4.
   - `GET /projects/:id/repository/blobs/:sha` now returns JSON attributes for the blob identified by `:sha`, instead of finding the commit identified by `:sha` and returning the raw content of the blob in that commit identified by the required `?filepath=:filepath`
   - Moved `GET /projects/:id/repository/commits/:sha/blob?file_path=:file_path`  and `GET /projects/:id/repository/blobs/:sha?file_path=:file_path` to `GET /projects/:id/repository/files/:file_path/raw?ref=:sha`
   - `GET /projects/:id/repository/tree` parameter `ref_name` has been renamed to `ref` for consistency
+- `confirm` parameter for `POST /users` has been deprecated in favor of `skip_confirmation` parameter

lib/api/users.rb

@@ -27,7 +27,7 @@ module API
           optional :location, type: String, desc: 'The location of the user'
           optional :admin, type: Boolean, desc: 'Flag indicating the user is an administrator'
           optional :can_create_group, type: Boolean, desc: 'Flag indicating the user can create groups'
-          optional :confirm, type: Boolean, desc: 'Flag indicating the account needs to be confirmed'
+          optional :skip_confirmation, type: Boolean, default: false, desc: 'Flag indicating the account is confirmed'
           optional :external, type: Boolean, desc: 'Flag indicating the user is an external user'
           all_or_none_of :extern_uid, :provider
         end
@@ -97,29 +97,10 @@ module API
       post do
         authenticated_as_admin!
 
-        # Filter out params which are used later
-        user_params = declared_params(include_missing: false)
-        identity_attrs = user_params.slice(:provider, :extern_uid)
-        confirm = user_params.delete(:confirm)
-        user = User.new(user_params.except(:extern_uid, :provider, :reset_password))
-
-        if user_params.delete(:reset_password)
-          user.attributes = {
-            force_random_password: true,
-            password_expires_at: nil,
-            created_by_id: current_user.id
-          }
-          user.generate_password
-          user.generate_reset_token
-        end
-
-        user.skip_confirmation! unless confirm
-
-        if identity_attrs.any?
-          user.identities.build(identity_attrs)
-        end
+        params = declared_params(include_missing: false)
+        user = ::Users::CreateService.new(current_user, params).execute
 
-        if user.save
+        if user.persisted?
           present user, with: Entities::UserPublic
         else
           conflict!('Email has already been taken') if User.

lib/api/v3/users.rb

@@ -9,6 +9,59 @@ module API
       end
 
       resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
+        helpers do
+          params :optional_attributes do
+            optional :skype, type: String, desc: 'The Skype username'
+            optional :linkedin, type: String, desc: 'The LinkedIn username'
+            optional :twitter, type: String, desc: 'The Twitter username'
+            optional :website_url, type: String, desc: 'The website of the user'
+            optional :organization, type: String, desc: 'The organization of the user'
+            optional :projects_limit, type: Integer, desc: 'The number of projects a user can create'
+            optional :extern_uid, type: String, desc: 'The external authentication provider UID'
+            optional :provider, type: String, desc: 'The external provider'
+            optional :bio, type: String, desc: 'The biography of the user'
+            optional :location, type: String, desc: 'The location of the user'
+            optional :admin, type: Boolean, desc: 'Flag indicating the user is an administrator'
+            optional :can_create_group, type: Boolean, desc: 'Flag indicating the user can create groups'
+            optional :confirm, type: Boolean, default: true, desc: 'Flag indicating the account needs to be confirmed'
+            optional :external, type: Boolean, desc: 'Flag indicating the user is an external user'
+            all_or_none_of :extern_uid, :provider
+          end
+        end
+
+        desc 'Create a user. Available only for admins.' do
+          success ::API::Entities::UserPublic
+        end
+        params do
+          requires :email, type: String, desc: 'The email of the user'
+          optional :password, type: String, desc: 'The password of the new user'
+          optional :reset_password, type: Boolean, desc: 'Flag indicating the user will be sent a password reset token'
+          at_least_one_of :password, :reset_password
+          requires :name, type: String, desc: 'The name of the user'
+          requires :username, type: String, desc: 'The username of the user'
+          use :optional_attributes
+        end
+        post do
+          authenticated_as_admin!
+
+          params = declared_params(include_missing: false)
+          user = ::Users::CreateService.new(current_user, params.merge!(skip_confirmation: !params[:confirm])).execute
+
+          if user.persisted?
+            present user, with: ::API::Entities::UserPublic
+          else
+            conflict!('Email has already been taken') if User.
+                where(email: user.email).
+                count > 0
+
+            conflict!('Username has already been taken') if User.
+                where(username: user.username).
+                count > 0
+
+            render_validation_error!(user)
+          end
+        end
+
         desc 'Get the SSH keys of a specified user. Available only for admins.' do
           success ::API::Entities::SSHKey
         end

lib/gitlab/o_auth/user.rb

@@ -147,10 +147,8 @@ module Gitlab
       end
 
       def build_new_user
-        user = ::User.new(user_attributes)
-        user.skip_confirmation!
-        user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider)
-        user
+        user_params = user_attributes.merge(extern_uid: auth_hash.uid, provider: auth_hash.provider, skip_confirmation: true)
+        Users::CreateService.new(nil, user_params).build
       end
 
       def user_attributes

spec/controllers/registrations_controller_spec.rb

@@ -30,6 +30,15 @@ describe RegistrationsController do
           expect(subject.current_user).to be_nil
         end
       end
+
+      context 'when signup_enabled? is false' do
+        it 'redirects to sign_in' do
+          allow_any_instance_of(ApplicationSetting).to receive(:signup_enabled?).and_return(false)
+
+          expect { post(:create, user_params) }.not_to change(User, :count)
+          expect(response).to redirect_to(new_user_session_path)
+        end
+      end
     end
 
     context 'when reCAPTCHA is enabled' do

spec/models/hooks/system_hook_spec.rb

@@ -6,6 +6,9 @@ describe SystemHook, models: true do
     let(:user)        { create(:user) }
     let(:project)     { create(:empty_project, namespace: user.namespace) }
     let(:group)       { create(:group) }
+    let(:params) do
+      { name: 'John Doe', username: 'jduser', email: 'jg@example.com', password: 'mydummypass' }
+    end
 
     before do
       WebMock.stub_request(:post, system_hook.url)
@@ -29,7 +32,7 @@ describe SystemHook, models: true do
     end
 
     it "user_create hook" do
-      create(:user)
+      Users::CreateService.new(nil, params).execute
 
       expect(WebMock).to have_requested(:post, system_hook.url).with(
         body: /user_create/,

spec/models/user_spec.rb

@@ -361,22 +361,10 @@ describe User, models: true do
   end
 
   describe '#generate_password' do
-    it "executes callback when force_random_password specified" do
-      user = build(:user, force_random_password: true)
-      expect(user).to receive(:generate_password)
-      user.save
-    end
-
     it "does not generate password by default" do
       user = create(:user, password: 'abcdefghe')
       expect(user.password).to eq('abcdefghe')
     end
-
-    it "generates password when forcing random password" do
-      allow(Devise).to receive(:friendly_token).and_return('123456789')
-      user = create(:user, password: 'abcdefg', force_random_password: true)
-      expect(user.password).to eq('12345678')
-    end
   end
 
   describe 'authentication token' do

spec/requests/api/v3/users_spec.rb

@@ -263,4 +263,18 @@ describe API::V3::Users, api: true  do
       expect(json_response['message']).to eq('404 User Not Found')
     end
   end
+
+  describe 'POST /users' do
+    it 'creates confirmed user when confirm parameter is false' do
+      optional_attributes = { confirm: false }
+      attributes = attributes_for(:user).merge(optional_attributes)
+
+      post v3_api('/users', admin), attributes
+
+      user_id = json_response['id']
+      new_user = User.find(user_id)
+
+      expect(new_user).to be_confirmed
+    end
+  end
 end

spec/services/users/create_service_spec.rb

+require 'spec_helper'
+
+describe Users::CreateService, services: true do
+  describe '#build' do
+    let(:params) do
+      { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
+    end
+
+    context 'with an admin user' do
+      let(:admin_user) { create(:admin) }
+      let(:service) { described_class.new(admin_user, params) }
+
+      it 'returns a valid user' do
+        expect(service.build).to be_valid
+      end
+    end
+
+    context 'with non admin user' do
+      let(:user) { create(:user) }
+      let(:service) { described_class.new(user, params) }
+
+      it 'raises AccessDeniedError exception' do
+        expect { service.build }.to raise_error Gitlab::Access::AccessDeniedError
+      end
+    end
+
+    context 'with nil user' do
+      let(:service) { described_class.new(nil, params) }
+
+      it 'returns a valid user' do
+        expect(service.build).to be_valid
+      end
+    end
+  end
+
+  describe '#execute' do
+    let(:admin_user) { create(:admin) }
+
+    context 'with an admin user' do
+      let(:service) { described_class.new(admin_user, params) }
+
+      context 'when required parameters are provided' do
+        let(:params) do
+          { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
+        end
+
+        it 'returns a persisted user' do
+          expect(service.execute).to be_persisted
+        end
+
+        it 'persists the given attributes' do
+          user = service.execute
+          user.reload
+
+          expect(user).to have_attributes(
+            name: params[:name],
+            username: params[:username],
+            email: params[:email],
+            password: params[:password],
+            created_by_id: admin_user.id
+          )
+        end
+
+        it 'user is not confirmed if skip_confirmation param is not present' do
+          expect(service.execute).not_to be_confirmed
+        end
+
+        it 'logs the user creation' do
+          expect(service).to receive(:log_info).with("User \"John Doe\" (jd@example.com) was created")
+
+          service.execute
+        end
+
+        it 'executes system hooks ' do
+          system_hook_service = spy(:system_hook_service)
+
+          expect(service).to receive(:system_hook_service).and_return(system_hook_service)
+
+          user = service.execute
+
+          expect(system_hook_service).to have_received(:execute_hooks_for).with(user, :create)
+        end
+
+        it 'does not send a notification email' do
+          notification_service = spy(:notification_service)
+
+          expect(service).not_to receive(:notification_service)
+
+          service.execute
+
+          expect(notification_service).not_to have_received(:new_user)
+        end
+      end
+
+      context 'when force_random_password parameter is true' do
+        let(:params) do
+          { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', force_random_password: true }
+        end
+
+        it 'generates random password' do
+          user = service.execute
+
+          expect(user.password).not_to eq 'mydummypass'
+          expect(user.password).to be_present
+        end
+      end
+
+      context 'when skip_confirmation parameter is true' do
+        let(:params) do
+          { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true }
+        end
+
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+      end
+
+      context 'when reset_password parameter is true' do
+        let(:params) do
+          { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', reset_password: true }
+        end
+
+        it 'resets password even if a password parameter is given' do
+          expect(service.execute).to be_recently_sent_password_reset
+        end
+
+        it 'sends a notification email' do
+          notification_service = spy(:notification_service)
+
+          expect(service).to receive(:notification_service).and_return(notification_service)
+
+          user = service.execute
+
+          expect(notification_service).to have_received(:new_user).with(user, an_instance_of(String))
+        end
+      end
+    end
+
+    context 'with nil user' do
+      let(:params) do
+        { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass', skip_confirmation: true }
+      end
+      let(:service) { described_class.new(nil, params) }
+
+      context 'when "send_user_confirmation_email" application setting is true' do
+        before do
+          current_application_settings = double(:current_application_settings, send_user_confirmation_email: true, signup_enabled?: true)
+          allow(service).to receive(:current_application_settings).and_return(current_application_settings)
+        end
+
+        it 'does not confirm the user' do
+          expect(service.execute).not_to be_confirmed
+        end
+      end
+
+      context 'when "send_user_confirmation_email" application setting is false' do
+        before do
+          current_application_settings = double(:current_application_settings, send_user_confirmation_email: false, signup_enabled?: true)
+          allow(service).to receive(:current_application_settings).and_return(current_application_settings)
+        end
+
+        it 'confirms the user' do
+          expect(service.execute).to be_confirmed
+        end
+
+        it 'persists the given attributes' do
+          user = service.execute
+          user.reload
+
+          expect(user).to have_attributes(
+            name: params[:name],
+            username: params[:username],
+            email: params[:email],
+            password: params[:password],
+            created_by_id: nil,
+            admin: false
+          )
+        end
+      end
+    end
+  end
+end