feat(ui): AuthCoordinator refactor + full SSO Playwright matrix (9 scenarios × 8 providers) - #31675
chirag-madlani wants to merge 44 commits into
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
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 |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Jest test CoverageUI tests summary
|
✅ Playwright Results — workflow succeededValidated commit ✅ 4509 passed · ❌ 0 failed · 🟡 10 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking 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) 1h 4m 0s ⏱️ Max setup 5m 44s · max shard execution 24m 18s · max shard-job elapsed before upload 27m 19s · reporting 24s 🌐 219.96 requests/attempt · 2.24 app boots/UI scenario · 41.15% common-shard skew Optimization targets still in progress:
🟡 10 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
Review round 1 — addressedP1s (Greptile)
P4s (gitar-bot)
Checkstyle
Test coverage
Warnings intentionally left for a separate cleanup PR: barrel-import + |
…validated-on-restart
…validated-on-restart
…validated-on-restart
Greptile P1 (r4023829117 on PR #31675): the leader's Web Lock was released the instant its `renewer()` returned, but `setOidcToken` + `notifyDone` still had to run afterwards. A sibling tab whose `ifAvailable:true` probe landed in that gap — even a few microseconds of an `await setOidcToken` — would acquire the freed lock, become another leader, and invoke `renewer()` again. With IdPs that rotate refresh tokens on use (Auth0, some OIDC providers) that duplicate rotation consumes the token the first leader just issued and invalidates the fresh session. Fix: extend `CrossTabLock.runExclusive` to accept an optional `publish(value)` hook that runs — still under the lock — immediately after `work()` resolves. Callers put persistence + broadcast in `publish` so both happen atomically before the lock releases. If either `work` OR `publish` throws, followers still receive `failed` (also under the lock) so they can attempt their own refresh instead of waiting the full timeout. AuthCoordinator's leader path now moves `setOidcToken` + `notifyDone` into that hook; the post-`runExclusive` code shrinks to just `applyRefreshed(outcome.value)` (emit `refreshed` + schedule proactive timer, both coordinator-side side-effects with no cross-tab visibility). The persist-before-broadcast ordering the original P1 fix cared about still holds — it's just a sequence inside the hook now. Test coverage: - Two new CrossTabLock unit tests pin the invariant: (1) the lock name is still `held` throughout the publish callback, (2) a concurrent `ifAvailable:true` probe fired during publish observes the lock as taken. Regression guards for the greptile finding. - The AuthCoordinator test harness's `mockRunExclusive` implementation updated to invoke the caller's `publish` hook — otherwise every leader-path assertion on `setOidcToken` / `notifyDone` would go silent when the real code was moved inside the hook. 45/45 tests in the AuthCoordinator suite pass. The `runWithoutWebLocks` Safari-private-mode fallback also invokes `publish` (single tab, no lock semantics needed but callers get consistent behaviour). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `import { LockTimeoutError }` sat below the two `jest.mock`
blocks so its explanatory comment could live next to the mock
strategy discussion, but `organize-imports-cli` (part of
`yarn ui-checkstyle` alongside prettier + eslint) hoists every
import to the top of the file. That flipped the file into
"organise-imports" failure state on the base PR's UI Checkstyle run.
Move the import next to the other top-of-file imports and keep the
strategy comment above the `mockRunExclusive` block where the swap
actually happens, with a note that hoisting the plain-class import
doesn't hit the TDZ hazard the jest.mock-based approach did.
24/24 AuthCoordinator tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`padding-line-between-statements` fires on the `return { role:
'leader', value }` inside the updated mockRunExclusive block —
the repo's ESLint config requires a blank line before `return`
after the `if (options?.publish) { ... }` guard. Reported by CI
as the one blocking error on UI Checkstyle:
src/utils/Auth/AuthCoordinator/__tests__/AuthCoordinator.test.ts:123:7
`padding-line-between-statements` Expected blank line before this statement.
Lint clean locally; 24/24 AuthCoordinator suite tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| await setOidcToken(result.idToken); | ||
| this.lock.notifyDone(result); |
There was a problem hiding this comment.
When service-worker, IndexedDB, or localStorage persistence fails, setOidcToken suppresses the error and this callback still broadcasts done, causing reloads, visibility checks, or newly opened tabs to reuse the expired token and restart renewal or return to sign-in.
Knowledge Base Used: Metadata web application
…validated-on-restart
…validated-on-restart
Follows the ratchet the docblock describes — main just bumped 970 → 975 in 8f77049 (`Move ClassificationTag/GlossaryTag/ DomainTag/DataProductTag/AutoClassificationTag and Icon into openmetadata-ui-core-components`) to accommodate that refactor's bootstrap growth. This PR sits at 999142 Brotli bytes after that merge — 742 bytes over the fresh 975 * 1024 ceiling — from the AuthCoordinator refactor's own bootstrap additions (RefreshQueue + CrossTabLock + VisibilityWatcher + fast-path check in ensureFreshToken, all imported by AuthProvider on the main-index graph). Bumping the ceiling by another 5 KiB preserves the ~4 KiB headroom the earlier docblock justifies for future dynamic-import churn without forcing an unrelated PR to fail. The five affected checks on the base PR — Maven SonarCloud CI, SSO Login Nightly (ldap / keycloak-oidc-public / msal-mock), playwright-visual-regression, and the RDF build lane — all failed on the same `Bundle budget exceeded: 999142 bytes (maximum 998400)` error, confirming the ceiling is the sole gate; nothing else needs changing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…validated-on-restart
The leader-path publish hook was calling setOidcToken, which swallows storage errors internally (best-effort semantics for the fail-silent callers in OidcAuthenticator / Auth0Authenticator). Inside the CrossTabLock this meant a broken IndexedDB write (private-browsing, quota, SW crash) still let the leader broadcast `done` with a payload no sibling tab could trust across a reload — followers accepted the token in memory, but the next cold-load read stale storage and re-triggered a refresh (or bounced to sign-in if the refresh path was also unhealthy at that moment). Add a strict variant that propagates the write error, and wire AuthCoordinator's publish hook to use it. The try/catch in CrossTabLock.runExclusive now broadcasts `failed` instead of `done`, so followers retry through the lock rather than trusting an unpersisted payload. Other setOidcToken callers keep the fail-silent contract they already relied on. Adds a regression test covering the coordinator-side observable: publish-hook throw suppresses notifyDone and emits refresh-failed with the storage-error reason. Greptile P1: r4035047159. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…overy Scenario 5 (cross-tab refresh coalesces to a single /auth/refresh call) was asserting `toHaveLength(1)`, but that's tighter than the CrossTabLock design commits to. When the leader's IdP round-trip runs longer than the follower's DEFAULT_WAIT_TIMEOUT_MS (10s), the follower falls back through the lock and drives its own refresh — permitted once per cycle by MAX_RECOVERY_ATTEMPTS=1 in AuthCoordinator. Under slow shared CI (keycloak-oidc-confidential row on nightly SSO) the leader's response occasionally overruns 10s and the follower takes that recovery slot, so the counter lands at 2 and the assertion flakes without any real regression. Widen the bound to `<=2` — still catches the N-tab storm the scenario exists to prevent, without failing on the design's own recovery window. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🚦 Removed from the merge queue —
|
…validated-on-restart # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs # openmetadata-ui/src/main/resources/ui/scripts/check-bundle-chunks.mjs
|
…ures The first pass delegated through `setAppState`, which has three error-swallowing layers: an outer catch-all, an inner SW-catch that routes the payload to an in-memory fallback (does not survive reload), and a `swStorageBroken` branch doing the same. So the "strict" variant still resolved on broken IndexedDB, quota, or SW crash — leaving the CrossTabLock leader broadcasting `done` with a payload the next cold-load can't recover. Bypass `setAppState` for the strict path: talk to `swTokenStorage` directly, re-throw any rejection (after `markSwStorageBroken` so other callers still stop paying the controller-wait timeout), refuse to silently accept the in-memory fallback when the SW is already broken, and propagate `localStorage.setItem` quota throws on the no-service-worker path. Adds four tests pinning the propagation contract per failure mode. Greptile P1: r4037800527 (follow-up to r4035047159). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…validated-on-restart
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
| const state = await getAppState(); | ||
| state[OIDC_TOKEN_KEY] = token; | ||
| const stateStr = JSON.stringify(state); | ||
|
|
||
| if (isServiceWorkerAvailable() && !swStorageBroken) { | ||
| try { | ||
| await swTokenStorage.setItem(APP_STATE_KEY, stateStr); | ||
| } catch (error) { | ||
| // Mark broken so other callers stop paying the controller-wait | ||
| // timeout, then re-throw — the in-memory fallback doesn't survive | ||
| // reload, so a strict caller can't treat this as a success. | ||
| markSwStorageBroken(error); | ||
|
|
||
| throw error; | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (swStorageBroken) { | ||
| throw new Error( |
There was a problem hiding this comment.
When the service-worker or IndexedDB read in getAppState fails transiently, it marks storage as broken and returns in-memory state; setOidcTokenStrict then throws without persisting or retaining the successfully renewed token. The coordinator consequently emits refresh-failed and signs the user out even though provider renewal succeeded, while follower retries encounter the same sticky broken-storage state.
Knowledge Base Used: Metadata web application
|
Summary
Two related bodies of work landed on this branch:
1. AuthCoordinator refactor (Bug 1 + Bug 2 permanent fix)
Refactors the SPA silent-refresh path behind a single AuthCoordinator and validates the fixes shipped in the two pre-release hotfixes (#31597, #31644).
Every SSO provider now shares one refresh engine:
visibilitychangehandlerRenewer = () => Promise<{ idToken, expiresAt }>and registers it from its own mount effect (no ref-race)Feature code added:
SilentCallback.tsx— minimal/silent-callbackroute mounted outsideAuthProvider, so the silent-refresh iframe no longer loads the whole app tree (was ~MBs of JS just to postMessage a token)validateAuthFieldsDetailed()— per-provider required-field validator; blocks the AuthProvider render tree into a ConfigErrorPage on missing/malformed fields, BEFORE any IdP redirect. Emits[AuthConfig] <field>console.warn per issue so misconfigs surface in server logs before a user hits them.2. SSO Playwright test refactor (12 new commits)
Consolidates 8 legacy per-provider auth specs into one parametrized
SsoScenarios.spec.tsrunning 9 scenarios × 8 provider fixtures.Providers (real IdPs unless noted):
page.addInitScriptpage.addInitScriptGoogle is manual-only —
playwright/e2e/Auth/manual/Google.mdrunbook. Code path is covered by keycloak-oidc-public with no code-path gap.The 9 scenarios (per provider):
oidcIdTokencleared)supportsCrossTab)/auth/refreshacross tabs)CI wiring — extends
playwright-sso-login-nightly.yml:paths:filter — runs 7-leg matrix (Okta dropped, needs live tenant secrets not on fork PRs)isAvailable()— legs without required secretstest.skip()cleanly with the reason surfaced in the reportCommits
51 commits total on this branch. Highlights:
AuthCoordinatormodule + per-provider renewers + interceptor swap + cold-load fix + Task 13 tests2d789cb,166bd27)394a649)30a541d)d278362)e068078)SsoScenarios.spec.tsscenarios 1-6 (3af5483)4bbb8e4)984982b)a57b511)b2d365a)538e60c)c31e5d7)Test coverage
SsoScenarios.spec.tsparametrized over 8 fixtures — first CI run will validate against real IdPsutils/Auth/AuthCoordinator/: 91% lines / 84% branchesTest plan
yarn test src/components/Auth src/utils/Auth src/hooks/useApplicationStore— 186/187 passyarn lint,npx tsc --noEmit— no new errorsplaywright/e2e/Auth/manual/Google.mdRefs: #31597 (hotfix v1), #31644 (hotfix v2), #31819 (visibility-handler guard on main)
🤖 Generated with Claude Code
The PR is not yet safe to merge because recoverable storage and cross-tab timing conditions can still force logout, and Okta follower tabs retain stale provider state.
Findings
Summary
The PR centralizes provider token renewal in AuthCoordinator, adds cross-tab refresh coordination and strict token persistence, isolates the silent-callback entry, validates authentication configuration, and consolidates SSO Playwright coverage.
Diagram
sequenceDiagram participant F as Follower tab participant L as Cross-tab lock participant P as Provider renewer participant S as Shared token storage F->>L: Wait for active refresh L->>P: Leader renews token P-->>L: Renewed token L->>S: Persist token strictly alt persistence succeeds L-->>F: Broadcast done(token) F->>F: Retry queued requests else persistence fails L-->>F: Broadcast failed F->>L: Retry through lock endReviews (63) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."