feat(ui): AuthCoordinator refactor + full SSO Playwright matrix (9 scenarios × 8 providers) - #31675
feat(ui): AuthCoordinator refactor + full SSO Playwright matrix (9 scenarios × 8 providers)#31675chirag-madlani wants to merge 10 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 failedValidated commit ✅ 4474 passed · ❌ 3 failed · 🟡 6 flaky · ⏭️ 5 skipped · 🧰 0 lifecycle flaky ❌ Action needed: test(s) failed on every attempt against this PR’s validated commit — see Genuine Failures below. These are test failures, not CI budget or infrastructure issues. 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) 36m 8s ⏱️ Max setup 5m 27s · max shard execution 19m 29s · max shard-job elapsed before upload 23m 15s · reporting 20s 🌐 216.59 requests/attempt · 2.31 app boots/UI scenario · 32.68% common-shard skew Optimization targets still in progress:
Genuine Failures (failed on all attempts)❌
|
Review round 1 — addressedP1s (Greptile)
P4s (gitar-bot)
Checkstyle
Test coverage
Warnings intentionally left for a separate cleanup PR: barrel-import + |
|
| Count | Rule |
|---|---|
| 13 | react-hooks/exhaustive-deps |
| 12 | openmetadata-imports/no-internal-barrel-imports |
| 2 | openmetadata-imports/no-hook-ui-imports |
| 1 | openmetadata-imports/no-lower-layer-page-imports |
| 1 | sonarjs/cognitive-complexity |
| 1 | openmetadata-imports/no-cross-page-imports |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Auth/AppAuthenticators/Auth0Authenticator.tsx:23:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.tsx:29:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.tsx:64:10 |
react-hooks/exhaustive-deps |
React Hook useCallback has unnecessary dependencies: 'setOidcToken' and 'setRefreshToken'. Either exclude them or remove the dependency array. Outer scope value |
| 🟡 | src/components/Auth/AppAuthenticators/GenericAuthenticator.tsx:24:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx:28:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx:244:8 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'handleRedirect'. Either include it or remove the dependency array. |
| 🟡 | src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx:30:1 |
openmetadata-imports/no-lower-layer-page-imports |
Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here. |
| 🟡 | src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx:31:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx:23:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.test.tsx:17:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:76:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:87:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:300:9 |
react-hooks/exhaustive-deps |
The 'onLoginHandler' function makes the dependencies of useMemo Hook (at line 927) change on every render. Move it inside the useMemo callback. Alternatively, w |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:380:6 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'navigate', 'setApplicationLoading', 'setCurrentUser', and 'setIsAuthenticated'. Either include them or remove |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:431:9 |
react-hooks/exhaustive-deps |
The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 927) change on every render. To fix this, wrap the definition of 'resetUserDetai |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:548:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'handleStoreProtectedRedirectPath', 'resetUserDetails', and 'setIsAuthenticated'. Either include them or remove t |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:550:9 |
react-hooks/exhaustive-deps |
The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 927) change on every render. Move it inside the useMemo callback. Alternatively |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:606:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has missing dependencies: 'authConfig?.provider', 'handledVerifiedUser', 'navigate', and 'resetUserDetails'. Either include them or remov |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:656:9 |
react-hooks/exhaustive-deps |
The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 927) change on every render. To fix this, wrap the definition of 'ini |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:695:36 |
sonarjs/cognitive-complexity |
Refactor this function to reduce its Cognitive Complexity from 21 to the 15 allowed. |
| 🟡 | src/components/Auth/AuthProviders/AuthProvider.tsx:916:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'fetchAuthConfig'. Either include it or remove the dependency array. |
| 🟡 | src/hooks/useApplicationStore.test.ts:15:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/hooks/useApplicationStore.ts:14:1 |
openmetadata-imports/no-hook-ui-imports |
Hooks must not import components or pages. Move shared logic/types below the UI layer. |
| 🟡 | src/hooks/useApplicationStore.ts:15:1 |
openmetadata-imports/no-hook-ui-imports |
Hooks must not import components or pages. Move shared logic/types below the UI layer. |
| 🟡 | src/hooks/useApplicationStore.ts:23:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
| 🟡 | src/pages/ForgotPassword/ForgotPassword.component.tsx:51:5 |
react-hooks/exhaustive-deps |
React Hook useCallback has a missing dependency: 't'. Either include it or remove the dependency array. |
| 🟡 | src/pages/LoginPage/SignInPage.tsx:149:6 |
react-hooks/exhaustive-deps |
React Hook useMemo has missing dependencies: 'authConfig?.providerName' and 't'. Either include them or remove the dependency array. |
| 🟡 | src/pages/LoginPage/SignInPage.tsx:157:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'navigate'. Either include it or remove the dependency array. |
| 🟡 | src/pages/SignUp/BasicSignup.component.tsx:26:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/utils/SseStreamUtils.ts:14:1 |
openmetadata-imports/no-internal-barrel-imports |
Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed
|
Too many files changed for review (102 files, 100 file limit). Bypass the limit by tagging |
| import { UserManager } from 'oidc-client'; | ||
|
|
||
| new UserManager({}).signinSilentCallback().catch(() => { |
| if (response.status === 401) { | ||
| state.consecutiveUnauthorized += 1; | ||
|
|
||
| if (state.consecutiveUnauthorized > 1) { | ||
| throw new FatalStreamError('down'); | ||
| } | ||
|
|
||
| await TokenService.getInstance().refreshToken(); | ||
| await authCoordinator.ensureFreshToken(); | ||
| } |
| # bitnami/openldap:2.6.7 was removed from the primary Docker Hub org | ||
| # after Bitnami moved older tags to `bitnamilegacy`. Point at that | ||
| # namespace so `docker pull` still succeeds in CI without a version | ||
| # bump. | ||
| image: bitnamilegacy/openldap:2.6.7 | ||
| container_name: openmetadata_openldap | ||
| environment: |
| async ensureFreshToken(): Promise<string> { | ||
| if (this.inflight) { | ||
| return this.inflight; | ||
| } | ||
| this.inflight = this.doRefresh(); | ||
| try { | ||
| return await this.inflight; | ||
| } finally { | ||
| this.inflight = null; | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 101 out of 101 changed files in this pull request and generated 7 comments.
Suppressed comments (1)
docker/development/docker-compose.yml:640
- The PR description says the new OpenLDAP service is “pinned digest”, but the compose file uses a floating tag (
bitnamilegacy/openldap:2.6.7) without an immutable digest. For CI reproducibility and supply-chain hygiene, pin this image to a specific sha256 digest.
# bitnami/openldap:2.6.7 was removed from the primary Docker Hub org
# after Bitnami moved older tags to `bitnamilegacy`. Point at that
# namespace so `docker pull` still succeeds in CI without a version
# bump.
image: bitnamilegacy/openldap:2.6.7
container_name: openmetadata_openldap
| // Scenario 2 — logout clears storage and returns to /signin. The | ||
| // oidcIdToken key is the single source of truth the coordinator reads on | ||
| // cold-load, so leaving it behind would silently re-auth the next tab. | ||
| test('logout', async ({ page }) => { | ||
| test.slow(); | ||
|
|
||
| await fixture.performLogin(page); | ||
| await fixture.performLogout(page); | ||
|
|
||
| await expect(page).toHaveURL(/\/signin$/); | ||
|
|
||
| const remainingToken = await page.evaluate(() => | ||
| localStorage.getItem('oidcIdToken') | ||
| ); | ||
|
|
||
| expect(remainingToken).toBeNull(); | ||
| }); |
|
|
||
| supportsCrossTab: false, | ||
| supportsSelfSignup: false, | ||
| supportsSilentCallback: false, | ||
| usesBackendRefresh: true, |
| const readTestMsalOverride = (): MsalContextShape | undefined => { | ||
| if (typeof window === 'undefined') { | ||
| return undefined; | ||
| } | ||
|
|
||
| return (window as unknown as { __omTestMsal?: MsalContextShape }) | ||
| .__omTestMsal; | ||
| }; |
| const readTestAuth0Override = (): Auth0ContextShape | undefined => { | ||
| if (typeof window === 'undefined') { | ||
| return undefined; | ||
| } | ||
|
|
||
| return (window as unknown as { __omTestAuth0?: Auth0ContextShape }) | ||
| .__omTestAuth0; | ||
| }; |
| } else { | ||
| // Older builds and no-ui mode don't ship this file — leave it null and let the | ||
| // servlet fall through to the SPA shell path if the route is ever hit. | ||
| silentCallbackRawHtml = null; | ||
| } |
| - [ ] **Scenario 2: Logout** — From authenticated state, click profile → | ||
| logout, land on `/signin`, verify `localStorage.getItem('oidcIdToken')` | ||
| is `null` in DevTools. | ||
| - [ ] **Scenario 3: Silent refresh** — In DevTools, run | ||
| `localStorage.setItem('oidcIdToken', <mangled JWT with exp in past>)`, | ||
| then navigate to `/my-data`. Verify Network tab shows a hidden iframe | ||
| calling Google's `/o/oauth2/v2/auth` and the app renders authenticated | ||
| without a redirect. |
| await locks.request( | ||
| this.lockName, | ||
| { mode: 'exclusive', ifAvailable: true }, | ||
| async (lock) => { | ||
| if (!lock) { | ||
| return; | ||
| } | ||
| acquired = true; | ||
| try { | ||
| leaderValue = await work(); | ||
| } catch (err) { | ||
| this.channel.postMessage({ | ||
| type: 'failed', | ||
| reason: err instanceof Error ? err.message : String(err), | ||
| } as LockFailedMessage); | ||
|
|
||
| throw err; | ||
| } | ||
| } | ||
| ); | ||
| if (acquired) { | ||
| return { role: 'leader', value: leaderValue as T }; | ||
| } | ||
| const message = await this.waitForMessage(waitTimeoutMs); | ||
|
|
||
| return { role: 'follower', message }; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 99 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
openmetadata-ui/src/main/resources/ui/src/hooks/useApplicationStore.ts:176
extractDetailsFromToken()can returnexp: undefinedwhen a token has noexpclaim, but the currentisExpiredcheck treats that as expired (!exp) and forces a refresh. This contradicts the coordinator’s own visibility logic (which treats missing/invalidexpas “no actionable expiry”) and can cause unnecessary refresh attempts or an incorrect unauthenticated state on cold load.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/Auth0Authenticator.tsx:114- Typo in comment: “emmit” → “emit”.
openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/SsoScenarios.spec.ts:193
- This logout assertion checks
localStorage['oidcIdToken'], but the app’s token source of truth isapp_state.primarystored via the service worker/IndexedDB (seeSwTokenStorageUtils). As written, the test can pass even if logout fails to clear the real stored token.
// Scenario 2 — logout clears storage and returns to /signin. The
// oidcIdToken key is the single source of truth the coordinator reads on
// cold-load, so leaving it behind would silently re-auth the next tab.
test('logout', async ({ page }) => {
test.slow();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 102 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/Auth0Authenticator.tsx:126
- Typo in comment: “emmit” → “emit”.
docker/development/docker-compose.yml:640
- The PR description claims the OpenLDAP service image is pinned by digest, but the compose file uses a mutable tag (
bitnamilegacy/openldap:2.6.7). For CI supply-chain stability and reproducibility, pin to an immutable@sha256:digest (or update the PR description if digest pinning is intentionally not done).
# bitnami/openldap:2.6.7 was removed from the primary Docker Hub org
# after Bitnami moved older tags to `bitnamilegacy`. Point at that
# namespace so `docker pull` still succeeds in CI without a version
# bump.
image: bitnamilegacy/openldap:2.6.7
container_name: openmetadata_openldap
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 102 out of 102 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
docker/development/docker-compose.yml:640
- PR description says the new OpenLDAP service is “pinned digest”, but the compose file uses a mutable tag (
bitnamilegacy/openldap:2.6.7). For supply-chain reproducibility and to match the stated behavior, this should be pinned to a sha256 digest (and ideally documented/updated in the same change).
# bitnami/openldap:2.6.7 was removed from the primary Docker Hub org
# after Bitnami moved older tags to `bitnamilegacy`. Point at that
# namespace so `docker pull` still succeeds in CI without a version
# bump.
image: bitnamilegacy/openldap:2.6.7
container_name: openmetadata_openldap
| const { exp } = extractDetailsFromToken(token); | ||
| const isExpired = | ||
| !exp || exp * 1000 - Date.now() < EXPIRY_THRESHOLD_MILLES; | ||
|
|
||
| if (!isExpired) { | ||
| set({ isAuthenticated: true, isAuthenticating: false }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
🔵 Needs a closer look
initializeAuthState currently forces an eager refresh for tokens with missing/non-positive exp, which can cause unnecessary refresh attempts or logout on cold load for opaque/non-standard tokens.
Review details
Suppressed comments (2)
openmetadata-ui/src/main/resources/ui/src/hooks/useApplicationStore.ts:177
initializeAuthStatetreats any token with a missing / non-positiveexpas "expired" and forcesauthCoordinator.ensureFreshToken()on cold load (!expbranch). This contradicts the coordinator’s own visibility gating (it treats missing/opaque/undecodableexpas “no valid expiry” and waits for a real 401) and can cause unnecessary refresh attempts (or immediate logout) for opaque tokens / tokens without anexpclaim.
docker/development/docker-compose.yml:640- The PR description says the new OpenLDAP service is “pinned digest”, but the compose definition uses a mutable tag (
bitnamilegacy/openldap:2.6.7). For supply-chain reproducibility and to match the stated intent, this should be pinned by digest (or the PR description updated).
# bitnami/openldap:2.6.7 was removed from the primary Docker Hub org
# after Bitnami moved older tags to `bitnamilegacy`. Point at that
# namespace so `docker pull` still succeeds in CI without a version
# bump.
image: bitnamilegacy/openldap:2.6.7
container_name: openmetadata_openldap
- Files reviewed: 102/102 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Squash of the branch's 42 commits into a single PR unit — see
git reflog on this branch for the incremental history if a bisect
lands here.
## Auth runtime — AuthCoordinator singleton (openmetadata-ui/.../utils/Auth/AuthCoordinator)
- Introduce `AuthCoordinator` with `RefreshQueue`, `ProactiveTimer`,
`CrossTabLock` (Web Locks + BroadcastChannel), `VisibilityWatcher`,
and a typed event bus. Central coordinator owns every silent-refresh
path; the axios 401 interceptor and each authenticator adapter go
through it instead of `TokenServiceUtil`.
- `CrossTabLock` attaches its BroadcastChannel listener BEFORE
`locks.request(ifAvailable:true)` runs. A leader whose refresh
finished in the microseconds between the request returning and the
listener attaching would otherwise post `done` into a void
(BroadcastChannel does not queue for late subscribers) and the
follower would time out and run a redundant refresh, breaking the
"exactly one /auth/refresh across tabs" guarantee.
- `ProactiveTimer` skips scheduling on `expiresAt <= 0`.
- `VisibilityWatcher` distinguishes "no exp yet" from "already-fresh"
so tab-focus does not fire a refresh when the token is either
unknown or comfortably fresh.
- `AuthCoordinator` waits for the renewer to register on cold-load;
the coordinator boot path refreshes expired tokens before flipping
`isAuthenticated`, closing the "cold-load with expired token"
window that used to render the app with a dead token and 401 the
first API call.
- `useApplicationStore.initializeAuthState` re-runs on callback routes
(`/callback`, `/auth/callback`) instead of bailing early; the
previous early-return stranded `isAuthenticating: true` so the
top-level loader gate blocked `SamlCallback` from ever mounting.
## Per-authenticator renewers
- Register a `Renewer` contract on every authenticator so the
coordinator can drive silent refresh through the correct SDK:
`BasicAuthAuthenticator`, `GenericAuthenticator` (SAML +
confidential OIDC), `OidcAuthenticator`, `MsalAuthenticator`,
`OktaAuthenticator`, `Auth0Authenticator`.
- `MsalAuthenticator` guards `handleRedirectPromise` against
StrictMode double-invocation.
- `OktaAuthenticator` mirrors the coordinator's fresh tokens back
into `tokenManager` so the Okta SDK's own consumers see the new
values.
- `OidcAuthenticator` drops the iframe double-write in the silent-
callback path; the coordinator owns state transitions now.
## /silent-callback → dedicated HTML entry
- Move the silent-refresh iframe route off the SPA shell to a
dedicated Vite entry (`silent-callback.html` +
`silentCallbackEntry.ts`). Its bundle graph is exactly `oidc-client`
+ the tiny bootstrap — no React, no Antd, none of the shared
app-utils that Rollup's `experimentalMinChunkSize` merger folds into
the SPA entry. `OpenMetadataAssetServlet` routes extensionless
`/silent-callback` (with or without base-path) to this HTML;
`IndexResource` caches the raw body with per-basePath substitution.
- `MsalAuthenticator` / `Auth0Authenticator` shim reads
(`window.__omTestMsal`, `window.__omTestAuth0`) are additionally
gated on `isPlaywrightBuild()` — reads Vite's build-time
`PW_E2E_BUILD` inlined by the `define:` block, isolated in its own
module so ts-jest doesn't have to parse `import.meta`. The
SSO-login-nightly workflow now sets `PW_E2E_BUILD=true` on its
setup step so the mocked-SDK shims arm.
## Auth config validation gate + toast
- Replace the old ConfigErrorPage full-screen block with a
`showErrorToast` in `AuthProvider` when the fetched config fails
`validateAuthFieldsDetailed`. Missing top-level fields no longer
hard-block the whole SPA.
- Track `hasValidConfig` state so Azure with an empty `clientId`
falls through to `SignInPage` (MSAL's `PublicClientApplication
.initialize()` rejects an empty clientId, previously leaving the
shell stuck on `<Loader />`).
## Playwright SSO matrix — SsoScenarios.spec.ts
- New `SsoProviderFixture` interface + fixtures for basic, ldap,
keycloak-{saml, oidc-confidential, oidc-public}, okta, msal-mock,
auth0-mock.
- Nine matrixed scenarios (login, logout, silent refresh, multi-tab
shared auth, cross-tab lock coalescing, cold-load with expired
token, silent-callback bundle budget, self-signup, session-limit
guard).
- Scenarios registered conditionally on fixture capability flags
(`usesBackendRefresh`, `supportsCrossTab`, `supportsSilentCallback`)
so unsupported (provider, scenario) pairs never enrol — no
runtime `test.skip()` calls.
- Logout scenario reads tokens from the SW/IndexedDB `AppDataStore`
DB, `keyValueStore` store, `app_state` key (JSON with `primary`
field) — the actual storage path, not the legacy
`localStorage['oidcIdToken']` key.
- Delete legacy `SSOAuthentication.spec.ts`,
`OktaSessionRenewalPublic.spec.ts`, `SSOLogin.spec.ts`,
`SSORenewal.spec.ts` — replaced by the new matrix.
- New nightly workflow `playwright-sso-login-nightly.yml` with an
8-way provider matrix; PR trigger paths scoped to auth-touching
BE files (`JwtFilter.java`, `AuthenticationCodeFlowHandler.java`,
`AuthServeletHandlerFactory.java`, `SecurityUtil.java`, security
`auth/` and `saml/` packages) so unrelated PRs don't spin up a
40 m docker leg.
## docker-compose
- Add `openldap` service under the `sso-playwright` compose profile
so the LDAP fixture runs against a real slapd, not a mock.
- Bootstrap LDIF seeds `ldapuser` / `openldap` under
`dc=openmetadata,dc=org`.
## Docs
- Manual runbook (`playwright/e2e/Auth/manual/Google.md`) walks the
tester through the SW/IndexedDB `AppDataStore -> keyValueStore ->
app_state` path plus the localStorage fallback, matching the
actual storage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ui-checkstyle src-eslint step runs `yarn organize-imports:cli`, `yarn lint:base --fix`, and `yarn pretty:base --write` on changed files and then checks `git status --porcelain`. My wrapped three-line `if (err instanceof LockTimeoutError && attempt < MAX_RECOVERY_ATTEMPTS)` on `AuthCoordinator.ts` fits under the prettier printWidth, so the autofix pass collapsed it to a single line and the git-status guard flagged the file. Applying the prettier form directly here to close the loop; no runtime change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…validated-on-restart # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs # openmetadata-ui/src/main/resources/ui/src/utils/AuthProvider.util.ts
…validated-on-restart
…validated-on-restart # Conflicts: # openmetadata-ui/src/main/resources/ui/vite.config.ts
…validated-on-restart # Conflicts: # openmetadata-ui/src/main/resources/ui/eslint-suppressions.json # openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/OktaSessionRenewalPublic.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/SSOAuthentication.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/SSOLogin.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/SSORenewal.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/e2e/Auth/SSOSelfSignup.spec.ts # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs # openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx
…n-restart' into azure-oidc-session-invalidated-on-restart
…validated-on-restart
…validated-on-restart # Conflicts: # openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
|
|
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
Greptile Summary
The PR centralizes SPA token renewal in AuthCoordinator, adds lightweight silent-callback and authentication-configuration handling, and replaces provider-specific SSO tests with a parametrized Playwright matrix.
Confidence Score: 1/5
The PR does not appear safe to merge while cross-tab renewal can still advertise unpersisted tokens, miss leader completion, force logout after follower fallback failure, and leave Okta follower state stale.
Renewed-token storage failures remain invisible to AuthCoordinator, the follower listener is still installed after a completion message can be lost, failed independent fallback can still clear an otherwise recoverable session, and follower-side Okta SDK state is not synchronized with the broadcast token.
Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/utils/Auth/AuthCoordinator/AuthCoordinator.ts; openmetadata-ui/src/main/resources/ui/src/utils/Auth/AuthCoordinator/CrossTabLock.ts; openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx
Important Files Changed
Sequence Diagram
Reviews (52): Last reviewed commit: "refactor(sso): drop test.skip() from Sso..." | Re-trigger Greptile
Context used (4)