1 files changed, 169 insertions, 49 deletions

diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb
index de39c8c7c5c..f0bae3f29c0 100644
--- a/spec/models/active_session_spec.rb
+++ b/spec/models/active_session_spec.rb
@@ -23,7 +23,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
   describe '#current?' do
     it 'returns true if the active session matches the current session' do
-      active_session = ActiveSession.new(session_id: rack_session)
+      active_session = ActiveSession.new(session_private_id: rack_session.private_id)
 
       expect(active_session.current?(session)).to be true
     end
@@ -45,7 +45,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
   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)
+      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)
 
@@ -106,8 +106,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.sadd(
           "session:lookup:user:gitlab:#{user.id}",
           %w[
-            6919a6f1bb119dd7396fadc38fd18d0d
-            59822c7d9fcdfa03725eff41782ad97d
+            2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae
+            2::d2ee6f70d6ef0e8701efa3f6b281cbe8e6bf3d109ef052a8b5ce88bfc7e71c26
           ]
         )
       end
@@ -135,7 +135,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.set("session:gitlab:#{rack_session.private_id}", Marshal.dump({ _csrf_token: 'abcd' }))
       end
 
-      expect(ActiveSession.sessions_from_ids([rack_session])).to eq [{ _csrf_token: 'abcd' }]
+      expect(ActiveSession.sessions_from_ids([rack_session.private_id])).to eq [{ _csrf_token: 'abcd' }]
     end
 
     it 'avoids a redis lookup for an empty array' do
@@ -150,12 +150,11 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
       redis = double(:redis)
       expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis)
 
-      sessions = %w[session-a session-b session-c session-d]
+      sessions = %w[session-a session-b]
       mget_responses = sessions.map { |session| [Marshal.dump(session)]}
-      expect(redis).to receive(:mget).exactly(4).times.and_return(*mget_responses)
+      expect(redis).to receive(:mget).twice.times.and_return(*mget_responses)
 
-      session_ids = [1, 2].map { |id| Rack::Session::SessionId.new(id.to_s) }
-      expect(ActiveSession.sessions_from_ids(session_ids).map(&:to_s)).to eql(sessions)
+      expect(ActiveSession.sessions_from_ids([1, 2])).to eql(sessions)
     end
   end
 
@@ -165,7 +164,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
       Gitlab::Redis::SharedState.with do |redis|
         expect(redis.scan_each.to_a).to include(
-          "session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d",
+          "session:user:gitlab:#{user.id}:2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae",
           "session:lookup:user:gitlab:#{user.id}"
         )
       end
@@ -208,13 +207,41 @@ 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' do
+  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(user, nil)
+      ActiveSession.destroy_with_rack_session_id(user, nil)
     end
 
     it 'removes the entry associated with the currently killed user session' do
@@ -224,7 +251,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '')
       end
 
-      ActiveSession.destroy(user, request.session.id)
+      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 [
@@ -240,7 +267,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d')
       end
 
-      ActiveSession.destroy(user, request.session.id)
+      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
@@ -249,12 +276,12 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
     it 'removes the devise session' do
       Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:#{rack_session.public_id}", '')
+        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(user, request.session.id)
+      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
@@ -262,37 +289,83 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
-  describe '.destroy_with_public_id' do
-    it 'receives a user and public id and destroys the associated session' do
-      ActiveSession.set(user, request)
-      session = ActiveSession.list(user).first
+  describe '.destroy_with_deprecated_encryption' do
+    shared_examples 'removes all session data' do
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set("session:user:gitlab:#{user.id}:#{active_session_lookup_key}", '')
+          # 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}", '')
+
+          redis.set(described_class.key_name(user.id, active_session_lookup_key),
+                    Marshal.dump(active_session))
+          redis.sadd(described_class.lookup_key_name(user.id),
+                     active_session_lookup_key)
+        end
+      end
+
+      it 'removes the devise session' do
+        subject
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
+        end
+      end
 
-      ActiveSession.destroy_with_public_id(user, session.public_id)
+      it 'removes the lookup entry' do
+        subject
 
-      total_sessions = ActiveSession.list(user).count
-      expect(total_sessions).to eq 0
+        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 ActiveSession' do
+        subject
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.scan_each(match: "session:user:gitlab:*").to_a).to be_empty
+        end
+      end
     end
 
-    it 'handles invalid input for public id' do
-      expect do
-        ActiveSession.destroy_with_public_id(user, nil)
-      end.not_to raise_error
+    context 'destroy called with Rack::Session::SessionId#private_id' do
+      subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) }
+
+      it 'calls .destroy_sessions' do
+        expect(ActiveSession).to(
+          receive(:destroy_sessions)
+            .with(anything, user, [rack_session.private_id]))
+
+        subject
+      end
 
-      expect do
-        ActiveSession.destroy_with_public_id(user, "")
-      end.not_to raise_error
+      context 'ActiveSession with session_private_id' do
+        let(:active_session) { ActiveSession.new(session_private_id: rack_session.private_id) }
+        let(:active_session_lookup_key) { rack_session.private_id }
 
-      expect do
-        ActiveSession.destroy_with_public_id(user, "aaaaaaaa")
-      end.not_to raise_error
+        include_examples 'removes all session data'
+      end
     end
 
-    it 'does not attempt to destroy session when given invalid input for public id' do
-      expect(ActiveSession).not_to receive(:destroy)
+    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, [active_session.public_id, rack_session.public_id, rack_session.private_id]))
+
+        subject
+      end
 
-      ActiveSession.destroy_with_public_id(user, nil)
-      ActiveSession.destroy_with_public_id(user, "")
-      ActiveSession.destroy_with_public_id(user, "aaaaaaaa")
+      context 'ActiveSession with session_id (deprecated)' do
+        include_examples 'removes all session data'
+      end
     end
   end
 
@@ -308,29 +381,43 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
 
       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')
+          # setup for current user
+          [current_session_id, '59822c7d9fcdfa03725eff41782ad97d'].each do |session_public_id|
+            session_private_id = Rack::Session::SessionId.new(session_public_id).private_id
+            active_session = ActiveSession.new(session_private_id: session_private_id)
+            redis.set(described_class.key_name(user.id, session_private_id),
+                      Marshal.dump(active_session))
+            redis.sadd(described_class.lookup_key_name(user.id),
+                       session_private_id)
+          end
+
+          # setup for unrelated user
+          unrelated_user_id = 9999
+          session_private_id = Rack::Session::SessionId.new('5c8611e4f9c69645ad1a1492f4131358').private_id
+          active_session = ActiveSession.new(session_private_id: session_private_id)
+
+          redis.set(described_class.key_name(unrelated_user_id, session_private_id),
+                    Marshal.dump(active_session))
+          redis.sadd(described_class.lookup_key_name(unrelated_user_id),
+                     session_private_id)
         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.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
+        session_private_id = Rack::Session::SessionId.new(current_session_id).private_id
         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]
+          expect(
+            redis.smembers(described_class.lookup_key_name(user.id))
+          ).to eq([session_private_id])
         end
       end
 
@@ -464,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