Commit b804db26 authored by James Lopez's avatar James Lopez

refactor update user service not to do auth checks

parent e2e0b175
...@@ -125,7 +125,7 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -125,7 +125,7 @@ class Admin::UsersController < Admin::ApplicationController
end end
respond_to do |format| 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! user.skip_reconfirmation!
end end
...@@ -211,7 +211,7 @@ class Admin::UsersController < Admin::ApplicationController ...@@ -211,7 +211,7 @@ class Admin::UsersController < Admin::ApplicationController
end end
def update_user def update_user
result = Users::UpdateService.new(current_user, user).execute do |user| result = Users::UpdateService.new(user).execute do |user|
yield(user) yield(user)
end end
......
...@@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController ...@@ -2,7 +2,7 @@ class Profiles::AvatarsController < Profiles::ApplicationController
def destroy def destroy
@user = current_user @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 redirect_to profile_path, status: 302
end end
......
...@@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController ...@@ -7,7 +7,7 @@ class Profiles::NotificationsController < Profiles::ApplicationController
end end
def update 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 if result[:status] == :success
flash[:notice] = "Notification settings saved" flash[:notice] = "Notification settings saved"
......
...@@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController ...@@ -21,10 +21,10 @@ class Profiles::PasswordsController < Profiles::ApplicationController
password_automatically_set: false 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 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' redirect_to root_path, notice: 'Password successfully changed'
else else
...@@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController ...@@ -46,7 +46,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController
return return
end end
result = Users::UpdateService.new(current_user, @user, password_attributes).execute result = Users::UpdateService.new(@user, password_attributes).execute
if result[:status] == :success if result[:status] == :success
flash[:notice] = "Password was successfully updated. Please login with it" flash[:notice] = "Password was successfully updated. Please login with it"
......
...@@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController ...@@ -6,7 +6,7 @@ class Profiles::PreferencesController < Profiles::ApplicationController
def update def update
begin begin
result = Users::UpdateService.new(current_user, user, preferences_params).execute result = Users::UpdateService.new(user, preferences_params).execute
if result[:status] == :success if result[:status] == :success
flash[:notice] = 'Preferences saved.' flash[:notice] = 'Preferences saved.'
......
...@@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController ...@@ -10,7 +10,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
current_user.otp_grace_period_started_at = Time.current current_user.otp_grace_period_started_at = Time.current
end 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? if two_factor_authentication_required? && !current_user.two_factor_enabled?
two_factor_authentication_reason( two_factor_authentication_reason(
...@@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController ...@@ -41,7 +41,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
def create def create
if current_user.validate_and_consume_otp!(params[:pin_code]) 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! @codes = user.generate_otp_backup_codes!
end end
...@@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController ...@@ -70,7 +70,7 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
end end
def codes 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! @codes = user.generate_otp_backup_codes!
end end
end end
......
...@@ -12,7 +12,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -12,7 +12,7 @@ class ProfilesController < Profiles::ApplicationController
user_params.except!(:email) if @user.external_email? user_params.except!(:email) if @user.external_email?
respond_to do |format| 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 if result[:status] == :success
message = "Profile was successfully updated" message = "Profile was successfully updated"
...@@ -27,7 +27,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -27,7 +27,7 @@ class ProfilesController < Profiles::ApplicationController
end end
def reset_private_token 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! user.reset_authentication_token!
end end
...@@ -37,7 +37,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -37,7 +37,7 @@ class ProfilesController < Profiles::ApplicationController
end end
def reset_incoming_email_token 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! user.reset_incoming_email_token!
end end
...@@ -47,7 +47,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -47,7 +47,7 @@ class ProfilesController < Profiles::ApplicationController
end end
def reset_rss_token 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! user.reset_rss_token!
end end
...@@ -63,7 +63,7 @@ class ProfilesController < Profiles::ApplicationController ...@@ -63,7 +63,7 @@ class ProfilesController < Profiles::ApplicationController
end end
def update_username 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 options = if result[:status] == :success
{ notice: "Username successfully changed" } { notice: "Username successfully changed" }
......
...@@ -60,7 +60,7 @@ class SessionsController < Devise::SessionsController ...@@ -60,7 +60,7 @@ class SessionsController < Devise::SessionsController
return unless user && user.require_password? 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 @token = user.generate_reset_token
end end
......
...@@ -53,7 +53,7 @@ class User < ActiveRecord::Base ...@@ -53,7 +53,7 @@ class User < ActiveRecord::Base
lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i) lease = Gitlab::ExclusiveLease.new("user_update_tracked_fields:#{id}", timeout: 1.hour.to_i)
return unless lease.try_obtain return unless lease.try_obtain
Users::UpdateService.new(self, self).execute(validate: false) Users::UpdateService.new(self).execute(validate: false)
end end
attr_accessor :force_random_password attr_accessor :force_random_password
...@@ -963,7 +963,7 @@ class User < ActiveRecord::Base ...@@ -963,7 +963,7 @@ class User < ActiveRecord::Base
if attempts_exceeded? if attempts_exceeded?
lock_access! unless access_locked? lock_access! unless access_locked?
else else
Users::UpdateService.new(self, self).execute(validate: false) Users::UpdateService.new(self).execute(validate: false)
end end
end end
...@@ -1122,7 +1122,7 @@ class User < ActiveRecord::Base ...@@ -1122,7 +1122,7 @@ class User < ActiveRecord::Base
&creation_block &creation_block
) )
Users::UpdateService.new(user, user).execute(validate: false) Users::UpdateService.new(user).execute(validate: false)
user user
ensure ensure
Gitlab::ExclusiveLease.cancel(lease_key, uuid) Gitlab::ExclusiveLease.cancel(lease_key, uuid)
......
module Emails module Emails
class DestroyService < ::Emails::BaseService class DestroyService < ::Emails::BaseService
def execute def execute
Email.find_by_email(@email).destroy && update_secondary_emails! Email.find_by_email!(@email).destroy && update_secondary_emails!
end end
private private
def update_secondary_emails! 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! user.update_secondary_emails!
end end
......
module Users module Users
# Service for updating a user. # Service for updating a user.
class UpdateService < BaseService class UpdateService < BaseService
def initialize(current_user, user, params = {}) def initialize(user, params = {})
@current_user = current_user
@user = user @user = user
@params = params.dup @params = params.dup
end end
def execute(skip_authorization: false, validate: true, &block) def execute(validate: true, &block)
assign_attributes(skip_authorization, &block) assign_attributes(&block)
if @user.save(validate: validate) if @user.save(validate: validate)
success success
...@@ -20,23 +19,17 @@ module Users ...@@ -20,23 +19,17 @@ module Users
def execute!(*args, &block) def execute!(*args, &block)
result = 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 true
end end
private private
def assign_attributes(skip_authorization, &block) def assign_attributes(&block)
raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_update_user?
yield(@user) if block_given? yield(@user) if block_given?
@user.assign_attributes(params) if params.any? @user.assign_attributes(params) if params.any?
end end
def can_update_user?
current_user == @user || current_user&.admin?
end
end end
end end
...@@ -132,7 +132,7 @@ module API ...@@ -132,7 +132,7 @@ module API
return { success: false, message: 'Two-factor authentication is not enabled for this user' } return { success: false, message: 'Two-factor authentication is not enabled for this user' }
end end
::Users::UpdateService.new(user, user).execute! do |user| ::Users::UpdateService.new(user).execute! do |user|
@codes = user.generate_otp_backup_codes! @codes = user.generate_otp_backup_codes!
end end
......
...@@ -35,7 +35,7 @@ module API ...@@ -35,7 +35,7 @@ module API
new_notification_email = params.delete(:notification_email) new_notification_email = params.delete(:notification_email)
if new_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 end
notification_setting.update(declared_params(include_missing: false)) notification_setting.update(declared_params(include_missing: false))
......
...@@ -156,7 +156,7 @@ module API ...@@ -156,7 +156,7 @@ module API
user_params[:password_expires_at] = Time.now if user_params[:password].present? 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 if result[:status] == :success
present user, with: Entities::UserPublic present user, with: Entities::UserPublic
......
...@@ -16,7 +16,7 @@ module Gitlab ...@@ -16,7 +16,7 @@ module Gitlab
def self.allowed?(user) def self.allowed?(user)
self.open(user) do |access| self.open(user) do |access|
if access.allowed? 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 true
else else
......
...@@ -32,7 +32,7 @@ module Gitlab ...@@ -32,7 +32,7 @@ module Gitlab
block_after_save = needs_blocking? 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 gl_user.block if block_after_save
......
...@@ -2,61 +2,42 @@ require 'spec_helper' ...@@ -2,61 +2,42 @@ require 'spec_helper'
describe Users::UpdateService, services: true do describe Users::UpdateService, services: true do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:admin) { create(:admin) }
describe '#execute' do describe '#execute' do
it 'updates the name' 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(result).to eq({ status: :success })
expect(user.name).to eq('New Name') expect(user.name).to eq('New Name')
end 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 it 'returns an error result when record cannot be updated' do
expect do expect do
update_user(user, create(:user), { name: 'New Name' }) update_user(user, { email: 'invalid' })
end.to raise_error Gitlab::Access::AccessDeniedError end.not_to change { user.reload.email }
end end
def update_user(current_user, user, opts) def update_user(user, opts)
described_class.new(current_user, user, opts).execute described_class.new(user, opts).execute
end end
end end
describe '#execute!' do describe '#execute!' do
it 'updates the name' 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(result).to be true
expect(user.name).to eq('New Name') expect(user.name).to eq('New Name')
end 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 it 'returns an error result when record cannot be updated' do
expect do expect do
update_user(user, create(:user), { name: 'New Name' }) update_user(user, { email: 'invalid' })
end.to raise_error Gitlab::Access::AccessDeniedError end.to raise_error(ActiveRecord::RecordInvalid)
end end
def update_user(current_user, user, opts) def update_user(user, opts)
described_class.new(current_user, user, opts).execute! described_class.new(user, opts).execute!
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment