bharos commented on code in PR #10437:
URL: https://github.com/apache/gravitino/pull/10437#discussion_r2942197344
##########
web-v2/web/src/lib/store/auth/index.js:
##########
@@ -102,8 +102,32 @@ export const logoutAction =
createAsyncThunk('auth/logoutAction', async ({ route
try {
const provider = await oauthProviderFactory.getProvider()
if (provider) {
+ // For OIDC providers, use signoutRedirect to end Keycloak session
+ if (provider.getUserManager) {
+ const userManager = provider.getUserManager()
+ if (userManager) {
+ // Clear legacy auth tokens before redirect
+ localStorage.removeItem('accessToken')
+ localStorage.removeItem('authParams')
+ localStorage.removeItem('expiredIn')
+ localStorage.removeItem('isIdle')
+ localStorage.removeItem('version')
+
+ dispatch(clearIntervalId())
+ dispatch(setAuthToken(''))
+
+ // Redirect to IdP logout endpoint — this will navigate away from
the app
+ // signoutRedirect() must be called before clearAuthData() because
it needs
+ // the stored id_token for the id_token_hint parameter in
RP-initiated logout
+ await userManager.signoutRedirect()
+
+ await provider.clearAuthData()
+
+ return { token: null }
+ }
+ }
+
await provider.clearAuthData()
Review Comment:
Will the browser already navigate away here, ie.
```
await userManager.signoutRedirect() // ← browser navigates AWAY here
await provider.clearAuthData() // ← DEAD CODE: never reached
return { token: null } // ← DEAD CODE: never reached
```
##########
web-v2/web/src/lib/store/auth/index.js:
##########
@@ -102,8 +102,32 @@ export const logoutAction =
createAsyncThunk('auth/logoutAction', async ({ route
try {
const provider = await oauthProviderFactory.getProvider()
if (provider) {
+ // For OIDC providers, use signoutRedirect to end Keycloak session
+ if (provider.getUserManager) {
+ const userManager = provider.getUserManager()
+ if (userManager) {
+ // Clear legacy auth tokens before redirect
+ localStorage.removeItem('accessToken')
+ localStorage.removeItem('authParams')
+ localStorage.removeItem('expiredIn')
+ localStorage.removeItem('isIdle')
+ localStorage.removeItem('version')
+
+ dispatch(clearIntervalId())
+ dispatch(setAuthToken(''))
+
+ // Redirect to IdP logout endpoint — this will navigate away from
the app
+ // signoutRedirect() must be called before clearAuthData() because
it needs
+ // the stored id_token for the id_token_hint parameter in
RP-initiated logout
+ await userManager.signoutRedirect()
+
+ await provider.clearAuthData()
+
+ return { token: null }
+ }
+ }
+
await provider.clearAuthData()
Review Comment:
Claude gives me this as a better flow
```
// 1. Read id_token from stored user BEFORE removing it
const user = await userManager.getUser()
// 2. Clear app state first
await provider.clearAuthData() // removeUser() from storage
dispatch(...)
// 3. Redirect with explicit id_token_hint
await userManager.signoutRedirect({ id_token_hint: user?.id_token })
// nothing after this — browser navigates away
```
Or at a minimum, to move signoutRedirect() to be the very last line and
remove the dead code after it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
