Skip to content

Commit

Permalink
Fix incorrect assert statements (they should be assert_equal).
Browse files Browse the repository at this point in the history
 - This exposes a bug in the test: placing the `get` request within a before block
   prevents any correct response from secondary requests (w/ forbidden params), and
   causes intermittent test failures.

 - Also exposes a bug in `DTA::Concerns::User#remove_tokens_after_password_reset`:
   Assuming the test is correct, the implementation should sort client/tokens by
   `:updated_at` timestamp rather by token `:expiry`.  This makes sense because
   `:expiry` **could** be `nil`. (If the test were to age client/tokens by `:expiry`
   instead, the original implementation would be correct.)
  • Loading branch information
Evan-M committed Feb 21, 2018
1 parent e25592a commit 49175db
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 16 deletions.
5 changes: 4 additions & 1 deletion app/models/devise_token_auth/concerns/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,10 @@ def remove_tokens_after_password_reset
# Using Enumerable#max_by on a Hash will typecast it into an associative
# Array and return a single key-value Array pair, so convert it back into
# a Hash.
client_id, token_data = tokens.max_by { |_cid, v| v[:expiry] || v["expiry"] }
client_id, token_data = tokens.max_by do |_cid, v|
Time.zone.parse(v[:updated_at] || v['updated_at'])
end

self.tokens = {client_id => token_data}
end

Expand Down
30 changes: 15 additions & 15 deletions test/controllers/demo_user_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -319,39 +319,39 @@ class DemoUserControllerTest < ActionDispatch::IntegrationTest
before do
DeviseTokenAuth.remove_tokens_after_password_reset = true

# adding one more token to simulate another logged in device
# Existing device is older
@old_auth_headers = @auth_headers
@auth_headers = @resource.create_new_auth_token
age_token(@resource, @client_id)
assert @resource.tokens.count > 1
age_token(@resource, @old_auth_headers['client'])

# password changed from new device
@resource.update_attributes(password: 'newsecret123',
password_confirmation: 'newsecret123')
# Add another token to simulate another logged in device
@new_auth_headers = @resource.create_new_auth_token
assert @resource.tokens.many?

get '/demo/members_only',
params: {},
headers: @auth_headers
# Change password from new device
@resource.update_attributes(
password: 'newsecret123',
password_confirmation: 'newsecret123'
)
end

after do
DeviseTokenAuth.remove_tokens_after_password_reset = false
end

it 'should have only one token' do
get '/demo/members_only', params: {}, headers: @new_auth_headers
assert_equal 1, @resource.tokens.count
end

it 'new request should be successful' do
assert 200, response.status
get '/demo/members_only', params: {}, headers: @new_auth_headers
assert_equal 200, response.status
end

describe 'another device should not be able to login' do
it 'should return forbidden status' do
get '/demo/members_only',
params: {},
headers: @old_auth_headers
assert 401, response.status
get '/demo/members_only', params: {}, headers: @old_auth_headers
assert_equal 401, response.status
end
end
end
Expand Down

0 comments on commit 49175db

Please sign in to comment.