Max number of devices in new session #1115

Evan-M · 2018-03-19T23:03:15Z

Additional refactoring, per discussion in #1109.

MaicolBen · 2018-03-20T20:05:50Z

@Evan-M You need to rebase

* There was prior discussion around removing this line of code, inside lynndylanhurley#990. See: lynndylanhurley#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).

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

Evan-M · 2018-03-22T20:51:02Z

@MaicolBen rebased.

MaicolBen · 2018-03-22T20:58:47Z

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

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


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

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?

MaicolBen · 2018-03-22T20:59:36Z

test/controllers/demo_user_controller_test.rb

+          #  to expedite tests! (Default is 10)
+          DeviseTokenAuth.max_number_of_devices = 5
+
+          # @max_devices = DeviseTokenAuth.max_number_of_devices


remove the commented line, or is this documentation?

It is cruft and I didn't mean to leave it in the commit! 😆

MaicolBen · 2018-03-22T20:59:54Z

test/controllers/demo_user_controller_test.rb

+            # Add a new device (and token) ahead of the next iteration
+            @resource.create_new_auth_token
+
+            # refute_equal initial_tokens, @resource.reload.tokens


why this refute is commented?

I actually meant to remove it. While the test still passes with it uncommented (along with line 423 above), it really doesn't add anything to the test here. We've already asserted that the number of concurrent tokens/devices is correct; if anything the addition of this test will make the test more brittle.

MaicolBen · 2018-03-22T21:00:28Z

test/controllers/devise_token_auth/sessions_controller_test.rb

+              post :create, params: @user_session_params
+            end
+
+            oldest_token, _ = @existing_user.reload.tokens \


no need for , _

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.

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

zachfeldman · 2018-03-23T13:46:00Z

Tests pass. Merging away!

Evan-M mentioned this pull request Mar 19, 2018

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

Merged

Evan-M added 4 commits March 22, 2018 13:39

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

a9b25dc

Simplify complex conditionals.

676c77a

Refactor #clean_old_tokens to reduce computational complexity.

04afcda

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

Evan-M force-pushed the max_number_of_devices_in_new_session branch from 3b63cfe to 04afcda Compare March 22, 2018 20:43

MaicolBen reviewed Mar 22, 2018

View reviewed changes

Apply small changes per the PR review.

3bda3d5

MaicolBen approved these changes Mar 23, 2018

View reviewed changes

zachfeldman merged commit a9c7ea6 into lynndylanhurley:master Mar 23, 2018

Evan-M deleted the max_number_of_devices_in_new_session branch March 23, 2018 16:53

Evan-M mentioned this pull request Mar 26, 2018

Rubocop Fixes #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Max number of devices in new session #1115

Max number of devices in new session #1115

Evan-M commented Mar 19, 2018

MaicolBen commented Mar 20, 2018

Evan-M commented Mar 22, 2018

MaicolBen Mar 22, 2018

Evan-M Mar 22, 2018

MaicolBen Mar 22, 2018

Evan-M Mar 22, 2018

MaicolBen Mar 22, 2018

Evan-M Mar 22, 2018

MaicolBen Mar 22, 2018

Evan-M Mar 22, 2018

MaicolBen Mar 22, 2018 •

edited

Loading

zachfeldman commented Mar 23, 2018

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

	headers
	end

	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

Max number of devices in new session #1115

Max number of devices in new session #1115

Conversation

Evan-M commented Mar 19, 2018

MaicolBen commented Mar 20, 2018

Evan-M commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaicolBen Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

zachfeldman commented Mar 23, 2018

MaicolBen Mar 22, 2018 •

edited

Loading