Replace passport server-side sessions with cookie session #349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
impactmass
commented
Oct 1, 2018
| @@ -13,10 +13,11 @@ KEYCLOAK_REDIRECT_URI=http://localhost:4000/auth | |||
| STRIPE_PUBLIC_API_KEY=ENTER_STRIPE_PUBLIC_KEY_HERE | |||
| OAUTH2_AUTH_URL=http://localhost:4444/oauth2/auth | |||
| OAUTH2_TOKEN_URL=http://hydra:4444/oauth2/token | |||
| HYDRA_ADMIN_URL=http://hydra:4445 | |||
There was a problem hiding this comment.
Unrelated to this PR, but discovered this unused env var. It shouldn't be here anymore.
| @@ -46,8 +46,8 @@ | |||
| "chalk": "^2.3.2", | |||
| "classnames": "^2.2.5", | |||
| "cookie-parser": "^1.4.3", | |||
| "cookie-session": "^2.0.0-beta.3", | |||
There was a problem hiding this comment.
-beta.3 is their latest version for about a year.
I'm not sure if to stay on this version or use a previous. Or maybe consider another package? suggestions welcomed?
There was a problem hiding this comment.
I also saw some errors in Snyk. I haven't looked well, but it looks like they are from nextjs and not the addition of this package.
aldeed
approved these changes
Oct 2, 2018
There was a problem hiding this comment.
I tested this pretty thoroughly and I believe it's working as intended.
On develop:
- Start storefront container
- Log in
- Restart storefront container
- Refresh page and I'm no longer logged in
On this branch:
- Start storefront container
- Log in
- Restart storefront container
- Refresh page and I am still logged in
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #344
Impact: major
Type: feature+bugfix
Issue
Our staging deploy of Starterkit fails to complete the OAuth flow. You either get a Forbidden error message or are not able to login.
This is believed to be caused session management issues. The current Passport setup uses
express-sessionwhich by default stores sessions in memory, which is a problem when the app is deployed on many instances.Solution
Make Passport use cookies to store session state (this was our previous setup before the addition of the OAuth flow). So the solution here is to update the Passport configuration to use cookies.
Testing