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.

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

feat: Refresh access tokens on 401 errors #394

Closed
wants to merge 2 commits into from

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Oct 15, 2018

Resolves #350
Type: feature

Issue

Get a new access token from Hydra.

Solution

Breaking changes

None

Testing

  1. Start Starterkit on this branch, and Reaction API on this PR feat: Add a CORS-enabled endpoint for token refresh in Hydra plugin reaction#4743
  2. Run Hydra on master. Set this ENV in the docker-compose yaml before starting the service: ACCESS_TOKEN_LIFESPAN=1m. This will reduce the wait time. The tokens will expire in 60s.
  3. Load up a page. Leave it for a while for the token to expire.
  4. Return back to the page, and browser (e.g make a view a product, pause, then come back to add to cart. Try different approaches).
  5. Note if there are any token denied errors or 401s. There should be none

@impactmass impactmass force-pushed the feat-350-impactmass-token-refresh branch from 602858c to b88c947 Compare October 16, 2018 14:14
@impactmass impactmass changed the title WIP - Feat 350 impactmass token refresh feat: Refresh access tokens on 401 errors Oct 16, 2018
@impactmass impactmass added this to the Oxford milestone Oct 16, 2018
@impactmass impactmass requested a review from mikemurray October 16, 2018 15:05
@impactmass impactmass self-assigned this Oct 16, 2018
@impactmass impactmass force-pushed the feat-350-impactmass-token-refresh branch from b88c947 to 3aafbcc Compare October 16, 2018 16:10
@impactmass impactmass requested a review from aldeed October 16, 2018 17:07
@impactmass
Copy link
Contributor Author

Added @mikemurray as primary reviewer.
@aldeed I added you to this PR because of changes around the cookie-session setup (now that it gets changed in the browser when token refreshes). I'd like your comments there.

aldeed
aldeed previously requested changes Oct 16, 2018
Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

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

@impactmass See some comments on the diff

src/server.js Outdated
} catch (error) {
logger.error("Error creating encoded config string", error);
}
res.cookie("oauth_client_config", encoded);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a cookie needed here? Can you add these three config values to the public app config instead? publicRuntimeConfig in next.config.js

src/lib/apollo/sessionData.js Show resolved Hide resolved
src/server.js Show resolved Hide resolved
@impactmass impactmass force-pushed the feat-350-impactmass-token-refresh branch from 3aafbcc to 6f3aee1 Compare October 16, 2018 23:17
@impactmass impactmass force-pushed the feat-350-impactmass-token-refresh branch from 6f3aee1 to 72403f6 Compare October 17, 2018 11:07
@impactmass impactmass removed the request for review from mikemurray October 17, 2018 11:07
@impactmass impactmass dismissed aldeed’s stale review October 17, 2018 11:27

changes made. Readme added for cookie issue

@@ -0,0 +1,28 @@
# Auth & Cookies

Copy link
Contributor Author

@impactmass impactmass Oct 17, 2018

Choose a reason for hiding this comment

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

@aldeed @ticean @rosshadden these proposed changes to how we use cookies need a few more eyes to review. I've written this readme doc, per Eric's earlier review, to list the changes and will like comments from you all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me @impactmass.

@spencern spencern removed this from the Oxford milestone Oct 18, 2018
Copy link
Member

@ticean ticean left a comment

Choose a reason for hiding this comment

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

Hi @impactmass. Thanks for the great writeup. Switching to an insecure cookie (not HTTP-only) is a blocker. Especially with a refresh token in it. Refreshes don't expire. All the conventional wisdom suggests that it shouldn't be stored in the browser [citation needed].

I understand the impact on user experience with short-lived access tokens. Can we take some intermediate, secure solutions to move toward the desired state? For example, a full redirect flow would be possible with the tradeoff that it's more intrusive on the user experience because it kicks out of the SPA. Would that be acceptable enough for now to move things forward until we get something like a tested, iframe solution going? Are there other intermediate (or permanent) solutions that would work? Have suggestions @aldeed?

@impactmass
Copy link
Contributor Author

Based on the discussions from this review (especially the case around using not HTTP-only cookie, I'm adding token "refresh flow with redirects only" in #399 (almost finishing up).
This can serve as a first solution while we figure out the implementation for in browser refresh (new issue here #400). I'll close this PR, but keep the branch up.

@impactmass impactmass closed this Oct 22, 2018
@impactmass impactmass deleted the feat-350-impactmass-token-refresh branch October 22, 2019 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants