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/use fresh tokens #1627

Merged
merged 13 commits into from
Jan 26, 2021
Merged

Fix/use fresh tokens #1627

merged 13 commits into from
Jan 26, 2021

Conversation

dac09
Copy link
Contributor

@dac09 dac09 commented Jan 12, 2021

Creating this PR again repeat of #1609.

New tasks:

  • verify and repro e2e test issue
  • fix getToken is not found error for apps without Auth
  • tested if token is automatically refreshed (on firebase)

@github-actions
Copy link

github-actions bot commented Jan 12, 2021

@dac09
Copy link
Contributor Author

dac09 commented Jan 12, 2021

Update: repro'd the test issue https://s.tape.sh/WIyxZqox

dac09 added 3 commits January 12, 2021 12:16
…h-tokens

* 'main' of github.com:redwoodjs/redwood:
  Move whatwg-fetch from devDep to dep
  Adds mockCurrentUser() to api-side jest
  v0.22.1
  v0.22.0
  Revert "Configure ApolloProvider to always use fresh tokens  (redwoodjs#1609)" (redwoodjs#1611)
  Configure ApolloProvider to always use fresh tokens  (redwoodjs#1609)
  Ignore *.scenarios.* files. (redwoodjs#1607)
  upgrade prisma v2.12.1 (redwoodjs#1604)
  Test Scenarios (redwoodjs#1465)
  Use relative path to config stories location (redwoodjs#1509)
…fix/use-fresh-tokens

* 'fix/use-fresh-tokens' of github.com:dac09/redwood:
@dac09 dac09 force-pushed the fix/use-fresh-tokens branch from a1e9711 to 7307ad3 Compare January 12, 2021 14:06
@dac09
Copy link
Contributor Author

dac09 commented Jan 12, 2021

Added extra checks, before calling getToken. e2e tests now pass https://s.tape.sh/UOJM6MYf

@dac09 dac09 requested review from peterp and thedavidprice January 12, 2021 15:34
@thedavidprice
Copy link
Contributor

Thanks for giving this another go @dac09 🚀

Release Notes

Include Deprecation Warning:

@deprecated auth tokens are now refreshed when they expire, use getToken() instead. authToken will be removed from this context in future releases

@thedavidprice thedavidprice added release:breaking This PR is a breaking change topic/auth labels Jan 12, 2021
@thedavidprice thedavidprice added this to the next release milestone Jan 12, 2021
@thedavidprice
Copy link
Contributor

@peterp handing over to you for review + merge

@dac09 dac09 linked an issue Jan 13, 2021 that may be closed by this pull request
7 tasks
const { getToken, type: authProviderType, isAuthenticated } = useAuth()

const withToken = setContext(async () => {
if (isAuthenticated && getToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case when getToken would be falsy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing the failure in the e2e test. Its falsy when you're not logged in - wasn't sure if there's any other case where its falsy apart from the first render of the auth provider.

Copy link
Contributor

Choose a reason for hiding this comment

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

getToken is falsey? But surely that shouldn't be the case since the function is provided via the provider? Maybe one of the "fake implementations" doesn't follow the providers spec.

@dac09
Copy link
Contributor Author

dac09 commented Jan 20, 2021

@peterp bumping this, responded to your comments :)

Co-authored-by: Peter Pistorius <peter.pistorius@gmail.com>
@thedavidprice thedavidprice modified the milestones: v0.23.0, next release Jan 26, 2021
@dthyresson dthyresson merged commit 2cb612a into redwoodjs:main Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change topic/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracking] Validate token refresh issue across providers
4 participants