Skip to content

Max number of devices in new session #1115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 23, 2018
Merged

Max number of devices in new session #1115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a9b25dc
Refactor tests for old token removal when max clients are exceeded.
Evan-M Mar 19, 2018
065e812
Remove superfluous call to `#create_new_auth_token`.
Evan-M Mar 19, 2018
676c77a
Simplify complex conditionals.
Evan-M Mar 19, 2018
04afcda
Refactor `#clean_old_tokens` to reduce computational complexity.
Evan-M Mar 19, 2018
3bda3d5
Apply small changes per the PR review.
Evan-M Mar 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/controllers/devise_token_auth/concerns/set_user_by_token.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update_auth_header should use this, but if you say so, we can remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather confusingly, there are two methods with that name.
the model concern:

devise_token_auth/app/models/devise_token_auth/concerns/user.rb

Lines 199 to 205 in 38d27cf

def update_auth_header(token, client_id='default')
headers = build_auth_header(token, client_id)
clean_old_tokens
save!
headers
end

and the controller concern:

devise_token_auth/app/controllers/devise_token_auth/concerns/set_user_by_token.rb

Lines 96 to 154 in 38d27cf

def update_auth_header
# cannot save object if model has invalid params
return unless defined?(@resource) && @resource && @resource.valid? && @client_id
# Generate new client_id with existing authentication
@client_id = nil unless @used_auth_by_token
if @used_auth_by_token && !DeviseTokenAuth.change_headers_on_each_request
# should not append auth header if @resource related token was
# cleared by sign out in the meantime
return if @resource.reload.tokens[@client_id].nil?
auth_header = @resource.build_auth_header(@token, @client_id)
# update the response header
response.headers.merge!(auth_header)
else
ensure_pristine_resource do
# Lock the user record during any auth_header updates to ensure
# we don't have write contention from multiple threads
@resource.with_lock do
# should not append auth header if @resource related token was
# cleared by sign out in the meantime
return if @used_auth_by_token && @resource.tokens[@client_id].nil?
# determine batch request status after request processing, in case
# another processes has updated it during that processing
@is_batch_request = is_batch_request?(@resource, @client_id)
auth_header = {}
# extend expiration of batch buffer to account for the duration of
# this request
if @is_batch_request
auth_header = @resource.extend_batch_buffer(@token, @client_id)
# Do not return token for batch requests to avoid invalidated
# tokens returned to the client in case of race conditions.
# Use a blank string for the header to still be present and
# being passed in a XHR response in case of
# 304 Not Modified responses.
auth_header[DeviseTokenAuth.headers_names[:"access-token"]] = ' '
auth_header[DeviseTokenAuth.headers_names[:"expiry"]] = ' '
# update Authorization response header with new token
else
auth_header = @resource.create_new_auth_token(@client_id)
end
# update the response header
response.headers.merge!(auth_header)
end # end lock
end # end ensure_pristine_resource
end
end

I'll assume you mean the one in the controller concern (the same file as this change)...

Regardless, I'm not sure I understand what you mean. This is inside a conditional block for the use case of when DeviseTokenAuth.enable_standard_devise_support = true and the user has already authenticated with Devise in the session via Warden. There is no need for an auth token in that case, so why would we create one here?

end
end

Expand Down
24 changes: 18 additions & 6 deletions app/models/devise_token_auth/concerns/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
53 changes: 44 additions & 9 deletions test/controllers/demo_user_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
49 changes: 36 additions & 13 deletions test/controllers/devise_token_auth/sessions_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for , _

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is purely convention, although I used it incorrectly...

https://github.com/bbatsov/ruby-style-guide#trailing-underscore-variables

It should either be:

oldest_token, = \
  @existing_user.reload.tokens \
    .min_by { |cid, v| v[:expiry] || v["expiry"] } # => [ <"client_id">, {...<token>...} ]

or

oldest_client_id, _oldest_token = \ 
  @existing_user.reload.tokens \
    .min_by { |cid, v| v[:expiry] || v["expiry"] } # => [ <"client_id">, {...<token>...} ]

If you really truly think I should remove the , _ entirely, I will add a comment that an array is returned by #min_by.

On a different note, I've been itching to create an issue to get rubocop setup on this repo, so that stylistic issues like this are a non-issue.

Copy link
Collaborator

@MaicolBen MaicolBen Mar 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, thank you for the long explanation, and if you enable rubocop, there will be tons of issues, that's why we have Code Climate that analyzes only the differences. I will create an issue for it 😄

EDIT: the issue is #1124

.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
Expand Down