max_number_of_devices should be used in a new session as well

[MaicolBen]

Resolves #1107
Resolves #637

[zachfeldman]

Looks good to me.

[MaicolBen]

Wait a minute, I removed the code and the test is still working...

[MaicolBen]

The code was correct but the test doesn't, this happens when change_headers_on_each_request is false, so I missed that in the test

[Evan-M]

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

[MaicolBen]

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?

[Evan-M]

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?

[MaicolBen]

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

[Evan-M]

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.

[MaicolBen]

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

[Evan-M]

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.

[Evan-M]

@MaicolBen: not sure if it will help here, but in my fork to get multi-auth working (#990), I refactored some of the test code in test/controllers/demo_user_controller_test.rb#L506 that tested the max number of client tokens.

From diff of test/controllers/demo_user_controller_test.rb in commit 3e46396:

describe 'maximum concurrent devices per user' do
  before do
    @max_devices = DeviseTokenAuth.max_number_of_devices
  end

  it 'should limit the maximum number of concurrent devices' do
    # increment the number of devices until the maximum is exceeded
    1.upto(@max_devices + 1).each do |n|
      assert_equal [n, @max_devices].min, @resource.reload.tokens.keys.length
      @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(@max_devices).each { @resource.create_new_auth_token }

    # get the oldest token
    oldest_token, _ = @resource.reload.tokens \
                        .min_by { |cid, v| v[:expiry] || v["expiry"] }

    # create another token, thereby dropping the oldest token
    @resource.create_new_auth_token

    assert_not_includes @resource.reload.tokens.keys, oldest_token
  end
end

I refactored the code, in part, because it seemed like testing expired token deletion should not be in the describe block that tested devise_mappings, let alone while testing standard_devise_support. Perhaps someone confused max_number_of_devices and devise? Additionally, I felt like the test in question hardly adequately tested the intended behavior.

Refer to PR 990#discussion_r163415486 for more context.

One thing of note regarding commit 3e46396, the line removed from diff of app/controllers/devise_token_auth/concerns/set_user_by_token.rb in commit 3e46396 was
added back to the test on the addition on line 425 test/controllers/demo_user_controller_test.rb in commit 3e46396:

...
@resource.create_new_auth_token
...

Additionally, I further refactored this code in commit 40c2e7d6, to set DeviseTokenAuth.max_number_of_devices = 5 and speed up the tests!

[MaicolBen]

Fixed test, @Evan-M will be making another PR improving clean_old_tokens, thank you for your comments!

[Evan-M]

@MaicolBen: I checked out the max_number_of_devices_in_new_session branch from your forked repo and added my code on top of your PR. So #1115 I think includes this PR as well, right?

You could either:
(1) merge #1115 instead of this one since it is included, or
(2) merge this PR (#1109), and I believe #1115 will update accordingly (since all the commit hashes should match up). Otherwise I can re-issue PR #1115 against master once #1109 is merged.

[MaicolBen]

@Evan-M You can merge this PR as it has Zach's approve, otherwise we can close this one in favor of #1115

[Evan-M]

@MaicolBen I've only got Contributor permissions (not Collaborator) so unfortunately I cannot merge PRs.

[MaicolBen]

I didn't know, sorry, can you approve?