diff options
| -rw-r--r-- | app/controllers/admin/users_controller.rb | 6 | ||||
| -rw-r--r-- | app/controllers/concerns/impersonation.rb | 6 | ||||
| -rw-r--r-- | app/controllers/profiles/passwords_controller.rb | 8 | ||||
| -rw-r--r-- | locale/gitlab.pot | 3 | ||||
| -rw-r--r-- | spec/controllers/admin/users_controller_spec.rb | 15 | ||||
| -rw-r--r-- | spec/features/profiles/password_spec.rb | 78 |
6 files changed, 22 insertions, 94 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index cdfb3a32f4c..dfc1434d909 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -45,7 +45,7 @@ class Admin::UsersController < Admin::ApplicationController end def impersonate - if can?(user, :log_in) && !impersonation_in_progress? + if can?(user, :log_in) session[:impersonator_id] = current_user.id warden.set_user(user, scope: :user) @@ -58,9 +58,7 @@ class Admin::UsersController < Admin::ApplicationController redirect_to root_path else flash[:alert] = - if impersonation_in_progress? - _("You are already impersonating another user") - elsif user.blocked? + if user.blocked? _("You cannot impersonate a blocked user") elsif user.internal? _("You cannot impersonate an internal user") diff --git a/app/controllers/concerns/impersonation.rb b/app/controllers/concerns/impersonation.rb index 539dd9ad69d..a8788e7f8bd 100644 --- a/app/controllers/concerns/impersonation.rb +++ b/app/controllers/concerns/impersonation.rb @@ -20,7 +20,7 @@ module Impersonation protected def check_impersonation_availability - return unless impersonation_in_progress? + return unless session[:impersonator_id] unless Gitlab.config.gitlab.impersonation_enabled stop_impersonation @@ -38,10 +38,6 @@ module Impersonation current_user end - def impersonation_in_progress? - session[:impersonator_id].present? - end - def log_impersonation_event Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}") end diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index c8c2dd1c7d6..85e901eb3eb 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -47,8 +47,6 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_attributes[:password_automatically_set] = false unless @user.password_automatically_set || @user.valid_password?(user_params[:current_password]) - handle_invalid_current_password_attempt! - redirect_to edit_profile_password_path, alert: _('You must provide a valid current password') return end @@ -87,12 +85,6 @@ class Profiles::PasswordsController < Profiles::ApplicationController render_404 unless @user.allow_password_authentication? end - def handle_invalid_current_password_attempt! - Gitlab::AppLogger.info(message: 'Invalid current password when attempting to update user password', username: @user.username, ip: request.remote_ip) - - @user.increment_failed_attempts! - end - def user_params params.require(:user).permit(:current_password, :password, :password_confirmation) end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 03a26c01cd5..6c0b33911ca 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38380,9 +38380,6 @@ msgstr "" msgid "You are already a member of this %{member_source}." msgstr "" -msgid "You are already impersonating another user" -msgstr "" - msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 3a2b5dcb99d..4d2c311c9a4 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -815,20 +815,5 @@ RSpec.describe Admin::UsersController do expect(response).to have_gitlab_http_status(:not_found) end end - - context 'when impersonating an admin and attempting to impersonate again' do - let(:admin2) { create(:admin) } - - before do - post :impersonate, params: { id: admin2.username } - end - - it 'does not allow double impersonation', :aggregate_failures do - post :impersonate, params: { id: user.username } - - expect(flash[:alert]).to eq(_('You are already impersonating another user')) - expect(warden.user).to eq(admin2) - end - end end end diff --git a/spec/features/profiles/password_spec.rb b/spec/features/profiles/password_spec.rb index 893dd2c76e0..c9059395377 100644 --- a/spec/features/profiles/password_spec.rb +++ b/spec/features/profiles/password_spec.rb @@ -78,80 +78,40 @@ RSpec.describe 'Profile > Password' do end end - context 'Change password' do - let(:new_password) { '22233344' } - + context 'Change passowrd' do before do sign_in(user) visit(edit_profile_password_path) end - shared_examples 'user enters an incorrect current password' do - subject do - page.within '.update-password' do - fill_in 'user_current_password', with: user_current_password - fill_passwords(new_password, new_password) - end + it 'does not change user passowrd without old one' do + page.within '.update-password' do + fill_passwords('22233344', '22233344') end - it 'handles the invalid password attempt, and prompts the user to try again', :aggregate_failures do - expect(Gitlab::AppLogger).to receive(:info) - .with(message: 'Invalid current password when attempting to update user password', username: user.username, ip: user.current_sign_in_ip) - - subject - - user.reload - - expect(user.failed_attempts).to eq(1) - expect(user.valid_password?(new_password)).to eq(false) - expect(current_path).to eq(edit_profile_password_path) - - page.within '.flash-container' do - expect(page).to have_content('You must provide a valid current password') - end + page.within '.flash-container' do + expect(page).to have_content 'You must provide a valid current password' end - - it 'locks the user account when user passes the maximum attempts threshold', :aggregate_failures do - user.update!(failed_attempts: User.maximum_attempts.pred) - - subject - - expect(current_path).to eq(new_user_session_path) - - page.within '.flash-container' do - expect(page).to have_content('Your account is locked.') - end - end - end - - context 'when current password is blank' do - let(:user_current_password) { nil } - - it_behaves_like 'user enters an incorrect current password' end - context 'when current password is incorrect' do - let(:user_current_password) {'invalid' } + it 'does not change password with invalid old password' do + page.within '.update-password' do + fill_in 'user_current_password', with: 'invalid' + fill_passwords('password', 'confirmation') + end - it_behaves_like 'user enters an incorrect current password' + page.within '.flash-container' do + expect(page).to have_content 'You must provide a valid current password' + end end - context 'when the password reset is successful' do - subject do - page.within '.update-password' do - fill_in "user_current_password", with: user.password - fill_passwords(new_password, new_password) - end + it 'changes user password' do + page.within '.update-password' do + fill_in "user_current_password", with: user.password + fill_passwords('22233344', '22233344') end - it 'changes the password, logs the user out and prompts them to sign in again', :aggregate_failures do - expect { subject }.to change { user.reload.valid_password?(new_password) }.to(true) - expect(current_path).to eq new_user_session_path - - page.within '.flash-container' do - expect(page).to have_content('Password was successfully updated. Please sign in again.') - end - end + expect(current_path).to eq new_user_session_path end end |