From 0f5093736cd5cacebf1221f56c4d13943862badf Mon Sep 17 00:00:00 2001 From: Leight Johnson Date: Tue, 8 Jul 2025 13:04:53 -0600 Subject: [PATCH] feat: enforce token lifespan limit when cleaning old tokens --- app/models/devise_token_auth/concerns/user.rb | 30 +++--- test/models/user_test.rb | 94 +++++++++++++++++++ 2 files changed, 113 insertions(+), 11 deletions(-) diff --git a/app/models/devise_token_auth/concerns/user.rb b/app/models/devise_token_auth/concerns/user.rb index d3133c409..602b3331b 100644 --- a/app/models/devise_token_auth/concerns/user.rb +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 17746b2a6..6d1ebb130 100644 --- a/test/models/user_test.rb +++ b/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