OCPBUGS-105789: Recover expired Playwright sessions via auto re-login #16954
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| import type { Frame, Page, TestInfo } from '@playwright/test'; | ||
|
|
||
| import { | ||
| isOnLoginPage, | ||
| performLogin, | ||
| resolveCredentialsForStorageState, | ||
| saveStorageState, | ||
| } from '../setup/login-helper'; | ||
|
|
||
| /** | ||
| * Guards against re-entrant / concurrent re-login attempts on the same page. | ||
| */ | ||
| const reloginInProgress = new WeakMap<Page, Promise<void>>(); | ||
|
|
||
| /** | ||
| * The last app route each page navigated to before any auth redirect. Used to | ||
| * send the page back where the test intended to be after re-authenticating, | ||
| * since by the time recovery runs the page URL is the OAuth/login page. | ||
| */ | ||
| const lastAppUrl = new WeakMap<Page, string>(); | ||
|
|
||
| function storageStatePath(testInfo: TestInfo): string | undefined { | ||
| const state = testInfo.project.use.storageState; | ||
| return typeof state === 'string' ? state : undefined; | ||
| } | ||
|
|
||
| /** | ||
| * True for OAuth server / console login URLs — the pages a session-expiry | ||
| * redirect passes through. These are never the route a test wants to resume at. | ||
| */ | ||
| function isAuthUrl(url: string): boolean { | ||
| return /\/oauth\/|\/oauth2\/|\/login(\/|$|\?)|\/auth\//.test(url); | ||
| } | ||
|
|
||
| /** | ||
| * Detects when a navigation has landed on the OAuth login page — meaning the | ||
| * session snapshot in storageState has expired — and transparently | ||
| * re-authenticates the current persona, refreshing the stored session so | ||
| * subsequent tests reuse the fresh state. After re-login, navigates back to | ||
| * the route the test was heading to so deep-link tests resume in place. | ||
| * | ||
| * The setup projects establish the session once; long runs can outlive the | ||
| * OAuth token, after which every navigation silently redirects to the login | ||
| * page and tests hang waiting for elements that never appear. This fixture is | ||
| * the self-healing recovery for that case. | ||
| */ | ||
| export async function recoverSessionIfExpired( | ||
| page: Page, | ||
| testInfo: TestInfo, | ||
| detectTimeoutMs = 0, | ||
| ): Promise<boolean> { | ||
| // If a re-login is already running (or being set up) for this page, await it | ||
| // rather than starting a second one. | ||
| const existing = reloginInProgress.get(page); | ||
| if (existing) { | ||
| await existing; | ||
| return true; | ||
| } | ||
|
|
||
| // Claim the re-login slot synchronously — before any await — so concurrent | ||
| // navigation events can't both pass the check above and launch duplicate | ||
| // logins. The deferred is published now and settled once we know whether a | ||
| // recovery is actually needed; if not, we release the slot immediately. | ||
| let release!: () => void; | ||
| const claim = new Promise<void>((resolve) => { | ||
| release = resolve; | ||
| }); | ||
| reloginInProgress.set(page, claim); | ||
|
Comment on lines
+103
to
+119
Contributor
There was a problem hiding this comment. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- storage-state configuration ---'
fd -i 'playwright.config.*|package.json' frontend -x sh -c \
'printf "\n--- %s ---\n" "$1"; rg -n -C 3 "storageState|workers|fullyParallel" "$1" || true' sh {}
printf '%s\n' '--- storage-state persistence implementation ---'
rg -n -C 5 --glob '*.ts' \
'function saveStorageState|const saveStorageState|saveStorageState\(|storageState\(\{.*path' \
frontend/e2eRepository: openshift/console Length of output: 6073 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- playwright projects and storage-state paths ---'
sed -n '1,190p' frontend/playwright.config.ts
printf '%s\n' '--- auth fixture and fixture scopes ---'
sed -n '1,150p' frontend/e2e/fixtures/auth-fixture.ts
sed -n '1,180p' frontend/e2e/fixtures/index.ts
printf '%s\n' '--- storage-state path definitions and save helper ---'
rg -n -C 8 --glob '*.ts' \
'adminStorageState|developerStorageState|STORAGE_STATE_DIR|storageState\(\{ path|saveStorageState' frontend/e2e frontend/playwright.config.tsRepository: openshift/console Length of output: 22794 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Playwright version ---'
rg -n -C 2 '"`@playwright/test`"|"playwright"' frontend/package.json package.json frontend/yarn.lock yarn.lock pnpm-lock.yaml package-lock.json 2>/dev/null || true
printf '%s\n' '--- repository-level synchronization around auth-state writes ---'
rg -n -C 4 --glob '!node_modules/**' \
'kubeadmin\.json|developer\.json|storageState\(\{ *path|saveStorageState|lockfile|proper-lockfile|mkdir.*lock|writeFile.*storage' .Repository: openshift/console Length of output: 16321 🌐 Web query:
💡 Result: The Playwright Citations:
Serialize storage-state writes by
🤖 Prompt for AI Agents
Comment on lines
+108
to
+119
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Propagate the actual recovery result to joiners. The shared Use Proposed fix- let release!: () => void;
+ let release!: (recovered: boolean) => void;
...
- const claim = new Promise<void>((resolve, reject) => {
+ const claim = new Promise<boolean>((resolve, reject) => {
...
if (existing) {
- await existing;
- return true;
+ return existing;
}
...
- release();
+ release(false);
...
- release();
+ release(false);
...
- release();
+ release(true);Also update the Also applies to: 117-118 🤖 Prompt for AI Agents |
||
|
|
||
| const finish = (): void => { | ||
| release(); | ||
| reloginInProgress.delete(page); | ||
| }; | ||
|
|
||
| if (!(await isOnLoginPage(page, detectTimeoutMs))) { | ||
| finish(); | ||
| return false; | ||
| } | ||
|
|
||
| const statePath = storageStatePath(testInfo); | ||
| const credentials = statePath ? resolveCredentialsForStorageState(statePath) : null; | ||
| if (!credentials) { | ||
| // No credentials to recover with (e.g. auth disabled or dev creds unset). | ||
| finish(); | ||
| return false; | ||
| } | ||
|
|
||
| const intendedUrl = lastAppUrl.get(page); | ||
| try { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `[auth] Session expired for "${testInfo.titlePath.join(' > ')}"; re-authenticating.`, | ||
| ); | ||
| await performLogin(page, credentials.username, credentials.password, credentials.idpName); | ||
| if (statePath) { | ||
| await saveStorageState(page, statePath); | ||
| } | ||
| // Restore the route the test was navigating to before the redirect, so | ||
| // deep-link tests resume where they expected rather than on the console | ||
| // home page that performLogin lands on. | ||
| if (intendedUrl && intendedUrl !== page.url()) { | ||
| await page.goto(intendedUrl, { waitUntil: 'domcontentloaded' }); | ||
| } | ||
| } finally { | ||
| finish(); | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Attaches a main-frame navigation listener that re-authenticates whenever a | ||
| * navigation lands on the login page. Returns a disposer to detach it. | ||
| */ | ||
| export function attachSessionRecovery(page: Page, testInfo: TestInfo): () => void { | ||
| const handler = (frame: Frame) => { | ||
| if (frame !== page.mainFrame()) { | ||
| return; | ||
| } | ||
| const url = frame.url(); | ||
| const onAuthUrl = isAuthUrl(url); | ||
| // Remember the most recent non-auth route so recovery can return to it. | ||
| if (url && url !== 'about:blank' && !onAuthUrl) { | ||
| lastAppUrl.set(page, url); | ||
| } | ||
| // On a normal (non-auth) navigation, detect the login page instantly so the | ||
| // hot path adds no latency. When we land on an auth URL the session likely | ||
| // expired but the login form may still be rendering, so give it a bounded | ||
| // window to appear before deciding recovery isn't needed. | ||
| // Fire-and-forget: recovery guards its own re-entrancy. Swallow errors so a | ||
| // transient navigation event doesn't reject an unrelated step. | ||
| void recoverSessionIfExpired(page, testInfo, onAuthUrl ? 5_000 : 0).catch(() => { | ||
| /* best-effort recovery */ | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }; | ||
| page.on('framenavigated', handler); | ||
| return () => page.off('framenavigated', handler); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Anchor authentication URL matching to the pathname.
The current expression matches
/oauth/and/auth/anywhere in the full URL. A normal application route or query value can then be classified as an auth URL. This preventslastAppUrlupdates and can start unnecessary recovery.Parse the URL, normalize its pathname, and match the complete auth-path shape.
Proposed fix
function isAuthUrl(url: string): boolean { - return /\/oauth\/|\/oauth2\/|\/login(\/|$|\?)|\/auth\//.test(url); + const pathname = new URL(url).pathname.normalize('NFC'); + return /^\/(?:oauth2?|login|auth)(?:\/.*)?$/.test(pathname); }As per path instructions, “Normalize Unicode and anchor regexes (^$); watch for ReDoS.”
📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions