Fix Azure AD SSO logout to terminate IdP session#26327
Conversation
Co-authored-by: chirag-madlani <12962843+chirag-madlani@users.noreply.github.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! |
| await instance.logoutRedirect({ | ||
| account: account ?? accounts[0], | ||
| postLogoutRedirectUri: window.location.origin + ROUTES.SIGNIN, | ||
| }); |
There was a problem hiding this comment.
We need to cleanup application state upon successful logout with handleSuccessfulLogout method
There was a problem hiding this comment.
Fixed in fc99b90. Moved handleSuccessfulLogout() into a finally block so app state is always cleaned up regardless of whether logoutRedirect succeeds or fails.
chirag-madlani
added
the
safe to test
|
@copilot address PR comments |
chirag-madlani
added
safe to test
🔴 Playwright Results — 7 failure(s), 30 flaky✅ 3291 passed · ❌ 7 failed · 🟡 30 flaky · ⏭️ 192 skipped
Genuine Failures (failed on all attempts)❌
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| @@ -68,14 +69,14 @@ const MsalAuthenticator = forwardRef<AuthenticatorRef, Props>( | |||
|
|
|||
| const logout = async () => { | |||
There was a problem hiding this comment.
⚠️ Edge Case: MSAL cache not cleared when logoutRedirect fails
The old code explicitly removed MSAL localStorage keys (-login.windows.net-, msal.*) before calling handleSuccessfulLogout(). The new code removes this manual cache cleanup entirely, relying on logoutRedirect to clear the MSAL cache. However, if logoutRedirect throws (caught on line 78), the MSAL cache remains intact in localStorage while the app state is already cleared (handleSuccessfulLogout ran on line 72). On the next page load, MSAL's ssoSilent or handleRedirectPromise can pick up the cached tokens and silently re-authenticate the user, making it impossible to log out when the IdP redirect fails (e.g., network issues, popup blockers).
Suggested fix:
const logout = async () => {
try {
handleSuccessfulLogout();
await instance.logoutRedirect({
account: account ?? accounts[0],
logoutHint: (account ?? accounts[0])?.username,
postLogoutRedirectUri: globalThis.location.origin + ROUTES.SIGNIN,
});
} catch {
// logoutRedirect failed; clear MSAL cache as fallback
instance.clearCache();
}
};
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
Code Review
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
|
Changes have been cherry-picked to the 1.12.2 branch. |
Describe your changes:
Azure AD logout only cleared the local OpenMetadata session, leaving the Azure AD IdP session active. With
enableAutoRedirect=trueandprompt=none, this caused immediate silent re-authentication after logout — making logout effectively broken.I worked on
MsalAuthenticator.tsxbecause the logout flow was not calling Azure AD'send_session_endpoint, causing the IdP session to remain active. The fix replaces the manual localStorage cleanup withinstance.logoutRedirect(), which properly terminates the Azure AD session.handleSuccessfulLogout()is now called beforelogoutRedirect()to guarantee app-level state cleanup regardless of the redirect outcome — sincelogoutRedirect()triggers a full-page navigation, any code after it never executes on the success path.MsalAuthenticator.tsx: Replace manual localStorage cleanup withinstance.logoutRedirect(), which calls Azure AD'send_session_endpointto terminate the IdP session and redirects back to/signinviapostLogoutRedirectUri.handleSuccessfulLogout()is called first to ensure non-MSAL tokens and app state are cleared before the browser navigates away.MsalAuthenticator.test.tsx: Updated tests to asserthandleSuccessfulLogoutis called beforelogoutRedirectwith the correctpostLogoutRedirectUri, and a fallback test verifies cleanup still occurs whenlogoutRedirectfails.Screen.Recording.2026-03-12.at.12.18.20.PM.mov
Type of change:
Checklist:
I have read the CONTRIBUTING document.
My PR title is
Fixes <issue-number>: <short explanation>I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
I have added a test that covers the exact scenario we are fixing.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.