Fix #26622: Suppress auto-redirect after explicit logout for SSO providers#26757
Fix #26622: Suppress auto-redirect after explicit logout for SSO providers#26757
Conversation
…iders When enableAutoRedirect is true, clicking Logout navigates to /signin where the auto-redirect logic immediately fires, sending the user back to the IdP and re-authenticating them. This makes it impossible to stay logged out while the IdP session is active. Set a sessionStorage flag (om_explicit_logout) before logout begins and check it on /signin mount to suppress auto-redirect for that one visit. The flag is tab-scoped and consumed on first read, so new tabs and subsequent visits still auto-redirect normally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
openmetadata-ui/src/main/resources/ui/src/pages/LoginPage/SignInPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Fixes the SSO logout loop when enableAutoRedirect: true by marking an explicit logout in tab-scoped sessionStorage and suppressing the one-time auto-redirect on the subsequent /signin mount.
Changes:
- Set a
sessionStorageflag at the start of logout to indicate an explicit user logout. - Read + consume that flag on
/signinto suppressenableAutoRedirectfor a single visit. - Add a unit test to ensure auto-redirect is suppressed and the flag is consumed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx |
Sets the explicit-logout session flag during logout flow. |
openmetadata-ui/src/main/resources/ui/src/pages/LoginPage/SignInPage.tsx |
Reads/consumes the flag and gates shouldAutoRedirect to avoid immediate re-login. |
openmetadata-ui/src/main/resources/ui/src/pages/LoginPage/SignInPage.test.tsx |
Adds regression coverage for post-logout suppression behavior. |
| const isPostLogout = useMemo(() => { | ||
| const flag = sessionStorage.getItem('om_explicit_logout'); | ||
| if (flag) { | ||
| sessionStorage.removeItem('om_explicit_logout'); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| }, []); |
There was a problem hiding this comment.
isPostLogout is computed via useMemo with a side effect (removing the sessionStorage flag) during render. In React 18 StrictMode / concurrent rendering, render work can be invoked more than once or discarded, which can consume the flag before the committed render reads it—defeating the suppression and reintroducing the auto-redirect loop. Make the render phase pure: read the flag without mutating storage (e.g., via useState initializer or a pure useMemo), and consume/remove the flag in a useLayoutEffect (declared before the auto-redirect effect) or in an effect that runs after commit while keeping isPostLogout stable for the first commit.
| const isPostLogout = useMemo(() => { | ||
| const flag = sessionStorage.getItem('om_explicit_logout'); | ||
| if (flag) { | ||
| sessionStorage.removeItem('om_explicit_logout'); | ||
|
|
There was a problem hiding this comment.
The sessionStorage key 'om_explicit_logout' is duplicated across AuthProvider, SignInPage, and the test. To prevent drift/typos and make the behavior easier to discover, consider extracting it into a shared constant (e.g., constants/auth.constants.ts) and importing it in all three places.
| const onLogoutHandler = useCallback(async () => { | ||
| clearTimeout(timeoutId); | ||
|
|
||
| sessionStorage.setItem('om_explicit_logout', 'true'); | ||
|
|
||
| // Let SSO complete the logout process | ||
| await authenticatorRef.current?.invokeLogout(); |
There was a problem hiding this comment.
sessionStorage.setItem(...) can throw (e.g., storage disabled, browser privacy modes, embedded/3rd-party contexts). Since this runs on every explicit logout, an exception here would prevent logout from completing. Wrap the storage write in a small try/catch (and proceed with logout even if it fails), ideally behind a helper shared with the /signin read path.
useMemo does not guarantee the cached value is retained — React may discard it and re-run the callback (e.g. for offscreen trees). Since the callback consumes the sessionStorage flag, a re-run would return false and re-enable auto-redirect. useState's lazy initializer runs exactly once per mount, making the read-and-consume side effect safe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 1 resolved / 1 findingsSuppresses auto-redirect after explicit logout for SSO providers by replacing useMemo with useState lazy initializer to prevent the isPostLogout flag from being lost on re-render. No issues found. ✅ 1 resolved✅ Bug: Side effect in useMemo may cause flag to be lost on re-render
OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
| sessionStorage.setItem('om_explicit_logout', 'true'); | ||
|
|
There was a problem hiding this comment.
The storage key 'om_explicit_logout' is duplicated here and in SignInPage. To prevent future drift/typos (which would silently break the suppression logic), consider extracting it into a shared constant (e.g., in a constants/auth module) and reusing it in both places.
| it('should NOT auto-redirect after explicit logout', async () => { | ||
| sessionStorage.setItem('om_explicit_logout', 'true'); | ||
|
|
There was a problem hiding this comment.
This test writes to sessionStorage, which is shared state across the whole test file. Add explicit cleanup (e.g., remove the key in a finally block or clear storage in afterEach) so the suite stays isolated even if an assertion fails mid-test.
| const onLogoutHandler = useCallback(async () => { | ||
| clearTimeout(timeoutId); | ||
|
|
||
| sessionStorage.setItem('om_explicit_logout', 'true'); |
There was a problem hiding this comment.
sessionStorage.setItem(...) can throw (e.g., storage disabled, quota exceeded), which would abort the logout flow before invokeLogout() runs and could leave the app in a partially logged-out state. Wrap this flag write in a try/catch (and optionally guard for typeof window !== 'undefined') so logout remains reliable even if storage is unavailable.
| sessionStorage.setItem('om_explicit_logout', 'true'); | |
| if (typeof window !== 'undefined' && window.sessionStorage) { | |
| try { | |
| window.sessionStorage.setItem('om_explicit_logout', 'true'); | |
| } catch { | |
| // Ignore storage errors to ensure logout flow continues | |
| } | |
| } |
Summary
enableAutoRedirect: trueis configured with an SSO provider (e.g., Azure AD), clicking Logout navigates to/signinwhere auto-redirect immediately fires, re-authenticating the user via the still-active IdP session — making it impossible to stay logged out.sessionStorageflag (om_explicit_logout) before logout begins and checks it on/signinmount to suppress auto-redirect for that single visit./signinin the same tab also auto-redirect normally).Changes
AuthProvider.tsxom_explicit_logoutflag insessionStorageat the start ofonLogoutHandlerSignInPage.tsxisPostLogoutmemo that reads + consumes the flag; add&& !isPostLogouttoshouldAutoRedirectSignInPage.test.tsxExpected behavior after fix
/signin/signin, shows SSO login buttonCloses #26622
Test plan
should NOT auto-redirect after explicit logout— sets flag, renders withenableAutoRedirect: true+ Azure, assertsonLoginHandlerwas NOT called and flag was consumedSSO providers should auto-redirect when enableAutoRedirect is true— passes (no regression)enableAutoRedirect: true, log in, click Logout, verify staying on/signin🤖 Generated with Claude Code