Allow sessions to work across subdomains #8090

Matt-Yorkley · 2021-08-21T15:09:56Z

What? Why?

First part of dealing with openfoodfoundation/ofn-install#643

Allows session storage to work across subdomains of the main host. This includes www., which is technically a subdomain. After this we should be able to implement a redirect without destroying all active sessions when we introduce it. 💪

Review note: the SessionCookieUpgrader code is temporary; we can remove it after it's been in production for a week or so.

What should we test?

Before deploying: go to a staging server, choose a shop, and add some items to the cart. Check out the cookies in the browser console, there should be one called _session_id an it should have a long random set of numbers and letters. Note it down.
Deploy this PR.
Reload the page in the browser. You should see: the session is intact (the same shop is still selected and the same items are in the cart) and the session cookie has now been updated (the cookie's name should now be _ofn_session_id and the long random identifier should be the same.

Release notes

Enabled sessions to pass seamlessly between subdomains, including www

Changelog Category: Technical changes

codecov · 2021-08-21T15:12:44Z

Codecov Report

Merging #8090 (39372f7) into master (697a073) will decrease coverage by 0.06%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #8090      +/-   ##
==========================================
- Coverage   93.21%   93.15%   -0.07%     
==========================================
  Files         645      646       +1     
  Lines       18161    18176      +15     
==========================================
+ Hits        16928    16931       +3     
- Misses       1233     1245      +12

Impacted Files	Coverage Δ
lib/session_cookie_upgrader.rb	`20.00% <20.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 697a073...39372f7. Read the comment docs.

Currently sessions set on (www.openfoodnetwork.xxx) are not usable on the bare domain (openfoonetwork.xxx). When transitioning from one to the other, the user's session is completely lost. This change means sessions on subdomains (including www) will be transferable.

This checks if the current request has been submitted using the old session key (_session_id) and transparently migrates the session id to a new session cookie with the new settings and the new key (_ofn_session_id).

The session cookie is now named `_ofn_session_id` instead of `_session_id`.

andrewpbrett

Looks good, this will be really nice to have. 👍

andrewpbrett · 2021-08-25T23:59:14Z

config/initializers/session_store.rb

@@ -1,10 +1,11 @@
 # Be sure to restart your server when you modify this file.

-# The cookie_store can be too small for very long URLs stored by Devise.
-# The maximum size of cookies is 4096 bytes.
-#Openfoodnetwork::Application.config.session_store :cookie_store, key: '_openfoodnetwork_session'


Commented code 👃 🔥

andrewpbrett · 2021-08-26T00:03:25Z

config/initializers/session_store.rb

+Openfoodnetwork::Application.config.session_store(
+  :active_record_store,
+  key: "_ofn_session_id",
+  domain: :all,


This is the line that does the magic, correct? By passing these additional arguments it shares cookies across all subdomains?

Yeah, the domain: :all and tld_length: 2 options mean the session cookie will work across any subdomains.

The name change is necessary here, as you can't modify the primary attributes of an existing cookie with the same name. If we didn't do it the changes wouldn't take effect until the user fully logged out and back in, and we don't know when that would happen.

RachL · 2021-08-28T16:00:21Z

@Matt-Yorkley

I've followed the test procedure, but I have not seen a change in the cookie name. Am I looking at the wrong place?

Before deployment:

After:

sigmundpetersen · 2021-08-28T17:02:02Z

@RachL you also have _ofn_session_id in addition to _session_id after deployment. 😉
Maybe _session_id just remains until some restart/refresh of some sort 🤷

RachL · 2021-08-28T17:16:19Z

@sigmundpetersen you are a ⭐ 🤗

I need new glasses!!

That being said it does not 100% validate the test. So I will wait for @Matt-Yorkley or @andrewpbrett to confirm this can move to ready to go :)

andrewpbrett · 2021-08-28T18:30:40Z

The biggest thing is that the value of the cookie didn't change, but it does look like we were expecting the old cookie to be deleted. I'd wait until @Matt-Yorkley can take a look to see why that might not have worked as expected.

Matt-Yorkley · 2021-08-30T10:11:55Z

Weird, it was working when I tested it. I did notice the storage/cookies section in the browser console in Firefox sometimes doesn't show updated values until you close it and re-open it... seems like a minor bug in Firefox 🤷‍♂️

I'll take another look. We shouldn't merge it if the old cookie is not removed 👍

Matt-Yorkley · 2021-08-30T20:32:22Z

I just retested with both Chrome and Firefox, and the old session cookie was correctly removed. It didn't show the change correctly in Firefox until I closed and reopened the dev toolbar (as mentioned above).

RachL · 2021-08-31T19:40:25Z

@Matt-Yorkley indeed, I have just re-tested. All good, this is ready to go!

Matt-Yorkley self-assigned this Aug 21, 2021

Matt-Yorkley force-pushed the session-storage branch 2 times, most recently from 72692ff to dd00357 Compare August 22, 2021 14:44

Matt-Yorkley added 3 commits August 22, 2021 15:53

Add Rack Middleware for transitioning existing sessions

1d472d0

This checks if the current request has been submitted using the old session key (_session_id) and transparently migrates the session id to a new session cookie with the new settings and the new key (_ofn_session_id).

Update cookies policy notes and related spec

39372f7

The session cookie is now named `_ofn_session_id` instead of `_session_id`.

Matt-Yorkley force-pushed the session-storage branch from dd00357 to 39372f7 Compare August 22, 2021 14:57

Matt-Yorkley marked this pull request as ready for review August 22, 2021 16:22

andrewpbrett approved these changes Aug 26, 2021

View reviewed changes

RachL self-assigned this Aug 28, 2021

RachL added the pr-staged-fr staging.coopcircuits.fr label Aug 28, 2021

RachL added feedback-needed pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Aug 28, 2021

RachL removed the pr-staged-fr staging.coopcircuits.fr label Aug 29, 2021

Matt-Yorkley added the pr-staged-uk staging.openfoodnetwork.org.uk label Aug 30, 2021

RachL removed pr-staged-uk staging.openfoodnetwork.org.uk feedback-needed labels Aug 31, 2021

Matt-Yorkley merged commit aaf45af into openfoodfoundation:master Aug 31, 2021

filipefurtad0 mentioned this pull request Sep 1, 2021

[master] Can't login to a user account (not in production) #8126

Closed

This was referenced Sep 1, 2021

Payment taken in Paypal but order in state = Payment #8109

Closed

Strip subdomains from session cookie host #8127

Merged

andrewpbrett mentioned this pull request Sep 3, 2021

Payment registered in Stripe when OFN declared a problem #7692

Closed

Matt-Yorkley mentioned this pull request Sep 7, 2021

OFN works on multiple domains, e.g. www.openfoodnetwork.org.uk and openfoodnetwork.org.uk openfoodfoundation/ofn-install#643

Closed

This was referenced Sep 8, 2021

Payment taken in Stripe but not marked complete in OFN #8143

Closed

[Spike] Investigate our Stripe Integration #8074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sessions to work across subdomains #8090

Allow sessions to work across subdomains #8090

Matt-Yorkley commented Aug 21, 2021 •

edited

Loading

codecov bot commented Aug 21, 2021 •

edited

Loading

andrewpbrett left a comment

andrewpbrett Aug 25, 2021

andrewpbrett Aug 26, 2021

Matt-Yorkley Aug 30, 2021 •

edited

Loading

RachL commented Aug 28, 2021

sigmundpetersen commented Aug 28, 2021 •

edited

Loading

RachL commented Aug 28, 2021 •

edited

Loading

andrewpbrett commented Aug 28, 2021

Matt-Yorkley commented Aug 30, 2021 •

edited

Loading

Matt-Yorkley commented Aug 30, 2021

RachL commented Aug 31, 2021

Allow sessions to work across subdomains #8090

Allow sessions to work across subdomains #8090

Conversation

Matt-Yorkley commented Aug 21, 2021 • edited Loading

What? Why?

What should we test?

Release notes

codecov bot commented Aug 21, 2021 • edited Loading

Codecov Report

andrewpbrett left a comment

Choose a reason for hiding this comment

andrewpbrett Aug 25, 2021

Choose a reason for hiding this comment

andrewpbrett Aug 26, 2021

Choose a reason for hiding this comment

Matt-Yorkley Aug 30, 2021 • edited Loading

Choose a reason for hiding this comment

RachL commented Aug 28, 2021

sigmundpetersen commented Aug 28, 2021 • edited Loading

RachL commented Aug 28, 2021 • edited Loading

andrewpbrett commented Aug 28, 2021

Matt-Yorkley commented Aug 30, 2021 • edited Loading

Matt-Yorkley commented Aug 30, 2021

RachL commented Aug 31, 2021

Matt-Yorkley commented Aug 21, 2021 •

edited

Loading

codecov bot commented Aug 21, 2021 •

edited

Loading

Matt-Yorkley Aug 30, 2021 •

edited

Loading

sigmundpetersen commented Aug 28, 2021 •

edited

Loading

RachL commented Aug 28, 2021 •

edited

Loading

Matt-Yorkley commented Aug 30, 2021 •

edited

Loading