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 (refresh-token): handle error in refresh-token plugin and fix reshOnlyToken type #667

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

anjarupnik
Copy link
Contributor

While implementing refresh token I ran into issue that whole app was crashing when there was an error in refreshToken call. This was due to not handling errors in refresh-token.server.ts, so I opened this PR with proposition to wrap this call in try catch block and reset token and refresh token in case of error.
Also I noticed that refreshOnlyToken type was not correct (it should be boolean that can be set to false, since it is set to true by default in defaultsByBackend).

Checklist:

  • issue number linked above after pound (#)
    • replace "Closes " with "Contributes to" or other if this PR does not close the issue
  • manually checked my feature / checking not applicable
  • wrote tests / testing not applicable
  • attached screenshots / screenshot not applicable

@zoey-kaiser zoey-kaiser added p4 Important Issue bug A bug that needs to be resolved labels Feb 23, 2024
@phoenix-ru
Copy link
Collaborator

@anjarupnik Could you please merge the latest main to your branch? I did some type improvements in #665 and it also touched the code which you suggest wrapping in try-catch.

@phoenix-ru
Copy link
Collaborator

phoenix-ru commented Apr 4, 2024

@zoey-kaiser Could you do a functional review and merge if all good?

You need to emulate an error somehow (maybe by introducing a typo in config)

edit: I did the functional testing to avoid merge conflicts in the future.


@anjarupnik Thank you for the PR :)

@phoenix-ru phoenix-ru merged commit a8000e0 into sidebase:main Apr 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug that needs to be resolved p4 Important Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants