Add method to remove old session entries

Add removing old sessions

Add proper order of sessions

Add more precise specs

Add more precise specs

Remove obsolete code

Add changelog entry

Update specs to use constant instead of fixed numbers

Refactor accessing redis

Refactor active session cleanup

Refactor cleanup method

Add cr remarks

Remove new line

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)
+      destroy_sessions(redis, user, [session_id])
+    end
+  end
 
-      deleted_keys = redis.del(key_name(user.id, session_id))
+  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}" }
 
-      # 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}")
-      end
-    end
+    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) }
+    entry_keys = session_ids.map { |session_id| key_name(user_id, session_id) }
+
+    redis.mget(entry_keys)
+  end
 
-      redis.mget(entry_keys)
+  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
   end
 
   describe '.cleanup' do
+    before do
+      stub_const("ActiveSession::ALLOWED_NUMBER_OF_ACTIVE_SESSIONS", 5)
+    end
+
     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