Skip to content

Fix #26622: Suppress auto-redirect after explicit logout for SSO providers #26757

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.

Sign up for GitHub

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

Open
aji-aju wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix #26622: Suppress auto-redirect after explicit logout for SSO providers #26757

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eed04ca
Fix #26622: Suppress auto-redirect after explicit logout for SSO prov…
aji-aju Mar 25, 2026
f8fb627
Use useState lazy initializer instead of useMemo for isPostLogout
aji-aju Mar 26, 2026
4e3b1c1
Merge branch 'main' into fix/26622-suppress-auto-redirect-after-logout
aji-aju Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,8 @@ export const AuthProvider = ({
const onLogoutHandler = useCallback(async () => {
clearTimeout(timeoutId);

sessionStorage.setItem('om_explicit_logout', 'true');
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
}
}

Copilot uses AI. Check for mistakes.

Comment on lines +180 to +181
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
// Let SSO complete the logout process
await authenticatorRef.current?.invokeLogout();
Comment on lines 177 to 183
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

Expand Down
24 changes: 24 additions & 0 deletions openmetadata-ui/src/main/resources/ui/src/pages/LoginPage/SignInPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,30 @@ describe('Test SignInPage Component', () => {
expect(signinButton).toBeInTheDocument();
});

it('should NOT auto-redirect after explicit logout', async () => {
sessionStorage.setItem('om_explicit_logout', 'true');

Comment on lines +142 to +144
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
const onLoginHandler = jest.fn();
mockUseAuthProvider.mockReturnValue({ onLoginHandler });

mockuseApplicationStore.mockReturnValue({
isAuthDisabled: false,
authConfig: { provider: 'azure', enableAutoRedirect: true },
onLogoutHandler: jest.fn(),
getOidcToken: jest.fn(),
});

const { container } = render(<SignInPage />, {
wrapper: MemoryRouter,
});

const signinButton = await findByText(container, /label.sign-in-with-sso/i);

expect(signinButton).toBeInTheDocument();
expect(onLoginHandler).not.toHaveBeenCalled();
expect(sessionStorage.getItem('om_explicit_logout')).toBeNull();
});

it('SSO providers should auto-redirect when enableAutoRedirect is true', async () => {
const onLoginHandler = jest.fn();
mockUseAuthProvider.mockReturnValue({ onLoginHandler });
Expand Down
14 changes: 13 additions & 1 deletion openmetadata-ui/src/main/resources/ui/src/pages/LoginPage/SignInPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ const SignInPage = () => {

const { handleLogin } = useBasicAuth();

const isPostLogout = useMemo(() => {
const flag = sessionStorage.getItem('om_explicit_logout');
if (flag) {
sessionStorage.removeItem('om_explicit_logout');

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return true;
}

return false;
}, []);
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

const handleSignIn = useCallback(() => {
onLoginHandler && onLoginHandler();
}, [onLoginHandler]);
Expand All @@ -76,7 +87,8 @@ const SignInPage = () => {
authConfig?.enableAutoRedirect &&
!isAuthProviderBasic &&
!isAuthenticated &&
Boolean(onLoginHandler);
Boolean(onLoginHandler) &&
!isPostLogout;

useLayoutEffect(() => {
if (shouldAutoRedirect && !hasTriggeredAutoRedirect.current) {
Expand Down
Loading