Skip to content

fix(ui): silent-refresh cold-load 401 and post-refresh reauth - #31597

Merged
chirag-madlani merged 3 commits into
mainfrom
hotfix/silent-refresh-cold-load-bug
Aug 17, 2026
Merged

chirag-madlani merged 3 commits into
mainfrom
hotfix/silent-refresh-cold-load-bug

Conversation

@chirag-madlani

@chirag-madlani chirag-madlani commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two user-visible SPA silent-refresh bugs, fixed with surgical changes to AuthProvider.tsx. No architecture changes — no coordinator, no storage schema, no SDK changes. Deliberately scoped to be a low-risk pre-release hotfix; a broader AuthCoordinator refactor is planned as a follow-up.

  • Bug 1 — cold-load 401 doesn't trigger silent refresh. On tab reopen after id-token expiry, /users/loggedInUser returns 401 {"message":"Expired token!"}, but TokenService.fetchNewToken returns null before the network refresh fires because the lazy authenticator's renewer isn't yet registered. The interceptor then force-logs the user out — no /token call ever happens.
  • Bug 2 — visibility-triggered refresh doesn't re-authenticate the UI. visibilitychange successfully refreshes the token, but nothing calls getLoggedInUserDetails afterward. Storage is fresh, UI stays on /signin.

Reported symptom (Azure OIDC public flow, but the mechanism is provider-agnostic):

Close a tab and reopen after the id-token has expired → /loggedInUser returns 401 Expired token! and NO refresh call is fired. Switch focus between tabs → refresh fires but app stays on the sign-in page.

Root cause

Both bugs share one root cause: the lazy authenticator wrapper (MSAL/Okta/Auth0/OIDC/Basic/Generic) registers renewToken on TokenService via a mount effect that races the code paths above. When they run first, refresh returns null and the user is force-logged-out.

Fix

In getLoggedInUserDetails — proactive gate before /loggedInUser:

  • Read the stored token. If expired, wait up to 2s for TokenService.renewToken to become a function.
  • If ready, call refreshToken() and only then issue /loggedInUser.
  • If refresh fails, fall through to resetUserDetails (unchanged behavior).
  • Defensive retry in the catch: if the request still landed a refreshable 401 (e.g. interceptor refresh happened but the retry failed), give the renewer one more chance and try once more.

In the visibilitychange handler:

  • await tokenService.current?.refreshToken().
  • After a successful refresh, read useApplicationStore.getState().isAuthenticated (avoids the mount-only effect's stale closure) and run getLoggedInUserDetails if false — flips isAuthenticated back to true.

Files changed

  • openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx (+118/-40)
  • openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx (+133)

Tests

  • 5 new regression tests pin down the contracts the fixes rely on:
    1. TokenService.refreshToken return-shape normalization (raw string vs AccessTokenResponse).
    2. REFRESHABLE_AUTH_ERRORS classifies Expired token! and Token signing key not found as refreshable.
    3. Visibility handler awaits refresh THEN reauths when !isAuthenticated.
    4. Skips reauth when already authenticated.
    5. Skips reauth when refresh fails.
  • All 14 pre-existing AuthProvider.test.tsx tests continue to pass unchanged (19/19 total).
  • yarn prettier --write, yarn eslint --fix, npx tsc --noEmit all clean; the one pre-existing tsc error in this file (ROUTES.AUTH_CALLBACK/SILENT_CALLBACK literal-type mismatch) exists on main too.

Manual smoke matrix

Reviewers, please tick these off. Regression scenarios per SSO provider:

  • Basic — login → wait past id-token TTL → API call → stays alive silently.
  • Basic — close tab → reopen after TTL → stays alive silently (Bug 1).
  • Basic — background tab → expire → foreground → refresh fires AND app authenticated (Bug 2).
  • Azure (MSAL) — same three scenarios.
  • Google — same three scenarios.
  • Okta — same three scenarios.
  • Auth0 — same three scenarios.
  • Custom OIDC / Cognito — same three scenarios.
  • SAML — login + close/reopen after TTL only (visibility path shares code with Basic).

Non-goals (intentional — pushed to follow-up)

  • No multi-tab single-refresh coordination (still uses the current localStorage-flag polling; races are possible but not new).
  • No inactive-tab pause.
  • No AuthCoordinator refactor — that PR follows this one after the release cycle and supersedes this hotfix's logic.

Follow-up refactor branch

A larger AuthCoordinator refactor branch (azure-oidc-session-invalidated-on-restart) already exists — it extracts silent-refresh, cross-tab lock, 401 queue, and focus handling into a single module. It supersedes this hotfix's logic and will rebase onto main once this merges.

🤖 Generated with Claude Code

Greptile Summary

The PR moves cold-load renewer synchronization into TokenService and awaits visibility-triggered refresh before restoring authenticated UI state.

  • Coalesces refresh callers around the existing in-flight refresh operation.
  • Waits for lazy authenticator renewer registration before fetching a replacement token.
  • Re-runs logged-in-user loading after a successful visibility refresh when the store is unauthenticated.
  • Adds TokenService regression coverage for delayed renewer registration and timeout behavior.

Confidence Score: 4/5

The PR is not yet safe to merge because a first lazy-authenticator load exceeding the new ten-second deadline still destroys an otherwise renewable session.

The replacement wait moves the existing race into TokenService but still returns null when registration misses a fixed deadline, and the interceptor interprets that result as terminal renewal failure and force-logs the user out.

Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts, openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx Visibility refresh now awaits token renewal and restores user state, while the interceptor still force-logs out on null refresh results.
openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts Centralizes delayed renewer handling, but the fixed deadline still converts slow first initialization into destructive refresh failure.
openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.test.ts Adds focused coverage for renewer registration waits, timeout behavior, and refresh result handling.

Sequence Diagram

sequenceDiagram
  participant API as loggedInUser request
  participant I as Axios interceptor
  participant T as TokenService
  participant A as Lazy authenticator
  participant UI as AuthProvider
  API-->>I: 401 expired token
  I->>T: refreshToken()
  T->>T: awaitRenewerReady(10s)
  alt authenticator registers in time
    A->>T: updateRenewToken()
    T->>A: renewToken()
    A-->>T: refreshed token
    T-->>I: token
    I->>API: retry queued request
  else registration exceeds timeout
    T-->>I: null
    I->>UI: resetUserDetails(true)
    UI->>UI: clear session and navigate to signin
  end
Loading

Reviews (3): Last reviewed commit: "fix(ui): move the cold-load renewer wait..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

Two user-visible bugs in the SPA silent-refresh path, fixed with
surgical changes to AuthProvider.tsx. No architecture changes.

Bug 1 — cold-load 401 doesn't trigger silent refresh:
On tab reopen after id-token expiry, /loggedInUser returns
401 {"code":401,"message":"Expired token!"}, but the axios
interceptor's refresh path races the lazy authenticator's renewer
registration and TokenService.fetchNewToken returns null before
the network call fires. The interceptor then force-logs the user
out — no /token call ever happens, and the user lands on /signin.

Fix: proactive gate in getLoggedInUserDetails — check the stored
token's expiry BEFORE calling /loggedInUser. If expired, wait up
to 2s for the renewer to become a function on TokenService, then
call refreshToken() and only after success issue /loggedInUser.
Additional defense: in the catch block, if the error is still a
refreshable 401 (e.g. the interceptor refresh happened but the
retry failed), give the renewer one more chance and retry once.
Falls through to resetUserDetails on any failure.

Bug 2 — visibility-triggered silent refresh doesn't reauth the UI:
visibilitychange handler successfully refreshes the token after
Bug 1 has already flipped isAuthenticated=false, but nothing
runs getLoggedInUserDetails afterward. Storage is fresh, UI stays
on the sign-in page.

Fix: in the visibility handler, await the refresh, then read
useApplicationStore.getState().isAuthenticated (avoids the mount-
only useEffect's stale closure) and run getLoggedInUserDetails
if false — that flips isAuthenticated back to true and drops the
user into the authenticated app.

Both fixes are self-contained helpers (waitForRenewerReady,
ensureFreshTokenBeforeUserFetch, retryLoggedInUserAfterRenewer,
applyLoggedInUser, isRefreshableAuthError) inside AuthProvider.tsx.
No storage schema, SDK, or interceptor architecture changes; no
coordinator, no cross-tab primitive additions.

Tests: 5 new unit tests pin down the contracts the fixes rely on —
TokenService.refreshToken return-shape normalization, REFRESHABLE_
AUTH_ERRORS classification, visibility handler sequencing, and the
skip-reauth branches. All 14 pre-existing tests continue to pass
unchanged (19/19 total).

Follow-up (separate PR after release): full AuthCoordinator refactor
that owns silent-refresh, cross-tab lock, 401 queue, and focus
handling as a single module.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 17, 2026 05:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

@github-actions github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 66%
66.85% (79882/119486) 51.24% (48752/95134) 52.23% (14595/27941)

karanh37
karanh37 previously approved these changes Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit c59bce4dc21fe2f1e041a0d1440b9eac7362a7ea in Playwright run 32004712261, attempt 1.

✅ 797 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 52m 57s

⏱️ Max setup 3m 30s · max shard execution 21m 10s · max shard-job elapsed before upload 24m 44s · reporting 5s

🌐 222.43 requests/attempt · 2.67 app boots/UI scenario · 28.82% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 28.82% (convergence target: at most 15%).
  • Browser traffic was 222.43 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.67 per UI scenario (2492 boots / 934 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard chromium-01 153 0 0 0 0 0
✅ Shard chromium-02 172 0 0 0 0 0
✅ Shard chromium-03 158 0 0 0 0 0
✅ Shard chromium-04 145 0 0 0 0 0
✅ Shard data-asset-rules-01 61 0 0 0 0 0
✅ Shard domain-isolation-01 14 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 17 0 0 0 0 0
✅ Shard ingestion-01 1 0 0 0 0 0
✅ Shard reindex-01 2 0 0 0 0 0
✅ Shard search-01 11 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

…l unit coverage

Addresses two PR-review findings:

Greptile P1 — "Renewer timeout destroys session":
Previously ensureFreshTokenBeforeUserFetch returned false on renewer-
registration timeout, and getLoggedInUserDetails called resetUserDetails()
on that false — which clears the valid refresh credential and forces
the user to /signin on merely-slow lazy authenticator load.

Fix: change the helper to Promise<void> with best-effort semantics —
if the renewer never registers, or the refresh throws, we resolve
silently. The caller then issues /loggedInUser normally and lets
the axios interceptor + catch-block retry handle a real 401 if one
arrives. A hard failure here is strictly worse than the original
bug it was trying to fix.

Gitar + Greptile P2 — "Tests duplicate production behavior":
Removed the five tautological tests that asserted against local
inline closures. Extracted the pure helpers — isRefreshableAuthError,
waitForRenewerReady, ensureFreshTokenBeforeUserFetch — from
AuthProvider.tsx into silentRefreshHelpers.ts, taking their
dependencies as parameters. Added silentRefreshHelpers.test.ts with
14 unit tests that exercise the actual exported functions, covering
every branch (including the best-effort no-throw contract that
guards against P1).

Total: 42/42 tests pass (14 pre-existing AuthProvider + 14 pre-
existing OktaAuthProvider + 14 new silentRefreshHelpers).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 17, 2026 06:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Follow-up to Greptile's P1 restatement: even with the best-effort
proactive gate in getLoggedInUserDetails, a lazy authenticator load
that takes longer than the caller's wait would fall through to the
/loggedInUser request, get a 401, and the response interceptor's
refreshToken() would still return null (because renewToken isn't
registered yet) — triggering resetUserDetails(true), which clears
storage before the caller's catch could retry.

Real fix: move the renewer wait into TokenService.fetchNewToken
itself. The interceptor's refresh call now blocks briefly (up to
10s) for the lazy authenticator to register its renewer, then
performs the refresh normally. resetUserDetails(true) only fires
when the renewer truly never registers within the wait window.

This makes the fix work for every caller of tokenService.refreshToken
(interceptor, visibility handler, proactive timer, SSE stream
reconnect) — not just getLoggedInUserDetails.

Cleanup in AuthProvider.tsx:
- Removed ensureFreshTokenBeforeUserFetch call and the
  retryLoggedInUserAfterRenewer catch-block retry — both were racing
  the same failure mode that TokenService now handles at the source.
- Removed the silentRefreshHelpers module (no longer used in
  production).
- getLoggedInUserDetails is back to a straightforward try/catch;
  refresh reliability now lives in TokenService where it belongs.

Bug 2 fix (visibility-triggered reauth) is unchanged.

Tests: 4 new TokenServiceUtil.test.ts cases pinning the wait
behavior — cold-load renewer race, hot-path short-circuit, timeout
without registration, and the pre-existing "no renewer" case now
short-circuits the wait via a spy. All 51 tests across TokenService
+ AuthProviders pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 17, 2026 07:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

* The 10s cap covers slow lazy-chunk loads on poor networks without
* hanging indefinitely if the authenticator module fails to load.
*/
async awaitRenewerReady(maxWaitMs = 10_000, pollMs = 100): Promise<void> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Renewer timeout still destroys session

When the first lazy authenticator load takes longer than ten seconds, awaitRenewerReady returns while renewToken is still absent, causing the refresh to resolve null. The 401 interceptor treats that as terminal renewal failure, clears the session, and forces the user to sign in even though the authenticator can still finish loading and renew the token.

Knowledge Base Used: OpenMetadata React UI

@@ -141,6 +141,13 @@ class TokenService {

// Call renewal method according to the provider
async fetchNewToken() {

@gitar-bot gitar-bot Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: 10s renewer wait can stall refresh and widen cross-tab race

fetchNewToken now awaits awaitRenewerReady (default 10s) after performRefresh has already set REFRESH_IN_PROGRESS_KEY. If the lazy authenticator chunk fails to load, every refresh call (interceptor, proactive timer, visibility handler) blocks the full 10s before returning null and force-logging out, stalling the UI spinner. It also holds the in-progress flag for up to 10s while sibling tabs only wait ~1s in waitForTokenPersistence, so they give up and fire their own /auth/refresh, potentially rotating the refresh token and causing 401s (multi-tab is a stated non-goal, but this widens the window). Consider a smaller default cap (e.g. 3–5s) or short-circuiting the wait once the authenticator module is known to have failed.

Lower the default wait cap to reduce the stall and cross-tab race window on cold-load.:

async awaitRenewerReady(maxWaitMs = 5_000, pollMs = 100): Promise<void> {

Was this helpful? React with 👍 / 👎

@sonarqubecloud

Copy link
Copy Markdown

@chirag-madlani
chirag-madlani merged commit b6b61b9 into main Aug 17, 2026
162 of 164 checks passed
@chirag-madlani
chirag-madlani deleted the hotfix/silent-refresh-cold-load-bug branch August 17, 2026 08:24
@github-actions

Copy link
Copy Markdown
Contributor

Failed to cherry-pick changes to the 1.13 branch.
Please cherry-pick the changes manually.
You can find more details here.

@github-actions

Copy link
Copy Markdown
Contributor

Changes have been cherry-picked to the 2.0 branch.

github-actions Bot pushed a commit that referenced this pull request Aug 17, 2026
* fix(ui): silent-refresh cold-load 401 and post-refresh reauth

Two user-visible bugs in the SPA silent-refresh path, fixed with
surgical changes to AuthProvider.tsx. No architecture changes.

Bug 1 — cold-load 401 doesn't trigger silent refresh:
On tab reopen after id-token expiry, /loggedInUser returns
401 {"code":401,"message":"Expired token!"}, but the axios
interceptor's refresh path races the lazy authenticator's renewer
registration and TokenService.fetchNewToken returns null before
the network call fires. The interceptor then force-logs the user
out — no /token call ever happens, and the user lands on /signin.

Fix: proactive gate in getLoggedInUserDetails — check the stored
token's expiry BEFORE calling /loggedInUser. If expired, wait up
to 2s for the renewer to become a function on TokenService, then
call refreshToken() and only after success issue /loggedInUser.
Additional defense: in the catch block, if the error is still a
refreshable 401 (e.g. the interceptor refresh happened but the
retry failed), give the renewer one more chance and retry once.
Falls through to resetUserDetails on any failure.

Bug 2 — visibility-triggered silent refresh doesn't reauth the UI:
visibilitychange handler successfully refreshes the token after
Bug 1 has already flipped isAuthenticated=false, but nothing
runs getLoggedInUserDetails afterward. Storage is fresh, UI stays
on the sign-in page.

Fix: in the visibility handler, await the refresh, then read
useApplicationStore.getState().isAuthenticated (avoids the mount-
only useEffect's stale closure) and run getLoggedInUserDetails
if false — that flips isAuthenticated back to true and drops the
user into the authenticated app.

Both fixes are self-contained helpers (waitForRenewerReady,
ensureFreshTokenBeforeUserFetch, retryLoggedInUserAfterRenewer,
applyLoggedInUser, isRefreshableAuthError) inside AuthProvider.tsx.
No storage schema, SDK, or interceptor architecture changes; no
coordinator, no cross-tab primitive additions.

Tests: 5 new unit tests pin down the contracts the fixes rely on —
TokenService.refreshToken return-shape normalization, REFRESHABLE_
AUTH_ERRORS classification, visibility handler sequencing, and the
skip-reauth branches. All 14 pre-existing tests continue to pass
unchanged (19/19 total).

Follow-up (separate PR after release): full AuthCoordinator refactor
that owns silent-refresh, cross-tab lock, 401 queue, and focus
handling as a single module.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ui): make cold-load refresh best-effort + extract helpers for real unit coverage

Addresses two PR-review findings:

Greptile P1 — "Renewer timeout destroys session":
Previously ensureFreshTokenBeforeUserFetch returned false on renewer-
registration timeout, and getLoggedInUserDetails called resetUserDetails()
on that false — which clears the valid refresh credential and forces
the user to /signin on merely-slow lazy authenticator load.

Fix: change the helper to Promise<void> with best-effort semantics —
if the renewer never registers, or the refresh throws, we resolve
silently. The caller then issues /loggedInUser normally and lets
the axios interceptor + catch-block retry handle a real 401 if one
arrives. A hard failure here is strictly worse than the original
bug it was trying to fix.

Gitar + Greptile P2 — "Tests duplicate production behavior":
Removed the five tautological tests that asserted against local
inline closures. Extracted the pure helpers — isRefreshableAuthError,
waitForRenewerReady, ensureFreshTokenBeforeUserFetch — from
AuthProvider.tsx into silentRefreshHelpers.ts, taking their
dependencies as parameters. Added silentRefreshHelpers.test.ts with
14 unit tests that exercise the actual exported functions, covering
every branch (including the best-effort no-throw contract that
guards against P1).

Total: 42/42 tests pass (14 pre-existing AuthProvider + 14 pre-
existing OktaAuthProvider + 14 new silentRefreshHelpers).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(ui): move the cold-load renewer wait into TokenService.fetchNewToken

Follow-up to Greptile's P1 restatement: even with the best-effort
proactive gate in getLoggedInUserDetails, a lazy authenticator load
that takes longer than the caller's wait would fall through to the
/loggedInUser request, get a 401, and the response interceptor's
refreshToken() would still return null (because renewToken isn't
registered yet) — triggering resetUserDetails(true), which clears
storage before the caller's catch could retry.

Real fix: move the renewer wait into TokenService.fetchNewToken
itself. The interceptor's refresh call now blocks briefly (up to
10s) for the lazy authenticator to register its renewer, then
performs the refresh normally. resetUserDetails(true) only fires
when the renewer truly never registers within the wait window.

This makes the fix work for every caller of tokenService.refreshToken
(interceptor, visibility handler, proactive timer, SSE stream
reconnect) — not just getLoggedInUserDetails.

Cleanup in AuthProvider.tsx:
- Removed ensureFreshTokenBeforeUserFetch call and the
  retryLoggedInUserAfterRenewer catch-block retry — both were racing
  the same failure mode that TokenService now handles at the source.
- Removed the silentRefreshHelpers module (no longer used in
  production).
- getLoggedInUserDetails is back to a straightforward try/catch;
  refresh reliability now lives in TokenService where it belongs.

Bug 2 fix (visibility-triggered reauth) is unchanged.

Tests: 4 new TokenServiceUtil.test.ts cases pinning the wait
behavior — cold-load renewer race, hot-path short-circuit, timeout
without registration, and the pre-existing "no renewer" case now
short-circuits the wait via a spy. All 51 tests across TokenService
+ AuthProviders pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(cherry picked from commit b6b61b9)
@gitar-bot

gitar-bot Bot commented Aug 17, 2026

Copy link
Copy Markdown
Code Review 👍 Approved with suggestions 1 resolved / 2 findings

Well-structured hotfix for cold-load and visibility refresh bugs with comprehensive regression tests. Consider reducing the 10-second renewer wait in fetchNewToken to prevent stalling the refresh flow.

💡 Edge Case: 10s renewer wait can stall refresh and widen cross-tab race

📄 openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:143-157 📄 openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:101-115

fetchNewToken now awaits awaitRenewerReady (default 10s) after performRefresh has already set REFRESH_IN_PROGRESS_KEY. If the lazy authenticator chunk fails to load, every refresh call (interceptor, proactive timer, visibility handler) blocks the full 10s before returning null and force-logging out, stalling the UI spinner. It also holds the in-progress flag for up to 10s while sibling tabs only wait ~1s in waitForTokenPersistence, so they give up and fire their own /auth/refresh, potentially rotating the refresh token and causing 401s (multi-tab is a stated non-goal, but this widens the window). Consider a smaller default cap (e.g. 3–5s) or short-circuiting the wait once the authenticator module is known to have failed.

Lower the default wait cap to reduce the stall and cross-tab race window on cold-load.
async awaitRenewerReady(maxWaitMs = 5_000, pollMs = 100): Promise<void> {
✅ 1 resolved
Quality: Regression tests re-implement logic instead of exercising it

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx:647-661 📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.test.tsx:590-604
The four new "silent-refresh recovery" tests (return-shape normalization, and the three visibility-handler reauth cases) assert against local inline closures (const handler = async () => {...}, const extract = ...) that duplicate the production logic rather than invoking getLoggedInUserDetails, ensureFreshTokenBeforeUserFetch, isRefreshableAuthError, or the real handleVisibilityChange. These tests are tautological: they would still pass if the actual code in AuthProvider.tsx regressed, so they provide no real protection for an auth-critical hotfix. Consider rendering the AuthProvider (or extracting the helpers into a testable module) and asserting on the real code paths so the tests can catch a real regression.

🤖 Prompt for agents
Code Review: Well-structured hotfix for cold-load and visibility refresh bugs with comprehensive regression tests. Consider reducing the 10-second renewer wait in fetchNewToken to prevent stalling the refresh flow.

1. 💡 Edge Case: 10s renewer wait can stall refresh and widen cross-tab race
   Files: openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:143-157, openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts:101-115

   `fetchNewToken` now awaits `awaitRenewerReady` (default 10s) after `performRefresh` has already set `REFRESH_IN_PROGRESS_KEY`. If the lazy authenticator chunk fails to load, every refresh call (interceptor, proactive timer, visibility handler) blocks the full 10s before returning null and force-logging out, stalling the UI spinner. It also holds the in-progress flag for up to 10s while sibling tabs only wait ~1s in `waitForTokenPersistence`, so they give up and fire their own `/auth/refresh`, potentially rotating the refresh token and causing 401s (multi-tab is a stated non-goal, but this widens the window). Consider a smaller default cap (e.g. 3–5s) or short-circuiting the wait once the authenticator module is known to have failed.

   Fix (Lower the default wait cap to reduce the stall and cross-tab race window on cold-load.):
   async awaitRenewerReady(maxWaitMs = 5_000, pollMs = 100): Promise<void> {

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

timothybrush pushed a commit to timothybrush/OpenMetadata that referenced this pull request Aug 17, 2026
…d 401 refresh (open-metadata#31644)

Follow-up to open-metadata#31597. The hotfix added TokenService.awaitRenewerReady so
the axios 401 interceptor's refresh call could wait briefly for the
renewer to be registered, but the underlying race — the ref-deps
useEffect that owned the registration — was never addressed. On any
provider where the lazy authenticator finishes mounting AFTER the
first /users/loggedInUser call has already 401'd (confidential +
Google is where this reliably reproduces, but every provider is
susceptible on cold-load with a slow chunk), the useEffect never
re-runs (ref changes don't schedule re-renders), TokenService.renewToken
stays null, awaitRenewerReady times out at 10s, and the interceptor
force-logs the user out — no /api/v1/auth/refresh call is ever fired.

Fix: each authenticator wrapper (BasicAuthAuthenticator,
GenericAuthenticator, OidcAuthenticator, MsalAuthenticator,
OktaAuthenticator, Auth0Authenticator) now calls
TokenService.updateRenewToken from its own mount useEffect. That
effect runs the moment useImperativeHandle populates the ref, with
no dependence on the parent re-rendering — no race, deterministic
registration.

AuthProvider.tsx: the ref-deps useEffect is gone.
updateRefreshSuccessCallback(startTokenExpiryTimer) moves to the main
mount effect (that callback lives in AuthProvider's closure).
TokenService.updateRenewToken now accepts `null` on cleanup, and
RenewTokenCallback widens to `() => Promise<unknown>` so each
provider's renewer can return its own SDK-native shape (Auth0
RenewTokenResponse, MSAL AuthenticationResult, oidc-client User, …)
without a per-provider cast at the registration call site.

Renewers wrapped in useCallback where they weren't already
(GenericAuthenticator handleSilentSignIn, MsalAuthenticator
renewIdToken, OktaAuthenticator renewToken, Auth0Authenticator
renewIdToken extracted from useImperativeHandle) so the register
effect's dep is stable and doesn't thrash.

Tests: each authenticator's test file now mocks TokenService via
jest.mock and asserts (a) the mount effect calls updateRenewToken
with a function, and (b) unmount calls updateRenewToken(null).
5 new regression tests, 138/138 total across the auth surface.

No behavior change on the happy path (renewer already registered
in the same render cycle). Only cold-load / slow-lazy-mount
scenarios where the old race lost.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chirag-madlani added a commit that referenced this pull request Aug 18, 2026
…d 401 refresh (#31644)

Follow-up to #31597. The hotfix added TokenService.awaitRenewerReady so
the axios 401 interceptor's refresh call could wait briefly for the
renewer to be registered, but the underlying race — the ref-deps
useEffect that owned the registration — was never addressed. On any
provider where the lazy authenticator finishes mounting AFTER the
first /users/loggedInUser call has already 401'd (confidential +
Google is where this reliably reproduces, but every provider is
susceptible on cold-load with a slow chunk), the useEffect never
re-runs (ref changes don't schedule re-renders), TokenService.renewToken
stays null, awaitRenewerReady times out at 10s, and the interceptor
force-logs the user out — no /api/v1/auth/refresh call is ever fired.

Fix: each authenticator wrapper (BasicAuthAuthenticator,
GenericAuthenticator, OidcAuthenticator, MsalAuthenticator,
OktaAuthenticator, Auth0Authenticator) now calls
TokenService.updateRenewToken from its own mount useEffect. That
effect runs the moment useImperativeHandle populates the ref, with
no dependence on the parent re-rendering — no race, deterministic
registration.

AuthProvider.tsx: the ref-deps useEffect is gone.
updateRefreshSuccessCallback(startTokenExpiryTimer) moves to the main
mount effect (that callback lives in AuthProvider's closure).
TokenService.updateRenewToken now accepts `null` on cleanup, and
RenewTokenCallback widens to `() => Promise<unknown>` so each
provider's renewer can return its own SDK-native shape (Auth0
RenewTokenResponse, MSAL AuthenticationResult, oidc-client User, …)
without a per-provider cast at the registration call site.

Renewers wrapped in useCallback where they weren't already
(GenericAuthenticator handleSilentSignIn, MsalAuthenticator
renewIdToken, OktaAuthenticator renewToken, Auth0Authenticator
renewIdToken extracted from useImperativeHandle) so the register
effect's dep is stable and doesn't thrash.

Tests: each authenticator's test file now mocks TokenService via
jest.mock and asserts (a) the mount effect calls updateRenewToken
with a function, and (b) unmount calls updateRenewToken(null).
5 new regression tests, 138/138 total across the auth surface.

No behavior change on the happy path (renewer already registered
in the same render cycle). Only cold-load / slow-lazy-mount
scenarios where the old race lost.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(cherry picked from commit 1511b12)

This branch was previously deployed

1 inactive deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs To release Will cherry-pick this PR into the release branch UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants