diff options
author | James Lopez <james@jameslopez.es> | 2017-06-22 12:27:37 +0300 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2017-06-23 12:41:42 +0300 |
commit | c9fd3dc42c462ce2551f6a9630035b4df00bc366 (patch) | |
tree | 5e4bd5226fff9fa00ba9db17056d21a59db527d7 | |
parent | 785cbb79e255c8369ca5eb916207304f39d188ad (diff) |
more refactoring based on feedback
-rw-r--r-- | app/controllers/admin/users_controller.rb | 4 | ||||
-rw-r--r-- | app/controllers/profiles/avatars_controller.rb | 5 | ||||
-rw-r--r-- | app/controllers/profiles/two_factor_auths_controller.rb | 12 | ||||
-rw-r--r-- | app/controllers/sessions_controller.rb | 7 | ||||
-rw-r--r-- | app/services/emails/base_service.rb | 6 | ||||
-rw-r--r-- | app/services/emails/create_service.rb | 4 | ||||
-rw-r--r-- | app/services/emails/destroy_service.rb | 4 | ||||
-rw-r--r-- | app/services/users/update_service.rb | 6 | ||||
-rw-r--r-- | lib/api/internal.rb | 7 | ||||
-rw-r--r-- | lib/gitlab/ldap/access.rb | 3 | ||||
-rw-r--r-- | spec/requests/api/users_spec.rb | 13 |
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 |