more refactoring based on feedback

11 files changed, 39 insertions, 32 deletions

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 065ebf81793..73fa21d3b3c 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -152,10 +152,10 @@ class Admin::UsersController < Admin::ApplicationController
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    Emails::DestroyService.new(current_user, user, email: email.email).execute
+    success = Emails::DestroyService.new(current_user, user, email: email.email).execute
 
     respond_to do |format|
-      if result[:status] == :success
+      if success
         format.html { redirect_back_or_admin_user(notice: "Successfully removed email.") }
         format.json { head :ok }
       else
diff --git a/app/controllers/profiles/avatars_controller.rb b/app/controllers/profiles/avatars_controller.rb
index 538604f8866..cab9ea24270 100644
--- a/app/controllers/profiles/avatars_controller.rb
+++ b/app/controllers/profiles/avatars_controller.rb
@@ -1,9 +1,10 @@
 class Profiles::AvatarsController < Profiles::ApplicationController
   def destroy
     @user = current_user
-    @user.remove_avatar!
 
-    Users::UpdateService.new(@user, @user).execute
+    Users::UpdateService.new(@user, @user).execute do |user|
+      user.remove_avatar!
+    end
 
     redirect_to profile_path, status: 302
   end
diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb
index a8b7e756ad1..95e05dff80a 100644
--- a/app/controllers/profiles/two_factor_auths_controller.rb
+++ b/app/controllers/profiles/two_factor_auths_controller.rb
@@ -41,9 +41,10 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
-      current_user.otp_required_for_login = true
-      @codes = current_user.generate_otp_backup_codes!
-      Users::UpdateService.new(current_user, current_user).execute!
+      Users::UpdateService.new(current_user, current_user).execute! do |user|
+        user.otp_required_for_login = true
+        @codes = user.generate_otp_backup_codes!
+      end
 
       render 'create'
     else
@@ -70,8 +71,9 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
   end
 
   def codes
-    @codes = current_user.generate_otp_backup_codes!
-    Users::UpdateService.new(current_user, current_user).execute!
+    Users::UpdateService.new(current_user, current_user).execute! do |user|
+      @codes = user.generate_otp_backup_codes!
+    end
   end
 
   def destroy
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index f7d9d3c80c8..cc9038f7607 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -60,10 +60,11 @@ class SessionsController < Devise::SessionsController
 
     return unless user && user.require_password?
 
-    token = user.generate_reset_token
-    Users::UpdateService.new(user, user).execute
+    Users::UpdateService.new(user, user).execute do |user|
+      @token = user.generate_reset_token
+    end
 
-    redirect_to edit_user_password_path(reset_password_token: token),
+    redirect_to edit_user_password_path(reset_password_token: @token),
       notice: "Please create a password for your new account."
   end
 
diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index 0f8617c08bb..8810f6d8803 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -5,11 +5,5 @@ module Emails
       @user = user
       @email = opts[:email]
     end
-
-    private
-
-    def can_manage_emails?
-      @current_user == @user || @current_user.admin?
-    end
   end
 end
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index ea65b82e418..b6491ee9804 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,8 +1,6 @@
 module Emails
   class CreateService < ::Emails::BaseService
-    def execute(skip_authorization: false)
-      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_manage_emails?
-
+    def execute
       @user.emails.create(email: @email)
     end
   end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index 8150918986c..94e4167d88b 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,8 +1,6 @@
 module Emails
   class DestroyService < ::Emails::BaseService
-    def execute(skip_authorization: false)
-      raise Gitlab::Access::AccessDeniedError unless skip_authorization || can_manage_emails?
-
+    def execute
       Email.find_by_email(@email).destroy && update_secondary_emails!
     end
 
diff --git a/app/services/users/update_service.rb b/app/services/users/update_service.rb
index a1db1dbc583..56e8739ab5e 100644
--- a/app/services/users/update_service.rb
+++ b/app/services/users/update_service.rb
@@ -10,7 +10,7 @@ module Users
     def execute(skip_authorization: false, validate: true, &block)
       assign_attributes(skip_authorization, &block)
 
-      if @user.save(validate: validate) || !@user.changed? && @user.errors.empty?
+      if @user.save(validate: validate) || @user.errors.empty?
         success
       else
         error(@user.errors.full_messages.uniq.join('. '))
@@ -18,9 +18,9 @@ module Users
     end
 
     def execute!(skip_authorization: false, &block)
-      assign_attributes(skip_authorization, &block)
+      result = execute(*args, &block)
 
-      @user.save! if @user.changed?
+      raise SomeCustomException(result[:message]) unless result[:status] == :success
     end
 
     private
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 8606ff75e11..ecfc822ba6a 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -132,10 +132,11 @@ module API
           return { success: false, message: 'Two-factor authentication is not enabled for this user' }
         end
 
-        codes = user.generate_otp_backup_codes!
-        ::Users::UpdateService.new(user, user).execute!
+        ::Users::UpdateService.new(user, user).execute! do |user|
+          @codes = user.generate_otp_backup_codes!
+        end
 
-        { success: true, recovery_codes: codes }
+        { success: true, recovery_codes: @codes }
       end
 
       post "/notify_post_receive" do
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 7430db828a3..998a8a7eb35 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -16,8 +16,7 @@ module Gitlab
       def self.allowed?(user)
         self.open(user) do |access|
           if access.allowed?
-            user.last_credential_check_at = Time.now
-            Users::UpdateService.new(user, user).execute
+            Users::UpdateService.new(user, user, last_credential_check_a: Time.now).execute
 
             true
           else
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index efb3dc69ea8..c0174b304c8 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -364,6 +364,7 @@ describe API::Users do
 
     it "updates user with new bio" do
       put api("/users/#{user.id}", admin), { bio: 'new test bio' }
+
       expect(response).to have_http_status(200)
       expect(json_response['bio']).to eq('new test bio')
       expect(user.reload.bio).to eq('new test bio')
@@ -396,6 +397,7 @@ describe API::Users do
 
     it 'updates user with his own email' do
       put api("/users/#{user.id}", admin), email: user.email
+
       expect(response).to have_http_status(200)
       expect(json_response['email']).to eq(user.email)
       expect(user.reload.email).to eq(user.email)
@@ -403,12 +405,14 @@ describe API::Users do
 
     it 'updates user with a new email' do
       put api("/users/#{user.id}", admin), email: 'new@email.com'
+
       expect(response).to have_http_status(200)
       expect(user.reload.notification_email).to eq('new@email.com')
     end
 
     it 'updates user with his own username' do
       put api("/users/#{user.id}", admin), username: user.username
+
       expect(response).to have_http_status(200)
       expect(json_response['username']).to eq(user.username)
       expect(user.reload.username).to eq(user.username)
@@ -416,12 +420,14 @@ describe API::Users do
 
     it "updates user's existing identity" do
       put api("/users/#{omniauth_user.id}", admin), provider: 'ldapmain', extern_uid: '654321'
+
       expect(response).to have_http_status(200)
       expect(omniauth_user.reload.identities.first.extern_uid).to eq('654321')
     end
 
     it 'updates user with new identity' do
       put api("/users/#{user.id}", admin), provider: 'github', extern_uid: 'john'
+
       expect(response).to have_http_status(200)
       expect(user.reload.identities.first.extern_uid).to eq('john')
       expect(user.reload.identities.first.provider).to eq('github')
@@ -429,12 +435,14 @@ describe API::Users do
 
     it "updates admin status" do
       put api("/users/#{user.id}", admin), { admin: true }
+
       expect(response).to have_http_status(200)
       expect(user.reload.admin).to eq(true)
     end
 
     it "updates external status" do
       put api("/users/#{user.id}", admin), { external: true }
+
       expect(response.status).to eq 200
       expect(json_response['external']).to eq(true)
       expect(user.reload.external?).to be_truthy
@@ -442,6 +450,7 @@ describe API::Users do
 
     it "does not update admin status" do
       put api("/users/#{admin_user.id}", admin), { can_create_group: false }
+
       expect(response).to have_http_status(200)
       expect(admin_user.reload.admin).to eq(true)
       expect(admin_user.can_create_group).to eq(false)
@@ -449,6 +458,7 @@ describe API::Users do
 
     it "does not allow invalid update" do
       put api("/users/#{user.id}", admin), { email: 'invalid email' }
+
       expect(response).to have_http_status(400)
       expect(user.reload.email).not_to eq('invalid email')
     end
@@ -465,6 +475,7 @@ describe API::Users do
 
     it "returns 404 for non-existing user" do
       put api("/users/999999", admin), { bio: 'update should fail' }
+
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
     end
@@ -515,6 +526,7 @@ describe API::Users do
 
       it 'returns 409 conflict error if email address exists' do
         put api("/users/#{@user.id}", admin), email: 'test@example.com'
+
         expect(response).to have_http_status(409)
         expect(@user.reload.email).to eq(@user.email)
       end
@@ -522,6 +534,7 @@ describe API::Users do
       it 'returns 409 conflict error if username taken' do
         @user_id = User.all.last.id
         put api("/users/#{@user.id}", admin), username: 'test'
+
         expect(response).to have_http_status(409)
         expect(@user.reload.username).to eq(@user.username)
       end