diff options
| -rw-r--r-- | app/controllers/admin/users_controller.rb | 12 | ||||
| -rw-r--r-- | app/controllers/registrations_controller.rb | 11 | ||||
| -rw-r--r-- | app/models/user.rb | 21 | ||||
| -rw-r--r-- | app/services/users/create_service.rb | 110 | ||||
| -rw-r--r-- | changelogs/unreleased/27878-new-service-for-creating-user.yml | 4 | ||||
| -rw-r--r-- | doc/api/v3_to_v4.md | 1 | ||||
| -rw-r--r-- | lib/api/users.rb | 27 | ||||
| -rw-r--r-- | lib/api/v3/users.rb | 53 | ||||
| -rw-r--r-- | lib/gitlab/o_auth/user.rb | 6 | ||||
| -rw-r--r-- | spec/controllers/registrations_controller_spec.rb | 9 | ||||
| -rw-r--r-- | spec/models/hooks/system_hook_spec.rb | 5 | ||||
| -rw-r--r-- | spec/models/user_spec.rb | 12 | ||||
| -rw-r--r-- | spec/requests/api/v3/users_spec.rb | 14 | ||||
| -rw-r--r-- | spec/services/users/create_service_spec.rb | 182 |
14 files changed, 393 insertions, 74 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 24504685e48..563bcc65bd6 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 b44f38d4a0c..a49a1f50a81 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 1c2821bb91a..612066654dc 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 00000000000..f4f0b80f30a --- /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 00000000000..c07f0cef8db --- /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 7f4426ee85d..8e002fe0022 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 2d4d5a25221..a4201fe6fed 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 14f54731730..5e18cecc431 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 fcf51b7fc5b..f98481c6d3a 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 8cc216445eb..902911071c4 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 e8caad00c44..8acec805584 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 570abd44dc6..a9e37be1157 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 17bbb0b53c1..b38cbe74b85 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 00000000000..5f79203701a --- /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 |