Merge branch 'mg-remove-gitlab-sessions-redis-store' into 'master'

Remove Gitlab::Sessions::RedisStore as session store

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/181637



Merged-by: Bob Van Landuyt <bob@gitlab.com>
Approved-by: Arpit Gogia <12347103-arpitgogia@users.noreply.gitlab.com>
Approved-by: Bob Van Landuyt <bob@gitlab.com>
Reviewed-by: Bob Van Landuyt <bob@gitlab.com>
Co-authored-by: Gregorius Marco <gmarco@gitlab.com>

config/initializers/session_store.rb

@@ -42,8 +42,6 @@
 
 ::Redis::Store::Factory.prepend(Gitlab::Patch::RedisStoreFactory)
 
-ENV['USE_REDIS_CACHE_STORE_AS_SESSION_STORE'] = 'true' if Rails.env.test? || Rails.env.development?
-
 session_store_class, options = Gitlab::Sessions::StoreBuilder.new(cookie_key, session_cookie_token_prefix).prepare
 
 Rails.application.configure do

lib/gitlab/sessions/redis_store.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Sessions
-    class RedisStore < ActionDispatch::Session::RedisStore
-      DELIMITER = '-'
-
-      attr_reader :session_cookie_token_prefix
-
-      def initialize(app, options = {})
-        super
-
-        @session_cookie_token_prefix = options.fetch(:session_cookie_token_prefix, "") || ""
-      end
-
-      def generate_sid
-        delimiter = session_cookie_token_prefix.empty? ? '' : DELIMITER
-        Rack::Session::SessionId.new(session_cookie_token_prefix + delimiter + super.public_id)
-      end
-    end
-  end
-end

lib/gitlab/sessions/redis_store_serializer.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Sessions
-    module RedisStoreSerializer
-      def self.load(val)
-        deserialized = Marshal.load(val) # rubocop:disable Security/MarshalLoad -- We're loading session data similar to Redis::Store::Serialization#get from redis-store gem
-
-        return deserialized if deserialized.is_a?(Hash)
-
-        # Session data can be an instance of ActiveSupport::Cache::Entry
-        # when we're using session store based on ActionDispatch::Session::CacheStore
-        # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/176108
-        deserialized&.value
-      rescue StandardError => e
-        Gitlab::ErrorTracking.track_and_raise_exception(e)
-      end
-
-      def self.dump(val)
-        Marshal.dump(val)
-      end
-    end
-  end
-end

lib/gitlab/sessions/store_builder.rb

@@ -11,44 +11,26 @@ def initialize(cookie_key, session_cookie_token_prefix)
       end
 
       def prepare
-        if Gitlab::Utils.to_boolean(ENV.fetch('USE_REDIS_CACHE_STORE_AS_SESSION_STORE', 'false'))
-          # Set expiry to very large number (practically permanent) instead of the default 1 week
-          # as some specs rely on time travel to a distant past or future.
-          Settings.gitlab['session_expire_delay'] = ::Gitlab::Database::MAX_INT_VALUE if Rails.env.test?
+        # Set expiry to very large number (practically permanent) instead of the default 1 week
+        # as some specs rely on time travel to a distant past or future.
+        Settings.gitlab['session_expire_delay'] = ::Gitlab::Database::MAX_INT_VALUE if Rails.env.test?
 
-          [
-            ::Gitlab::Sessions::CacheStore, # Using the cookie_store would enable session replay attacks
-            {
-              cache: ActiveSupport::Cache::RedisCacheStore.new(
-                namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE,
-                redis: Gitlab::Redis::Sessions,
-                expires_in: Settings.gitlab['session_expire_delay'] * 60,
-                coder: Gitlab::Sessions::CacheStoreCoder
-              ),
-              key: cookie_key,
-              secure: Gitlab.config.gitlab.https,
-              httponly: true,
-              path: Rails.application.config.relative_url_root.presence || '/',
-              session_cookie_token_prefix: session_cookie_token_prefix
-            }
-          ]
-        else
-          [
-            Gitlab::Sessions::RedisStore, # Using the cookie_store would enable session replay attacks
-            {
-              redis_server: Gitlab::Redis::Sessions.params.merge(
-                namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE,
-                serializer: Gitlab::Sessions::RedisStoreSerializer
-              ),
-              key: cookie_key,
-              secure: Gitlab.config.gitlab.https,
-              httponly: true,
+        [
+          ::Gitlab::Sessions::CacheStore, # Using the cookie_store would enable session replay attacks
+          {
+            cache: ActiveSupport::Cache::RedisCacheStore.new(
+              namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE,
+              redis: Gitlab::Redis::Sessions,
               expires_in: Settings.gitlab['session_expire_delay'] * 60,
-              path: Rails.application.config.relative_url_root.presence || '/',
-              session_cookie_token_prefix: session_cookie_token_prefix
-            }
-          ]
-        end
+              coder: Gitlab::Sessions::CacheStoreCoder
+            ),
+            key: cookie_key,
+            secure: Gitlab.config.gitlab.https,
+            httponly: true,
+            path: Rails.application.config.relative_url_root.presence || '/',
+            session_cookie_token_prefix: session_cookie_token_prefix
+          }
+        ]
       end
     end
   end

spec/lib/gitlab/sessions/redis_store_serializer_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Sessions::RedisStoreSerializer, feature_category: :system_access do
-  let(:hash) { { a: 1, b: 2 } }
-  let(:serialized) { Marshal.dump(val) }
-
-  describe '.load' do
-    shared_examples 'unmarshal' do
-      it 'returns original value' do
-        expect(load).to eq(expected)
-      end
-    end
-
-    subject(:load) { described_class.load(serialized) }
-
-    context 'with hash value' do
-      let(:val) { hash }
-      let(:expected) { hash }
-
-      it_behaves_like 'unmarshal'
-    end
-
-    context 'with ActiveSupport::Cache::Entry value' do
-      let(:val) { ActiveSupport::Cache::Entry.new(hash) }
-      let(:expected) { hash }
-
-      it_behaves_like 'unmarshal'
-    end
-
-    context 'with nil value' do
-      let(:val) { nil }
-      let(:expected) { nil }
-
-      it_behaves_like 'unmarshal'
-    end
-
-    context 'with unrecognized type' do
-      let(:val) { %w[a b c] }
-
-      it 'tracks and raises an exception' do
-        expect(Gitlab::ErrorTracking).to receive(:track_and_raise_exception).with(instance_of(NoMethodError))
-
-        load
-      end
-    end
-  end
-
-  describe '.dump' do
-    subject(:dump) { described_class.dump(hash) }
-
-    it 'calls Marshal.dump' do
-      expect(Marshal).to receive(:dump).with(hash)
-
-      dump
-    end
-
-    it 'returns marshalled object' do
-      expect(dump).to eq(Marshal.dump(hash))
-    end
-  end
-end

spec/lib/gitlab/sessions/redis_store_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Sessions::RedisStore, feature_category: :cell do
-  using RSpec::Parameterized::TableSyntax
-
-  describe '#generate_sid' do
-    let(:redis_store) do
-      described_class.new(Rails.application, { session_cookie_token_prefix: session_cookie_token_prefix })
-    end
-
-    context 'when passing `session_cookie_token_prefix` in options' do
-      where(:prefix, :calculated_prefix) do
-        nil              | ''
-        ''               | ''
-        'random_prefix_' | 'random_prefix_-'
-        '_random_prefix' | '_random_prefix-'
-      end
-
-      with_them do
-        let(:session_cookie_token_prefix) { prefix }
-
-        it 'generates sid that is prefixed with the configured prefix' do
-          generated_sid = redis_store.generate_sid
-          expect(generated_sid).to be_a Rack::Session::SessionId
-          expect(generated_sid.public_id).to match(/^#{calculated_prefix}[a-z0-9]{32}$/)
-        end
-      end
-    end
-
-    context 'when not passing `session_cookie_token_prefix` in options' do
-      let(:redis_store) { described_class.new(Rails.application) }
-
-      it 'generates sid that is not prefixed' do
-        generated_sid = redis_store.generate_sid
-        expect(generated_sid).to be_a Rack::Session::SessionId
-        expect(generated_sid.public_id).to match(/^[a-z0-9]{32}$/)
-      end
-    end
-  end
-end

spec/lib/gitlab/sessions/store_builder_spec.rb

@@ -8,11 +8,7 @@
 
   subject(:prepare) { described_class.new(cookie_key, session_cookie_token_prefix).prepare }
 
-  context 'when env var USE_REDIS_CACHE_STORE_AS_SESSION_STORE=true' do
-    before do
-      stub_env('USE_REDIS_CACHE_STORE_AS_SESSION_STORE', 'true')
-    end
-
+  describe '#prepare' do
     it 'returns Gitlab::Sessions::CacheStore' do
       expect(prepare).to match([
         ::Gitlab::Sessions::CacheStore,
@@ -24,24 +20,4 @@
       ])
     end
   end
-
-  context 'when env var USE_REDIS_CACHE_STORE_AS_SESSION_STORE=false' do
-    before do
-      stub_env('USE_REDIS_CACHE_STORE_AS_SESSION_STORE', 'false')
-    end
-
-    it 'returns Gitlab::Sessions::RedisStore' do
-      expect(prepare).to match([
-        Gitlab::Sessions::RedisStore,
-        a_hash_including(
-          redis_server: Gitlab::Redis::Sessions.params.merge(
-            namespace: Gitlab::Redis::Sessions::SESSION_NAMESPACE,
-            serializer: Gitlab::Sessions::RedisStoreSerializer
-          ),
-          key: cookie_key,
-          session_cookie_token_prefix: session_cookie_token_prefix
-        )
-      ])
-    end
-  end
 end

spec/models/active_session_spec.rb

@@ -197,6 +197,10 @@ def make_session(id)
       end
     end
 
+    # Historically, sessions were written by Gitlab::Sessions::RedisStore in a different format
+    # than the current Gitlab::Sessions::CacheStore.
+    # The RedisStore is removed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/181637,
+    # but we'll keep this specs to ensure backwards compatibility.
     context 'with old session format from Gitlab::Sessions::RedisStore' do
       it 'uses the ActiveSession lookup to return original sessions' do
         Gitlab::Redis::Sessions.with do |redis|