Merge branch '31611-limit-the-number-of-stored-sessions-per-user' into 'master'

Resolve "Limit the number of stored sessions per user"

Closes #31611

See merge request gitlab-org/gitlab!19325

app/models/active_session.rb

@@ -4,6 +4,7 @@ class ActiveSession
   include ActiveModel::Model
 
   SESSION_BATCH_SIZE = 200
+  ALLOWED_NUMBER_OF_ACTIVE_SESSIONS = 100
 
   attr_accessor :created_at, :updated_at,
     :session_id, :ip_address,
@@ -65,21 +66,22 @@ class ActiveSession
 
   def self.destroy(user, session_id)
     Gitlab::Redis::SharedState.with do |redis|
-      redis.srem(lookup_key_name(user.id), session_id)
-
-      deleted_keys = redis.del(key_name(user.id, session_id))
-
-      # only allow deleting the devise session if we could actually find a
-      # related active session. this prevents another user from deleting
-      # someone else's session.
-      if deleted_keys > 0
-        redis.del("#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}")
+      destroy_sessions(redis, user, [session_id])
     end
   end
+
+  def self.destroy_sessions(redis, user, session_ids)
+    key_names = session_ids.map {|session_id| key_name(user.id, session_id) }
+    session_names = session_ids.map {|session_id| "#{Gitlab::Redis::SharedState::SESSION_NAMESPACE}:#{session_id}" }
+
+    redis.srem(lookup_key_name(user.id), session_ids)
+    redis.del(key_names)
+    redis.del(session_names)
   end
 
   def self.cleanup(user)
     Gitlab::Redis::SharedState.with do |redis|
+      clean_up_old_sessions(redis, user)
       cleaned_up_lookup_entries(redis, user)
     end
   end
@@ -118,19 +120,39 @@ class ActiveSession
     end
   end
 
-  def self.raw_active_session_entries(session_ids, user_id)
+  def self.raw_active_session_entries(redis, session_ids, user_id)
     return [] if session_ids.empty?
 
-    Gitlab::Redis::SharedState.with do |redis|
     entry_keys = session_ids.map { |session_id| key_name(user_id, session_id) }
 
     redis.mget(entry_keys)
   end
+
+  def self.active_session_entries(session_ids, user_id, redis)
+    return [] if session_ids.empty?
+
+    entry_keys = raw_active_session_entries(redis, session_ids, user_id)
+
+    entry_keys.map do |raw_session|
+      Marshal.load(raw_session) # rubocop:disable Security/MarshalLoad
+    end
+  end
+
+  def self.clean_up_old_sessions(redis, user)
+    session_ids = session_ids_for_user(user.id)
+
+    return if session_ids.count <= ALLOWED_NUMBER_OF_ACTIVE_SESSIONS
+
+    # remove sessions if there are more than ALLOWED_NUMBER_OF_ACTIVE_SESSIONS.
+    sessions = active_session_entries(session_ids, user.id, redis)
+    sessions.sort_by! {|session| session.updated_at }.reverse!
+    sessions = sessions[ALLOWED_NUMBER_OF_ACTIVE_SESSIONS..-1].map { |session| session.session_id }
+    destroy_sessions(redis, user, sessions)
   end
 
   def self.cleaned_up_lookup_entries(redis, user)
     session_ids = session_ids_for_user(user.id)
-    entries = raw_active_session_entries(session_ids, user.id)
+    entries = raw_active_session_entries(redis, session_ids, user.id)
 
     # remove expired keys.
     # only the single key entries are automatically expired by redis, the

changelogs/unreleased/31611-limit-the-number-of-stored-sessions-per-user.yml

+---
+title: Resolve Limit the number of stored sessions per user
+merge_request: 19325
+author:
+type: added

doc/user/profile/active_sessions.md

@@ -18,6 +18,9 @@ review the sessions, and revoke any you don't recognize.
 
 ![Active sessions list](img/active_sessions_list.png)
 
+CAUTION: **Caution:**
+It is currently possible to have 100 active sessions at once. If the number of active sessions exceed 100, the oldest ones will be deleted.
+
 <!-- ## Troubleshooting
 
 Include any troubleshooting steps that you can foresee. If you know beforehand what issues

lib/tasks/gitlab/cleanup.rake

@@ -92,7 +92,7 @@ namespace :gitlab do
               lookup_key_count = redis.scard(key)
 
               session_ids = ActiveSession.session_ids_for_user(user_id)
-              entries = ActiveSession.raw_active_session_entries(session_ids, user_id)
+              entries = ActiveSession.raw_active_session_entries(redis, session_ids, user_id)
               session_ids_and_entries = session_ids.zip(entries)
 
               inactive_session_ids = session_ids_and_entries.map do |session_id, session|

spec/models/active_session_spec.rb

@@ -242,23 +242,13 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
       end
     end
-
-    it 'does not remove the devise session if the active session could not be found' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:gitlab:6919a6f1bb119dd7396fadc38fd18d0d", '')
   end
 
-      other_user = create(:user)
-
-      ActiveSession.destroy(other_user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:gitlab:*").to_a).not_to be_empty
-      end
-    end
+  describe '.cleanup' do
+    before do
+      stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5)
     end
 
-  describe '.cleanup' do
     it 'removes obsolete lookup entries' do
       Gitlab::Redis::SharedState.with do |redis|
         redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
@@ -276,5 +266,47 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     it 'does not bail if there are no lookup entries' do
       ActiveSession.cleanup(user)
     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 }
+
+      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}", 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
+
+      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}")
+
+          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
   end
 end