- Notifications
You must be signed in to change notification settings - Fork1.1k
chore: return failed refresh errors on external auth as string (was boolean)#13402
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Failed refreshes should return errors. These errors are capturedas validate errors. A fair classification of the the error
Uh oh!
There was an error while loading.Please reload this page.
kylecarbs left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd change this to be of a custom error type:ErrRefreshFailed and store the validation error there. It may be useful in the future as well if we want to store other properties.
Emyrk commentedMay 31, 2024
I was considering that too, I just dislike that I would need to return an If I was to go that route, I'd remove the boolean return entirely. Just Let me give that a go with a custom error type and see how it looks |
kylecarbs commentedMay 31, 2024
Sweet! |
| // InvalidTokenError is a case where the "RefreshToken" failed to complete | ||
| // as a result of invalid credentials. Error contains the reason of the failure. | ||
| typeInvalidTokenErrorstring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: might wanna make this a struct instead, just so we can doe.ValidationMessage or something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's more anInvalidationMessage, but more specifically it could be related to refreshing, a deployment configuration (disabled refreshes), or a validation error.
In all cases, it is anerror. And the error is related to being invalid for w/e reason. I think continuing to use it as anerr.Error() makes sense
Uh oh!
There was an error while loading.Please reload this page.
Failed refreshing of tokens should return the error for debugging. The function no longer returns a boolean, and only returns an error. So the error can be checked if it is a validation error, or a runtime error.
This was added because multiple reports of failed refreshes have occurred. Debugging these failures are extremely difficult since errors are silenced. This will begin to raise said errors.
We might want to clean up errors from IDPs in the future if they get a bit too technical in their verbage.