Clean up active session file

From to-do items

Remove binding.pry

Fix warden to-do

Add warden guard clause

Add changelog entry

Change naming in changelog

app/controllers/profiles/active_sessions_controller.rb

@@ -8,9 +8,8 @@ class Profiles::ActiveSessionsController < Profiles::ApplicationController
   end
 
   def destroy
-    # params[:id] can be either an Rack::Session::SessionId#private_id
-    # or an encrypted Rack::Session::SessionId#public_id
-    ActiveSession.destroy_with_deprecated_encryption(current_user, params[:id])
+    # params[:id] can be an Rack::Session::SessionId#private_id
+    ActiveSession.destroy_session(current_user, params[:id])
     current_user.forget_me!
 
     respond_to do |format|

app/helpers/active_sessions_helper.rb

@@ -24,11 +24,6 @@ module ActiveSessionsHelper
   end
 
   def revoke_session_path(active_session)
-    if active_session.session_private_id
-      profile_active_session_path(active_session.session_private_id)
-    else
-      # TODO: remove in 13.7
-      profile_active_session_path(active_session.public_id)
-    end
+    profile_active_session_path(active_session.session_private_id)
   end
 end

app/models/active_session.rb

@@ -42,13 +42,6 @@ class ActiveSession
     device_type&.titleize
   end
 
-  # This is not the same as Rack::Session::SessionId#public_id, but we
-  # need to preserve this for backwards compatibility.
-  # TODO: remove in 13.7
-  def public_id
-    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
-  end
-
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
       session_private_id = request.session.id.private_id
@@ -63,8 +56,6 @@ class ActiveSession
         device_type: client.device_type,
         created_at: user.current_sign_in_at || timestamp,
         updated_at: timestamp,
-        # TODO: remove in 13.7
-        session_id: request.session.id.public_id,
         session_private_id: session_private_id,
         is_impersonated: request.session[:impersonator_id].present?
       )
@@ -80,20 +71,10 @@ class ActiveSession
           lookup_key_name(user.id),
           session_private_id
         )
-
-        # We remove the ActiveSession stored by using public_id to avoid
-        # duplicate entries
-        remove_deprecated_active_sessions_with_public_id(redis, user.id, request.session.id.public_id)
       end
     end
   end
 
-  # TODO: remove in 13.7
-  private_class_method def self.remove_deprecated_active_sessions_with_public_id(redis, user_id, rack_session_public_id)
-    redis.srem(lookup_key_name(user_id), rack_session_public_id)
-    redis.del(key_name(user_id, rack_session_public_id))
-  end
-
   def self.list(user)
     Gitlab::Redis::SharedState.with do |redis|
       cleaned_up_lookup_entries(redis, user).map do |raw_session|
@@ -109,18 +90,6 @@ class ActiveSession
     end
   end
 
-  # TODO: remove in 13.7
-  # After upgrade there might be a duplicate ActiveSessions:
-  # - one with the public_id stored in #session_id
-  # - another with private_id stored in #session_private_id
-  def self.destroy_with_rack_session_id(user, rack_session_id)
-    return unless rack_session_id
-
-    Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [rack_session_id.public_id, rack_session_id.private_id])
-    end
-  end
-
   def self.destroy_sessions(redis, user, session_ids)
     key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
 
@@ -132,19 +101,11 @@ class ActiveSession
     end
   end
 
-  # TODO: remove in 13.7
-  # After upgrade, .destroy might be called with the session id encrypted
-  # by .public_id.
-  def self.destroy_with_deprecated_encryption(user, session_id)
+  def self.destroy_session(user, session_id)
     return unless session_id
 
-    decrypted_session_id = decrypt_public_id(session_id)
-    rack_session_private_id = if decrypted_session_id
-                                Rack::Session::SessionId.new(decrypted_session_id).private_id
-                              end
-
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
+      destroy_sessions(redis, user, [session_id].compact)
     end
   end
 
@@ -275,11 +236,4 @@ class ActiveSession
 
     entries.compact
   end
-
-  # TODO: remove in 13.7
-  private_class_method def self.decrypt_public_id(public_id)
-    Gitlab::CryptoHelper.aes256_gcm_decrypt(public_id)
-  rescue
-    nil
-  end
 end

changelogs/unreleased/security-clean-up-active-sessions.yml

+---
+title: Do not store marshalled sessions ids in Redis
+merge_request:
+author:
+type: security

config/initializers/warden.rb

@@ -42,8 +42,7 @@ Rails.application.configure do |config|
     activity = Gitlab::Auth::Activity.new(opts)
     tracker = Gitlab::Auth::BlockedUserTracker.new(user, auth)
 
-    # TODO: switch to `auth.request.session.id.private_id` in 13.7
-    ActiveSession.destroy_with_rack_session_id(user, auth.request.session.id)
+    ActiveSession.destroy_session(user, auth.request.session.id.private_id) if auth.request.session.id
     activity.user_session_destroyed!
 
     ##

spec/controllers/profiles/active_sessions_controller_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Profiles::ActiveSessionsController do
 
     it 'invalidates all remember user tokens' do
       ActiveSession.set(user, request)
-      session_id = request.session.id.public_id
+      session_id = request.session.id.private_id
       user.remember_me!
 
       delete :destroy, params: { id: session_id }

spec/models/active_session_spec.rb

@@ -42,17 +42,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
-  describe '#public_id' do
-    it 'returns an encrypted, url-encoded session id' do
-      original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
-      active_session = ActiveSession.new(session_id: original_session_id.public_id)
-      encrypted_id = active_session.public_id
-      derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
-
-      expect(original_session_id.public_id).to eq derived_session_id
-    end
-  end
-
   describe '.list' do
     it 'returns all sessions by user' do
       Gitlab::Redis::SharedState.with do |redis|
@@ -207,89 +196,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         end
       end
     end
-
-    context 'ActiveSession stored by deprecated rack_session_public_key' do
-      let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
-      let(:deprecated_active_session_lookup_key) { rack_session.public_id }
-
-      before do
-        Gitlab::Redis::SharedState.with do |redis|
-          redis.set("session:user:gitlab:#{user.id}:#{deprecated_active_session_lookup_key}",
-                    '')
-          redis.sadd(described_class.lookup_key_name(user.id),
-                     deprecated_active_session_lookup_key)
-        end
-      end
-
-      it 'removes deprecated key and stores only new one' do
-        expected_session_keys = ["session:user:gitlab:#{user.id}:#{rack_session.private_id}",
-                                 "session:lookup:user:gitlab:#{user.id}"]
-
-        ActiveSession.set(user, request)
-
-        Gitlab::Redis::SharedState.with do |redis|
-          actual_session_keys = redis.scan_each(match: 'session:*').to_a
-          expect(actual_session_keys).to(match_array(expected_session_keys))
-
-          expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq [rack_session.private_id]
-        end
-      end
-    end
   end
 
-  describe '.destroy_with_rack_session_id' do
-    it 'gracefully handles a nil session ID' do
-      expect(described_class).not_to receive(:destroy_sessions)
-
-      ActiveSession.destroy_with_rack_session_id(user, nil)
-    end
-
-    it 'removes the entry associated with the currently killed user session' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.set("session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d", '')
-        redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:user:gitlab:*")).to match_array [
-          "session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d",
-          "session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358"
-        ]
-      end
-    end
-
-    it 'removes the lookup entry' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty
-      end
-    end
-
-    it 'removes the devise session' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:#{rack_session.private_id}", '')
-        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
-        redis.set("session:gitlab:#{rack_session.private_id}", '')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
-      end
-    end
-  end
-
-  describe '.destroy_with_deprecated_encryption' do
+  describe '.destroy_session' do
     shared_examples 'removes all session data' do
       before do
         Gitlab::Redis::SharedState.with do |redis|
@@ -330,7 +239,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
 
     context 'destroy called with Rack::Session::SessionId#private_id' do
-      subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) }
+      subject { ActiveSession.destroy_session(user, rack_session.private_id) }
 
       it 'calls .destroy_sessions' do
         expect(ActiveSession).to(
@@ -347,26 +256,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         include_examples 'removes all session data'
       end
     end
-
-    context 'destroy called with ActiveSession#public_id (deprecated)' do
-      let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
-      let(:encrypted_active_session_id) { active_session.public_id }
-      let(:active_session_lookup_key) { rack_session.public_id }
-
-      subject { ActiveSession.destroy_with_deprecated_encryption(user, encrypted_active_session_id) }
-
-      it 'calls .destroy_sessions' do
-        expect(ActiveSession).to(
-          receive(:destroy_sessions)
-            .with(anything, user, [encrypted_active_session_id, rack_session.public_id, rack_session.private_id]))
-
-        subject
-      end
-
-      context 'ActiveSession with session_id (deprecated)' do
-        include_examples 'removes all session data'
-      end
-    end
   end
 
   describe '.destroy_all_but_current' do