diff --git a/docs/changelog.md b/docs/changelog.md index 7917b73..5710e8d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -2,7 +2,15 @@ All notable changes to this project will be documented in this file. -## [2.1.11] - Current +## [2.1.12] - Current + +### Fixed + +- **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] ### Fixed diff --git a/package.json b/package.json index 054cbef..d88922a 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.12", "description": "Test utilities for RHDH E2E tests", "license": "Apache-2.0", "repository": { diff --git a/src/playwright/helpers/common.ts b/src/playwright/helpers/common.ts index 1b7fa67..08fc30c 100644 --- a/src/playwright/helpers/common.ts +++ b/src/playwright/helpers/common.ts @@ -1,12 +1,132 @@ 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 lockfile from "proper-lockfile"; import { DEFAULT_USERS } from "../../deployment/keycloak/constants.js"; +/** + * 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 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. + * + * 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): 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. + */ +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. 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`; + 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(); + } +} + +/** + * 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; @@ -102,54 +222,66 @@ export class LoginHelper { async loginAsGithubUser( userid: string = process.env.VAULT_GH_USER_ID as string, ) { - const sessionFileName = `authState_${userid}.json`; - - // Check if a session file for this specific user already exists - if (fs.existsSync(sessionFileName)) { - // 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("/"); - 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", - ); - } + const sessionFileName = githubSessionFile(userid); + 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); + } + } - 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 this.page.context().storageState({ path: sessionFileName }); - console.log(`Authentication state saved for user: ${userid}`); + private async _reuseGithubSession(userid: string, sessionFileName: string) { + const cookies = readStoredCookies(sessionFileName); + if (!cookies) { + throw new Error( + `GitHub session file for ${userid} disappeared between the check and the read: ${sessionFileName}`, + ); + } + + // 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 new file mode 100644 index 0000000..9c7a8dc --- /dev/null +++ b/src/playwright/helpers/github-session.test.ts @@ -0,0 +1,263 @@ +import { describe, it } from "node:test"; +import assert from "node:assert"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { + ensureGithubSession, + 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 users different files, and is stable for one user", () => { + assert.notStrictEqual( + 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"))); + }); + + 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("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), []); + }); +}); + +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); + }); +}); + +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"); + }); +});