Commit 7c74a020 authored by George Andrinopoulos's avatar George Andrinopoulos Committed by Rémy Coutable
Browse files

Implement new service for creating user

parent e19d4c51
......@@ -95,18 +95,14 @@ def disable_two_factor
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
......
class RegistrationsController < Devise::RegistrationsController
before_action :signup_enabled?
include Recaptcha::Verify
def new
......@@ -21,6 +20,8 @@ def create
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 @@ def after_inactive_sign_up_path_for(_resource)
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 @@ def resource_name
end
def resource
@resource ||= User.new(sign_up_params)
@resource ||= Users::CreateService.new(current_user, sign_up_params).build
end
def devise_mapping
......
......@@ -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 @@ def to_reference(_from_project = nil, target_project: nil, full: nil)
"#{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 @@ def generate_reset_token
@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 @@ def ensure_namespace_correct
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)
......
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
---
title: Implement user create service
merge_request: 9220
author: George Andrinopoulos
......@@ -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
......@@ -27,7 +27,7 @@ def find_user(params)
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 @@ def find_user(params)
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.
......
......@@ -9,6 +9,59 @@ class Users < Grape::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
......
......@@ -147,10 +147,8 @@ def find_by_uid_and_provider
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
......
......@@ -30,6 +30,15 @@
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
......
......@@ -6,6 +6,9 @@
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 @@
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/,
......
......@@ -361,22 +361,10 @@
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
......
......@@ -263,4 +263,18 @@
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
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,