Skip to content
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

Add support for sending and receiving the auth token via a server cookie #1453

Merged
merged 8 commits into from
Mar 11, 2021
Merged

Add support for sending and receiving the auth token via a server cookie #1453

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5043c7
Add support for sending and receiving the token via a cookie
theblang Jan 4, 2021
d20d24f
tweak config and add to docs
theblang Jan 4, 2021
c623d51
test work and flatten cookie config
theblang Jan 5, 2021
3cdc09b
be a little more defensive
theblang Jan 5, 2021
7592336
allow a Proc being passed the request as a domain config
theblang Jan 6, 2021
536340b
fix bug with mutating the original cookie_attributes config. fix oddi…
theblang Jan 7, 2021
66f8fa9
remove the Proc option since domain is more robust than I realized
theblang Jan 7, 2021
a486ba8
Merge remote-tracking branch 'upstream/master'
theblang Mar 8, 2021
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ test/dummy/.sass-cache
test/dummy/config/application.yml
coverage
.idea
.byebug_history
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drive-by that popped up while debugging a test

.irb_history
.ruby-version
.ruby-gemset
Expand Down
29 changes: 25 additions & 4 deletions 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 @@ -35,11 +35,20 @@ def set_user_by_token(mapping = nil)
access_token_name = DeviseTokenAuth.headers_names[:'access-token']
client_name = DeviseTokenAuth.headers_names[:'client']

# gets values from cookie if configured and present
Copy link
Collaborator

Choose a reason for hiding this comment

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

can all this new cookie code be in a new module like set_user_by_cookie or something like that, so we don't pollute more this big file, you can include the concern/module in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can, my only push back would be that hopefully in the future we'll allow turning off headers (for the security reasons mentioned in point two of the description). In that case, it might feel a bit disjointed to have the header logic in set_user_by_token while the cookie logic is in set_user_by_cookie, when really both logics deal with handling the token. In that future world, would we move the header logic to another concern called set_user_by_headers? If so, wouldn't set_user_by_token just become a skeleton file?

Copy link
Contributor Author

@theblang theblang Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, see my recent realization. That may clean up the amount of noise in this file considerably.

parsed_auth_cookie = {}
if DeviseTokenAuth.cookie_enabled
auth_cookie = request.cookies[DeviseTokenAuth.cookie_name]
if auth_cookie.present?
parsed_auth_cookie = JSON.parse(auth_cookie)
end
end

# parse header for values necessary for authentication
uid = request.headers[uid_name] || params[uid_name]
uid = request.headers[uid_name] || params[uid_name] || parsed_auth_cookie[uid_name]
@token = DeviseTokenAuth::TokenFactory.new unless @token
@token.token ||= request.headers[access_token_name] || params[access_token_name]
@token.client ||= request.headers[client_name] || params[client_name]
@token.token ||= request.headers[access_token_name] || params[access_token_name] || parsed_auth_cookie[access_token_name]
@token.client ||= request.headers[client_name] || params[client_name] || parsed_auth_cookie[client_name]

# client isn't required, set to 'default' if absent
@token.client ||= 'default'
Expand Down Expand Up @@ -101,6 +110,11 @@ def update_auth_header
# update the response header
response.headers.merge!(auth_header)

# set a server cookie if configured
if DeviseTokenAuth.cookie_enabled
cookies[DeviseTokenAuth.cookie_name] = DeviseTokenAuth.cookie_attributes.merge(value: auth_header.to_json)
end

else
unless @resource.reload.valid?
@resource = @resource.class.find(@resource.to_param) # errors remain after reload
Expand All @@ -123,8 +137,15 @@ def refresh_headers
# cleared by sign out in the meantime
return if @used_auth_by_token && @resource.tokens[@token.client].nil?

_auth_header_from_batch_request = auth_header_from_batch_request

# update the response header
response.headers.merge!(auth_header_from_batch_request)
response.headers.merge!(_auth_header_from_batch_request)

# set a server cookie if configured
if DeviseTokenAuth.cookie_enabled
cookies[DeviseTokenAuth.cookie_name] = DeviseTokenAuth.cookie_attributes.merge(value: _auth_header_from_batch_request.to_json)
end
end # end lock
end

Expand Down
10 changes: 10 additions & 0 deletions app/controllers/devise_token_auth/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ def destroy
user.tokens.delete(client)
user.save!

if DeviseTokenAuth.cookie_enabled
if DeviseTokenAuth.cookie_attributes[:domain]
Copy link
Contributor Author

@theblang theblang Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I just realized that we might need to be smarter about determining the domain attribute. We might need to determine it by looking at the request's domain (in which case we'd probably want to allow configuring a whitelist, like we do with redirect_whitelist). This is because the Domain cookie attribute only allows one value, so as the code sits now you wouldn't be able to handle multiple domains being served from the same box.

Copy link

Choose a reason for hiding this comment

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

Make sense, remember to use allowlist instead of whitelist

Copy link
Contributor Author

@theblang theblang Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@apuntovanini Another option is to just support passing to the cookie_attributes[:domain] config either a string, which will work for most people, or a proc / lambda, which could accept the request and do whatever logic (in our case, return the domain of the request so long as it matches the allow list).

Copy link
Contributor Author

@theblang theblang Jan 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Here are the changes for that second option I mentioned. And our project's config looks like:

    config.cookie_attributes = {
        domain: lambda { |request| request.domain if valid_cookie_domains.include?(request.domain) },
        secure: Rails.env.production?,
        httponly: true,
        same_site: 'Strict',
        expires: token_lifespan.from_now,
        encrypt: true
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 I just saw this in the in the Rails docs:

To support multiple domains, provide an array, and the first domain matching request.host will be used. Make sure to specify the :domain option with :all or Array again when deleting cookies.

So I can totally gut this Proc logic now, and instead just pass along either a string or Array from the config to the cookie call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this commit. So much cleaner! I wish I had realized that earlier, lol.

# If a cookie is set with a domain specified then it must be deleted with that domain specified
# See https://stackoverflow.com/a/6244724/1747491
cookies.delete(DeviseTokenAuth.cookie_name, domain: DeviseTokenAuth.cookie_attributes[:domain])
else
cookies.delete(DeviseTokenAuth.cookie_name)
end
end

yield user if block_given?

render_destroy_success
Expand Down
3 changes: 3 additions & 0 deletions docs/config/initialization.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ The following settings are available for configuration in `config/initializers/d
| **`enable_standard_devise_support`** (`false`) | By default, only Bearer Token authentication is implemented out of the box. If, however, you wish to integrate with legacy Devise authentication, you can do so by enabling this flag. NOTE: This feature is highly experimental! |
| **`remove_tokens_after_password_reset`** (`false`) | By default, old tokens are not invalidated when password is changed. Enable this option if you want to make passwords updates to logout other devices. |
| **`default_callbacks`** (`true`) | By default User model will include the `DeviseTokenAuth::Concerns::UserOmniauthCallbacks` concern, which has `email`, `uid` validations & `uid` synchronization callbacks. |
| **`cookie_enabled`** (`false`) | Specifies if DeviseTokenAuth should send and receive the auth token in a cookie.
| **`cookie_name`** (`"auth_cookie"`) | Sets the name of the cookie containing the auth token.
| **`cookie_attributes`** (`{}`) | Sets attributes on the cookie containing the auth token (ex. `domain`, `secure`, `httponly`, `same_site`, `expires`, `encrypt`). See [this MDN doc](https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies) for additional documentation on cookie attributes.
| **`bypass_sign_in`** (`true`) | By default DeviseTokenAuth will not check user's `#active_for_authentication?` which includes confirmation check on each call (it will do it only on sign in). If you want it to be validated on each request (for example, to be able to deactivate logged in users on the fly), set it to false. |
| **`send_confirmation_email`** (`false`) | By default DeviseTokenAuth will not send confirmation email, even when including devise confirmable module. If you want to use devise confirmable module and send email, set it to true. (This is a setting for compatibility) |
| **`require_client_password_reset_token`** (`false`) | By default, the password-reset confirmation link redirects to the client with valid session credentials as querystring params. With this option enabled, the redirect will NOT include the valid session credentials. Instead the redirect will include a password_reset_token querystring param that can be used to reset the users password. Once the user has reset their password, the password-reset success response headers will contain valid session credentials. |
Expand Down
6 changes: 6 additions & 0 deletions lib/devise_token_auth/engine.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ class Engine < ::Rails::Engine
:remove_tokens_after_password_reset,
:default_callbacks,
:headers_names,
:cookie_enabled,
:cookie_name,
:cookie_attributes,
:bypass_sign_in,
:send_confirmation_email,
:require_client_password_reset_token
Expand All @@ -47,6 +50,9 @@ class Engine < ::Rails::Engine
'expiry': 'expiry',
'uid': 'uid',
'token-type': 'token-type' }
self.cookie_enabled = false
self.cookie_name = 'auth_cookie'
self.cookie_attributes = {}
self.bypass_sign_in = true
self.send_confirmation_email = false
self.require_client_password_reset_token = false
Expand Down
43 changes: 28 additions & 15 deletions test/controllers/devise_token_auth/registrations_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@

class DeviseTokenAuth::RegistrationsControllerTest < ActionDispatch::IntegrationTest
describe DeviseTokenAuth::RegistrationsController do

def mock_registration_params
{
email: Faker::Internet.email,
password: 'secret123',
password_confirmation: 'secret123',
confirm_success_url: Faker::Internet.url,
unpermitted_param: '(x_x)'
}
end

describe 'Validate non-empty body' do
before do
# need to post empty data
Expand Down Expand Up @@ -41,13 +52,7 @@ class DeviseTokenAuth::RegistrationsControllerTest < ActionDispatch::Integration
@mails_sent = ActionMailer::Base.deliveries.count

post '/auth',
params: {
email: Faker::Internet.email,
password: 'secret123',
password_confirmation: 'secret123',
confirm_success_url: Faker::Internet.url,
unpermitted_param: '(x_x)'
}
params: mock_registration_params

@resource = assigns(:resource)
@data = JSON.parse(response.body)
Expand Down Expand Up @@ -87,24 +92,32 @@ class DeviseTokenAuth::RegistrationsControllerTest < ActionDispatch::Integration
before do
@original_duration = Devise.allow_unconfirmed_access_for
Devise.allow_unconfirmed_access_for = nil
post '/auth',
params: {
email: Faker::Internet.email,
password: 'secret123',
password_confirmation: 'secret123',
confirm_success_url: Faker::Internet.url,
unpermitted_param: '(x_x)'
}
end

test 'auth headers were returned in response' do
post '/auth', params: mock_registration_params
assert response.headers['access-token']
assert response.headers['token-type']
assert response.headers['client']
assert response.headers['expiry']
assert response.headers['uid']
end

describe 'using auth cookie' do
before do
DeviseTokenAuth.cookie_enabled = true
end

test 'auth cookie was returned in response' do
post '/auth', params: mock_registration_params
assert response.cookies[DeviseTokenAuth.cookie_name]
end

after do
DeviseTokenAuth.cookie_enabled = false
end
end

after do
Devise.allow_unconfirmed_access_for = @original_duration
end
Expand Down
49 changes: 39 additions & 10 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 @@ -17,11 +17,12 @@ class DeviseTokenAuth::SessionsControllerTest < ActionController::TestCase

describe 'success' do
before do
post :create,
params: {
email: @existing_user.email,
password: @existing_user.password
}
@user_session_params = {
email: @existing_user.email,
password: @existing_user.password
}

post :create, params: @user_session_params

@resource = assigns(:resource)
@data = JSON.parse(response.body)
Expand All @@ -35,17 +36,27 @@ class DeviseTokenAuth::SessionsControllerTest < ActionController::TestCase
assert_equal @existing_user.email, @data['data']['email']
end

describe 'using auth cookie' do
before do
DeviseTokenAuth.cookie_enabled = true
end

test 'request should return auth cookie' do
post :create, params: @user_session_params
assert response.cookies[DeviseTokenAuth.cookie_name]
end

after do
DeviseTokenAuth.cookie_enabled = false
end
end

describe "with multiple clients and headers don't change in each request" do
before do
# 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

@user_session_params = {
email: @existing_user.email,
password: @existing_user.password
}
end

test 'should limit the maximum number of concurrent devices' do
Expand Down Expand Up @@ -159,6 +170,24 @@ def @controller.reset_session
test 'session was destroyed' do
assert_equal true, @controller.reset_session_called
end

describe 'using auth cookie' do
before do
DeviseTokenAuth.cookie_enabled = true
@auth_token = @existing_user.create_new_auth_token
@controller.send(:cookies)[DeviseTokenAuth.cookie_name] = { value: @auth_token.to_json }
end

test 'auth cookie was destroyed' do
assert_equal @auth_token.to_json, @controller.send(:cookies)[DeviseTokenAuth.cookie_name] # sanity check
delete :destroy, format: :json
assert_nil @controller.send(:cookies)[DeviseTokenAuth.cookie_name]
end

after do
DeviseTokenAuth.cookie_enabled = false
end
end
end

describe 'unauthed user sign out' do
Expand Down