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

fix: fix logic for retrying 401s with silent re-auth #471

Merged
merged 2 commits into from
Jan 7, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Jan 4, 2019

Resolves #470
Impact: minor
Type: bugfix

Issue

Due to GraphQL server update to Apollo Server 2.0 as well as subsequent changes in starterkit, the earlier work to silently re-authenticate when a 401 error is detected stopped working.

Solution

Revamped how we're doing it to be more foolproof.

Breaking changes

None

Testing

I recommend testing against reactioncommerce/reaction#4894 or the re-auth from browser will not work.

There are two ways this token refreshing is supposed to work, both of which need to be tested. The setup is the same for both:

  • Add - ACCESS_TOKEN_LIFESPAN=1m in the environment section of reaction-hydra project's docker-compose.yml file.
  • dc down and dc up -d reaction-hydra
  • Log out of starterkit, and then log back in.

This will force auth tokens to expire after only 1 minute, making it faster to test this.

Test silent re-auth in browser

  1. After waiting at least a minute since you logged in, click something in the app to cause a GraphQL request to happen. Make sure it's something you haven't recently clicked or Apollo will actually be using its cache. If you don't see a graphql-alpha request in the browser network log, then keep clicking until you do.
  2. You should see a 401 response and "Attempting silent re-auth" message in the browser console, which may immediately disappear. The page may flash and then you should still be logged in, or worst case it should bring you to the login page if it was not able to silently refresh the token.

Test silent re-auth from NextJS server

  1. After waiting at least a minute since you logged in (or since you did the last token refresh test), refresh the page.
  2. You should see an "Attempting silent re-auth" message in the NextJS server log. When the page finishes refreshing, you should still be logged in.

@aldeed aldeed self-assigned this Jan 4, 2019
@aldeed aldeed force-pushed the fix-470-aldeed-fix-401-handling branch from fce83a8 to 6eeeaac Compare January 4, 2019 00:52

return new ApolloClient({
connectToDevTools: process.browser,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apollo dev tools wasn't working for me anymore until I removed this. It automatically enables only for non-production, so it should be fine.

} else {
logger.error(`Error while running getDataFromTree: ${error}`);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Essentially this same code was just moved to initApollo.js, where the browser redirect is happening, so that all of the logic is in one place (and because this catch was no longer being hit).

@aldeed aldeed requested a review from kieckhafer January 4, 2019 00:55
@aldeed aldeed added this to the 🏔 Shavano milestone Jan 4, 2019
return (
<ButtonBase
disableRipple
className={classNames(variantButton, { [activeVariant]: isActive }, { [soldOutVariant]: variantInventoryStatus && variantInventoryStatus.type === "SOLD_OUT" })}
Copy link
Member

Choose a reason for hiding this comment

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

Is this change meant to be in this PR? Seems out of place with the rest of the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not related, but it was an eslint warning so I fixed it

@kieckhafer
Copy link
Member

@aldeed I updated the snyk expiration date check here to fix that testing issue, at least until 2019-01-31. This looks pretty good, just had one question (above), but either way you answer that question it is still good to merge, just want to make sure you want that change here.

@kieckhafer kieckhafer merged commit 181bf27 into develop Jan 7, 2019
@kieckhafer kieckhafer deleted the fix-470-aldeed-fix-401-handling branch January 7, 2019 21:20
This was referenced Jan 15, 2019
@spencern spencern mentioned this pull request Jan 25, 2019
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.

Handle 401s better
2 participants