diff options
Diffstat (limited to 'spec/models/active_session_spec.rb')
-rw-r--r-- | spec/models/active_session_spec.rb | 535 |
1 files changed, 363 insertions, 172 deletions
diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb index 2fd7b127500..751d31ad95a 100644 --- a/spec/models/active_session_spec.rb +++ b/spec/models/active_session_spec.rb @@ -2,7 +2,8 @@ require 'spec_helper' -RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do +RSpec.describe ActiveSession, :clean_gitlab_redis_sessions do + let(:lookup_key) { described_class.lookup_key_name(user.id) } let(:user) do create(:user).tap do |user| user.current_sign_in_at = Time.current @@ -43,52 +44,88 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end describe '.list' do + def make_session(id) + described_class.new(session_id: id) + end + it 'returns all sessions by user' do - Gitlab::Redis::SharedState.with do |redis| - redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ session_id: 'a' })) - redis.set("session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d", Marshal.dump({ session_id: 'b' })) - redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '') + Gitlab::Redis::Sessions.with do |redis| + # Some deprecated sessions + redis.set(described_class.key_name_v1(user.id, "6919a6f1bb119dd7396fadc38fd18d0d"), Marshal.dump(make_session('a'))) + redis.set(described_class.key_name_v1(user.id, "59822c7d9fcdfa03725eff41782ad97d"), Marshal.dump(make_session('b'))) + # Some new sessions + redis.set(described_class.key_name(user.id, 'some-unique-id-x'), make_session('c').dump) + redis.set(described_class.key_name(user.id, 'some-unique-id-y'), make_session('d').dump) + # Some red herrings + redis.set(described_class.key_name(9999, "5c8611e4f9c69645ad1a1492f4131358"), 'irrelevant') + redis.set(described_class.key_name_v1(9999, "5c8611e4f9c69645ad1a1492f4131358"), 'irrelevant') redis.sadd( - "session:lookup:user:gitlab:#{user.id}", + lookup_key, %w[ 6919a6f1bb119dd7396fadc38fd18d0d 59822c7d9fcdfa03725eff41782ad97d + some-unique-id-x + some-unique-id-y ] ) end - expect(ActiveSession.list(user)).to match_array [{ session_id: 'a' }, { session_id: 'b' }] + expect(described_class.list(user)).to contain_exactly( + have_attributes(session_id: 'a'), + have_attributes(session_id: 'b'), + have_attributes(session_id: 'c'), + have_attributes(session_id: 'd') + ) end - it 'does not return obsolete entries and cleans them up' do - Gitlab::Redis::SharedState.with do |redis| - redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", Marshal.dump({ session_id: 'a' })) + shared_examples 'ignoring obsolete entries' do + let(:session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } + let(:session) { described_class.new(session_id: 'a') } - redis.sadd( - "session:lookup:user:gitlab:#{user.id}", - %w[ - 6919a6f1bb119dd7396fadc38fd18d0d - 59822c7d9fcdfa03725eff41782ad97d - ] - ) - end + it 'does not return obsolete entries and cleans them up' do + Gitlab::Redis::Sessions.with do |redis| + redis.set(session_key, serialized_session) + + redis.sadd( + lookup_key, + [ + session_id, + '59822c7d9fcdfa03725eff41782ad97d' + ] + ) + end - expect(ActiveSession.list(user)).to eq [{ session_id: 'a' }] + expect(ActiveSession.list(user)).to contain_exactly(session) - Gitlab::Redis::SharedState.with do |redis| - expect(redis.sscan_each("session:lookup:user:gitlab:#{user.id}").to_a).to eq ['6919a6f1bb119dd7396fadc38fd18d0d'] + Gitlab::Redis::Sessions.with do |redis| + expect(redis.sscan_each(lookup_key)).to contain_exactly session_id + end end end - it 'returns an empty array if the use does not have any active session' do - expect(ActiveSession.list(user)).to eq [] + context 'when the current session is in the old format' do + let(:session_key) { described_class.key_name_v1(user.id, session_id) } + let(:serialized_session) { Marshal.dump(session) } + + it_behaves_like 'ignoring obsolete entries' + end + + context 'when the current session is in the new format' do + let(:session_key) { described_class.key_name(user.id, session_id) } + let(:serialized_session) { session.dump } + + it_behaves_like 'ignoring obsolete entries' + end + + it 'returns an empty array if the user does not have any active session' do + expect(ActiveSession.list(user)).to be_empty end end describe '.list_sessions' do it 'uses the ActiveSession lookup to return original sessions' do - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.with do |redis| # 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}", Marshal.dump({ _csrf_token: 'abcd' })) @@ -107,19 +144,17 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '.session_ids_for_user' do it 'uses the user lookup table to return session ids' do - session_ids = ['59822c7d9fcdfa03725eff41782ad97d'] - - Gitlab::Redis::SharedState.with do |redis| - redis.sadd("session:lookup:user:gitlab:#{user.id}", session_ids) + Gitlab::Redis::Sessions.with do |redis| + redis.sadd(lookup_key, %w[a b c]) end - expect(ActiveSession.session_ids_for_user(user.id).map(&:to_s)).to eq(session_ids) + expect(ActiveSession.session_ids_for_user(user.id).map(&:to_s)).to match_array(%w[a b c]) end end describe '.sessions_from_ids' do it 'uses the ActiveSession lookup to return original sessions' do - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.with do |redis| # 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}", Marshal.dump({ _csrf_token: 'abcd' })) end @@ -128,7 +163,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do end it 'avoids a redis lookup for an empty array' do - expect(Gitlab::Redis::SharedState).not_to receive(:with) + expect(Gitlab::Redis::Sessions).not_to receive(:with) expect(ActiveSession.sessions_from_ids([])).to eq([]) end @@ -137,7 +172,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do stub_const('ActiveSession::SESSION_BATCH_SIZE', 1) redis = double(:redis) - expect(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis) + expect(Gitlab::Redis::Sessions).to receive(:with).and_yield(redis) sessions = %w[session-a session-b] mget_responses = sessions.map { |session| [Marshal.dump(session)]} @@ -151,49 +186,67 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do it 'sets a new redis entry for the user session and a lookup entry' do ActiveSession.set(user, request) - Gitlab::Redis::SharedState.with do |redis| + session_id = "2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae" + + Gitlab::Redis::Sessions.with do |redis| expect(redis.scan_each.to_a).to include( - "session:user:gitlab:#{user.id}:2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae", - "session:lookup:user:gitlab:#{user.id}" + described_class.key_name(user.id, session_id), # current session + described_class.key_name_v1(user.id, session_id), # support for mixed deployment + lookup_key ) end end it 'adds timestamps and information from the request' do - Timecop.freeze(Time.zone.parse('2018-03-12 09:06')) do - ActiveSession.set(user, request) + time = Time.zone.parse('2018-03-12 09:06') - session = ActiveSession.list(user) + travel_to(time) do + described_class.set(user, request) - expect(session.count).to eq 1 - expect(session.first).to have_attributes( + sessions = described_class.list(user) + + expect(sessions).to contain_exactly have_attributes( ip_address: '127.0.0.1', browser: 'Mobile Safari', os: 'iOS', device_name: 'iPhone 6', device_type: 'smartphone', - created_at: Time.zone.parse('2018-03-12 09:06'), - updated_at: Time.zone.parse('2018-03-12 09:06') + created_at: eq(time), + updated_at: eq(time) ) end end + it 'is possible to log in only using the old session key' do + session_id = "2::418729c72310bbf349a032f0bb6e3fce9f5a69df8f000d8ae0ac5d159d8f21ae" + ActiveSession.set(user, request) + + Gitlab::Redis::SharedState.with do |redis| + redis.del(described_class.key_name(user.id, session_id)) + end + + sessions = ActiveSession.list(user) + + expect(sessions).to be_present + end + it 'keeps the created_at from the login on consecutive requests' do - now = Time.zone.parse('2018-03-12 09:06') + created_at = Time.zone.parse('2018-03-12 09:06') + updated_at = created_at + 1.minute - Timecop.freeze(now) do + travel_to(created_at) do ActiveSession.set(user, request) + end - Timecop.freeze(now + 1.minute) do - ActiveSession.set(user, request) + travel_to(updated_at) do + ActiveSession.set(user, request) - session = ActiveSession.list(user) + session = ActiveSession.list(user) - expect(session.first).to have_attributes( - created_at: Time.zone.parse('2018-03-12 09:06'), - updated_at: Time.zone.parse('2018-03-12 09:07') - ) - end + expect(session.first).to have_attributes( + created_at: eq(created_at), + updated_at: eq(updated_at) + ) end end end @@ -201,22 +254,20 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do describe '.destroy_session' do shared_examples 'removes all session data' do before do - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.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) + redis.set(session_key, serialized_session) + redis.sadd(lookup_key, active_session_lookup_key) end end it 'removes the devise session' do subject - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.with do |redis| expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty end end @@ -224,15 +275,15 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do it 'removes the lookup entry' do subject - Gitlab::Redis::SharedState.with do |redis| - expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty + Gitlab::Redis::Sessions.with do |redis| + expect(redis.scan_each(match: lookup_key).to_a).to be_empty end end it 'removes the ActiveSession' do subject - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.with do |redis| expect(redis.scan_each(match: "session:user:gitlab:*").to_a).to be_empty end end @@ -253,7 +304,19 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do let(:active_session) { ActiveSession.new(session_private_id: rack_session.private_id) } let(:active_session_lookup_key) { rack_session.private_id } - include_examples 'removes all session data' + context 'when using old session key serialization' do + let(:session_key) { described_class.key_name_v1(user.id, active_session_lookup_key) } + let(:serialized_session) { Marshal.dump(active_session) } + + include_examples 'removes all session data' + end + + context 'when using new session key serialization' do + let(:session_key) { described_class.key_name(user.id, active_session_lookup_key) } + let(:serialized_session) { active_session.dump } + + include_examples 'removes all session data' + end end end end @@ -265,19 +328,17 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do ActiveSession.destroy_all_but_current(user, nil) end - context 'with user sessions' do + shared_examples 'with user sessions' do let(:current_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } before do - Gitlab::Redis::SharedState.with do |redis| + Gitlab::Redis::Sessions.with do |redis| # 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) + redis.set(key_name(user.id, session_private_id), dump_session(active_session)) + redis.sadd(lookup_key, session_private_id) end # setup for unrelated user @@ -285,10 +346,8 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do 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) + redis.set(key_name(unrelated_user_id, session_private_id), dump_session(active_session)) + redis.sadd(described_class.lookup_key_name(unrelated_user_id), session_private_id) end end @@ -303,19 +362,17 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state 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([session_private_id]) + Gitlab::Redis::Sessions.with do |redis| + expect(redis.smembers(lookup_key)).to contain_exactly session_private_id end end it 'does not remove impersonated sessions' do impersonated_session_id = '6919a6f1bb119dd7396fadc38fd18eee' - Gitlab::Redis::SharedState.with do |redis| - redis.set(described_class.key_name(user.id, impersonated_session_id), - Marshal.dump(ActiveSession.new(session_id: Rack::Session::SessionId.new(impersonated_session_id), is_impersonated: true))) - redis.sadd(described_class.lookup_key_name(user.id), impersonated_session_id) + Gitlab::Redis::Sessions.with do |redis| + redis.set(key_name(user.id, impersonated_session_id), + dump_session(ActiveSession.new(session_id: Rack::Session::SessionId.new(impersonated_session_id), is_impersonated: true))) + redis.sadd(lookup_key, impersonated_session_id) end expect { ActiveSession.destroy_all_but_current(user, request.session) }.to change { ActiveSession.session_ids_for_user(user.id).size }.from(3).to(2) @@ -323,155 +380,289 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do expect(ActiveSession.session_ids_for_user(9999).size).to eq(1) end end - end - describe '.cleanup' do - before do - stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5) - end + context 'with legacy sessions' do + def key_name(user_id, id) + described_class.key_name_v1(user_id, id) + end - it 'removes obsolete lookup entries' do - Gitlab::Redis::SharedState.with do |redis| - redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '') - redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d') - redis.sadd("session:lookup:user:gitlab:#{user.id}", '59822c7d9fcdfa03725eff41782ad97d') + def dump_session(session) + Marshal.dump(session) end - ActiveSession.cleanup(user) + it_behaves_like 'with user sessions' + end - Gitlab::Redis::SharedState.with do |redis| - expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq ['6919a6f1bb119dd7396fadc38fd18d0d'] + context 'with new sessions' do + def key_name(user_id, id) + described_class.key_name(user_id, id) + end + + def dump_session(session) + session.dump end + + it_behaves_like 'with user sessions' end + end - it 'does not bail if there are no lookup entries' do - ActiveSession.cleanup(user) + describe '.cleanup' do + before do + stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5) end - context 'cleaning up old sessions' 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 } + shared_examples 'cleaning up' do + context 'when removing obsolete sessions' do + let(:current_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } - 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_id: number.to_s, updated_at: number.days.ago)) - ) - redis.sadd( - "session:lookup:user:gitlab:#{user.id}", - "#{number}" - ) + it 'removes obsolete lookup entries' do + Gitlab::Redis::Sessions.with do |redis| + redis.set(session_key, '') + redis.sadd(lookup_key, current_session_id) + redis.sadd(lookup_key, '59822c7d9fcdfa03725eff41782ad97d') + end + + ActiveSession.cleanup(user) + + Gitlab::Redis::Sessions.with do |redis| + expect(redis.smembers(lookup_key)).to contain_exactly current_session_id end end end - it 'removes obsolete active sessions entries' do + it 'does not bail if there are no lookup entries' do ActiveSession.cleanup(user) + end - Gitlab::Redis::SharedState.with do |redis| - sessions = redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a + context 'cleaning up old sessions' 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::Sessions.with do |redis| + max_number_of_sessions_plus_two.times do |number| + redis.set( + key_name(user.id, number), + dump_session(ActiveSession.new(session_id: number.to_s, updated_at: number.days.ago)) + ) + redis.sadd(lookup_key, number.to_s) + end + end + end + + it 'removes obsolete active sessions entries' do + ActiveSession.cleanup(user) + + Gitlab::Redis::Sessions.with do |redis| + sessions = described_class.list(user) - 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}") + expect(sessions.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) + expect(sessions).not_to include( + have_attributes(session_id: max_number_of_sessions_plus_one), + have_attributes(session_id: max_number_of_sessions_plus_two) + ) + end end - end - it 'removes obsolete lookup entries' do - ActiveSession.cleanup(user) + it 'removes obsolete lookup entries' do + ActiveSession.cleanup(user) - Gitlab::Redis::SharedState.with do |redis| - lookup_entries = redis.smembers("session:lookup:user:gitlab:#{user.id}") + Gitlab::Redis::Sessions.with do |redis| + lookup_entries = redis.smembers(lookup_key) - expect(lookup_entries.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) - expect(lookup_entries).not_to include(max_number_of_sessions_plus_one.to_s, max_number_of_sessions_plus_two.to_s) + expect(lookup_entries.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) + expect(lookup_entries).not_to include(max_number_of_sessions_plus_one.to_s, max_number_of_sessions_plus_two.to_s) + end end - end - it 'removes obsolete lookup entries even without active session' do - Gitlab::Redis::SharedState.with do |redis| - redis.sadd( - "session:lookup:user:gitlab:#{user.id}", - "#{max_number_of_sessions_plus_two + 1}" - ) + it 'removes obsolete lookup entries even without active session' do + Gitlab::Redis::Sessions.with do |redis| + redis.sadd(lookup_key, "#{max_number_of_sessions_plus_two + 1}") + end + + ActiveSession.cleanup(user) + + Gitlab::Redis::Sessions.with do |redis| + lookup_entries = redis.smembers(lookup_key) + + expect(lookup_entries.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) + expect(lookup_entries).not_to include( + max_number_of_sessions_plus_one.to_s, + max_number_of_sessions_plus_two.to_s, + (max_number_of_sessions_plus_two + 1).to_s + ) + end end - ActiveSession.cleanup(user) + context 'when the number of active sessions is lower than the limit' do + before do + Gitlab::Redis::Sessions.with do |redis| + ((max_number_of_sessions_plus_two - 4)..max_number_of_sessions_plus_two).each do |number| + redis.del(key_name(user.id, number)) + end + end + end - Gitlab::Redis::SharedState.with do |redis| - lookup_entries = redis.smembers("session:lookup:user:gitlab:#{user.id}") + it 'does not remove active session entries, but removes lookup entries' do + lookup_entries_before_cleanup = Gitlab::Redis::Sessions.with do |redis| + redis.smembers(lookup_key) + end - expect(lookup_entries.count).to eq(ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) - expect(lookup_entries).not_to include( - max_number_of_sessions_plus_one.to_s, - max_number_of_sessions_plus_two.to_s, - (max_number_of_sessions_plus_two + 1).to_s - ) + sessions_before_cleanup = described_class.list(user) + + described_class.cleanup(user) + + Gitlab::Redis::Sessions.with do |redis| + lookup_entries = redis.smembers(lookup_key) + sessions = described_class.list(user) + + expect(sessions.count).to eq(sessions_before_cleanup.count) + expect(lookup_entries.count).to be < lookup_entries_before_cleanup.count + end + end end end - context 'when the number of active sessions is lower than the limit' do + 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| - ((max_number_of_sessions_plus_two - 4)..max_number_of_sessions_plus_two).each do |number| - redis.del("session:user:gitlab:#{user.id}:#{number}") + Gitlab::Redis::Sessions.with do |redis| + (1..max_number_of_sessions_plus_two).each do |number| + redis.set( + key_name(user.id, number), + dump_session(ActiveSession.new(session_private_id: number.to_s, updated_at: number.days.ago)) + ) + redis.sadd(lookup_key, number.to_s) end end end - it 'does not remove active session entries, but removes lookup entries' do - lookup_entries_before_cleanup = Gitlab::Redis::SharedState.with do |redis| - redis.smembers("session:lookup:user:gitlab:#{user.id}") - end + it 'removes obsolete active sessions entries' do + described_class.cleanup(user) - sessions_before_cleanup = Gitlab::Redis::SharedState.with do |redis| - redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a - end + Gitlab::Redis::Sessions.with do |redis| + sessions = described_class.list(user) - ActiveSession.cleanup(user) - - Gitlab::Redis::SharedState.with do |redis| - lookup_entries = redis.smembers("session:lookup:user:gitlab:#{user.id}") - sessions = redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a - expect(sessions.count).to eq(sessions_before_cleanup.count) - expect(lookup_entries.count).to be < lookup_entries_before_cleanup.count + expect(sessions.count).to eq(described_class::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS) + expect(sessions).not_to include( + key_name(user.id, max_number_of_sessions_plus_one), + key_name(user.id, max_number_of_sessions_plus_two) + ) end 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 } + context 'with legacy sessions' do + let(:session_key) { described_class.key_name_v1(user.id, current_session_id) } + + def key_name(user_id, session_id) + described_class.key_name_v1(user_id, session_id) + end + + def dump_session(session) + Marshal.dump(session) + end + + it_behaves_like 'cleaning up' + end + + context 'with new sessions' do + let(:session_key) { described_class.key_name(user.id, current_session_id) } + + def key_name(user_id, session_id) + described_class.key_name(user_id, session_id) + end + + def dump_session(session) + session.dump + end + + it_behaves_like 'cleaning up' + end + end + + describe '.cleaned_up_lookup_entries' do + before do + stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5) + end + + shared_examples 'cleaning up lookup entries' do + let(:current_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' } + let(:active_count) { 3 } before do Gitlab::Redis::SharedState.with do |redis| - (1..max_number_of_sessions_plus_two).each do |number| + active_count.times 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}" + key_name(user.id, number), + dump_session(ActiveSession.new(session_id: number.to_s, updated_at: number.days.ago)) ) + + redis.sadd(lookup_key, number.to_s) end + + redis.sadd(lookup_key, (active_count + 1).to_s) + redis.sadd(lookup_key, (active_count + 2).to_s) end end - it 'removes obsolete active sessions entries' do - ActiveSession.cleanup(user) + it 'removes obsolete lookup entries' do + active = Gitlab::Redis::SharedState.with do |redis| + ActiveSession.cleaned_up_lookup_entries(redis, user) + end + + expect(active.count).to eq(active_count) Gitlab::Redis::SharedState.with do |redis| - sessions = redis.scan_each(match: "session:user:gitlab:#{user.id}:*").to_a + lookup_entries = redis.smembers(lookup_key) - 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}")) + expect(lookup_entries.count).to eq(active_count) + expect(lookup_entries).not_to include( + (active_count + 1).to_s, + (active_count + 2).to_s + ) end end + + it 'reports the removed entries' do + removed = [] + Gitlab::Redis::SharedState.with do |redis| + ActiveSession.cleaned_up_lookup_entries(redis, user, removed) + end + + expect(removed.count).to eq(2) + end + end + + context 'with legacy sessions' do + let(:session_key) { described_class.key_name_v1(user.id, current_session_id) } + + def key_name(user_id, session_id) + described_class.key_name_v1(user_id, session_id) + end + + def dump_session(session) + Marshal.dump(session) + end + + it_behaves_like 'cleaning up lookup entries' + end + + context 'with new sessions' do + let(:session_key) { described_class.key_name(user.id, current_session_id) } + + def key_name(user_id, session_id) + described_class.key_name(user_id, session_id) + end + + def dump_session(session) + session.dump + end + + it_behaves_like 'cleaning up lookup entries' end end end |