Skip to content

max_number_of_devices should be used in a new session as well #1109

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 1 commit into from
Mar 20, 2018
Merged

max_number_of_devices should be used in a new session as well #1109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 9 additions & 7 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 @@ -98,6 +98,8 @@ def create_token(client_id: nil, token: nil, expiry: nil, **token_extras)
expiry: expiry
}.merge!(token_extras)

clean_old_tokens

[client_id, token, expiry]
end

Expand Down Expand Up @@ -196,25 +198,19 @@ def build_auth_header(token, client_id='default')

def update_auth_header(token, client_id='default')
headers = build_auth_header(token, client_id)
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)
end

clean_old_tokens
save!

headers
end


def build_auth_url(base_url, args)
args[:uid] = uid
args[:expiry] = tokens[args[:client_id]]['expiry']

DeviseTokenAuth::Url.generate(base_url, args)
end


def extend_batch_buffer(token, client_id)
self.tokens[client_id]['updated_at'] = Time.now
update_auth_header(token, client_id)
Expand Down Expand Up @@ -257,4 +253,10 @@ def remove_tokens_after_password_reset
end
end

def clean_old_tokens
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this code technically O(n2)? (The Enumberable#min_by call is inside a while loop).

Even though Enumberable#min_by returns a single key/value pair, it still iterates over the entire collection every time it's called.

I would think that it is more efficient to iterate over the collection once, sorting the key/value pairs by :expiry of the value, and then shift the oldest key/values from tokens in a while loop.

  ...
    protected
    ...
    def max_client_tokens_exceeded?
      tokens.length > DeviseTokenAuth.max_number_of_devices
    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?
      end
    end
  ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change breaks randomically the test that you mentioned: test/controllers/demo_user_controller_test.rb#L506. I tried for a day fixing it but it's quite weird (and hard to debug with minitest). If we merge the PR without this change, can you create a new one and try to fix it with your comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, no problem. I can submit a second PR. Although, I think I saw random test failures on travis-ci before I even made my comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but they weren't random, they were my errors, I haven't updated the PR yet

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the debugging a challenge because the Minitest::Reporters::ProgressReporter leaves out line numbers? I've definitely been challenged with debugging failing tests while using that reporter. While I prefer the ProgressReporter, I find that the default minitest reporter provides much more useful debugging info.

I usually just temporarily comment out line 27 of the test_helper.rb while running these tests. YMMV.

I've been meaning to look into the the issue and submit a PR to kern/minitest-reporters with a fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the tip! It doesn't throw a backtrace either 😞

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)
end
end
end
27 changes: 27 additions & 0 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 @@ -72,6 +72,33 @@ class DeviseTokenAuth::SessionsControllerTest < ActionController::TestCase
assert_equal '0.0.0.0', @new_last_sign_in_ip
end
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't you assert that the correct tokens (i.e. the oldest tokens) are deleted here?

E.g.:
Given that the max_number_of_devices = 5, and there are 5 existing tokens,
When an additional token is added, the oldest token is dropped and the new one is present.

describe "with multiple clients and headers don't change in each request" do
before do
DeviseTokenAuth.max_number_of_devices = 1
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
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
end

after do
DeviseTokenAuth.max_number_of_devices = 10
DeviseTokenAuth.change_headers_on_each_request = true
end
end
end

describe 'get sign_in is not supported' do
Expand Down