Merge branch 'security-257884-fix_active_session_cleanup_with_private_id' into 'master'

Fix ActiveSession.cleanup when stored by session_private_id

See merge request gitlab-org/security/gitlab!981

app/models/active_session.rb

@@ -229,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
 

spec/models/active_session_spec.rb

@@ -551,5 +551,38 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         end
       end
     end
+
+    context 'cleaning up old sessions stored by Rack::Session::SessionId#private_id' do
+      let(:max_number_of_sessions_plus_one) { ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS + 1 }
+      let(:max_number_of_sessions_plus_two) { ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS + 2 }
+
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          (1..max_number_of_sessions_plus_two).each do |number|
+            redis.set(
+              "session:user:gitlab:#{user.id}:#{number}",
+              Marshal.dump(ActiveSession.new(session_private_id: number.to_s, updated_at: number.days.ago))
+            )
+            redis.sadd(
+              "session:lookup:user:gitlab:#{user.id}",
+              "#{number}"
+            )
+          end
+        end
+      end
+
+      it 'removes obsolete active sessions entries' do
+        ActiveSession.cleanup(user)
+
+        Gitlab::Redis::SharedState.with do |redis|
+          sessions = redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a
+
+          expect(sessions.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS)
+          expect(sessions).not_to(
+            include("session:user:gitlab:#{user.id}:#{max_number_of_sessions_plus_one}",
+                    "session:user:gitlab:#{user.id}:#{max_number_of_sessions_plus_two}"))
+        end
+      end
+    end
   end
 end