Max number of devices in new session (#1115)

* Refactor tests for old token removal when max clients are exceeded.

* Remove superfluous call to `#create_new_auth_token`.

 * There was prior discussion around removing this line of code, inside
   #990.
   See: #990 (comment)

 * While line in question _is_ superfluous, removing it was blocked by a
   bad test.  This test was corrected in the previous commit (9ebc5bd).

* Simplify complex conditionals.

* Refactor `#clean_old_tokens` to reduce computational complexity.

 * Previous version featured an `Enumerable#min_by` loop _inside_ a
   `while` loop, resulting in `O(n^2)` complexity.

 * Instead, break things into two separate loops, and skip altogether if
   they aren't even necessary.

* Apply small changes per the PR review.

Author: Evan-M

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

@@ -60,7 +60,9 @@ def set_user_by_token(mapping=nil)
       if devise_warden_user && devise_warden_user.tokens[@client_id].nil?
         @used_auth_by_token = false
         @resource = devise_warden_user
-        @resource.create_new_auth_token
+        # REVIEW: The following line _should_ be safe to remove;
+        #  the generated token does not get used anywhere.
+        # @resource.create_new_auth_token
       end
     end
 

app/models/devise_token_auth/concerns/user.rb

@@ -244,19 +244,31 @@ def destroy_expired_tokens
   end
 
   def remove_tokens_after_password_reset
-    should_remove_old_tokens = DeviseTokenAuth.remove_tokens_after_password_reset &&
-                               encrypted_password_changed? && tokens && tokens.many?
+    return unless encrypted_password_changed? &&
+                DeviseTokenAuth.remove_tokens_after_password_reset
 
-    if should_remove_old_tokens
+    if tokens.present? && tokens.many?
       client_id, token_data = tokens.max_by { |cid, v| v[:expiry] || v["expiry"] }
       self.tokens = {client_id => token_data}
     end
   end
 
+  def max_client_tokens_exceeded?
+    tokens.length > DeviseTokenAuth.max_number_of_devices
+  end
+
   def clean_old_tokens
-    while tokens.length > 0 && DeviseTokenAuth.max_number_of_devices < tokens.length
-      oldest_client_id, _tk = tokens.min_by { |_cid, v| v[:expiry] || v["expiry"] }
-      tokens.delete(oldest_client_id)
+    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?
     end
   end
 end

test/controllers/demo_user_controller_test.rb

@@ -407,6 +407,50 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
           DeviseTokenAuth.headers_names[:'access-token'] = 'access-token'
         end
       end
+
+      describe 'maximum concurrent devices per user' do
+        before do
+          # Set the max_number_of_devices to a lower number
+          #  to expedite tests! (Default is 10)
+          DeviseTokenAuth.max_number_of_devices = 5
+        end
+
+        it 'should limit the maximum number of concurrent devices' do
+          # increment the number of devices until the maximum is exceeded
+          1.upto(DeviseTokenAuth.max_number_of_devices + 1).each do |n|
+
+            assert_equal(
+              [n, DeviseTokenAuth.max_number_of_devices].min,
+              @resource.reload.tokens.length
+            )
+
+            # Add a new device (and token) ahead of the next iteration
+            @resource.create_new_auth_token
+
+          end
+        end
+
+        it 'should drop the oldest token when the maximum number of devices is exceeded' do
+          # create the maximum number of tokens
+          1.upto(DeviseTokenAuth.max_number_of_devices).each do
+            @resource.create_new_auth_token
+          end
+
+          # get the oldest token client_id
+          oldest_client_id, = @resource.reload.tokens.min_by do |cid, v|
+                                v[:expiry] || v["expiry"]
+                              end # => [ 'CLIENT_ID', {token: ...} ]
+
+          # create another token, thereby dropping the oldest token
+          @resource.create_new_auth_token
+
+          assert_not_includes @resource.reload.tokens.keys, oldest_client_id
+        end
+
+        after do
+          DeviseTokenAuth.max_number_of_devices = 10
+        end
+      end
     end
 
     describe 'bypass_sign_in' do
@@ -503,17 +547,8 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
             refute_equal @resource, @controller.current_mang
           end
 
-          it 'should increase the number of tokens by a factor of 2 up to 11' do
-            @first_token = @resource.tokens.keys.first
 
-            DeviseTokenAuth.max_number_of_devices = 11
-            (1..10).each do |n|
-              assert_equal [11, 2 * n].min, @resource.reload.tokens.keys.length
-              get '/demo/members_only', params: {}, headers: nil
-            end
 
-            assert_not_includes @resource.reload.tokens.keys, @first_token
-          end
         end
 
         it 'should return success status' do

test/controllers/devise_token_auth/sessions_controller_test.rb

@@ -75,23 +75,46 @@ class DeviseTokenAuth::SessionsControllerTest < ActionController::TestCase
 
         describe "with multiple clients and headers don't change in each request" do
           before do
-            DeviseTokenAuth.max_number_of_devices = 1
+            # Set the max_number_of_devices to a lower number
+            #  to expedite tests! (Default is 10)
+            DeviseTokenAuth.max_number_of_devices = 2
             DeviseTokenAuth.change_headers_on_each_request = false
-            @tokens = []
-            (1..3).each do |n|
-              post :create,
-                   params: {
-                     email: @existing_user.email,
-                     password: 'secret123'
-                   }
-              @tokens << @existing_user.reload.tokens
+
+            @user_session_params = {
+              email: @existing_user.email,
+              password: 'secret123'
+            }
+          end
+
+          test 'should limit the maximum number of concurrent devices' do
+            # increment the number of devices until the maximum is exceeded
+            1.upto(DeviseTokenAuth.max_number_of_devices + 1).each do |n|
+              initial_tokens = @existing_user.reload.tokens
+
+              assert_equal(
+                [n, DeviseTokenAuth.max_number_of_devices].min,
+                @existing_user.reload.tokens.length
+              )
+
+              # Already have the max number of devices
+              post :create, params: @user_session_params
+
+              # A session for a new device maintains the max number of concurrent devices
+              refute_equal initial_tokens, @existing_user.reload.tokens
             end
           end
 
-          test 'should delete old tokens' do
-            current_tokens = @existing_user.reload.tokens
-            assert_equal 1, current_tokens.count
-            assert_equal @tokens.pop.keys.first, current_tokens.keys.first
+          test 'should drop old tokens when max number of devices is exceeded' do
+            1.upto(DeviseTokenAuth.max_number_of_devices).each do |n|
+              post :create, params: @user_session_params
+            end
+
+            oldest_token, _ = @existing_user.reload.tokens \
+                                .min_by { |cid, v| v[:expiry] || v["expiry"] }
+
+            post :create, params: @user_session_params
+
+            assert_not_includes @existing_user.reload.tokens.keys, oldest_token
           end
 
           after do