refactor update user service not to do auth checks

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