refactor update user service not to do auth checks

11 files changed, 27 insertions, 34 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