diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 24504685e48a8689c4fcbf293de51dceedb4a260..563bcc65bd6c8c9cadc9a9ebfad6dd0dc36ddfda 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/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
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index b44f38d4a0c648a2ee67cefe8e8391250afbebfa..a49a1f50a81e652849e9e1bb00cf76f6052ae5bc 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -1,5 +1,4 @@
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
diff --git a/app/models/user.rb b/app/models/user.rb
index 1c2821bb91a1b397eae7f367adfe63a38d6539e3..612066654dcb7600ead24ee64b291ce75a8d5b95 100644
--- a/app/models/user.rb
+++ b/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)
diff --git a/app/services/users/create_service.rb b/app/services/users/create_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..f4f0b80f30ab9fa3ccfc3c91fffbb8258120bfda
--- /dev/null
+++ b/app/services/users/create_service.rb
@@ -0,0 +1,110 @@
+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
diff --git a/changelogs/unreleased/27878-new-service-for-creating-user.yml b/changelogs/unreleased/27878-new-service-for-creating-user.yml
new file mode 100644
index 0000000000000000000000000000000000000000..c07f0cef8db9a033eb9693c724a720e7604eb8b0
--- /dev/null
+++ b/changelogs/unreleased/27878-new-service-for-creating-user.yml
@@ -0,0 +1,4 @@
+---
+title: Implement user create service
+merge_request: 9220
+author: George Andrinopoulos
diff --git a/doc/api/v3_to_v4.md b/doc/api/v3_to_v4.md
index 7f4426ee85d0ef35bfa12b31c531671fd59a3e08..8e002fe0022c5eb753d47f436e62b43399c7cfcd 100644
--- a/doc/api/v3_to_v4.md
+++ b/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
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 2d4d5a25221eeaae6e99d74bb89dd0651b2929f2..a4201fe6fed99923071e23a30cf3cb4ddea5ae21 100644
--- a/lib/api/users.rb
+++ b/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.
diff --git a/lib/api/v3/users.rb b/lib/api/v3/users.rb
index 14f54731730a54488d5b55d5c80fafda9884a32d..5e18cecc431a8b73c2e883e2433fc7b40014fd17 100644
--- a/lib/api/v3/users.rb
+++ b/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
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index fcf51b7fc5bfc157bb7884c73a4ae79e1771ea9f..f98481c6d3a9d0f20a7919e5162e1dceea63c7bd 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/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
diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb
index 8cc216445eb569c7889444beeeb42dc5a2dba1e8..902911071c4aa20a467ea2fe0d116ed4ff907437 100644
--- a/spec/controllers/registrations_controller_spec.rb
+++ b/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
diff --git a/spec/models/hooks/system_hook_spec.rb b/spec/models/hooks/system_hook_spec.rb
index e8caad00c4435afbd50af8ad08f49fd70f75fb1f..8acec80558402593756db6c66cc12e33ac949f2a 100644
--- a/spec/models/hooks/system_hook_spec.rb
+++ b/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/,
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 570abd44dc6c18058c40a4df45318fe99625e0c7..a9e37be1157799575920400ddd0b14ae3dda2ce4 100644
--- a/spec/models/user_spec.rb
+++ b/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
diff --git a/spec/requests/api/v3/users_spec.rb b/spec/requests/api/v3/users_spec.rb
index 17bbb0b53c1d7e0733916bd34aaff2985aef1cec..b38cbe74b858ee24f77cdb8f72e1d5780d3a43f7 100644
--- a/spec/requests/api/v3/users_spec.rb
+++ b/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
diff --git a/spec/services/users/create_service_spec.rb b/spec/services/users/create_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..5f79203701a0dba6be0885ac229367dcf750ba0f
--- /dev/null
+++ b/spec/services/users/create_service_spec.rb
@@ -0,0 +1,182 @@
+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