-
Notifications
You must be signed in to change notification settings - Fork 265
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
signOut: clear TokenManager and provide options for revoke and redirect #288
Conversation
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.
No concerns over the way we're exposing the options to the developer. I do want us to make another pass as the README.
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.
Final README comments, then LGTM 👍
dd8cef2
to
5591a94
Compare
README.md
Outdated
* Will change the `window.location` to an Okta-hosted page before redirecting to a URI of your choice. | ||
* No issue with 3rd-party cookies. | ||
* Requires a `postLogoutRedirectUri` to be specified. This URI must be whitelisted in the Okta application's settings. | ||
* Requires a valid `idToken`. If the `idToken` is invalid or session does not exist the redirect may end on a 400 error page from Okta. This error will be visible to the user and cannot be handled by the app. |
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.
Is this true? I thought only a non-whitelisted post_logout_redirect_uri gave errors
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.
Yeah, good catch. I see that an invalid session does not produce an error. I'll see if I can suss out any other error conditions.
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.
Documentation here lists some error conditions (including invalid id_token), I'll try to repro: https://developer.okta.com/docs/reference/api/oidc/#error-conditions
a009e42
to
482d898
Compare
Current behavior is preserved with default options
Passing a
postLogoutRedirectUri
will use post logout redirect flowAutomatically reads idToken from TokenManager
Can manually specify idToken for advanced/custom scenarios
If
postLogoutRedirectUri
is specified, redirect logic will happen even if idToken is not availableClears TokenManager on signOut. All OIDC sdks have been doing this step before calling signOut. The server-side redirect method requires an idToken, so the TokenManager should not be cleared before calling signOut(). The call to
tokenManager.clear()
can be removed from the SDKs once they are using this version.