feat: enforce token lifespan limit when cleaning old tokens (#1657)

Author: leightjohnson93

app/models/devise_token_auth/concerns/user.rb

@@ -258,17 +258,25 @@ def max_client_tokens_exceeded?
   end
 
   def clean_old_tokens
-    if tokens.present? && max_client_tokens_exceeded?
-      # Using Enumerable#sort_by on a Hash will typecast it into an associative
-      #   Array (i.e. an Array of key-value Array pairs). However, since Hashes
-      #   have an internal order in Ruby 1.9+, the resulting sorted associative
-      #   Array can be converted back into a Hash, while maintaining the sorted
-      #   order.
-      self.tokens = tokens.sort_by { |_cid, v| v[:expiry] || v['expiry'] }.to_h
-
-      # Since the tokens are sorted by expiry, shift the oldest client token
-      #   off the Hash until it no longer exceeds the maximum number of clients
-      tokens.shift while max_client_tokens_exceeded?
+    return if tokens.blank? || !max_client_tokens_exceeded?
+
+    # First, remove any tokens with expiry greater than current max allowed lifespan
+    #   this handles the case where token lifespan was reduced and old tokens exist
+    max_lifespan_expiry = Time.now.to_i + DeviseTokenAuth.token_lifespan.to_i
+    tokens_to_keep = tokens.select do |_cid, v|
+      expiry = (v[:expiry] || v['expiry']).to_i
+      expiry <= max_lifespan_expiry
     end
+
+    # Using Enumerable#sort_by on a Hash will typecast it into an associative
+    #   Array (i.e. an Array of key-value Array pairs). However, since Hashes
+    #   have an internal order in Ruby 1.9+, the resulting sorted associative
+    #   Array can be converted back into a Hash, while maintaining the sorted
+    #   order.
+    self.tokens = tokens_to_keep.sort_by { |_cid, v| v[:expiry] || v['expiry'] }.to_h
+
+    # Since the tokens are sorted by expiry, shift the oldest client token
+    #   off the Hash until it no longer exceeds the maximum number of clients
+    tokens.shift while max_client_tokens_exceeded?
   end
 end

test/models/user_test.rb

@@ -127,4 +127,98 @@ class UserTest < ActiveSupport::TestCase
       end
     end
   end
+
+  describe 'clean_old_tokens' do
+    before do
+      @resource = create(:user, :confirmed)
+      @token_lifespan = DeviseTokenAuth.token_lifespan
+      @max_client_count = DeviseTokenAuth.max_number_of_devices
+      DeviseTokenAuth.max_number_of_devices = 2
+      DeviseTokenAuth.token_lifespan = 1.week
+    end
+
+    after do
+      DeviseTokenAuth.token_lifespan = @token_lifespan
+      DeviseTokenAuth.max_number_of_devices = @max_client_count
+    end
+
+    test 'removes tokens with expiry beyond the maximum lifespan' do
+      # Create tokens with different expiry times
+      current_time = Time.now.to_i
+
+      max_lifespan = current_time + DeviseTokenAuth.token_lifespan.to_i
+
+      # Valid token within lifespan
+      @resource.tokens['valid_client'] = {
+        'token' => 'valid_token',
+        'expiry' => current_time + 1.day.to_i
+      }
+
+      # Token exactly at max lifespan (should be kept)
+      @resource.tokens['edge_client'] = {
+        'token' => 'edge_token',
+        'expiry' => max_lifespan
+      }
+
+      # Token beyond max lifespan (should be removed)
+      @resource.tokens['expired_client'] = {
+        'token' => 'expired_token',
+        'expiry' => max_lifespan + 1.day.to_i
+      }
+
+      # Call the method under test
+      @resource.send(:clean_old_tokens)
+
+      # Assert that tokens beyond lifespan were removed
+      assert @resource.tokens.key?('valid_client'), 'Valid token should be kept'
+      assert @resource.tokens.key?('edge_client'), 'Edge case token at max lifespan should be kept'
+      refute @resource.tokens.key?('expired_client'), 'Token beyond max lifespan should be removed'
+    end
+
+    test 'handles token lifespan reduction when creating token' do
+      # Setup: Create the maximum allowed number of tokens with a longer lifespan
+      DeviseTokenAuth.token_lifespan = 2.weeks
+      DeviseTokenAuth.max_number_of_devices = 3
+
+      # Create tokens at different times but all within the initial long lifespan
+      @resource.tokens = {}
+      @resource.tokens['client_1'] = {
+        'token' => 'token_1',
+        'expiry' => Time.now.to_i + 12.days.to_i
+      }
+
+      @resource.tokens['client_2'] = {
+        'token' => 'token_2',
+        'expiry' => Time.now.to_i + 10.days.to_i
+      }
+
+      @resource.tokens['client_3'] = {
+        'token' => 'token_3',
+        'expiry' => Time.now.to_i + 5.days.to_i
+      }
+
+      # We've reached the maximum number of devices/tokens
+      assert_equal 3, @resource.tokens.length
+
+      # Now reduce token lifespan - simulating a config change
+      DeviseTokenAuth.token_lifespan = 1.week
+
+      # Create a new token which should trigger clean_old_tokens
+      new_auth_headers = @resource.create_new_auth_token
+      new_client = new_auth_headers['client']
+
+      # The new token should exist
+      assert @resource.tokens.key?(new_client), 'New token should exist'
+
+      # Tokens exceeding the new reduced lifespan should be removed
+      refute @resource.tokens.key?('client_1'), 'Token with expiry > new lifespan should be removed'
+      refute @resource.tokens.key?('client_2'), 'Token with expiry > new lifespan should be removed'
+
+      # Token within new lifespan should be kept
+      assert @resource.tokens.key?('client_3'), 'Token within new reduced lifespan should be kept'
+
+      # We should have exactly 2 tokens: the new one and client_3
+      assert_equal 2, @resource.tokens.length
+    end
+  end
 end