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

theblang · 2021-01-05T15:25:48Z

There are two main reasons to use a server cookie to send and receive the auth token.

For sharing auth state across subdomains with the Domain attribute. From the MDN docs:

The Domain attribute specifies which hosts are allowed to receive the cookie. If unspecified, it defaults to the same origin that set the cookie, excluding subdomains. If Domain is specified, then subdomains are always included. Therefore, specifying Domain is less restrictive than omitting it. However, it can be helpful when subdomains need to share information about a user.

Note that you cannot access localStorage across subdomains.

For security reasons. If you only send the auth token in an HttpOnly server cookie then JavaScript can never access it, which helps protect against XSS attacks. These three issues are mostly pertaining to the security aspect: Where to store token securely? #1281 Storing auth token in browser best practise #1321 Does DTA support HTTP Only Cookie Refresh tokens along site access tokens? #1371

This PR introduces a server cookie as an additive opt-in feature to send and receive the auth token, which will allow for the subdomain sharing mentioned in point one. By introducing the option to use a server cookie, we lay the groundwork for addressing the security aspect in point two. I didn't want to make the changes for point two yet as that will be more impactful, because we'd need to make it possible to not expose the auth token in headers. I imagine if and when we do that work we'll probably add an analogous headers_enabled config to hinge off of in code to turn off sending and receiving the auth token in headers.

cc @booleanbetrayal @nbrustein @apuntovanini

theblang · 2021-01-05T19:53:48Z

app/controllers/devise_token_auth/sessions_controller.rb

@@ -55,6 +55,16 @@ def destroy
        user.tokens.delete(client)
        user.save!

+        if DeviseTokenAuth.cookie_enabled
+          if DeviseTokenAuth.cookie_attributes[:domain]


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.

Make sense, remember to use allowlist instead of whitelist

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

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 }

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

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

apuntovanini · 2021-01-05T20:20:53Z

I tested in our application and works like charm. I only have issues logging out (we use a graphql endpoint to make our calls, we should probably switch to classic controllers flow for authentication instead). But sharing cookies with our subdomains is crucial to us, well done!

theblang · 2021-01-05T20:36:54Z

I tested in our application and works like charm. I only have issues logging out (we use a graphql endpoint to make our calls, we should probably switch to classic controllers flow for authentication instead). But sharing cookies with our subdomains is crucial to us, well done!

Yay, thanks so much! I'm so glad this will help y'all as well!

theblang · 2021-01-06T13:59:25Z

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

+    elsif config_value.is_a?(Proc)
+      config_value.call(request)
+    end
+  end


See this discussion

theblang · 2021-01-06T14:44:52Z

.gitignore

@@ -9,6 +9,7 @@ test/dummy/.sass-cache
 test/dummy/config/application.yml
 coverage
 .idea
+.byebug_history


Drive-by that popped up while debugging a test

MaicolBen

I don't like using cookies in APIs but your reasoning of why is a good alternative sounds good

MaicolBen · 2021-01-06T23:24:24Z

app/controllers/devise_token_auth/sessions_controller.rb

+          if cookie_domain.present?
+            # 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: cookie_domain)


This line is unreached by specs but it could be ok

This conditional will be moot now, see this recent commit.

MaicolBen · 2021-01-06T23:24:36Z

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

+
+    cookie_domain = get_cookie_domain
+    if cookie_domain.present?
+      cookie_attributes.merge!(domain: cookie_domain)


This line is unreached by specs but it could be ok

This conditional will be moot now, see this recent commit.

MaicolBen · 2021-01-06T23:25:44Z

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

+    # more flexibility, such as to dynamically choose the domain when serving multiple domains
+    # from a single server, you can set a Proc that will be passed the request.
+    config_value = DeviseTokenAuth.cookie_attributes[:domain]
+    if config_value.is_a?(String)


These cases are unreached in specs but that could be ok

Moot now that I've gutted the unnecessary Proc logic.

MaicolBen · 2021-01-06T23:27:18Z

app/controllers/devise_token_auth/concerns/set_user_by_token.rb

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


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

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?

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

apuntovanini · 2021-01-06T23:46:54Z

I don't like using cookies in APIs but your reasoning of why is a good alternative sounds good

Just to understand, how would how tackle cross-subdomain authentication with api without a cookie? Thanks!

…ty with setting domain to the Proc if the Proc returns nil. clean up unncessary present conditionals.

theblang · 2021-01-07T16:03:36Z

I don't like using cookies in APIs but your reasoning of why is a good alternative sounds good

Yeah, we weren't too keen on it either. But it's the only way to share state across subdomains. And the general consensus seems to be that it's more secure for auth (once you're using only the HttpOnly cookie, which we'll hopefully allow in later work).

theblang · 2021-01-17T21:56:57Z

@MaicolBen @booleanbetrayal Could one of you retry the failed Travis job? It seems that just one of the jobs failed with:

Fetching nokogiri 1.10.10
Bundler::GemspecError: Could not read gem at
/home/travis/build/lynndylanhurley/devise_token_auth/gemfiles/vendor/bundle/ruby/2.3.0/cache/nokogiri-1.10.10.gem.
It may be corrupted.
An error occurred while installing nokogiri (1.10.10), and Bundler
cannot continue.
Make sure that `gem install nokogiri -v '1.10.10' --source
'https://rubygems.org/'` succeeds before bundling.
In rails_5_1.gemfile:
  devise_token_auth was resolved to 1.1.4, which depends on
    rails was resolved to 5.2.4.4, which depends on
      actioncable was resolved to 5.2.4.4, which depends on
        actionpack was resolved to 5.2.4.4, which depends on
          actionview was resolved to 5.2.4.4, which depends on
            rails-dom-testing was resolved to 2.0.3, which depends on
              nokogiri

booleanbetrayal · 2021-01-18T00:37:59Z

@theblang - running this now produces a new error:

You are using an old OmniAuth version, please ensure you have 1.0.0.pr2 version or later installed.

It looks like Devise has hard-coded the omniauth dependency in a way that it's breaking it's 2.0.0 release. It would be preferable if they fixed this on their end, but we might want to constrain the Gem version in the project if they are slow with a release. (cc: @MaicolBen )

MaicolBen · 2021-01-26T00:47:40Z

I'm going to wait for heartcombo/devise#5326 for a few days

theblang · 2021-02-01T22:51:15Z

Looks like we're close!

MaicolBen · 2021-03-06T15:55:40Z

Can you rebase/update with last master?

theblang and others added 4 commits January 4, 2021 09:01

Add support for sending and receiving the token via a cookie

c5043c7

tweak config and add to docs

d20d24f

test work and flatten cookie config

c623d51

be a little more defensive

3cdc09b

theblang commented Jan 5, 2021

View reviewed changes

allow a Proc being passed the request as a domain config

7592336

theblang commented Jan 6, 2021

View reviewed changes

MaicolBen reviewed Jan 6, 2021

View reviewed changes

fix bug with mutating the original cookie_attributes config. fix oddi…

536340b

…ty with setting domain to the Proc if the Proc returns nil. clean up unncessary present conditionals.

remove the Proc option since domain is more robust than I realized

66f8fa9

theblang requested a review from MaicolBen January 7, 2021 20:25

Merge remote-tracking branch 'upstream/master'

a486ba8

MaicolBen merged commit 05696c7 into lynndylanhurley:master Mar 11, 2021

This was referenced Apr 14, 2021

Where to store token securely? #1281

Open

Does DTA support HTTP Only Cookie Refresh tokens along site access tokens? #1371

Closed

Persistent cookie may be unauthenticated #1103

Open

theblang mentioned this pull request Jun 24, 2022

Set cookie token on omniauth success to avoid having to store the scraped query param token in the client #1539

Closed

theblang mentioned this pull request Jul 8, 2022

Set cookie token immediately in reset password and OmniAuth success flows #1542

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

theblang commented Jan 5, 2021

theblang Jan 5, 2021 •

edited

Loading

apuntovanini Jan 5, 2021

theblang Jan 5, 2021 •

edited

Loading

theblang Jan 6, 2021 •

edited

Loading

theblang Jan 7, 2021

theblang Jan 7, 2021

apuntovanini commented Jan 5, 2021

theblang commented Jan 5, 2021

theblang Jan 6, 2021

theblang Jan 6, 2021

MaicolBen left a comment

MaicolBen Jan 6, 2021

theblang Jan 7, 2021

MaicolBen Jan 6, 2021

theblang Jan 7, 2021

MaicolBen Jan 6, 2021

theblang Jan 7, 2021

MaicolBen Jan 6, 2021

theblang Jan 7, 2021

theblang Jan 7, 2021 •

edited

Loading

apuntovanini commented Jan 6, 2021

theblang commented Jan 7, 2021

theblang commented Jan 17, 2021 •

edited

Loading

booleanbetrayal commented Jan 18, 2021

MaicolBen commented Jan 26, 2021

theblang commented Feb 1, 2021

MaicolBen commented Mar 6, 2021

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

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

Conversation

theblang commented Jan 5, 2021

theblang Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theblang Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

theblang Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apuntovanini commented Jan 5, 2021

theblang commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaicolBen left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theblang Jan 7, 2021 • edited Loading

Choose a reason for hiding this comment

apuntovanini commented Jan 6, 2021

theblang commented Jan 7, 2021

theblang commented Jan 17, 2021 • edited Loading

booleanbetrayal commented Jan 18, 2021

MaicolBen commented Jan 26, 2021

theblang commented Feb 1, 2021

MaicolBen commented Mar 6, 2021

theblang Jan 5, 2021 •

edited

Loading

theblang Jan 5, 2021 •

edited

Loading

theblang Jan 6, 2021 •

edited

Loading

theblang Jan 7, 2021 •

edited

Loading

theblang commented Jan 17, 2021 •

edited

Loading