From 56361ddab4515bc08d5de41a44ec3b4ad2f99192 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Fri, 21 Aug 2026 14:59:46 -0300 Subject: [PATCH 1/7] fix(nfs): make a lane that is not actually running NFS fail instead of pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found while triaging the overlay migration, all with the same shape: the harness cannot tell a working NFS lane from one that quietly fell back to the legacy shell. RHIDP-16457 — a workspace could disable the NFS it asked for. The NFS secret layer merges before the workspace's own tests/config/rhdh-secrets.yaml, so a workspace setting APP_CONFIG_app_packageName or ENABLE_STANDARD_MODULE_FEDERATION for its own reasons overrode it. Nothing then failed: the legacy suite re-ran and passed, and the only thing the lane existed to prove was never exercised. deploy() now checks the merged secret still carries both markers and throws naming the key, the value found, and the file responsible. The merge order is untouched — a workspace has to be able to override defaults, which is exactly why the outcome needs checking rather than assuming. Also from RHIDP-16457, and from RHIDP-16461: configure() now logs which of the three mechanisms decided the shell, because none of them is visible from a single file and the answer currently requires reading a project name, an environment variable and a configure() call together. A namespace ending in -app-next that was explicitly configured with useNewFrontendSystem: false warns, since it deploys legacy under a name that reads as NFS everywhere. Only that direction is checked: enabling NFS without the suffix is legitimate and is what github and homepage do. RHIDP-16459 — the GitHub session file was a bare relative authState_.json, resolved against process.cwd(), which the worker fixture sets to the same workspace directory for every project. A workspace's lanes shared one file per user, unlocked: one lane could inject another's storage state, and a reader landing mid-write died on truncated JSON as a flake that looked nothing like the plugin under test. Every lane added is another writer. The path is now absolute and keyed by project as well as user, the write goes through a temp file and a rename, and an unusable session falls through to a full login rather than throwing. RHIDP-16458 — SidebarTabs hardcoded "Self-service", which is the legacy shell's scaffolder title; under app-next the page is "Create", so a spec running there could not type-check without a cast. Both are members now. Deliberately only that: which label a lane should use is RHIDP-16462, three workarounds are in flight for it, and picking one here would be a fourth. All four new behaviours are mutation-tested — reverting each turns the matching tests red and nothing else. 136 tests pass; typecheck, eslint and prettier are clean; the docs build validates its own links. Co-Authored-By: Claude Opus 5 (1M context) --- docs/changelog.md | 18 ++- package.json | 2 +- src/deployment/rhdh/deployment.ts | 35 +++++ src/deployment/rhdh/nfs-guard.test.ts | 145 ++++++++++++++++++ src/deployment/rhdh/nfs-guard.ts | 137 +++++++++++++++++ src/playwright/helpers/common.ts | 75 ++++++++- src/playwright/helpers/github-session.test.ts | 82 ++++++++++ src/playwright/helpers/navbar.ts | 12 ++ 8 files changed, 497 insertions(+), 9 deletions(-) create mode 100644 src/deployment/rhdh/nfs-guard.test.ts create mode 100644 src/deployment/rhdh/nfs-guard.ts create mode 100644 src/playwright/helpers/github-session.test.ts diff --git a/docs/changelog.md b/docs/changelog.md index 09325a2..4a585fa 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,7 +2,23 @@ All notable changes to this project will be documented in this file. -## [2.1.9] - Current +## [2.1.11] - Current + +> Takes 2.1.11 rather than 2.1.10 because #149 claims 2.1.10. Whichever of the two +> merges second needs its version and this heading moved up: the Version Bump Check +> compares against `main` only, so it cannot see the collision. + +### Added + +- **A lane now says which shell it runs, and where that was decided** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): `configure()` logs `[nfs] : new frontend system ON|off, from `. Three mechanisms can enable NFS — the `-app-next` project name, `USE_NEW_FRONTEND_SYSTEM=true`, and `configure({ useNewFrontendSystem: true })` — and none of them is visible from a single file, so answering "is this lane NFS?" meant reading three. A namespace ending in `-app-next` that was explicitly configured with `useNewFrontendSystem: false` also warns, because it deploys the legacy shell under a name that reads as an NFS lane in every report. Only that direction is checked: a lane that enables NFS without the suffix is legitimate and common. +- **`SidebarTabs` accepts both shells' scaffolder label** ([RHIDP-16458](https://redhat.atlassian.net/browse/RHIDP-16458)): the union hardcoded `"Self-service"`, the legacy shell's title. Under app-next the same page is `"Create"`, so a spec running there could not type-check without a cast. Both are members now. This does not decide which label a lane should use — that is [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462), and three different workarounds are in flight for it. + +### Fixed + +- **A workspace could silently disable the new frontend system it asked for** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): the NFS secret layer is merged *before* the workspace's own `tests/config/rhdh-secrets.yaml`, so a workspace setting `APP_CONFIG_app_packageName` or `ENABLE_STANDARD_MODULE_FEDERATION` for its own reasons overrode it and the lane booted the legacy shell. Nothing failed — the legacy suite re-ran and passed, and the one thing the lane existed to prove was never exercised. `deploy()` now checks the merged secret still carries both markers and throws naming the key, the value found, and the file that overrode it. The merge order itself is unchanged: a workspace must be able to override defaults, which is precisely why the outcome needs checking rather than assuming. +- **The GitHub session file was shared across projects with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. A workspace's lanes therefore shared one file per user: one lane could inject another's storage state, and a reader landing mid-write failed on truncated JSON as a flake that looked nothing like the plugin under test. Every added lane adds a writer, so the migration makes it worse. The path is now absolute and keyed by project as well as user, the write goes through a temp file and a rename, and an unreadable or empty session falls through to a full login instead of throwing. + +## [2.1.9] ### Changed diff --git a/package.json b/package.json index 2c158d6..054cbef 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@red-hat-developer-hub/e2e-test-utils", - "version": "2.1.9", + "version": "2.1.11", "description": "Test utilities for RHDH E2E tests", "license": "Apache-2.0", "repository": { diff --git a/src/deployment/rhdh/deployment.ts b/src/deployment/rhdh/deployment.ts index bbfc5cc..a58488b 100644 --- a/src/deployment/rhdh/deployment.ts +++ b/src/deployment/rhdh/deployment.ts @@ -1,5 +1,10 @@ import { KubernetesClientHelper } from "../../utils/kubernetes-client.js"; import { WorkspacePaths } from "../../utils/workspace-paths.js"; +import { + assertNfsMarkersSurvived, + describeNfsIntentConflict, + describeNfsSource, +} from "./nfs-guard.js"; import { $ } from "../../utils/bash.js"; import yaml from "js-yaml"; import os from "os"; @@ -120,6 +125,16 @@ export class RHDHDeployment { const secretPayload = substituted as Record; + if (this.deploymentConfig.useNewFrontendSystem) { + // The workspace's own secrets file merges last, so the NFS markers can be + // overwritten here and nothing downstream would notice: the lane would boot + // the legacy shell and pass. + assertNfsMarkersSurvived( + (secretPayload as { stringData?: Record }).stringData, + this.deploymentConfig.namespace, + ); + } + await this.k8sClient.applySecretFromObject( "rhdh-secrets", secretPayload as { stringData?: Record }, @@ -535,11 +550,31 @@ export class RHDHDeployment { this.deploymentConfig = this._buildDeploymentConfig(deploymentOptions); this.rhdhUrl = this._buildBaseUrl(); } + this._reportFrontendSystem(deploymentOptions?.useNewFrontendSystem); await this.k8sClient.createNamespaceIfNotExists( this.deploymentConfig.namespace, ); } + /** + * Says which shell this lane will run, and where that was decided. + * + * Three mechanisms can turn NFS on and none of them is visible from a single + * file, so "is this lane NFS?" is otherwise answered by reading a project name, + * an environment variable and a `configure()` call together. Logging it once at + * configure time puts the answer in the lane's own output. + */ + private _reportFrontendSystem(explicitChoice: boolean | undefined): void { + const { namespace, useNewFrontendSystem } = this.deploymentConfig; + const conflict = describeNfsIntentConflict(namespace, explicitChoice); + if (conflict) console.warn(conflict); + console.log( + `[nfs] ${namespace}: new frontend system ` + + `${useNewFrontendSystem ? "ON" : "off"}, from ` + + `${describeNfsSource(namespace, explicitChoice)}`, + ); + } + private _buildBaseUrl(): string { const prefix = this.deploymentConfig.method === "helm" diff --git a/src/deployment/rhdh/nfs-guard.test.ts b/src/deployment/rhdh/nfs-guard.test.ts new file mode 100644 index 0000000..25274a6 --- /dev/null +++ b/src/deployment/rhdh/nfs-guard.test.ts @@ -0,0 +1,145 @@ +import { describe, it } from "node:test"; +import assert from "node:assert"; +import { + NFS_SECRET_MARKERS, + assertNfsMarkersSurvived, + describeNfsIntentConflict, + describeNfsSource, + findDroppedNfsMarkers, +} from "./nfs-guard.js"; + +const [PACKAGE_NAME_KEY] = NFS_SECRET_MARKERS[0]; +const [MF_KEY] = NFS_SECRET_MARKERS[1]; + +/** A merged secret in which both markers survived, plus unrelated workspace data. */ +const intact = (): Record => ({ + ...Object.fromEntries(NFS_SECRET_MARKERS), + ["SOME_TOKEN"]: "x", +}); + +describe("nfs secret markers", () => { + it("finds nothing dropped when both markers survive the merge", () => { + assert.deepStrictEqual(findDroppedNfsMarkers(intact()), []); + }); + + it("reports a marker the workspace overwrote", () => { + // tests/config/rhdh-secrets.yaml merges after the NFS defaults, so this is + // what a workspace setting the key for its own reasons produces. + const dropped = findDroppedNfsMarkers({ + ...intact(), + [PACKAGE_NAME_KEY]: "app", + }); + assert.deepStrictEqual(dropped, [ + { key: PACKAGE_NAME_KEY, expected: "app-next", actual: "app" }, + ]); + }); + + it("reports a marker that is absent, distinctly from one that is wrong", () => { + const rest = intact(); + delete rest[MF_KEY]; + const dropped = findDroppedNfsMarkers(rest); + assert.strictEqual(dropped.length, 1); + assert.strictEqual(dropped[0].actual, undefined); + }); + + it("accepts an unquoted YAML true, which is the same secret value", () => { + // The guard is looking for a *different* value, not for a typing mistake: + // unquoted `true` in YAML parses as a boolean but serializes into the Secret + // as "true" and enables module federation just the same. Comparing without + // coercing would raise a false alarm on a working lane. + const dropped = findDroppedNfsMarkers({ + ...intact(), + [MF_KEY]: true, + }); + assert.deepStrictEqual(dropped, []); + }); + + it("counts a missing stringData block as both markers dropped", () => { + assert.strictEqual(findDroppedNfsMarkers(undefined).length, 2); + }); + + it("throws naming the namespace, the key, and where the override comes from", () => { + assert.throws( + () => + assertNfsMarkersSurvived( + { ...intact(), [PACKAGE_NAME_KEY]: "app" }, + "bulk-import-app-next", + ), + (err: Error) => { + assert.match(err.message, /bulk-import-app-next/); + assert.match(err.message, /APP_CONFIG_app_packageName is "app"/); + assert.match(err.message, /rhdh-secrets\.yaml is merged after/); + return true; + }, + ); + }); + + it("does not throw when the markers are intact", () => { + assert.doesNotThrow(() => + assertNfsMarkersSurvived(intact(), "ws-app-next"), + ); + }); +}); + +describe("nfs intent conflict", () => { + it("flags an -app-next namespace explicitly opted out of NFS", () => { + const msg = describeNfsIntentConflict("quay-app-next", false); + assert.match(String(msg), /named -app-next/); + }); + + it("says nothing when the name and the intent agree", () => { + assert.strictEqual( + describeNfsIntentConflict("quay-app-next", true), + undefined, + ); + assert.strictEqual(describeNfsIntentConflict("quay", false), undefined); + }); + + it("says nothing when no explicit choice was made", () => { + // The default path resolves to NFS from the name, so there is no conflict. + assert.strictEqual( + describeNfsIntentConflict("quay-app-next", undefined), + undefined, + ); + }); + + it("does not flag the reverse, which is legitimate", () => { + // github and homepage enable NFS through configure() without renaming, and a + // global USE_NEW_FRONTEND_SYSTEM may turn it on everywhere. + assert.strictEqual(describeNfsIntentConflict("github", true), undefined); + }); +}); + +describe("nfs source", () => { + it("names an explicit choice over the project name", () => { + assert.match(describeNfsSource("ws-app-next", true), /configure\(/); + }); + + it("names the project name when nothing was passed", () => { + assert.match(describeNfsSource("ws-app-next", undefined), /-app-next/); + }); + + it("names the environment variable when it is what decided", () => { + const prev = process.env.USE_NEW_FRONTEND_SYSTEM; + process.env.USE_NEW_FRONTEND_SYSTEM = "true"; + try { + assert.match( + describeNfsSource("ws", undefined), + /USE_NEW_FRONTEND_SYSTEM/, + ); + } finally { + if (prev === undefined) delete process.env.USE_NEW_FRONTEND_SYSTEM; + else process.env.USE_NEW_FRONTEND_SYSTEM = prev; + } + }); + + it("says so when nothing enabled it", () => { + const prev = process.env.USE_NEW_FRONTEND_SYSTEM; + delete process.env.USE_NEW_FRONTEND_SYSTEM; + try { + assert.match(describeNfsSource("ws", undefined), /legacy shell/); + } finally { + if (prev !== undefined) process.env.USE_NEW_FRONTEND_SYSTEM = prev; + } + }); +}); diff --git a/src/deployment/rhdh/nfs-guard.ts b/src/deployment/rhdh/nfs-guard.ts new file mode 100644 index 0000000..22e0fa0 --- /dev/null +++ b/src/deployment/rhdh/nfs-guard.ts @@ -0,0 +1,137 @@ +/** + * Guards against a lane that believes it is running the new frontend system + * while the deployed instance is running the legacy shell. + * + * That combination is the worst failure this harness can produce, because it is + * not a failure: the legacy suite re-runs, every assertion passes, and the only + * thing the lane exists to prove — that the plugin works under NFS — was never + * exercised. Nothing errors and nothing is red. + * + * The checks here are deliberately cluster-free. They run on the configuration + * this library is about to apply, which is where the two reachable causes live. + */ + +/** + * The secret entries that actually switch RHDH to the new frontend system. + * + * Mirrors `config/new-frontend-system/secrets.yaml`. `APP_CONFIG_app_packageName` + * selects the `app-next` bundle; `ENABLE_STANDARD_MODULE_FEDERATION` is what stops + * RHDH overriding the dynamic-features service away from standard module federation. + * + * Entries rather than an object because these are external environment-variable + * names, not JavaScript identifiers — writing them as property names would fight + * the repo's camelCase rule everywhere they appear. + */ +export const NFS_SECRET_MARKERS: ReadonlyArray = [ + ["APP_CONFIG_app_packageName", "app-next"], + ["ENABLE_STANDARD_MODULE_FEDERATION", "true"], +]; + +export type DroppedMarker = { + key: string; + expected: string; + /** `undefined` when the key is absent rather than overwritten. */ + actual: string | undefined; +}; + +/** + * Markers that did not survive the secret merge. + * + * The NFS layer is merged *before* the workspace's own `rhdh-secrets.yaml`, so a + * workspace that sets either key for its own reasons silently wins and the lane + * boots legacy. That ordering is intentional — a workspace must be able to override + * defaults — which is exactly why the outcome has to be checked rather than assumed. + */ +export function findDroppedNfsMarkers( + stringData: Record | undefined, +): DroppedMarker[] { + const dropped: DroppedMarker[] = []; + for (const [key, expected] of NFS_SECRET_MARKERS) { + const raw = stringData?.[key]; + const actual = raw === undefined ? undefined : String(raw); + if (actual !== expected) dropped.push({ key, expected, actual }); + } + return dropped; +} + +/** + * Throws when the applied secret would not turn NFS on after all. + * + * @param stringData - the merged `stringData` about to be applied + * @param namespace - named in the message, because the reader is looking at one + * lane's output among many + */ +export function assertNfsMarkersSurvived( + stringData: Record | undefined, + namespace: string, +): void { + const dropped = findDroppedNfsMarkers(stringData); + if (dropped.length === 0) return; + + const detail = dropped + .map(({ key, expected, actual }) => + actual === undefined + ? ` ${key} is missing (expected "${expected}")` + : ` ${key} is "${actual}" (expected "${expected}")`, + ) + .join("\n"); + + throw new Error( + `[nfs] "${namespace}" is configured for the new frontend system, but the ` + + `secret about to be applied would not enable it:\n${detail}\n` + + `The workspace's own tests/config/rhdh-secrets.yaml is merged after the NFS ` + + `defaults, so setting either key there overrides them. Remove it, or set it ` + + `to the value above. Without this the lane boots the legacy shell and passes ` + + `while proving nothing about NFS.`, + ); +} + +/** + * A stated intent that contradicts the namespace it is deployed into. + * + * `-app-next` is one of the three ways a lane says it wants NFS, and it is the one + * that also names the Kubernetes namespace, so a lane called `-app-next` running + * legacy is indistinguishable from a working one in any report. An explicit + * `useNewFrontendSystem: false` is the only way to reach that state, so it is worth + * naming rather than honouring silently. + * + * Only this direction is checkable. The converse — a lane *not* named `-app-next` + * that resolves to NFS — is legitimate: `USE_NEW_FRONTEND_SYSTEM=true` may turn it + * on globally, and `configure({ useNewFrontendSystem: true })` is a supported way + * to opt in without renaming the project. + * + * @returns the message to warn with, or `undefined` when there is no contradiction + */ +export function describeNfsIntentConflict( + namespace: string, + explicitChoice: boolean | undefined, +): string | undefined { + if (explicitChoice !== false) return undefined; + if (!namespace.endsWith("-app-next")) return undefined; + return ( + `[nfs] "${namespace}" is named -app-next but was configured with ` + + `useNewFrontendSystem: false, so it will deploy the legacy shell under a ` + + `name that reads as an NFS lane. Rename the project or drop the override.` + ); +} + +/** + * Which of the three mechanisms decided this lane's frontend, for the deploy log. + * + * There is no single documented way to enable NFS (RHIDP-16461), so a lane's output + * has to say which one fired — otherwise "is this lane NFS?" is answered by reading + * three different files. + */ +export function describeNfsSource( + namespace: string, + explicitChoice: boolean | undefined, +): string { + if (explicitChoice !== undefined) { + return `configure({ useNewFrontendSystem: ${explicitChoice} })`; + } + if (namespace.endsWith("-app-next")) return `the -app-next project name`; + if (process.env.USE_NEW_FRONTEND_SYSTEM === "true") { + return `USE_NEW_FRONTEND_SYSTEM=true`; + } + return `nothing — the default is the legacy shell`; +} diff --git a/src/playwright/helpers/common.ts b/src/playwright/helpers/common.ts index 1b7fa67..11f7e7a 100644 --- a/src/playwright/helpers/common.ts +++ b/src/playwright/helpers/common.ts @@ -1,12 +1,75 @@ import { UIhelper } from "./ui-helper.js"; import { authenticator } from "otplib"; import { test, expect } from "@playwright/test"; -import type { Browser, Page, TestInfo } from "@playwright/test"; +import type { Browser, BrowserContext, Page, TestInfo } from "@playwright/test"; import { SETTINGS_PAGE_COMPONENTS } from "../page-objects/page-obj.js"; import * as path from "path"; import * as fs from "fs"; import { DEFAULT_USERS } from "../../deployment/keycloak/constants.js"; +/** + * Where a GitHub storage state is cached, keyed by project as well as user. + * + * The name used to be a bare relative `authState_.json`, resolved against + * `process.cwd()` — which the worker fixture sets to the workspace's `e2e-tests` + * directory, the same value for every project in that workspace. So all of a + * workspace's lanes shared one file for a given user, with no lock and no owner: + * one lane could read another's state, or read a file mid-write and fail on + * truncated JSON. Adding a lane adds a writer, so the migration makes it worse. + * + * Keying by project gives each lane its own file, which removes the sharing. The + * remaining writer within a project is handled by the atomic write below. + */ +export function githubSessionFile(userid: string, project?: string): string { + const scope = project ?? currentProjectName() ?? "no-project"; + const safe = (value: string) => value.replace(/[^a-zA-Z0-9._-]/g, "_"); + return path.resolve(`authState_${safe(scope)}_${safe(userid)}.json`); +} + +/** The Playwright project of the calling test, or undefined outside one. */ +function currentProjectName(): string | undefined { + try { + return test.info().project.name; + } catch { + return undefined; + } +} + +/** + * Cookies from a stored session, or `undefined` when there is nothing usable. + * + * A cached session is an optimisation, so a missing, truncated or malformed file + * must fall through to a full login rather than fail the test. Before this, a + * partially written file threw out of `JSON.parse` and read as a plugin failure. + */ +export type StoredCookies = Parameters[0]; + +export function readStoredCookies(file: string): StoredCookies | undefined { + try { + const parsed = JSON.parse(fs.readFileSync(file, "utf-8")); + const cookies = parsed?.cookies as StoredCookies | undefined; + return Array.isArray(cookies) && cookies.length > 0 ? cookies : undefined; + } catch { + return undefined; + } +} + +/** + * Writes the storage state so a concurrent reader never sees a partial file. + * + * `storageState({ path })` writes in place, so a reader can observe the file + * between create and write. Writing to a temp name and renaming makes the + * appearance of the final path atomic. + */ +export async function writeStorageStateAtomically( + page: Page, + file: string, +): Promise { + const pending = `${file}.${process.pid}.tmp`; + await page.context().storageState({ path: pending }); + fs.renameSync(pending, file); +} + export class LoginHelper { page: Page; uiHelper: UIhelper; @@ -102,14 +165,12 @@ export class LoginHelper { async loginAsGithubUser( userid: string = process.env.VAULT_GH_USER_ID as string, ) { - const sessionFileName = `authState_${userid}.json`; + const sessionFileName = githubSessionFile(userid); // Check if a session file for this specific user already exists - if (fs.existsSync(sessionFileName)) { + const cookies = readStoredCookies(sessionFileName); + if (cookies) { // Load and reuse existing authentication state - const cookies = JSON.parse( - fs.readFileSync(sessionFileName, "utf-8"), - ).cookies; await this.page.context().addCookies(cookies); console.log(`Reusing existing authentication state for user: ${userid}`); await this.page.goto("/"); @@ -147,7 +208,7 @@ export class LoginHelper { await this.uiHelper.clickButton("Sign In"); await this.checkAndReauthorizeGithubApp(); await this.page.waitForSelector("nav a", { timeout: 10_000 }); - await this.page.context().storageState({ path: sessionFileName }); + await writeStorageStateAtomically(this.page, sessionFileName); console.log(`Authentication state saved for user: ${userid}`); } } diff --git a/src/playwright/helpers/github-session.test.ts b/src/playwright/helpers/github-session.test.ts new file mode 100644 index 0000000..c9cb6f9 --- /dev/null +++ b/src/playwright/helpers/github-session.test.ts @@ -0,0 +1,82 @@ +import { describe, it } from "node:test"; +import assert from "node:assert"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { githubSessionFile, readStoredCookies } from "./common.js"; + +const tmp = () => fs.mkdtempSync(path.join(os.tmpdir(), "gh-session-test-")); + +describe("github session file naming", () => { + it("gives two projects different files for the same user", () => { + // The bug: one workspace's lanes share a cwd, so they shared one file — one + // lane could read another's storage state, or read one mid-write. + const a = githubSessionFile("rhdh-qe", "bulk-import"); + const b = githubSessionFile("rhdh-qe", "bulk-import-app-next"); + assert.notStrictEqual(a, b); + }); + + it("gives two users different files within one project", () => { + assert.notStrictEqual( + githubSessionFile("user-a", "ws"), + githubSessionFile("user-b", "ws"), + ); + }); + + it("is absolute, so it does not follow a later chdir", () => { + assert.ok(path.isAbsolute(githubSessionFile("rhdh-qe", "ws"))); + }); + + it("keeps a project name with a path separator inside one file name", () => { + // A separator in the project name would otherwise turn into a directory that + // does not exist, and the write would fail rather than the read. + const file = githubSessionFile("rhdh-qe", "group/ws"); + assert.strictEqual(path.dirname(file), process.cwd()); + }); + + it("falls back to a named scope outside a Playwright context", () => { + // node:test has no test.info(), which is the same situation as a helper + // called from globalSetup. + assert.match(githubSessionFile("rhdh-qe"), /no-project/); + }); +}); + +describe("reading a stored session", () => { + it("returns the cookies of a well-formed file", () => { + const dir = tmp(); + const file = path.join(dir, "s.json"); + fs.writeFileSync(file, JSON.stringify({ cookies: [{ name: "a" }] })); + assert.deepStrictEqual(readStoredCookies(file), [{ name: "a" }]); + }); + + it("returns undefined rather than throwing on a truncated file", () => { + // What a concurrent reader saw while storageState() was mid-write. It used to + // come out of JSON.parse as a test failure that looked like a plugin bug. + const dir = tmp(); + const file = path.join(dir, "s.json"); + fs.writeFileSync(file, '{"cookies":[{"name"'); + assert.strictEqual(readStoredCookies(file), undefined); + }); + + it("returns undefined for a file that does not exist", () => { + assert.strictEqual( + readStoredCookies(path.join(tmp(), "absent.json")), + undefined, + ); + }); + + it("treats an empty cookie list as no session", () => { + // Reusing it would send the user through a login the caller thinks it skipped. + const dir = tmp(); + const file = path.join(dir, "s.json"); + fs.writeFileSync(file, JSON.stringify({ cookies: [] })); + assert.strictEqual(readStoredCookies(file), undefined); + }); + + it("treats a file with no cookies key as no session", () => { + const dir = tmp(); + const file = path.join(dir, "s.json"); + fs.writeFileSync(file, JSON.stringify({ origins: [] })); + assert.strictEqual(readStoredCookies(file), undefined); + }); +}); diff --git a/src/playwright/helpers/navbar.ts b/src/playwright/helpers/navbar.ts index 651984e..dcfdfd4 100644 --- a/src/playwright/helpers/navbar.ts +++ b/src/playwright/helpers/navbar.ts @@ -1,9 +1,21 @@ +/** + * Sidebar entries a suite can open. + * + * The label depends on which shell is running. The legacy RHDH shell titles the + * scaffolder entry point "Self-service"; under the new frontend system the same + * page is "Create", and `packages/app-next` ships no global header at all. The + * union carries both so a spec running in either shell type-checks without a cast + * — it does not decide which one a given lane should use, which is + * [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462). + */ export type SidebarTabs = | "Catalog" | "Settings" | "My Group" | "Home" + // Legacy shell label for the scaffolder; "Create" is the app-next label. | "Self-service" + | "Create" | "Learning Paths" | "Extensions" | "Bulk import" From 36ccc28354bc1931dce1ffd84df9ca0c73b6b852 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Fri, 21 Aug 2026 16:33:45 -0300 Subject: [PATCH 2/7] fix(nfs): the session fix would have caused the failure it prevents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found two of the three changes wrong in ways the tests did not reach. Keying the GitHub session file per project removed cross-lane sharing, which is what the ticket asked for — and would have made every lane of a workspace log in to the same GitHub account concurrently. logintoGithub derives its 2FA code from one shared TOTP secret, so lanes starting inside the same 30-second window submit the identical code and GitHub rejects the second. This file already carries retry handling for exactly that error, which is how it is known to happen; bulk-import would have gone from one login to three simultaneous ones. So the file is one per user again, and what was actually missing is added instead: a lock spanning read-or-login-and-write, not just the write, because two lanes that both decide there is no session go on to log in anyway. RHDH cookies from another lane were never the hazard — each lane's RHDH lives on its own namespace hostname. The [nfs] line was logged from configure(), but the worker fixture calls configure() with no arguments for every project before any spec runs. For github and homepage — the two lanes the change was written for, which opt in through configure({ useNewFrontendSystem: true }) — the first line said "off, from nothing", and the first line is the one a reader greps. Reported from deploy() now, once, after configuration is final. Two smaller ones. The guard accepted an unquoted YAML `true` by coercing in the comparison only, so it green-lit a payload the API server rejects with "cannot unmarshal bool into Go struct field ... of type string" — an opaque k8s failure in place of the guard's own message. It now rewrites the value as well. And the temp file survived a failed storageState write, accumulating untracked authState*.tmp files in a directory nothing gitignores them from. New tests for the lock, both write outcomes, and the boolean rewrite; the serialisation test waits for the first holder rather than racing two callers from the same tick, which was flaky. Mutation-tested: removing the lock, the temp-file cleanup, or the in-place coercion each turns exactly one test red. 141 pass, five consecutive runs. Co-Authored-By: Claude Opus 5 (1M context) --- docs/changelog.md | 4 +- src/deployment/rhdh/deployment.ts | 19 ++- src/deployment/rhdh/nfs-guard.test.ts | 22 +-- src/deployment/rhdh/nfs-guard.ts | 15 ++ src/playwright/helpers/common.ts | 91 ++++++++---- src/playwright/helpers/github-session.test.ts | 136 +++++++++++++++--- 6 files changed, 222 insertions(+), 65 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 4a585fa..3016c1f 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,13 +10,13 @@ All notable changes to this project will be documented in this file. ### Added -- **A lane now says which shell it runs, and where that was decided** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): `configure()` logs `[nfs] : new frontend system ON|off, from `. Three mechanisms can enable NFS — the `-app-next` project name, `USE_NEW_FRONTEND_SYSTEM=true`, and `configure({ useNewFrontendSystem: true })` — and none of them is visible from a single file, so answering "is this lane NFS?" meant reading three. A namespace ending in `-app-next` that was explicitly configured with `useNewFrontendSystem: false` also warns, because it deploys the legacy shell under a name that reads as an NFS lane in every report. Only that direction is checked: a lane that enables NFS without the suffix is legitimate and common. +- **A lane now says which shell it runs, and where that was decided** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): `deploy()` logs `[nfs] : new frontend system ON|off, from ` — from `deploy()` rather than `configure()`, because the worker fixture calls `configure()` with no arguments for every project before any spec runs, so a lane opting in through `configure({ useNewFrontendSystem: true })` would print `off` first and `ON` second, and the first line is the one a reader greps. Three mechanisms can enable NFS — the `-app-next` project name, `USE_NEW_FRONTEND_SYSTEM=true`, and `configure({ useNewFrontendSystem: true })` — and none of them is visible from a single file, so answering "is this lane NFS?" meant reading three. A namespace ending in `-app-next` that was explicitly configured with `useNewFrontendSystem: false` also warns, because it deploys the legacy shell under a name that reads as an NFS lane in every report. Only that direction is checked: a lane that enables NFS without the suffix is legitimate and common. - **`SidebarTabs` accepts both shells' scaffolder label** ([RHIDP-16458](https://redhat.atlassian.net/browse/RHIDP-16458)): the union hardcoded `"Self-service"`, the legacy shell's title. Under app-next the same page is `"Create"`, so a spec running there could not type-check without a cast. Both are members now. This does not decide which label a lane should use — that is [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462), and three different workarounds are in flight for it. ### Fixed - **A workspace could silently disable the new frontend system it asked for** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): the NFS secret layer is merged *before* the workspace's own `tests/config/rhdh-secrets.yaml`, so a workspace setting `APP_CONFIG_app_packageName` or `ENABLE_STANDARD_MODULE_FEDERATION` for its own reasons overrode it and the lane booted the legacy shell. Nothing failed — the legacy suite re-ran and passed, and the one thing the lane existed to prove was never exercised. `deploy()` now checks the merged secret still carries both markers and throws naming the key, the value found, and the file that overrode it. The merge order itself is unchanged: a workspace must be able to override defaults, which is precisely why the outcome needs checking rather than assuming. -- **The GitHub session file was shared across projects with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. A workspace's lanes therefore shared one file per user: one lane could inject another's storage state, and a reader landing mid-write failed on truncated JSON as a flake that looked nothing like the plugin under test. Every added lane adds a writer, so the migration makes it worse. The path is now absolute and keyed by project as well as user, the write goes through a temp file and a rename, and an unreadable or empty session falls through to a full login instead of throwing. +- **The GitHub session file was shared with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. So every lane and every worker shared one file with no lock: a reader landing mid-write failed on truncated JSON, as a flake that looked nothing like the plugin under test, and every added lane adds a writer. Access is now serialised across the whole run, the write goes through a temp file and a rename (removed even when it fails), the path is absolute, and an unreadable or empty session falls through to a full login instead of throwing. Deliberately still **one file per user, not per project**: scoping it per project is the obvious fix and is the wrong one, because `logintoGithub` derives its 2FA code from a single shared TOTP secret, so lanes logging in inside the same 30-second window submit the identical code and GitHub rejects the second. ## [2.1.9] diff --git a/src/deployment/rhdh/deployment.ts b/src/deployment/rhdh/deployment.ts index a58488b..b2da3ab 100644 --- a/src/deployment/rhdh/deployment.ts +++ b/src/deployment/rhdh/deployment.ts @@ -38,6 +38,8 @@ export class RHDHDeployment { public k8sClient = new KubernetesClientHelper(); public rhdhUrl: string; public deploymentConfig: DeploymentConfig; + /** `configure({ useNewFrontendSystem })` as the caller passed it, for the report. */ + private _explicitFrontendSystemChoice: boolean | undefined; constructor(namespace: string) { this.deploymentConfig = this._buildDeploymentConfig({ namespace }); @@ -54,6 +56,7 @@ export class RHDHDeployment { const executed = await runOnce( `deploy-${this.deploymentConfig.namespace}`, async () => { + this._reportFrontendSystem(); this._log("Starting RHDH deployment..."); this._log("RHDH Base URL: " + this.rhdhUrl); console.table(this.deploymentConfig); @@ -512,6 +515,9 @@ export class RHDHDeployment { "helm"; const namespace = input.namespace ?? this.deploymentConfig.namespace; + // Kept so the deploy-time report can name the mechanism; a resolved boolean + // cannot distinguish the three sources. + this._explicitFrontendSystemChoice = input.useNewFrontendSystem; const useNewFrontendSystem = input.useNewFrontendSystem ?? (namespace.endsWith("-app-next") || @@ -550,7 +556,6 @@ export class RHDHDeployment { this.deploymentConfig = this._buildDeploymentConfig(deploymentOptions); this.rhdhUrl = this._buildBaseUrl(); } - this._reportFrontendSystem(deploymentOptions?.useNewFrontendSystem); await this.k8sClient.createNamespaceIfNotExists( this.deploymentConfig.namespace, ); @@ -561,11 +566,17 @@ export class RHDHDeployment { * * Three mechanisms can turn NFS on and none of them is visible from a single * file, so "is this lane NFS?" is otherwise answered by reading a project name, - * an environment variable and a `configure()` call together. Logging it once at - * configure time puts the answer in the lane's own output. + * an environment variable and a `configure()` call together. + * + * Reported from `deploy()` rather than `configure()`: the worker fixture calls + * `configure()` with no arguments for every project before any spec runs, so a lane + * that opts in with `configure({ useNewFrontendSystem: true })` — which is how + * `github` and `homepage` do it — would print `off` first and `ON` second, and the + * first line is the one a reader greps. */ - private _reportFrontendSystem(explicitChoice: boolean | undefined): void { + private _reportFrontendSystem(): void { const { namespace, useNewFrontendSystem } = this.deploymentConfig; + const explicitChoice = this._explicitFrontendSystemChoice; const conflict = describeNfsIntentConflict(namespace, explicitChoice); if (conflict) console.warn(conflict); console.log( diff --git a/src/deployment/rhdh/nfs-guard.test.ts b/src/deployment/rhdh/nfs-guard.test.ts index 25274a6..59894b5 100644 --- a/src/deployment/rhdh/nfs-guard.test.ts +++ b/src/deployment/rhdh/nfs-guard.test.ts @@ -42,18 +42,22 @@ describe("nfs secret markers", () => { assert.strictEqual(dropped[0].actual, undefined); }); - it("accepts an unquoted YAML true, which is the same secret value", () => { - // The guard is looking for a *different* value, not for a typing mistake: - // unquoted `true` in YAML parses as a boolean but serializes into the Secret - // as "true" and enables module federation just the same. Comparing without - // coercing would raise a false alarm on a working lane. - const dropped = findDroppedNfsMarkers({ - ...intact(), - [MF_KEY]: true, - }); + it("accepts an unquoted YAML true rather than raising a false alarm", () => { + // A typing mistake in the workspace's YAML, not a different value — the intent is + // plainly the same. Comparing without coercing would fail a working lane. + const dropped = findDroppedNfsMarkers({ ...intact(), [MF_KEY]: true }); assert.deepStrictEqual(dropped, []); }); + it("rewrites a boolean marker to the string the API server requires", () => { + // stringData goes straight onto the V1Secret body, so a JSON boolean is rejected + // with "cannot unmarshal bool into Go struct field ... of type string". Tolerating + // it in the comparison alone would green-light a config that cannot apply. + const payload = { ...intact(), [MF_KEY]: true }; + assertNfsMarkersSurvived(payload, "ws-app-next"); + assert.strictEqual(payload[MF_KEY], "true"); + }); + it("counts a missing stringData block as both markers dropped", () => { assert.strictEqual(findDroppedNfsMarkers(undefined).length, 2); }); diff --git a/src/deployment/rhdh/nfs-guard.ts b/src/deployment/rhdh/nfs-guard.ts index 22e0fa0..ec848dd 100644 --- a/src/deployment/rhdh/nfs-guard.ts +++ b/src/deployment/rhdh/nfs-guard.ts @@ -65,6 +65,21 @@ export function assertNfsMarkersSurvived( stringData: Record | undefined, namespace: string, ): void { + // Coerce before comparing AND in place: applySecretFromObject puts stringData + // straight on the V1Secret body, so an unquoted YAML `true` reaches the API server + // as a JSON boolean and is rejected with "cannot unmarshal bool into Go struct + // field ... of type string". Accepting it in the comparison alone would green-light + // a config that cannot apply, and the failure would surface as an opaque k8s error + // instead of this guard's message. + if (stringData) { + for (const [key] of NFS_SECRET_MARKERS) { + const raw = stringData[key]; + if (raw !== undefined && typeof raw !== "string") { + stringData[key] = String(raw); + } + } + } + const dropped = findDroppedNfsMarkers(stringData); if (dropped.length === 0) return; diff --git a/src/playwright/helpers/common.ts b/src/playwright/helpers/common.ts index 11f7e7a..bba76a2 100644 --- a/src/playwright/helpers/common.ts +++ b/src/playwright/helpers/common.ts @@ -5,42 +5,38 @@ import type { Browser, BrowserContext, Page, TestInfo } from "@playwright/test"; import { SETTINGS_PAGE_COMPONENTS } from "../page-objects/page-obj.js"; import * as path from "path"; import * as fs from "fs"; +import lockfile from "proper-lockfile"; import { DEFAULT_USERS } from "../../deployment/keycloak/constants.js"; /** - * Where a GitHub storage state is cached, keyed by project as well as user. + * Where a GitHub storage state is cached, and the lock that serialises access to it. * * The name used to be a bare relative `authState_.json`, resolved against * `process.cwd()` — which the worker fixture sets to the workspace's `e2e-tests` - * directory, the same value for every project in that workspace. So all of a - * workspace's lanes shared one file for a given user, with no lock and no owner: - * one lane could read another's state, or read a file mid-write and fail on - * truncated JSON. Adding a lane adds a writer, so the migration makes it worse. + * directory, the same value for every project in that workspace. So every lane and + * every worker shared one file with no lock: a reader could land mid-write and fail on + * truncated JSON, and a stale file could survive into a run that needed a fresh login. * - * Keying by project gives each lane its own file, which removes the sharing. The - * remaining writer within a project is handled by the atomic write below. + * Deliberately still one file per *user*, not per project. Scoping it per project was + * the obvious fix and is the wrong one: `logintoGithub` derives its 2FA code from a + * single shared TOTP secret, so two lanes logging in inside the same 30-second window + * submit the identical code and GitHub rejects the second — a failure this file already + * has retry handling for. Sharing the session is the point of caching it; what was + * missing was making concurrent access safe, which is what the lock and the atomic + * write below do. RHDH cookies from another lane are harmless: each lane's RHDH lives + * on its own namespace hostname, so they are never sent anywhere they matter. */ -export function githubSessionFile(userid: string, project?: string): string { - const scope = project ?? currentProjectName() ?? "no-project"; - const safe = (value: string) => value.replace(/[^a-zA-Z0-9._-]/g, "_"); - return path.resolve(`authState_${safe(scope)}_${safe(userid)}.json`); -} - -/** The Playwright project of the calling test, or undefined outside one. */ -function currentProjectName(): string | undefined { - try { - return test.info().project.name; - } catch { - return undefined; - } +export function githubSessionFile(userid: string): string { + const safe = String(userid).replace(/[^a-zA-Z0-9._-]/g, "_"); + return path.resolve(`authState_${safe}.json`); } /** * Cookies from a stored session, or `undefined` when there is nothing usable. * - * A cached session is an optimisation, so a missing, truncated or malformed file - * must fall through to a full login rather than fail the test. Before this, a - * partially written file threw out of `JSON.parse` and read as a plugin failure. + * A cached session is an optimisation, so a missing, truncated or malformed file must + * fall through to a full login rather than fail the test. Before this, a partially + * written file threw out of `JSON.parse` and read as a plugin failure. */ export type StoredCookies = Parameters[0]; @@ -57,17 +53,49 @@ export function readStoredCookies(file: string): StoredCookies | undefined { /** * Writes the storage state so a concurrent reader never sees a partial file. * - * `storageState({ path })` writes in place, so a reader can observe the file - * between create and write. Writing to a temp name and renaming makes the - * appearance of the final path atomic. + * `storageState({ path })` writes in place, so a reader can observe the file between + * create and write. Writing to a temp name and renaming makes the appearance of the + * final path atomic. The temp name carries the pid because Playwright workers are + * separate processes, and it is removed even when the write fails so failed runs do + * not litter the workspace. */ export async function writeStorageStateAtomically( page: Page, file: string, ): Promise { const pending = `${file}.${process.pid}.tmp`; - await page.context().storageState({ path: pending }); - fs.renameSync(pending, file); + try { + await page.context().storageState({ path: pending }); + fs.renameSync(pending, file); + } finally { + fs.rmSync(pending, { force: true }); + } +} + +/** + * Runs `fn` with exclusive access to the session file, across workers and lanes. + * + * Without this the first lane to start would not have finished writing before the + * others decided there was no session and each began its own login — which is the + * TOTP collision described above, not merely wasted work. The lock target is created + * rather than assumed: `proper-lockfile` needs an existing path, and the session file + * itself does not exist on the run that has to create it. + */ +export async function withGithubSessionLock( + file: string, + fn: () => Promise, +): Promise { + const target = `${file}.lock-target`; + fs.writeFileSync(target, "", { flag: "a" }); + const release = await lockfile.lock(target, { + retries: { retries: 60, minTimeout: 1_000 }, + stale: 300_000, + }); + try { + return await fn(); + } finally { + await release(); + } } export class LoginHelper { @@ -166,7 +194,14 @@ export class LoginHelper { userid: string = process.env.VAULT_GH_USER_ID as string, ) { const sessionFileName = githubSessionFile(userid); + // The lock spans read-or-login-and-write, not just the write: two lanes that both + // decide there is no session go on to submit the same TOTP code. + await withGithubSessionLock(sessionFileName, async () => { + await this._loginAsGithubUser(userid, sessionFileName); + }); + } + private async _loginAsGithubUser(userid: string, sessionFileName: string) { // Check if a session file for this specific user already exists const cookies = readStoredCookies(sessionFileName); if (cookies) { diff --git a/src/playwright/helpers/github-session.test.ts b/src/playwright/helpers/github-session.test.ts index c9cb6f9..1b2d6c8 100644 --- a/src/playwright/helpers/github-session.test.ts +++ b/src/playwright/helpers/github-session.test.ts @@ -3,41 +3,133 @@ import assert from "node:assert"; import fs from "fs"; import os from "os"; import path from "path"; -import { githubSessionFile, readStoredCookies } from "./common.js"; +import { + githubSessionFile, + readStoredCookies, + withGithubSessionLock, + writeStorageStateAtomically, +} from "./common.js"; const tmp = () => fs.mkdtempSync(path.join(os.tmpdir(), "gh-session-test-")); describe("github session file naming", () => { - it("gives two projects different files for the same user", () => { - // The bug: one workspace's lanes share a cwd, so they shared one file — one - // lane could read another's storage state, or read one mid-write. - const a = githubSessionFile("rhdh-qe", "bulk-import"); - const b = githubSessionFile("rhdh-qe", "bulk-import-app-next"); - assert.notStrictEqual(a, b); - }); - - it("gives two users different files within one project", () => { + it("gives two users different files, and is stable for one user", () => { assert.notStrictEqual( - githubSessionFile("user-a", "ws"), - githubSessionFile("user-b", "ws"), + githubSessionFile("user-a"), + githubSessionFile("user-b"), + ); + assert.strictEqual( + githubSessionFile("rhdh-qe"), + githubSessionFile("rhdh-qe"), ); }); + it("is deliberately not keyed by project", () => { + // Scoping per project was the obvious fix and is the wrong one: logintoGithub + // derives its 2FA code from one shared TOTP secret, so lanes logging in within the + // same 30-second window submit the identical code and GitHub rejects the second. + // Sharing the session is the point; withGithubSessionLock is what makes it safe. + const file = githubSessionFile("rhdh-qe"); + assert.doesNotMatch(path.basename(file), /app-next|project/); + }); + it("is absolute, so it does not follow a later chdir", () => { - assert.ok(path.isAbsolute(githubSessionFile("rhdh-qe", "ws"))); + assert.ok(path.isAbsolute(githubSessionFile("rhdh-qe"))); }); - it("keeps a project name with a path separator inside one file name", () => { - // A separator in the project name would otherwise turn into a directory that - // does not exist, and the write would fail rather than the read. - const file = githubSessionFile("rhdh-qe", "group/ws"); - assert.strictEqual(path.dirname(file), process.cwd()); + it("keeps a separator in the user id inside one file name", () => { + // Otherwise it becomes a directory that does not exist and the write fails. + assert.strictEqual( + path.dirname(githubSessionFile("org/user")), + process.cwd(), + ); }); - it("falls back to a named scope outside a Playwright context", () => { - // node:test has no test.info(), which is the same situation as a helper - // called from globalSetup. - assert.match(githubSessionFile("rhdh-qe"), /no-project/); + it("does not throw when the vault user id is unset", () => { + // loginAsGithubUser defaults to `process.env.VAULT_GH_USER_ID as string`, and the + // cast hides the undefined. Building the path must not be where that surfaces — + // a TypeError here points nowhere near the missing variable. + assert.doesNotThrow(() => + githubSessionFile(undefined as unknown as string), + ); + }); +}); + +describe("the session lock", () => { + it("holds off a second caller until the first is done", async () => { + const dir = tmp(); + const file = path.join(dir, "s.json"); + const order: string[] = []; + let held: () => void = () => {}; + const acquired = new Promise((resolve) => { + held = resolve; + }); + + // The second caller must not start until the first demonstrably holds the lock — + // racing them from the same tick would test the scheduler, not the lock. + const first = withGithubSessionLock(file, async () => { + order.push("first-in"); + held(); + await new Promise((resolve) => setTimeout(resolve, 50)); + order.push("first-out"); + }); + await acquired; + const second = withGithubSessionLock(file, async () => { + order.push("second-in"); + }); + await Promise.all([first, second]); + + assert.deepStrictEqual(order, ["first-in", "first-out", "second-in"]); + }); + + it("releases the lock when the body throws", async () => { + const dir = tmp(); + const file = path.join(dir, "s.json"); + await assert.rejects( + withGithubSessionLock(file, async () => { + throw new Error("boom"); + }), + ); + // A lock held after a failed login would hang every other lane for `stale`. + await withGithubSessionLock(file, async () => {}); + }); +}); + +describe("writing a stored session", () => { + /** Enough of a Page for the write path; a real one needs a browser. */ + const fakePage = (write: (file: string) => void) => + ({ + context: () => ({ + storageState: async ({ path: target }: { path: string }) => + write(target), + }), + }) as unknown as Parameters[0]; + + it("leaves only the final file behind", async () => { + const dir = tmp(); + const file = path.join(dir, "s.json"); + await writeStorageStateAtomically( + fakePage((target) => fs.writeFileSync(target, '{"cookies":[]}')), + file, + ); + assert.deepStrictEqual(fs.readdirSync(dir), ["s.json"]); + }); + + it("removes the temp file when the write fails", async () => { + // Otherwise a failed run litters the workspace's e2e-tests directory, where + // nothing gitignores authState*. + const dir = tmp(); + const file = path.join(dir, "s.json"); + await assert.rejects( + writeStorageStateAtomically( + fakePage((target) => { + fs.writeFileSync(target, "partial"); + throw new Error("browser went away"); + }), + file, + ), + ); + assert.deepStrictEqual(fs.readdirSync(dir), []); }); }); From ec975adeafd69bbe79e378f9066aac8218bf32f2 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Mon, 24 Aug 2026 09:53:45 -0300 Subject: [PATCH 3/7] chore: take 2.1.10, now that #149 claims no version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #149 was reduced to a documentation change, so it no longer bumps the version and 2.1.10 is free. The note about the two PRs colliding goes with it — there is only one version-bumping PR open now. Co-Authored-By: Claude Opus 5 (1M context) --- docs/changelog.md | 6 +----- package.json | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 3016c1f..9cc58c1 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,11 +2,7 @@ All notable changes to this project will be documented in this file. -## [2.1.11] - Current - -> Takes 2.1.11 rather than 2.1.10 because #149 claims 2.1.10. Whichever of the two -> merges second needs its version and this heading moved up: the Version Bump Check -> compares against `main` only, so it cannot see the collision. +## [2.1.10] - Current ### Added diff --git a/package.json b/package.json index 054cbef..847bfee 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@red-hat-developer-hub/e2e-test-utils", - "version": "2.1.11", + "version": "2.1.10", "description": "Test utilities for RHDH E2E tests", "license": "Apache-2.0", "repository": { From 2650f49538d0c48658194bf151556ea833d65e17 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Mon, 24 Aug 2026 10:01:48 -0300 Subject: [PATCH 4/7] =?UTF-8?q?chore:=20back=20to=202.1.11=20=E2=80=94=20m?= =?UTF-8?q?ain=20took=202.1.10?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #151 merged today and claimed 2.1.10, so the renumber in the previous commit collided with main and the Version Bump Check would have refused it. Same number as before, different reason: this is no longer about #149, which now bumps nothing at all. Worth noting separately: main says 2.1.10 but the publish workflow for it failed, so npm latest is still 2.1.9. If that is not resolved before this merges, 2.1.11 publishes over a 2.1.10 that never existed. Co-Authored-By: Claude Opus 5 (1M context) --- docs/changelog.md | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 9cc58c1..f68bab1 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file. -## [2.1.10] - Current +## [2.1.11] - Current ### Added diff --git a/package.json b/package.json index 847bfee..054cbef 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@red-hat-developer-hub/e2e-test-utils", - "version": "2.1.10", + "version": "2.1.11", "description": "Test utilities for RHDH E2E tests", "license": "Apache-2.0", "repository": { From 39ea79ba3d14ecf85d5f1a7512ed3ced4729b2e5 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Mon, 24 Aug 2026 12:26:24 -0300 Subject: [PATCH 5/7] docs(nfs): the deployment guide said the workspace secret always wins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It does not any more, and the guide said it in as many words: "your rhdh-secrets.yaml still wins on conflicts". True for every key except the two NFS markers, where overriding now throws instead of quietly deploying the legacy shell — so the sentence this PR contradicts is corrected in the same change rather than left to be discovered from a stack trace. Also documented, because a new hard failure with no docs costs someone an hour: the `[nfs]` line and what to grep for when a lane behaves like the wrong shell, why it comes from deploy() rather than configure(), the exact throw and the fact that it is a post-condition on the caller's own config, and the -app-next namespace warning together with why the reverse is not checked. The dynamic-plugins layer is named as the unguarded one. Disabling app-auth or app-integrations from a workspace's own dynamic-plugins.yaml degrades the lane the same silent way, and nothing catches it. No workspace does that today — 14 ship their own dynamic-plugins.yaml and none names either package — so a second guard would be insurance against something nobody does, which is the argument that just closed rhdh-plugin-export-overlays#3375. Documented instead, and it is the first place to look when an NFS lane behaves like the legacy shell. deploy()'s step list gains both the report and the throw. 141 tests pass, yarn check clean, docs build clean. --- docs/guide/deployment/rhdh-deployment.md | 54 +++++++++++++++++++----- 1 file changed, 44 insertions(+), 10 deletions(-) diff --git a/docs/guide/deployment/rhdh-deployment.md b/docs/guide/deployment/rhdh-deployment.md index c824ebf..071f1a0 100644 --- a/docs/guide/deployment/rhdh-deployment.md +++ b/docs/guide/deployment/rhdh-deployment.md @@ -89,10 +89,43 @@ await rhdh.deploy(); What gets merged (same order as other config: package defaults → auth → NFS defaults → your workspace files; later wins; secrets are merged then **envsubst** runs once on the result): -1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"` (your `rhdh-secrets.yaml` still wins on conflicts and can use `$VAR` substitution). -2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins). +1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"`. Your `rhdh-secrets.yaml` merges last and wins on every other key, and can use `$VAR` substitution — but **not on these two**: overriding either would leave the lane running the legacy shell while calling itself NFS, so `deploy()` throws instead. See below. +2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins). Re-pinning is fine; **disabling** either degrades the lane the same way the secrets above would, and that one is not guarded — if an NFS lane behaves like the legacy shell, check this list first. 3. **Helm** — Package `config/new-frontend-system/value_file.yaml`, then your `value_file.yaml`, then optional `tests/config/value_file-app-next.yaml` when that file exists. +#### Knowing which shell actually ran + +`deploy()` logs the answer once, because three mechanisms can decide it and none of them +is visible from a single file: + +``` +[nfs] tech-radar-app-next: new frontend system ON, from the -app-next project name +[nfs] github: new frontend system ON, from configure({ useNewFrontendSystem: true }) +[nfs] rbac: new frontend system off, from nothing — the default is the legacy shell +``` + +Grep `[nfs]` when a lane behaves like the wrong shell. It is logged from `deploy()` +rather than `configure()` on purpose: the worker fixture calls `configure()` with no +arguments for every project before any spec runs, so a lane opting in through +`configure({ useNewFrontendSystem: true })` would otherwise print `off` first. + +A namespace ending in `-app-next` that is explicitly configured with +`useNewFrontendSystem: false` warns — it deploys the legacy shell under a name that +reads as an NFS lane in every report. The reverse is not checked: enabling NFS without +the suffix is normal. + +If the merged secret would not actually enable NFS, `deploy()` throws rather than +deploying: + +``` +[nfs] "bulk-import-app-next" is configured for the new frontend system, but the +secret about to be applied would not enable it: + APP_CONFIG_app_packageName is "app" (expected "app-next") +``` + +That is a post-condition on your own config, not a bug in the harness: remove the +override from `tests/config/rhdh-secrets.yaml`, or set it to the value named. + Workspace-specific **app-config** (titles, plugin routes, etc.) remains your responsibility. ### Example: Full Configuration @@ -150,14 +183,15 @@ await rhdh.deploy({ timeout: null }); `deploy()` automatically skips if the deployment already succeeded in the current test run (e.g., after a worker restart due to test failure). This prevents expensive re-deployments. This method: -1. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins -2. Substitutes environment variables in the merged secrets (`envsubst`) -3. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config -4. Applies ConfigMaps (app-config, dynamic-plugins) -5. Applies Secrets -6. Installs RHDH via Helm or Operator -7. Waits for the deployment to be ready -8. Sets `RHDH_BASE_URL` environment variable +1. Reports which frontend shell this lane will run, and what decided it — see [New frontend system](#new-frontend-system-usenewfrontendsystem) +2. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins +3. Substitutes environment variables in the merged secrets (`envsubst`) +4. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config +5. Applies ConfigMaps (app-config, dynamic-plugins) +6. Applies Secrets — **throws** first if the lane asked for the new frontend system and the merged secret would not enable it +7. Installs RHDH via Helm or Operator +8. Waits for the deployment to be ready +9. Sets `RHDH_BASE_URL` environment variable #### Base URL format From 19dee27182a8c3c74006db8eee233c1c345c63e4 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Tue, 25 Aug 2026 18:59:40 -0300 Subject: [PATCH 6/7] fix(nfs): fail the lane on an intent conflict instead of warning about it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard found the conflict and then let the run continue. A lane named -app-next configured with useNewFrontendSystem: false deployed the legacy shell, re-ran the legacy suite, passed, and left one console.warn behind on a run that exited 0 — which is indistinguishable from a run that had nothing to say. That is the exact failure this module exists to stop, and the PR's own title claims it does. assertNfsMarkersSurvived already throws for the other path, where a workspace's own rhdh-secrets.yaml overwrites the markers. The two paths now behave the same. describeNfsIntentConflict stays a pure describer with its existing tests; assertNfsIntentMatches is a thin wrapper over it so the throwing behaviour is directly testable rather than reachable only through a private method on RHDHDeployment. The [nfs] line is logged before the check so the resolved state is on record even when the next line ends the run. Mutation-verified: turning the throw back into a return fails "fails the deploy on the conflict, rather than only naming it" and nothing else. Closes the library half of RHIDP-16457 — bulk-import's local expect(rhdh.deploymentConfig.useNewFrontendSystem).toBe(true) was stronger than the library guard until now, which is the inverse of what that ticket asks for. --- src/deployment/rhdh/deployment.ts | 7 ++++--- src/deployment/rhdh/nfs-guard.test.ts | 19 +++++++++++++++++++ src/deployment/rhdh/nfs-guard.ts | 19 +++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/deployment/rhdh/deployment.ts b/src/deployment/rhdh/deployment.ts index b2da3ab..2ac19fc 100644 --- a/src/deployment/rhdh/deployment.ts +++ b/src/deployment/rhdh/deployment.ts @@ -1,8 +1,8 @@ import { KubernetesClientHelper } from "../../utils/kubernetes-client.js"; import { WorkspacePaths } from "../../utils/workspace-paths.js"; import { + assertNfsIntentMatches, assertNfsMarkersSurvived, - describeNfsIntentConflict, describeNfsSource, } from "./nfs-guard.js"; import { $ } from "../../utils/bash.js"; @@ -577,13 +577,14 @@ export class RHDHDeployment { private _reportFrontendSystem(): void { const { namespace, useNewFrontendSystem } = this.deploymentConfig; const explicitChoice = this._explicitFrontendSystemChoice; - const conflict = describeNfsIntentConflict(namespace, explicitChoice); - if (conflict) console.warn(conflict); + // Logged before the check so the resolved state is on record even when the + // next line ends the run. console.log( `[nfs] ${namespace}: new frontend system ` + `${useNewFrontendSystem ? "ON" : "off"}, from ` + `${describeNfsSource(namespace, explicitChoice)}`, ); + assertNfsIntentMatches(namespace, explicitChoice); } private _buildBaseUrl(): string { diff --git a/src/deployment/rhdh/nfs-guard.test.ts b/src/deployment/rhdh/nfs-guard.test.ts index 59894b5..7e7a023 100644 --- a/src/deployment/rhdh/nfs-guard.test.ts +++ b/src/deployment/rhdh/nfs-guard.test.ts @@ -3,6 +3,7 @@ import assert from "node:assert"; import { NFS_SECRET_MARKERS, assertNfsMarkersSurvived, + assertNfsIntentMatches, describeNfsIntentConflict, describeNfsSource, findDroppedNfsMarkers, @@ -107,6 +108,24 @@ describe("nfs intent conflict", () => { ); }); + it("fails the deploy on the conflict, rather than only naming it", () => { + // A warning on a run that exits 0 is the failure this module exists to stop: + // the legacy suite re-runs, everything passes, and nothing proved NFS works. + assert.throws( + () => assertNfsIntentMatches("quay-app-next", false), + /named -app-next/, + ); + }); + + it("lets every non-conflicting combination through", () => { + assert.doesNotThrow(() => assertNfsIntentMatches("quay-app-next", true)); + assert.doesNotThrow(() => + assertNfsIntentMatches("quay-app-next", undefined), + ); + assert.doesNotThrow(() => assertNfsIntentMatches("quay", false)); + assert.doesNotThrow(() => assertNfsIntentMatches("github", true)); + }); + it("does not flag the reverse, which is legitimate", () => { // github and homepage enable NFS through configure() without renaming, and a // global USE_NEW_FRONTEND_SYSTEM may turn it on everywhere. diff --git a/src/deployment/rhdh/nfs-guard.ts b/src/deployment/rhdh/nfs-guard.ts index ec848dd..c8bd959 100644 --- a/src/deployment/rhdh/nfs-guard.ts +++ b/src/deployment/rhdh/nfs-guard.ts @@ -130,6 +130,25 @@ export function describeNfsIntentConflict( ); } +/** + * Throws on the conflict {@link describeNfsIntentConflict} finds. + * + * Reporting it is not enough. The lane exists to prove the plugin works under NFS, + * and a warning on a run that exits 0 is indistinguishable from a run that had + * nothing to say — which is the exact failure this module is here to stop. Both + * ways out are cheap: rename the project, or drop the override. + * + * @param namespace - the lane's namespace, which is its Playwright project name + * @param explicitChoice - `configure({ useNewFrontendSystem })` as the caller passed it + */ +export function assertNfsIntentMatches( + namespace: string, + explicitChoice: boolean | undefined, +): void { + const conflict = describeNfsIntentConflict(namespace, explicitChoice); + if (conflict) throw new Error(conflict); +} + /** * Which of the three mechanisms decided this lane's frontend, for the deploy log. * From 13c24c9e58384e6efb7b6db2f43c2b60fd3ddbb8 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Wed, 26 Aug 2026 09:44:05 -0300 Subject: [PATCH 7/7] fix(github-session): lock only session creation, and scope this PR to that MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes from review. Lock scope. The lock spanned read-or-login-and-write, so the reuse path — addCookies, goto, Sign In, wait for nav or popup — ran inside it too. Reuse needs no exclusivity: the lanes are different namespace hosts and only creation submits a TOTP. Every loginAsGithubUser therefore queued behind a sign-in it did not need, and a waiter could exhaust Playwright's default test timeout before proper-lockfile's own retries ran out, because test.setTimeout(260_000) is raised inside logintoGithub — precisely the path a waiter is not on. ensureGithubSession now checks for a session outside the lock and takes it only to create one, re-reading inside so a caller that queued behind the lane that created it reuses that session rather than logging in again with the same TOTP code. loginAsGithubUser splits into _reuseGithubSession and _createGithubSession; creation already leaves the page signed in, so the reuse path is not replayed after it. Mutation-verified, one test each: dropping the outer check fails "reuses an existing session without taking the lock"; dropping the inner re-read fails "creates once when two callers find no session at the same time". The first version of that first test was wrong and passed under the mutation — it started ensureGithubSession in the same tick as the lock holder, so it raced the scheduler rather than the lock and the result depended on how loaded the run was. It now waits until the lock is demonstrably held, and is bounded by a timer so reintroducing the bug fails instead of deadlocking CI. Scope. The NFS work is removed — nfs-guard, the deploy-time report and marker check, the SidebarTabs label, and the docs for them. With the old frontend coming out there is no legacy shell to silently fall back into, so guarding against it earns little and would be deleted in the cleanup anyway. --- docs/changelog.md | 10 +- docs/guide/deployment/rhdh-deployment.md | 54 +----- src/deployment/rhdh/deployment.ts | 47 ----- src/deployment/rhdh/nfs-guard.test.ts | 168 ----------------- src/deployment/rhdh/nfs-guard.ts | 171 ------------------ src/playwright/helpers/common.ts | 130 ++++++++----- src/playwright/helpers/github-session.test.ts | 89 +++++++++ src/playwright/helpers/navbar.ts | 12 -- 8 files changed, 185 insertions(+), 496 deletions(-) delete mode 100644 src/deployment/rhdh/nfs-guard.test.ts delete mode 100644 src/deployment/rhdh/nfs-guard.ts diff --git a/docs/changelog.md b/docs/changelog.md index 14b133a..5710e8d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -4,15 +4,11 @@ All notable changes to this project will be documented in this file. ## [2.1.12] - Current -### Added - -- **A lane now says which shell it runs, and where that was decided** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): `deploy()` logs `[nfs] : new frontend system ON|off, from ` — from `deploy()` rather than `configure()`, because the worker fixture calls `configure()` with no arguments for every project before any spec runs, so a lane opting in through `configure({ useNewFrontendSystem: true })` would print `off` first and `ON` second, and the first line is the one a reader greps. Three mechanisms can enable NFS — the `-app-next` project name, `USE_NEW_FRONTEND_SYSTEM=true`, and `configure({ useNewFrontendSystem: true })` — and none of them is visible from a single file, so answering "is this lane NFS?" meant reading three. A namespace ending in `-app-next` that was explicitly configured with `useNewFrontendSystem: false` also warns, because it deploys the legacy shell under a name that reads as an NFS lane in every report. Only that direction is checked: a lane that enables NFS without the suffix is legitimate and common. -- **`SidebarTabs` accepts both shells' scaffolder label** ([RHIDP-16458](https://redhat.atlassian.net/browse/RHIDP-16458)): the union hardcoded `"Self-service"`, the legacy shell's title. Under app-next the same page is `"Create"`, so a spec running there could not type-check without a cast. Both are members now. This does not decide which label a lane should use — that is [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462), and three different workarounds are in flight for it. - ### Fixed -- **A workspace could silently disable the new frontend system it asked for** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): the NFS secret layer is merged *before* the workspace's own `tests/config/rhdh-secrets.yaml`, so a workspace setting `APP_CONFIG_app_packageName` or `ENABLE_STANDARD_MODULE_FEDERATION` for its own reasons overrode it and the lane booted the legacy shell. Nothing failed — the legacy suite re-ran and passed, and the one thing the lane existed to prove was never exercised. `deploy()` now checks the merged secret still carries both markers and throws naming the key, the value found, and the file that overrode it. The merge order itself is unchanged: a workspace must be able to override defaults, which is precisely why the outcome needs checking rather than assuming. -- **The GitHub session file was shared with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. So every lane and every worker shared one file with no lock: a reader landing mid-write failed on truncated JSON, as a flake that looked nothing like the plugin under test, and every added lane adds a writer. Access is now serialised across the whole run, the write goes through a temp file and a rename (removed even when it fails), the path is absolute, and an unreadable or empty session falls through to a full login instead of throwing. Deliberately still **one file per user, not per project**: scoping it per project is the obvious fix and is the wrong one, because `logintoGithub` derives its 2FA code from a single shared TOTP secret, so lanes logging in inside the same 30-second window submit the identical code and GitHub rejects the second. +- **The GitHub session file was shared with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. So every lane and every worker shared one file with no lock: a reader landing mid-write failed on truncated JSON, as a flake that looked nothing like the plugin under test, and every added lane adds a writer. Access to *creating* the session is now serialised across the whole run, the write goes through a temp file and a rename (removed even when it fails), the path is absolute, and an unreadable or empty session falls through to a full login instead of throwing. Deliberately still **one file per user, not per project**: scoping it per project is the obvious fix and is the wrong one, because `logintoGithub` derives its 2FA code from a single shared TOTP secret, so lanes logging in inside the same 30-second window submit the identical code and GitHub rejects the second. + + Only creation takes the lock. Reusing an existing session is cookies plus a Sign In click against a different namespace host, and holding the lock across it made every lane queue behind a sign-in it did not need — long enough that a waiter could exhaust Playwright's default test timeout, since `test.setTimeout` is raised inside the login itself, which is exactly the path a waiter is not on. The session file is re-read inside the lock so a caller that queued behind the lane that created it reuses that session instead of logging in again. ## [2.1.11] diff --git a/docs/guide/deployment/rhdh-deployment.md b/docs/guide/deployment/rhdh-deployment.md index 071f1a0..c824ebf 100644 --- a/docs/guide/deployment/rhdh-deployment.md +++ b/docs/guide/deployment/rhdh-deployment.md @@ -89,43 +89,10 @@ await rhdh.deploy(); What gets merged (same order as other config: package defaults → auth → NFS defaults → your workspace files; later wins; secrets are merged then **envsubst** runs once on the result): -1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"`. Your `rhdh-secrets.yaml` merges last and wins on every other key, and can use `$VAR` substitution — but **not on these two**: overriding either would leave the lane running the legacy shell while calling itself NFS, so `deploy()` throws instead. See below. -2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins). Re-pinning is fine; **disabling** either degrades the lane the same way the secrets above would, and that one is not guarded — if an NFS lane behaves like the legacy shell, check this list first. +1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"` (your `rhdh-secrets.yaml` still wins on conflicts and can use `$VAR` substitution). +2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins). 3. **Helm** — Package `config/new-frontend-system/value_file.yaml`, then your `value_file.yaml`, then optional `tests/config/value_file-app-next.yaml` when that file exists. -#### Knowing which shell actually ran - -`deploy()` logs the answer once, because three mechanisms can decide it and none of them -is visible from a single file: - -``` -[nfs] tech-radar-app-next: new frontend system ON, from the -app-next project name -[nfs] github: new frontend system ON, from configure({ useNewFrontendSystem: true }) -[nfs] rbac: new frontend system off, from nothing — the default is the legacy shell -``` - -Grep `[nfs]` when a lane behaves like the wrong shell. It is logged from `deploy()` -rather than `configure()` on purpose: the worker fixture calls `configure()` with no -arguments for every project before any spec runs, so a lane opting in through -`configure({ useNewFrontendSystem: true })` would otherwise print `off` first. - -A namespace ending in `-app-next` that is explicitly configured with -`useNewFrontendSystem: false` warns — it deploys the legacy shell under a name that -reads as an NFS lane in every report. The reverse is not checked: enabling NFS without -the suffix is normal. - -If the merged secret would not actually enable NFS, `deploy()` throws rather than -deploying: - -``` -[nfs] "bulk-import-app-next" is configured for the new frontend system, but the -secret about to be applied would not enable it: - APP_CONFIG_app_packageName is "app" (expected "app-next") -``` - -That is a post-condition on your own config, not a bug in the harness: remove the -override from `tests/config/rhdh-secrets.yaml`, or set it to the value named. - Workspace-specific **app-config** (titles, plugin routes, etc.) remains your responsibility. ### Example: Full Configuration @@ -183,15 +150,14 @@ await rhdh.deploy({ timeout: null }); `deploy()` automatically skips if the deployment already succeeded in the current test run (e.g., after a worker restart due to test failure). This prevents expensive re-deployments. This method: -1. Reports which frontend shell this lane will run, and what decided it — see [New frontend system](#new-frontend-system-usenewfrontendsystem) -2. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins -3. Substitutes environment variables in the merged secrets (`envsubst`) -4. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config -5. Applies ConfigMaps (app-config, dynamic-plugins) -6. Applies Secrets — **throws** first if the lane asked for the new frontend system and the merged secret would not enable it -7. Installs RHDH via Helm or Operator -8. Waits for the deployment to be ready -9. Sets `RHDH_BASE_URL` environment variable +1. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins +2. Substitutes environment variables in the merged secrets (`envsubst`) +3. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config +4. Applies ConfigMaps (app-config, dynamic-plugins) +5. Applies Secrets +6. Installs RHDH via Helm or Operator +7. Waits for the deployment to be ready +8. Sets `RHDH_BASE_URL` environment variable #### Base URL format diff --git a/src/deployment/rhdh/deployment.ts b/src/deployment/rhdh/deployment.ts index 2ac19fc..bbfc5cc 100644 --- a/src/deployment/rhdh/deployment.ts +++ b/src/deployment/rhdh/deployment.ts @@ -1,10 +1,5 @@ import { KubernetesClientHelper } from "../../utils/kubernetes-client.js"; import { WorkspacePaths } from "../../utils/workspace-paths.js"; -import { - assertNfsIntentMatches, - assertNfsMarkersSurvived, - describeNfsSource, -} from "./nfs-guard.js"; import { $ } from "../../utils/bash.js"; import yaml from "js-yaml"; import os from "os"; @@ -38,8 +33,6 @@ export class RHDHDeployment { public k8sClient = new KubernetesClientHelper(); public rhdhUrl: string; public deploymentConfig: DeploymentConfig; - /** `configure({ useNewFrontendSystem })` as the caller passed it, for the report. */ - private _explicitFrontendSystemChoice: boolean | undefined; constructor(namespace: string) { this.deploymentConfig = this._buildDeploymentConfig({ namespace }); @@ -56,7 +49,6 @@ export class RHDHDeployment { const executed = await runOnce( `deploy-${this.deploymentConfig.namespace}`, async () => { - this._reportFrontendSystem(); this._log("Starting RHDH deployment..."); this._log("RHDH Base URL: " + this.rhdhUrl); console.table(this.deploymentConfig); @@ -128,16 +120,6 @@ export class RHDHDeployment { const secretPayload = substituted as Record; - if (this.deploymentConfig.useNewFrontendSystem) { - // The workspace's own secrets file merges last, so the NFS markers can be - // overwritten here and nothing downstream would notice: the lane would boot - // the legacy shell and pass. - assertNfsMarkersSurvived( - (secretPayload as { stringData?: Record }).stringData, - this.deploymentConfig.namespace, - ); - } - await this.k8sClient.applySecretFromObject( "rhdh-secrets", secretPayload as { stringData?: Record }, @@ -515,9 +497,6 @@ export class RHDHDeployment { "helm"; const namespace = input.namespace ?? this.deploymentConfig.namespace; - // Kept so the deploy-time report can name the mechanism; a resolved boolean - // cannot distinguish the three sources. - this._explicitFrontendSystemChoice = input.useNewFrontendSystem; const useNewFrontendSystem = input.useNewFrontendSystem ?? (namespace.endsWith("-app-next") || @@ -561,32 +540,6 @@ export class RHDHDeployment { ); } - /** - * Says which shell this lane will run, and where that was decided. - * - * Three mechanisms can turn NFS on and none of them is visible from a single - * file, so "is this lane NFS?" is otherwise answered by reading a project name, - * an environment variable and a `configure()` call together. - * - * Reported from `deploy()` rather than `configure()`: the worker fixture calls - * `configure()` with no arguments for every project before any spec runs, so a lane - * that opts in with `configure({ useNewFrontendSystem: true })` — which is how - * `github` and `homepage` do it — would print `off` first and `ON` second, and the - * first line is the one a reader greps. - */ - private _reportFrontendSystem(): void { - const { namespace, useNewFrontendSystem } = this.deploymentConfig; - const explicitChoice = this._explicitFrontendSystemChoice; - // Logged before the check so the resolved state is on record even when the - // next line ends the run. - console.log( - `[nfs] ${namespace}: new frontend system ` + - `${useNewFrontendSystem ? "ON" : "off"}, from ` + - `${describeNfsSource(namespace, explicitChoice)}`, - ); - assertNfsIntentMatches(namespace, explicitChoice); - } - private _buildBaseUrl(): string { const prefix = this.deploymentConfig.method === "helm" diff --git a/src/deployment/rhdh/nfs-guard.test.ts b/src/deployment/rhdh/nfs-guard.test.ts deleted file mode 100644 index 7e7a023..0000000 --- a/src/deployment/rhdh/nfs-guard.test.ts +++ /dev/null @@ -1,168 +0,0 @@ -import { describe, it } from "node:test"; -import assert from "node:assert"; -import { - NFS_SECRET_MARKERS, - assertNfsMarkersSurvived, - assertNfsIntentMatches, - describeNfsIntentConflict, - describeNfsSource, - findDroppedNfsMarkers, -} from "./nfs-guard.js"; - -const [PACKAGE_NAME_KEY] = NFS_SECRET_MARKERS[0]; -const [MF_KEY] = NFS_SECRET_MARKERS[1]; - -/** A merged secret in which both markers survived, plus unrelated workspace data. */ -const intact = (): Record => ({ - ...Object.fromEntries(NFS_SECRET_MARKERS), - ["SOME_TOKEN"]: "x", -}); - -describe("nfs secret markers", () => { - it("finds nothing dropped when both markers survive the merge", () => { - assert.deepStrictEqual(findDroppedNfsMarkers(intact()), []); - }); - - it("reports a marker the workspace overwrote", () => { - // tests/config/rhdh-secrets.yaml merges after the NFS defaults, so this is - // what a workspace setting the key for its own reasons produces. - const dropped = findDroppedNfsMarkers({ - ...intact(), - [PACKAGE_NAME_KEY]: "app", - }); - assert.deepStrictEqual(dropped, [ - { key: PACKAGE_NAME_KEY, expected: "app-next", actual: "app" }, - ]); - }); - - it("reports a marker that is absent, distinctly from one that is wrong", () => { - const rest = intact(); - delete rest[MF_KEY]; - const dropped = findDroppedNfsMarkers(rest); - assert.strictEqual(dropped.length, 1); - assert.strictEqual(dropped[0].actual, undefined); - }); - - it("accepts an unquoted YAML true rather than raising a false alarm", () => { - // A typing mistake in the workspace's YAML, not a different value — the intent is - // plainly the same. Comparing without coercing would fail a working lane. - const dropped = findDroppedNfsMarkers({ ...intact(), [MF_KEY]: true }); - assert.deepStrictEqual(dropped, []); - }); - - it("rewrites a boolean marker to the string the API server requires", () => { - // stringData goes straight onto the V1Secret body, so a JSON boolean is rejected - // with "cannot unmarshal bool into Go struct field ... of type string". Tolerating - // it in the comparison alone would green-light a config that cannot apply. - const payload = { ...intact(), [MF_KEY]: true }; - assertNfsMarkersSurvived(payload, "ws-app-next"); - assert.strictEqual(payload[MF_KEY], "true"); - }); - - it("counts a missing stringData block as both markers dropped", () => { - assert.strictEqual(findDroppedNfsMarkers(undefined).length, 2); - }); - - it("throws naming the namespace, the key, and where the override comes from", () => { - assert.throws( - () => - assertNfsMarkersSurvived( - { ...intact(), [PACKAGE_NAME_KEY]: "app" }, - "bulk-import-app-next", - ), - (err: Error) => { - assert.match(err.message, /bulk-import-app-next/); - assert.match(err.message, /APP_CONFIG_app_packageName is "app"/); - assert.match(err.message, /rhdh-secrets\.yaml is merged after/); - return true; - }, - ); - }); - - it("does not throw when the markers are intact", () => { - assert.doesNotThrow(() => - assertNfsMarkersSurvived(intact(), "ws-app-next"), - ); - }); -}); - -describe("nfs intent conflict", () => { - it("flags an -app-next namespace explicitly opted out of NFS", () => { - const msg = describeNfsIntentConflict("quay-app-next", false); - assert.match(String(msg), /named -app-next/); - }); - - it("says nothing when the name and the intent agree", () => { - assert.strictEqual( - describeNfsIntentConflict("quay-app-next", true), - undefined, - ); - assert.strictEqual(describeNfsIntentConflict("quay", false), undefined); - }); - - it("says nothing when no explicit choice was made", () => { - // The default path resolves to NFS from the name, so there is no conflict. - assert.strictEqual( - describeNfsIntentConflict("quay-app-next", undefined), - undefined, - ); - }); - - it("fails the deploy on the conflict, rather than only naming it", () => { - // A warning on a run that exits 0 is the failure this module exists to stop: - // the legacy suite re-runs, everything passes, and nothing proved NFS works. - assert.throws( - () => assertNfsIntentMatches("quay-app-next", false), - /named -app-next/, - ); - }); - - it("lets every non-conflicting combination through", () => { - assert.doesNotThrow(() => assertNfsIntentMatches("quay-app-next", true)); - assert.doesNotThrow(() => - assertNfsIntentMatches("quay-app-next", undefined), - ); - assert.doesNotThrow(() => assertNfsIntentMatches("quay", false)); - assert.doesNotThrow(() => assertNfsIntentMatches("github", true)); - }); - - it("does not flag the reverse, which is legitimate", () => { - // github and homepage enable NFS through configure() without renaming, and a - // global USE_NEW_FRONTEND_SYSTEM may turn it on everywhere. - assert.strictEqual(describeNfsIntentConflict("github", true), undefined); - }); -}); - -describe("nfs source", () => { - it("names an explicit choice over the project name", () => { - assert.match(describeNfsSource("ws-app-next", true), /configure\(/); - }); - - it("names the project name when nothing was passed", () => { - assert.match(describeNfsSource("ws-app-next", undefined), /-app-next/); - }); - - it("names the environment variable when it is what decided", () => { - const prev = process.env.USE_NEW_FRONTEND_SYSTEM; - process.env.USE_NEW_FRONTEND_SYSTEM = "true"; - try { - assert.match( - describeNfsSource("ws", undefined), - /USE_NEW_FRONTEND_SYSTEM/, - ); - } finally { - if (prev === undefined) delete process.env.USE_NEW_FRONTEND_SYSTEM; - else process.env.USE_NEW_FRONTEND_SYSTEM = prev; - } - }); - - it("says so when nothing enabled it", () => { - const prev = process.env.USE_NEW_FRONTEND_SYSTEM; - delete process.env.USE_NEW_FRONTEND_SYSTEM; - try { - assert.match(describeNfsSource("ws", undefined), /legacy shell/); - } finally { - if (prev !== undefined) process.env.USE_NEW_FRONTEND_SYSTEM = prev; - } - }); -}); diff --git a/src/deployment/rhdh/nfs-guard.ts b/src/deployment/rhdh/nfs-guard.ts deleted file mode 100644 index c8bd959..0000000 --- a/src/deployment/rhdh/nfs-guard.ts +++ /dev/null @@ -1,171 +0,0 @@ -/** - * Guards against a lane that believes it is running the new frontend system - * while the deployed instance is running the legacy shell. - * - * That combination is the worst failure this harness can produce, because it is - * not a failure: the legacy suite re-runs, every assertion passes, and the only - * thing the lane exists to prove — that the plugin works under NFS — was never - * exercised. Nothing errors and nothing is red. - * - * The checks here are deliberately cluster-free. They run on the configuration - * this library is about to apply, which is where the two reachable causes live. - */ - -/** - * The secret entries that actually switch RHDH to the new frontend system. - * - * Mirrors `config/new-frontend-system/secrets.yaml`. `APP_CONFIG_app_packageName` - * selects the `app-next` bundle; `ENABLE_STANDARD_MODULE_FEDERATION` is what stops - * RHDH overriding the dynamic-features service away from standard module federation. - * - * Entries rather than an object because these are external environment-variable - * names, not JavaScript identifiers — writing them as property names would fight - * the repo's camelCase rule everywhere they appear. - */ -export const NFS_SECRET_MARKERS: ReadonlyArray = [ - ["APP_CONFIG_app_packageName", "app-next"], - ["ENABLE_STANDARD_MODULE_FEDERATION", "true"], -]; - -export type DroppedMarker = { - key: string; - expected: string; - /** `undefined` when the key is absent rather than overwritten. */ - actual: string | undefined; -}; - -/** - * Markers that did not survive the secret merge. - * - * The NFS layer is merged *before* the workspace's own `rhdh-secrets.yaml`, so a - * workspace that sets either key for its own reasons silently wins and the lane - * boots legacy. That ordering is intentional — a workspace must be able to override - * defaults — which is exactly why the outcome has to be checked rather than assumed. - */ -export function findDroppedNfsMarkers( - stringData: Record | undefined, -): DroppedMarker[] { - const dropped: DroppedMarker[] = []; - for (const [key, expected] of NFS_SECRET_MARKERS) { - const raw = stringData?.[key]; - const actual = raw === undefined ? undefined : String(raw); - if (actual !== expected) dropped.push({ key, expected, actual }); - } - return dropped; -} - -/** - * Throws when the applied secret would not turn NFS on after all. - * - * @param stringData - the merged `stringData` about to be applied - * @param namespace - named in the message, because the reader is looking at one - * lane's output among many - */ -export function assertNfsMarkersSurvived( - stringData: Record | undefined, - namespace: string, -): void { - // Coerce before comparing AND in place: applySecretFromObject puts stringData - // straight on the V1Secret body, so an unquoted YAML `true` reaches the API server - // as a JSON boolean and is rejected with "cannot unmarshal bool into Go struct - // field ... of type string". Accepting it in the comparison alone would green-light - // a config that cannot apply, and the failure would surface as an opaque k8s error - // instead of this guard's message. - if (stringData) { - for (const [key] of NFS_SECRET_MARKERS) { - const raw = stringData[key]; - if (raw !== undefined && typeof raw !== "string") { - stringData[key] = String(raw); - } - } - } - - const dropped = findDroppedNfsMarkers(stringData); - if (dropped.length === 0) return; - - const detail = dropped - .map(({ key, expected, actual }) => - actual === undefined - ? ` ${key} is missing (expected "${expected}")` - : ` ${key} is "${actual}" (expected "${expected}")`, - ) - .join("\n"); - - throw new Error( - `[nfs] "${namespace}" is configured for the new frontend system, but the ` + - `secret about to be applied would not enable it:\n${detail}\n` + - `The workspace's own tests/config/rhdh-secrets.yaml is merged after the NFS ` + - `defaults, so setting either key there overrides them. Remove it, or set it ` + - `to the value above. Without this the lane boots the legacy shell and passes ` + - `while proving nothing about NFS.`, - ); -} - -/** - * A stated intent that contradicts the namespace it is deployed into. - * - * `-app-next` is one of the three ways a lane says it wants NFS, and it is the one - * that also names the Kubernetes namespace, so a lane called `-app-next` running - * legacy is indistinguishable from a working one in any report. An explicit - * `useNewFrontendSystem: false` is the only way to reach that state, so it is worth - * naming rather than honouring silently. - * - * Only this direction is checkable. The converse — a lane *not* named `-app-next` - * that resolves to NFS — is legitimate: `USE_NEW_FRONTEND_SYSTEM=true` may turn it - * on globally, and `configure({ useNewFrontendSystem: true })` is a supported way - * to opt in without renaming the project. - * - * @returns the message to warn with, or `undefined` when there is no contradiction - */ -export function describeNfsIntentConflict( - namespace: string, - explicitChoice: boolean | undefined, -): string | undefined { - if (explicitChoice !== false) return undefined; - if (!namespace.endsWith("-app-next")) return undefined; - return ( - `[nfs] "${namespace}" is named -app-next but was configured with ` + - `useNewFrontendSystem: false, so it will deploy the legacy shell under a ` + - `name that reads as an NFS lane. Rename the project or drop the override.` - ); -} - -/** - * Throws on the conflict {@link describeNfsIntentConflict} finds. - * - * Reporting it is not enough. The lane exists to prove the plugin works under NFS, - * and a warning on a run that exits 0 is indistinguishable from a run that had - * nothing to say — which is the exact failure this module is here to stop. Both - * ways out are cheap: rename the project, or drop the override. - * - * @param namespace - the lane's namespace, which is its Playwright project name - * @param explicitChoice - `configure({ useNewFrontendSystem })` as the caller passed it - */ -export function assertNfsIntentMatches( - namespace: string, - explicitChoice: boolean | undefined, -): void { - const conflict = describeNfsIntentConflict(namespace, explicitChoice); - if (conflict) throw new Error(conflict); -} - -/** - * Which of the three mechanisms decided this lane's frontend, for the deploy log. - * - * There is no single documented way to enable NFS (RHIDP-16461), so a lane's output - * has to say which one fired — otherwise "is this lane NFS?" is answered by reading - * three different files. - */ -export function describeNfsSource( - namespace: string, - explicitChoice: boolean | undefined, -): string { - if (explicitChoice !== undefined) { - return `configure({ useNewFrontendSystem: ${explicitChoice} })`; - } - if (namespace.endsWith("-app-next")) return `the -app-next project name`; - if (process.env.USE_NEW_FRONTEND_SYSTEM === "true") { - return `USE_NEW_FRONTEND_SYSTEM=true`; - } - return `nothing — the default is the legacy shell`; -} diff --git a/src/playwright/helpers/common.ts b/src/playwright/helpers/common.ts index bba76a2..08fc30c 100644 --- a/src/playwright/helpers/common.ts +++ b/src/playwright/helpers/common.ts @@ -98,6 +98,35 @@ export async function withGithubSessionLock( } } +/** + * Creates the shared GitHub session if it is missing, and says which happened. + * + * Only creation needs to be exclusive: it drives a real GitHub sign-in whose 2FA + * code comes from one shared TOTP secret, so two lanes doing it inside the same + * 30-second window submit the identical code and the second is rejected. Reusing + * an existing session is just cookies plus a Sign In click against a different + * namespace host, and serialising that behind the lock made every lane queue for + * a sign-in it did not need — long enough that a waiter could exhaust Playwright's + * default test timeout before the lock's own retries ran out. `test.setTimeout` + * is raised inside the login itself, which is precisely the path a waiter is not on. + * + * The re-read inside the lock is what keeps that safe: whoever held the lock before + * us has almost certainly just created the session, and logging in again would be + * the same collision the lock exists to prevent. + */ +export async function ensureGithubSession( + file: string, + create: () => Promise, +): Promise<"reused" | "created"> { + if (readStoredCookies(file)) return "reused"; + + return await withGithubSessionLock(file, async () => { + if (readStoredCookies(file)) return "reused"; + await create(); + return "created"; + }); +} + export class LoginHelper { page: Page; uiHelper: UIhelper; @@ -194,58 +223,65 @@ export class LoginHelper { userid: string = process.env.VAULT_GH_USER_ID as string, ) { const sessionFileName = githubSessionFile(userid); - // The lock spans read-or-login-and-write, not just the write: two lanes that both - // decide there is no session go on to submit the same TOTP code. - await withGithubSessionLock(sessionFileName, async () => { - await this._loginAsGithubUser(userid, sessionFileName); - }); + const outcome = await ensureGithubSession(sessionFileName, () => + this._createGithubSession(userid, sessionFileName), + ); + // Creating already left this page signed in; replaying the reuse path would + // click Sign In a second time against a session that is already live. + if (outcome === "reused") { + await this._reuseGithubSession(userid, sessionFileName); + } } - private async _loginAsGithubUser(userid: string, sessionFileName: string) { - // Check if a session file for this specific user already exists + private async _reuseGithubSession(userid: string, sessionFileName: string) { const cookies = readStoredCookies(sessionFileName); - if (cookies) { - // Load and reuse existing authentication state - await this.page.context().addCookies(cookies); - console.log(`Reusing existing authentication state for user: ${userid}`); - await this.page.goto("/"); - await this.uiHelper.waitForLoad(12000); - await this.uiHelper.clickButton("Sign In"); - - // Wait for either: sidebar appears (auto-login) or popup opens (needs auth) - const navPromise = this.page - .waitForSelector("nav a", { timeout: 15_000 }) - .then(() => "nav" as const) - .catch(() => null); - - const popupPromise = this.page - .waitForEvent("popup", { timeout: 15_000 }) - .then((popup) => ({ popup })) - .catch(() => null); - - const result = await Promise.race([navPromise, popupPromise]); - - if (result === null) { - throw new Error( - "GitHub login failed: neither sidebar nor popup appeared after Sign In — session file may be stale", - ); - } + if (!cookies) { + throw new Error( + `GitHub session file for ${userid} disappeared between the check and the read: ${sessionFileName}`, + ); + } - if (typeof result === "object" && "popup" in result) { - // Popup opened — handle reauthorization - await this.handleGithubPopupReauth(result.popup); - } - } else { - // Perform login if no session file exists, then save the state - await this.logintoGithub(userid); - await this.page.goto("/"); - await this.uiHelper.waitForLoad(240000); - await this.uiHelper.clickButton("Sign In"); - await this.checkAndReauthorizeGithubApp(); - await this.page.waitForSelector("nav a", { timeout: 10_000 }); - await writeStorageStateAtomically(this.page, sessionFileName); - console.log(`Authentication state saved for user: ${userid}`); + // Load and reuse existing authentication state + await this.page.context().addCookies(cookies); + console.log(`Reusing existing authentication state for user: ${userid}`); + await this.page.goto("/"); + await this.uiHelper.waitForLoad(12000); + await this.uiHelper.clickButton("Sign In"); + + // Wait for either: sidebar appears (auto-login) or popup opens (needs auth) + const navPromise = this.page + .waitForSelector("nav a", { timeout: 15_000 }) + .then(() => "nav" as const) + .catch(() => null); + + const popupPromise = this.page + .waitForEvent("popup", { timeout: 15_000 }) + .then((popup) => ({ popup })) + .catch(() => null); + + const result = await Promise.race([navPromise, popupPromise]); + + if (result === null) { + throw new Error( + "GitHub login failed: neither sidebar nor popup appeared after Sign In — session file may be stale", + ); } + + if (typeof result === "object" && "popup" in result) { + // Popup opened — handle reauthorization + await this.handleGithubPopupReauth(result.popup); + } + } + + private async _createGithubSession(userid: string, sessionFileName: string) { + await this.logintoGithub(userid); + await this.page.goto("/"); + await this.uiHelper.waitForLoad(240000); + await this.uiHelper.clickButton("Sign In"); + await this.checkAndReauthorizeGithubApp(); + await this.page.waitForSelector("nav a", { timeout: 10_000 }); + await writeStorageStateAtomically(this.page, sessionFileName); + console.log(`Authentication state saved for user: ${userid}`); } async checkAndReauthorizeGithubApp() { diff --git a/src/playwright/helpers/github-session.test.ts b/src/playwright/helpers/github-session.test.ts index 1b2d6c8..9c7a8dc 100644 --- a/src/playwright/helpers/github-session.test.ts +++ b/src/playwright/helpers/github-session.test.ts @@ -4,6 +4,7 @@ import fs from "fs"; import os from "os"; import path from "path"; import { + ensureGithubSession, githubSessionFile, readStoredCookies, withGithubSessionLock, @@ -172,3 +173,91 @@ describe("reading a stored session", () => { assert.strictEqual(readStoredCookies(file), undefined); }); }); + +describe("ensuring the shared session", () => { + const writeSession = (file: string) => + fs.writeFileSync(file, JSON.stringify({ cookies: [{ name: "a" }] })); + + it("reuses an existing session without taking the lock", async () => { + // The point of the split: reuse is cookies plus a Sign In against a different + // namespace host. Holding the lock across it made every lane queue behind one + // sign-in it did not need. Proven by holding the lock elsewhere — if reuse + // still waited on it, this would block until the holder released. + const dir = tmp(); + const file = path.join(dir, "s.json"); + writeSession(file); + + let release: () => void = () => {}; + const holding = new Promise((resolve) => { + release = resolve; + }); + let held: () => void = () => {}; + const acquired = new Promise((resolve) => { + held = resolve; + }); + const holder = withGithubSessionLock(file, async () => { + held(); + await holding; + }); + // Wait until the lock is demonstrably held. Starting from the same tick races + // the scheduler instead of the lock, and the result then depends on how loaded + // the run is — it passed alone and passed under the full suite for different + // reasons, neither of them the one under test. + await acquired; + + try { + // Bounded rather than a plain await: if reuse ever waits on the lock again + // this deadlocks, and a hung CI job is harder to read than a failed + // assertion. proper-lockfile retries for 60s, far past this deadline. + const outcome = await Promise.race([ + ensureGithubSession(file, async () => { + throw new Error("must not create when a session already exists"); + }), + new Promise((_, reject) => { + setTimeout( + () => reject(new Error("reuse waited on the session lock")), + 2_000, + ).unref(); + }), + ]); + assert.strictEqual(outcome, "reused"); + } finally { + release(); + await holder; + } + }); + + it("creates once when two callers find no session at the same time", async () => { + // Both pass the outer check, so the re-read inside the lock is the only thing + // stopping the second from logging in again and submitting the same TOTP code. + const dir = tmp(); + const file = path.join(dir, "s.json"); + let creates = 0; + + const create = async () => { + creates += 1; + await new Promise((resolve) => setTimeout(resolve, 20)); + writeSession(file); + }; + + const outcomes = await Promise.all([ + ensureGithubSession(file, create), + ensureGithubSession(file, create), + ]); + + assert.strictEqual(creates, 1); + assert.deepStrictEqual(outcomes.filter((o) => o === "created").length, 1); + assert.deepStrictEqual(outcomes.filter((o) => o === "reused").length, 1); + }); + + it("reports creation so the caller does not replay the reuse path", async () => { + // Creating leaves the page signed in. A "created" that read as "reused" would + // click Sign In a second time against a live session. + const dir = tmp(); + const file = path.join(dir, "s.json"); + const outcome = await ensureGithubSession(file, async () => + writeSession(file), + ); + assert.strictEqual(outcome, "created"); + }); +}); diff --git a/src/playwright/helpers/navbar.ts b/src/playwright/helpers/navbar.ts index dcfdfd4..651984e 100644 --- a/src/playwright/helpers/navbar.ts +++ b/src/playwright/helpers/navbar.ts @@ -1,21 +1,9 @@ -/** - * Sidebar entries a suite can open. - * - * The label depends on which shell is running. The legacy RHDH shell titles the - * scaffolder entry point "Self-service"; under the new frontend system the same - * page is "Create", and `packages/app-next` ships no global header at all. The - * union carries both so a spec running in either shell type-checks without a cast - * — it does not decide which one a given lane should use, which is - * [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462). - */ export type SidebarTabs = | "Catalog" | "Settings" | "My Group" | "Home" - // Legacy shell label for the scaffolder; "Create" is the app-next label. | "Self-service" - | "Create" | "Learning Paths" | "Extensions" | "Bulk import"