Remove all sessions but current while enabling 2FA

Remove all sessions but current while enabling 2FA

Add changelog entry

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

Add cr remarks

app/controllers/profiles/two_factor_auths_controller.rb

@@ -38,6 +38,8 @@ class Profiles::TwoFactorAuthsController < Profiles::ApplicationController
 
   def create
     if current_user.validate_and_consume_otp!(params[:pin_code])
+      ActiveSession.destroy_all_but_current(current_user, session)
+
       Users::UpdateService.new(current_user, user: current_user, otp_required_for_login: true).execute! do |user|
         @codes = user.generate_otp_backup_codes!
       end

app/models/active_session.rb

@@ -105,6 +105,19 @@ class ActiveSession
     end
   end
 
+  def self.destroy_all_but_current(user, current_session)
+    session_ids = not_impersonated(user)
+    session_ids.reject! { |session| session.current?(current_session) } if current_session
+
+    Gitlab::Redis::SharedState.with do |redis|
+      destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any?
+    end
+  end
+
+  def self.not_impersonated(user)
+    list(user).reject(&:is_impersonated)
+  end
+
   def self.key_name(user_id, session_id = '*')
     "#{Gitlab::Redis::SharedState::USER_SESSIONS_NAMESPACE}:#{user_id}:#{session_id}"
   end

changelogs/unreleased/security-213-delete-other-sessions-when-activating-2fa.yml

+---
+title: Remove all sessions but current while enabling 2FA
+merge_request:
+author:
+type: security

spec/controllers/profiles/two_factor_auths_controller_spec.rb

@@ -57,6 +57,12 @@ RSpec.describe Profiles::TwoFactorAuthsController do
         expect(assigns[:codes]).to match_array %w(a b c)
       end
 
+      it 'calls to delete other sessions' do
+        expect(ActiveSession).to receive(:destroy_all_but_current)
+
+        go
+      end
+
       it 'renders create' do
         go
         expect(response).to render_template(:create)

spec/models/active_session_spec.rb

@@ -296,6 +296,59 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
+  describe '.destroy_all_but_current' do
+    it 'gracefully handles a nil session ID' do
+      expect(described_class).not_to receive(:destroy_sessions)
+
+      ActiveSession.destroy_all_but_current(user, nil)
+    end
+
+    context 'with user sessions' do
+      let(:current_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' }
+
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set(described_class.key_name(user.id, current_session_id),
+            Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(current_session_id))))
+          redis.set(described_class.key_name(user.id, '59822c7d9fcdfa03725eff41782ad97d'),
+            Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('59822c7d9fcdfa03725eff41782ad97d'))))
+          redis.set(described_class.key_name(9999, '5c8611e4f9c69645ad1a1492f4131358'),
+            Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new('5c8611e4f9c69645ad1a1492f4131358'))))
+          redis.sadd(described_class.lookup_key_name(user.id), '59822c7d9fcdfa03725eff41782ad97d')
+          redis.sadd(described_class.lookup_key_name(user.id), current_session_id)
+          redis.sadd(described_class.lookup_key_name(9999), '5c8611e4f9c69645ad1a1492f4131358')
+        end
+      end
+
+      it 'removes the entry associated with the all user sessions but current' do
+        expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(2).to(1)
+
+        expect(ActiveSession.session_ids_for_user(9999).size).to eq(1)
+      end
+
+      it 'removes the lookup entry of deleted sessions' do
+        ActiveSession.destroy_all_but_current(user, request.session)
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.smembers(described_class.lookup_key_name(user.id))).to eq [current_session_id]
+        end
+      end
+
+      it 'does not remove impersonated sessions' do
+        impersonated_session_id = '6919a6f1bb119dd7396fadc38fd18eee'
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set(described_class.key_name(user.id, impersonated_session_id),
+            Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(impersonated_session_id), is_impersonated: true)))
+          redis.sadd(described_class.lookup_key_name(user.id), impersonated_session_id)
+        end
+
+        expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(3).to(2)
+
+        expect(ActiveSession.session_ids_for_user(9999).size).to eq(1)
+      end
+    end
+  end
+
   describe '.cleanup' do
     before do
       stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5)