Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee

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