diff options
-rw-r--r-- | app/controllers/admin/users_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/profiles/avatars_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/profiles/notifications_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/profiles/passwords_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/profiles/preferences_controller.rb | 2 | ||||
-rw-r--r-- | app/controllers/profiles/two_factor_auths_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/profiles_controller.rb | 10 | ||||
-rw-r--r-- | app/controllers/sessions_controller.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 6 | ||||
-rw-r--r-- | app/services/emails/destroy_service.rb | 4 | ||||
-rw-r--r-- | app/services/users/update_service.rb | 17 | ||||
-rw-r--r-- | lib/api/internal.rb | 2 | ||||
-rw-r--r-- | lib/api/notification_settings.rb | 2 | ||||
-rw-r--r-- | lib/api/users.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/ldap/access.rb | 2 | ||||
-rw-r--r-- | lib/gitlab/o_auth/user.rb | 2 | ||||
-rw-r--r-- | spec/services/users/update_service_spec.rb | 39 |
17 files changed, 42 insertions, 68 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index c44f381664f..3b90cd77be0 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -125,7 +125,7 @@ class Admin::UsersController < Admin::ApplicationController end respond_to do |format| - result = Users::UpdateService.new(current_user, user, user_params_with_pass).execute do |user| + result = Users::UpdateService.new(user, user_params_with_pass).execute do |user| user.skip_reconfirmation! end @@ -211,7 +211,7 @@ class Admin::UsersController < Admin::ApplicationController end def update_user - result = Users::UpdateService.new(current_user, user).execute do |user| + result = Users::UpdateService.new(user).execute do |user| yield(user) end diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb index 851885d2224..408650aac54 100644 --- a/app/controllers/profiles/avatars_controller.rb +++ b/app/controllers/profiles/avatars_controller.rb @@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController def destroy @user = current_user - Users::UpdateService.new(@user, @user).execute { |user| user.remove_avatar! } + Users::UpdateService.new(@user).execute { |user| user.remove_avatar! } redirect_to profile_path, status: 302 end diff --git a/app/controllers/profiles/notifications_controller.rb b/app/controllers/profiles/notifications_controller.rb index 45d7bca27bb..960b7512602 100644 --- a/app/controllers/profiles/notifications_controller.rb +++ b/app/controllers/profiles/notifications_controller.rb @@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController end def update - result = Users::UpdateService.new(current_user, current_user, user_params).execute + result = Users::UpdateService.new(current_user, user_params).execute if result[:status] == :success flash[:notice] = "Notification settings saved" diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 2c4c2490735..10145bae0d3 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_automatically_set: false } - result = Users::UpdateService.new(current_user, @user, password_attributes).execute + result = Users::UpdateService.new(@user, password_attributes).execute if result[:status] == :success - Users::UpdateService.new(current_user, @user, password_expires_at: nil).execute + Users::UpdateService.new(@user, password_expires_at: nil).execute redirect_to root_path, notice: 'Password successfully changed' else @@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController return end - result = Users::UpdateService.new(current_user, @user, password_attributes).execute + result = Users::UpdateService.new(@user, password_attributes).execute if result[:status] == :success flash[:notice] = "Password was successfully updated. Please login with it" diff --git a/app/controllers/profiles/preferences_controller.rb b/app/controllers/profiles/preferences_controller.rb index 6845256e9d8..1e557c47638 100644 --- a/app/controllers/profiles/preferences_controller.rb +++ b/app/controllers/profiles/preferences_controller.rb @@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController def update begin - result = Users::UpdateService.new(current_user, user, preferences_params).execute + result = Users::UpdateService.new(user, preferences_params).execute if result[:status] == :success flash[:notice] = 'Preferences saved.' diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb index b590846257b..1a4f77639e7 100644 --- a/app/controllers/profiles/two_factor_auths_controller.rb +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController current_user.otp_grace_period_started_at = Time.current end - Users::UpdateService.new(current_user, current_user).execute! + Users::UpdateService.new(current_user).execute! if two_factor_authentication_required? && !current_user.two_factor_enabled? two_factor_authentication_reason( @@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController def create if current_user.validate_and_consume_otp!(params[:pin_code]) - Users::UpdateService.new(current_user, current_user, otp_required_for_login: true).execute! do |user| + Users::UpdateService.new(current_user, otp_required_for_login: true).execute! do |user| @codes = user.generate_otp_backup_codes! end @@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController end def codes - Users::UpdateService.new(current_user, current_user).execute! do |user| + Users::UpdateService.new(current_user).execute! do |user| @codes = user.generate_otp_backup_codes! end end diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 9f596c3dc9c..e4985fdb2eb 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -12,7 +12,7 @@ class ProfilesController < Profiles::ApplicationController user_params.except!(:email) if @user.external_email? respond_to do |format| - result = Users::UpdateService.new(current_user, @user, user_params).execute + result = Users::UpdateService.new(@user, user_params).execute if result[:status] == :success message = "Profile was successfully updated" @@ -27,7 +27,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_private_token - Users::UpdateService.new(current_user, @user).execute!(skip_authorization: true) do |user| + Users::UpdateService.new(@user).execute!(skip_authorization: true) do |user| user.reset_authentication_token! end @@ -37,7 +37,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_incoming_email_token - Users::UpdateService.new(current_user, @user).execute!(skip_authorization: true) do |user| + Users::UpdateService.new(@user).execute!(skip_authorization: true) do |user| user.reset_incoming_email_token! end @@ -47,7 +47,7 @@ class ProfilesController < Profiles::ApplicationController end def reset_rss_token - Users::UpdateService.new(current_user, @user).execute!(skip_authorization: true) do |user| + Users::UpdateService.new(@user).execute!(skip_authorization: true) do |user| user.reset_rss_token! end @@ -63,7 +63,7 @@ class ProfilesController < Profiles::ApplicationController end def update_username - result = Users::UpdateService.new(current_user, @user, username: user_params[:username]).execute + result = Users::UpdateService.new(@user, username: user_params[:username]).execute options = if result[:status] == :success { notice: "Username successfully changed" } diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index cc9038f7607..f39441a281e 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -60,7 +60,7 @@ class SessionsController < Devise::SessionsController return unless user && user.require_password? - Users::UpdateService.new(user, user).execute do |user| + Users::UpdateService.new(user).execute do |user| @token = user.generate_reset_token end diff --git a/app/models/user.rb b/app/models/user.rb index d53837fbdb2..e95334e2542 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -53,7 +53,7 @@ class User < ActiveRecord::Base lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) return unless lease.try_obtain - Users::UpdateService.new(self, self).execute(validate: false) + Users::UpdateService.new(self).execute(validate: false) end attr_accessor :force_random_password @@ -963,7 +963,7 @@ class User < ActiveRecord::Base if attempts_exceeded? lock_access! unless access_locked? else - Users::UpdateService.new(self, self).execute(validate: false) + Users::UpdateService.new(self).execute(validate: false) end end @@ -1122,7 +1122,7 @@ class User < ActiveRecord::Base &creation_block ) - Users::UpdateService.new(user, user).execute(validate: false) + Users::UpdateService.new(user).execute(validate: false) user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb index 94e4167d88b..2032f0dc3a9 100644 --- a/app/services/emails/destroy_service.rb +++ b/app/services/emails/destroy_service.rb @@ -1,13 +1,13 @@ module Emails class DestroyService < ::Emails::BaseService def execute - Email.find_by_email(@email).destroy && update_secondary_emails! + Email.find_by_email!(@email).destroy && update_secondary_emails! end private def update_secondary_emails! - result = ::Users::UpdateService.new(@current_user, @current_user).execute do |user| + result = ::Users::UpdateService.new(@current_user).execute do |user| user.update_secondary_emails! end diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb index 36dcc69f8cf..2037664f56a 100644 --- a/app/services/users/update_service.rb +++ b/app/services/users/update_service.rb @@ -1,14 +1,13 @@ module Users # Service for updating a user. class UpdateService < BaseService - def initialize(current_user, user, params = {}) - @current_user = current_user + def initialize(user, params = {}) @user = user @params = params.dup end - def execute(skip_authorization: false, validate: true, &block) - assign_attributes(skip_authorization, &block) + def execute(validate: true, &block) + assign_attributes(&block) if @user.save(validate: validate) success @@ -20,23 +19,17 @@ module Users def execute!(*args, &block) result = execute(*args, &block) - raise ActiveRecord::RecordInvalid(result[:message]) unless result[:status] == :success + raise ActiveRecord::RecordInvalid.new(@user) unless result[:status] == :success true end private - def assign_attributes(skip_authorization, &block) - raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_update_user? - + def assign_attributes(&block) yield(@user) if block_given? @user.assign_attributes(params) if params.any? end - - def can_update_user? - current_user == @user || current_user&.admin? - end end end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index ecfc822ba6a..9b035808693 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -132,7 +132,7 @@ module API return { success: false, message: 'Two-factor authentication is not enabled for this user' } end - ::Users::UpdateService.new(user, user).execute! do |user| + ::Users::UpdateService.new(user).execute! do |user| @codes = user.generate_otp_backup_codes! end diff --git a/lib/api/notification_settings.rb b/lib/api/notification_settings.rb index 5f88488ccee..5d113c94b22 100644 --- a/lib/api/notification_settings.rb +++ b/lib/api/notification_settings.rb @@ -35,7 +35,7 @@ module API new_notification_email = params.delete(:notification_email) if new_notification_email - ::Users::UpdateService.new(current_user, current_user, notification_email: new_notification_email).execute + ::Users::UpdateService.new(current_user, notification_email: new_notification_email).execute end notification_setting.update(declared_params(include_missing: false)) diff --git a/lib/api/users.rb b/lib/api/users.rb index 6d7f2e7e250..175db3a4a18 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -156,7 +156,7 @@ module API user_params[:password_expires_at] = Time.now if user_params[:password].present? - result = ::Users::UpdateService.new(current_user, user, user_params.except(:extern_uid, :provider)).execute + result = ::Users::UpdateService.new(user, user_params.except(:extern_uid, :provider)).execute if result[:status] == :success present user, with: Entities::UserPublic diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 998a8a7eb35..8779577258b 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -16,7 +16,7 @@ module Gitlab def self.allowed?(user) self.open(user) do |access| if access.allowed? - Users::UpdateService.new(user, user, last_credential_check_a: Time.now).execute + Users::UpdateService.new(user, last_credential_check_a: Time.now).execute true else diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index 8f37f96dcab..b3f453e506d 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -32,7 +32,7 @@ module Gitlab block_after_save = needs_blocking? - Users::UpdateService.new(gl_user, gl_user).execute! + Users::UpdateService.new(gl_user).execute! gl_user.block if block_after_save diff --git a/spec/services/users/update_service_spec.rb b/spec/services/users/update_service_spec.rb index fd32a4f0f33..6c33a232cb0 100644 --- a/spec/services/users/update_service_spec.rb +++ b/spec/services/users/update_service_spec.rb @@ -2,61 +2,42 @@ require 'spec_helper' describe Users::UpdateService, services: true do let(:user) { create(:user) } - let(:admin) { create(:admin) } describe '#execute' do it 'updates the name' do - result = update_user(user, user, name: 'New Name') + result = update_user(user, name: 'New Name') expect(result).to eq({ status: :success }) expect(user.name).to eq('New Name') end - context 'when updated by an admin' do - it 'updates the name' do - result = update_user(admin, user, name: 'New Name') - - expect(result).to eq({ status: :success }) - expect(user.name).to eq('New Name') - end - end - it 'returns an error result when record cannot be updated' do expect do - update_user(user, create(:user), { name: 'New Name' }) - end.to raise_error Gitlab::Access::AccessDeniedError + update_user(user, { email: 'invalid' }) + end.not_to change { user.reload.email } end - def update_user(current_user, user, opts) - described_class.new(current_user, user, opts).execute + def update_user(user, opts) + described_class.new(user, opts).execute end end describe '#execute!' do it 'updates the name' do - result = update_user(user, user, name: 'New Name') + result = update_user(user, name: 'New Name') expect(result).to be true expect(user.name).to eq('New Name') end - context 'when updated by an admin' do - it 'updates the name' do - result = update_user(admin, user, name: 'New Name') - - expect(result).to be true - expect(user.name).to eq('New Name') - end - end - it 'returns an error result when record cannot be updated' do expect do - update_user(user, create(:user), { name: 'New Name' }) - end.to raise_error Gitlab::Access::AccessDeniedError + update_user(user, { email: 'invalid' }) + end.to raise_error(ActiveRecord::RecordInvalid) end - def update_user(current_user, user, opts) - described_class.new(current_user, user, opts).execute! + def update_user(user, opts) + described_class.new(user, opts).execute! end end end |