1 files changed, 62 insertions, 52 deletions

diff --git a/app/models/active_session.rb b/app/models/active_session.rb
index 4908290e06b..dded0eb1dc3 100644
--- a/app/models/active_session.rb
+++ b/app/models/active_session.rb
@@ -9,14 +9,14 @@ class ActiveSession
   attr_accessor :created_at, :updated_at,
     :ip_address, :browser, :os,
     :device_name, :device_type,
-    :is_impersonated, :session_id
+    :is_impersonated, :session_id, :session_private_id
 
-  def current?(session)
-    return false if session_id.nil? || session.id.nil?
+  def current?(rack_session)
+    return false if session_private_id.nil? || rack_session.id.nil?
 
     # Rack v2.0.8+ added private_id, which uses the hash of the
     # public_id to avoid timing attacks.
-    session_id.private_id == session.id.private_id
+    session_private_id == rack_session.id.private_id
   end
 
   def human_device_type
@@ -25,13 +25,14 @@ class ActiveSession
 
   # 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.public_id)
+    Gitlab::CryptoHelper.aes256_gcm_encrypt(session_id)
   end
 
   def self.set(user, request)
     Gitlab::Redis::SharedState.with do |redis|
-      session_id = request.session.id.public_id
+      session_private_id = request.session.id.private_id
       client = DeviceDetector.new(request.user_agent)
       timestamp = Time.current
 
@@ -43,25 +44,37 @@ class ActiveSession
         device_type: client.device_type,
         created_at: user.current_sign_in_at || timestamp,
         updated_at: timestamp,
-        session_id: session_id,
+        # 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?
       )
 
       redis.pipelined do
         redis.setex(
-          key_name(user.id, session_id),
+          key_name(user.id, session_private_id),
           Settings.gitlab['session_expire_delay'] * 60,
           Marshal.dump(active_user_session)
         )
 
         redis.sadd(
           lookup_key_name(user.id),
-          session_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|
@@ -70,27 +83,29 @@ class ActiveSession
     end
   end
 
-  def self.destroy(user, session_id)
-    return unless session_id
-
+  def self.cleanup(user)
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, [session_id])
+      clean_up_old_sessions(redis, user)
+      cleaned_up_lookup_entries(redis, user)
     end
   end
 
-  def self.destroy_with_public_id(user, public_id)
-    decrypted_id = decrypt_public_id(public_id)
+  # 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
 
-    return if decrypted_id.nil?
-
-    session_id = Rack::Session::SessionId.new(decrypted_id)
-    destroy(user, 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.public_id) }
+    key_names = session_ids.map { |session_id| key_name(user.id, session_id) }
 
-    redis.srem(lookup_key_name(user.id), session_ids.map(&:public_id))
+    redis.srem(lookup_key_name(user.id), session_ids)
 
     Gitlab::Instrumentation::RedisClusterValidator.allow_cross_slot_commands do
       redis.del(key_names)
@@ -98,19 +113,29 @@ class ActiveSession
     end
   end
 
-  def self.cleanup(user)
+  # 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)
+    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|
-      clean_up_old_sessions(redis, user)
-      cleaned_up_lookup_entries(redis, user)
+      destroy_sessions(redis, user, [session_id, decrypted_session_id, rack_session_private_id].compact)
     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
+  def self.destroy_all_but_current(user, current_rack_session)
+    sessions = not_impersonated(user)
+    sessions.reject! { |session| session.current?(current_rack_session) } if current_rack_session
 
     Gitlab::Redis::SharedState.with do |redis|
-      destroy_sessions(redis, user, session_ids.map(&:session_id)) if session_ids.any?
+      session_ids = (sessions.map(&:session_id) | sessions.map(&:session_private_id)).compact
+      destroy_sessions(redis, user, session_ids) if session_ids.any?
     end
   end
 
@@ -132,17 +157,16 @@ class ActiveSession
 
   # Lists the relevant session IDs for the user.
   #
-  # Returns an array of Rack::Session::SessionId objects
+  # Returns an array of strings
   def self.session_ids_for_user(user_id)
     Gitlab::Redis::SharedState.with do |redis|
-      session_ids = redis.smembers(lookup_key_name(user_id))
-      session_ids.map { |id| Rack::Session::SessionId.new(id) }
+      redis.smembers(lookup_key_name(user_id))
     end
   end
 
   # Lists the session Hash objects for the given session IDs.
   #
-  # session_ids - An array of Rack::Session::SessionId objects
+  # session_ids - An array of strings
   #
   # Returns an array of ActiveSession objects
   def self.sessions_from_ids(session_ids)
@@ -168,27 +192,12 @@ class ActiveSession
   # Returns an ActiveSession object
   def self.load_raw_session(raw_session)
     # rubocop:disable Security/MarshalLoad
-    session = Marshal.load(raw_session)
+    Marshal.load(raw_session)
     # rubocop:enable Security/MarshalLoad
+  end
 
-    # Older ActiveSession models serialize `session_id` as strings, To
-    # avoid breaking older sessions, we keep backwards compatibility
-    # with older Redis keys and initiate Rack::Session::SessionId here.
-    session.session_id = Rack::Session::SessionId.new(session.session_id) if session.try(:session_id).is_a?(String)
-    session
-  end
-
-  def self.rack_session_keys(session_ids)
-    session_ids.each_with_object([]) do |session_id, arr|
-      # This is a redis-rack implementation detail
-      # (https://github.com/redis-store/redis-rack/blob/master/lib/rack/session/redis.rb#L88)
-      #
-      # We need to delete session keys based on the legacy public key name
-      # and the newer private ID keys, but there's no well-defined interface
-      # so we have to do it directly.
-      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.public_id}"
-      arr << "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id.private_id}"
-    end
+  def self.rack_session_keys(rack_session_ids)
+    rack_session_ids.map { |session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
   end
 
   def self.raw_active_session_entries(redis, session_ids, user_id)
@@ -220,7 +229,7 @@ class ActiveSession
     sessions = active_session_entries(session_ids, user.id, redis)
     sessions.sort_by! {|session| session.updated_at }.reverse!
     destroyable_sessions = sessions.drop(ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
-    destroyable_session_ids = destroyable_sessions.map { |session| session.session_id }
+    destroyable_session_ids = destroyable_sessions.flat_map { |session| [session.session_id, session.session_private_id] }.compact
     destroy_sessions(redis, user, destroyable_session_ids) if destroyable_session_ids.any?
   end
 
@@ -244,6 +253,7 @@ 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