From e007379934f155f63dbe99b181951b72090a51ca Mon Sep 17 00:00:00 2001 From: Collin Beczak Date: Wed, 21 Oct 2026 10:54:29 -0500 Subject: [PATCH 1/2] fix end to end tests that are failing from removal of api key functionality --- .env.test | 3 - E2E_TESTS.md | 44 ++-- docker-compose.test.yaml | 19 +- e2e/challenge-managers.spec.ts | 10 +- e2e/challenge-reports.spec.ts | 15 +- e2e/challenge-team-managers.spec.ts | 8 +- e2e/challenge-teams.spec.ts | 6 +- e2e/fixtures.ts | 335 ++++++++++++++-------------- e2e/global-setup.ts | 66 ++++++ e2e/profile-settings.spec.ts | 11 +- e2e/project-team-ownership.spec.ts | 14 +- e2e/super-admin.spec.ts | 33 +-- e2e/task-feature-collection.spec.ts | 4 +- e2e/task-locking.spec.ts | 48 ++-- playwright.config.ts | 1 + 15 files changed, 312 insertions(+), 305 deletions(-) create mode 100644 e2e/global-setup.ts diff --git a/.env.test b/.env.test index 8701be4cc..6b88bfc92 100644 --- a/.env.test +++ b/.env.test @@ -4,6 +4,3 @@ VITE_APP_URL="http://localhost:3005" VITE_API_BASE_URL="http://localhost:9000" VITE_MAP_ROULETTE_SERVER_WEBSOCKET_URL="ws://localhost:9000/ws" - -# Must match MR_SUPER_KEY in docker-compose.test.yaml. -VITE_SERVER_API_KEY="super-secret-key" diff --git a/E2E_TESTS.md b/E2E_TESTS.md index 5261fbd3e..4a94d2bad 100644 --- a/E2E_TESTS.md +++ b/E2E_TESTS.md @@ -1,15 +1,21 @@ # Playwright E2E Tests -What `npm run test:e2e` covers, grouped by feature. **43 tests across 21 spec -files**: 41 run, and 2 are skipped because they need a second *authenticated* -identity the harness cannot provision on its own (see -[Not covered](#not-covered)). +What `npm run test:e2e` covers, grouped by feature. **71 tests across 26 spec +files**, all of which run. Specs live in [`e2e/`](e2e/); shared setup and API helpers are in [`e2e/fixtures.ts`](e2e/fixtures.ts). Most fixtures build their data over HTTP; `seedUser` is the exception and writes to the stack's database directly, because the backend has no endpoint that creates users. +Every request runs as a seeded super user unless a test asks for someone else. +The backend no longer takes API keys, and a real login needs OpenStreetMap, so +the harness logs in by signing the backend's session cookie itself with the +test stack's `APPLICATION_SECRET` (`sessionCookie` in `e2e/fixtures.ts`). Any +seeded user can be acted as this way (`mapperRequest`, `reviewerPage`). +`e2e/global-setup.ts` seeds the super user and restarts the backend once so it +picks them up from `MR_SUPER_ACCOUNTS`, which it reads only at boot. + --- ## Challenges @@ -44,7 +50,7 @@ because the backend has no endpoint that creates users. **Locking** — [`task-locking.spec.ts`](e2e/task-locking.spec.ts) - A second user is told a task is locked when they open it after the first - user *(needs `E2E_REVIEWER_API_KEY`)* + user ## Projects @@ -104,6 +110,15 @@ because the backend has no endpoint that creates users. **Profile** — [`profile-settings.spec.ts`](e2e/profile-settings.spec.ts) - A user can update their custom basemap URL setting +## Super admin + +**Users** — [`super-admin.spec.ts`](e2e/super-admin.spec.ts) +- The users page lists the authenticated super user, and search filters it + +**Challenge reports** — [`challenge-reports.spec.ts`](e2e/challenge-reports.spec.ts) +- Ordinary mappers file reports, and are held to one open report each +- Only a super admin sees reporters' contact details or can rule on a report + ## Explore **Location** — [`explore-location.spec.ts`](e2e/explore-location.spec.ts) @@ -113,26 +128,11 @@ because the backend has no endpoint that creates users. ## Not covered -Every request the harness makes carries `MR_SUPER_KEY`, which the backend maps -to a single synthetic super user rather than to any row in the users table. No -endpoint creates users — real ones only arrive through OSM OAuth — so anything -needing a second person, or needing permissions to actually bite, is out of -reach. - -- **Acting *as* a granted user.** A user can be seeded into the database to be - granted a role (`seedUser` in `e2e/fixtures.ts`), so granting, re-roling and - revoking are all covered. What that grant actually lets them *do* is not: - authenticating as them needs an API key. That is covered in the backend's - `ChallengeServiceSpec`. +- **Logging in.** The harness signs its own session cookies, so the OSM OAuth + flow itself is never exercised. - **Applying a tag fix.** The apply step runs inside an embedded iD editor against real OpenStreetMap data the test stack cannot reach. What a tag-fix challenge and its tasks propose is covered; carrying it out is not. -- **Super admin** — [`super-admin.spec.ts`](e2e/super-admin.spec.ts) is - `test.skip`: the users page needs a real super-admin-granted identity. - `src/lib/SuperAdminGuard.test.ts` unit-tests the guard predicate instead. -- **Two-user task locking** — runs only when `E2E_REVIEWER_API_KEY` is set to a - real user's `"|"` key. Provisioning one is a manual, - out-of-band step documented in `e2e/fixtures.ts`. - **Review workflow** — no specs at all; there is no reviewer UI yet. ## Running them diff --git a/docker-compose.test.yaml b/docker-compose.test.yaml index daa5c9f7e..640e52f8b 100644 --- a/docker-compose.test.yaml +++ b/docker-compose.test.yaml @@ -50,20 +50,17 @@ services: MR_DATABASE_URL: jdbc:postgresql://db:5432/maproulette MR_DATABASE_USERNAME: maproulette MR_DATABASE_PASSWORD: hunter2 - # Must match VITE_SERVER_API_KEY in .env.test. Any request bearing this - # value in the `apiKey` header is treated by the backend as a super-user, - # so every request the E2E harness makes by default runs with full - # privileges — there's no built-in way to exercise a non-admin user or - # real permission boundaries (e.g. review workflows, /super-admin/* - # access checks) against this stack. See the REVIEWER_KEY fixture in - # e2e/fixtures.ts for the (manual, out-of-band) way to provision a - # second, non-super-user identity, and the skip comment in - # e2e/super-admin.spec.ts for a concrete case this limitation blocks. - MR_SUPER_KEY: super-secret-key + # Also the key Play signs session cookies with, so e2e/fixtures.ts signs + # its own with it to log in as seeded users. API keys, which the harness + # used before, were removed from the backend in 7a8cdb17. APPLICATION_SECRET: test-application-secret-at-least-32-chars MAPROULETTE_SECRET_KEY: test-maproulette-secret-key-at-least-32-chars + # The OSM id of the harness's super user (SUPER_USER_OSM_ID in + # e2e/fixtures.ts). The backend only promotes it at boot, so global setup + # seeds the user and then restarts the backend once. + MR_SUPER_ACCOUNTS: "800000000" # OAuth values are required for the backend to boot but are not used - # (the tests all use API key auth) + # (the harness signs its own session cookies; see e2e/fixtures.ts) MR_OSM_SERVER: https://master.apis.dev.openstreetmap.org MR_OAUTH_CONSUMER_KEY: not_a_real_key MR_OAUTH_CONSUMER_SECRET: not_a_real_secret diff --git a/e2e/challenge-managers.spec.ts b/e2e/challenge-managers.spec.ts index fd1f1f0b1..c15080be5 100644 --- a/e2e/challenge-managers.spec.ts +++ b/e2e/challenge-managers.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' // Granting an individual user a role on a single challenge, which reaches // where the parent project's grants do not. @@ -113,9 +113,7 @@ test("a manager's role can be changed, and they keep just the one", async ({ // A user holds one role per challenge: re-granting replaces rather than // accumulates, so the server should report exactly one entry. await expect(async () => { - const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}/managers`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}/managers`) const managers = (await response.json()) as Array<{ userId: number; role: number }> expect(managers).toHaveLength(1) expect(managers[0]).toMatchObject({ userId: seededUser.id, role: 1 }) @@ -136,9 +134,7 @@ test('a manager can be removed again', async ({ page, request, challenge, seeded await expect(dialog.getByText('No managers yet.')).toBeVisible({ timeout: 15_000 }) await expect(async () => { - const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}/managers`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}/managers`) expect(await response.json()).toEqual([]) }).toPass({ timeout: 20_000 }) }) diff --git a/e2e/challenge-reports.spec.ts b/e2e/challenge-reports.spec.ts index bc98e9d0f..e09ddbcc9 100644 --- a/e2e/challenge-reports.spec.ts +++ b/e2e/challenge-reports.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' // Reporting a challenge, end to end: a mapper files one, a second mapper files // their own, and a super admin triages them. @@ -100,7 +100,6 @@ test('a mapper sees the reports on a challenge, without the admin side of them', // The super admin does see it. const asAdmin = await request.get(`${BACKEND_URL}/api/v2/challenge/reports`, { - headers: { apiKey: SUPER_KEY }, params: { challengeId: challenge.id }, }) const adminView = (await asAdmin.json()) as Array<{ reporterEmail?: string | null }> @@ -118,12 +117,9 @@ test('two different mappers can each report the same challenge', async ({ }) // A second real identity, built the same way the fixture builds the first. - const { mintApiKey, seedUser, deleteSeededUser } = await import('./fixtures') + const { newSessionRequest, seedUser, deleteSeededUser } = await import('./fixtures') const second = seedUser(`e2e-second-${Date.now()}`, 920_000_000 + Math.floor(Math.random() * 1e7)) - const secondKey = await mintApiKey(request, second.id) - const secondContext = await playwright.request.newContext({ - extraHTTPHeaders: { apiKey: secondKey, 'Content-Type': 'application/json' }, - }) + const secondContext = await newSessionRequest(playwright.request, second) try { const response = await secondContext.post( @@ -134,7 +130,6 @@ test('two different mappers can each report the same challenge', async ({ expect(response.status()).toBe(201) const all = await request.get(`${BACKEND_URL}/api/v2/challenge/reports`, { - headers: { apiKey: SUPER_KEY }, params: { challengeId: challenge.id }, }) const reports = (await all.json()) as Array<{ reporterId: number }> @@ -159,7 +154,7 @@ test('a super admin can action a report, and the reporter sees the outcome only' const report = (await filed.json()) as { id: number } const decided = await request.put(`${BACKEND_URL}/api/v2/challenge/report/${report.id}/status`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { status: 'actioned', reviewComment: 'archived the challenge' }, }) expect(decided.ok()).toBe(true) @@ -194,7 +189,7 @@ test('resolving a report frees the reporter to raise the challenge again', async const report = (await filed.json()) as { id: number } await request.put(`${BACKEND_URL}/api/v2/challenge/report/${report.id}/status`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { status: 'dismissed' }, }) diff --git a/e2e/challenge-team-managers.spec.ts b/e2e/challenge-team-managers.spec.ts index e8f29a809..1a23f4d68 100644 --- a/e2e/challenge-team-managers.spec.ts +++ b/e2e/challenge-team-managers.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' // Attaching a team to a single challenge, which reaches that challenge alone // without handing over the whole project. Users could always be granted a role @@ -13,8 +13,7 @@ const attachTeamToChallenge = async ( challengeId: number ) => { const response = await request.post( - `${BACKEND_URL}/api/v2/team/${teamId}/challenge/${challengeId}/2`, - { headers: { apiKey: SUPER_KEY } } + `${BACKEND_URL}/api/v2/team/${teamId}/challenge/${challengeId}/2` ) if (!response.ok()) { throw new Error( @@ -99,8 +98,7 @@ test('attaching a team to a challenge leaves its project alone', async ({ // Reaching one challenge is the whole point of granting here rather than on // the project, so the project must not pick the team up. const response = await request.get( - `${BACKEND_URL}/api/v2/teams/projectManagers/${challenge.projectId}`, - { headers: { apiKey: SUPER_KEY } } + `${BACKEND_URL}/api/v2/teams/projectManagers/${challenge.projectId}` ) const body = (await response.json()) as Array<{ team: { id: number } }> expect(body.some((entry) => entry.team.id === team.id)).toBe(false) diff --git a/e2e/challenge-teams.spec.ts b/e2e/challenge-teams.spec.ts index 857728c18..afe99c82b 100644 --- a/e2e/challenge-teams.spec.ts +++ b/e2e/challenge-teams.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, giveChallengeToTeam, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, giveChallengeToTeam, test } from './fixtures' // Handing a challenge to a team: the team becomes its byline and the team's // image goes on its card. @@ -66,9 +66,7 @@ test('choosing a team on the form gives the challenge to that team', async ({ // The form's own round trip is what matters here: read the challenge back // rather than trusting the page it navigates to. await expect(async () => { - const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${challenge.id}`) const body = (await response.json()) as { ownerTeamId?: number | null } expect(body.ownerTeamId).toBe(team.id) }).toPass({ timeout: 20_000 }) diff --git a/e2e/fixtures.ts b/e2e/fixtures.ts index 287980bb2..07a44fe20 100644 --- a/e2e/fixtures.ts +++ b/e2e/fixtures.ts @@ -1,50 +1,89 @@ import { execFileSync } from 'node:child_process' -import { type APIRequestContext, test as base, type Page } from '@playwright/test' +import { createHash, createHmac, randomUUID } from 'node:crypto' +import { type APIRequest, type APIRequestContext, test as base, type Page } from '@playwright/test' // Where the test stack's backend is listening. Overridable so the stack can sit // beside a local development backend on 9000 rather than fighting it for the // port — set E2E_BACKEND_URL to match .env.test.local's VITE_API_BASE_URL, so // the browser and these fixtures talk to the same server. export const BACKEND_URL = process.env.E2E_BACKEND_URL ?? 'http://localhost:9000' -export const SUPER_KEY = 'super-secret-key' - -// A second, distinct backend identity, for tests that need two separate -// users (e.g. a mapper who completes a task and a separate reviewer who -// accepts or rejects it) — a workflow with zero coverage today because -// every fixture above authenticates as the same single synthetic identity. -// -// SUPER_KEY can't just be duplicated with a different string: the backend -// maps *any* value configured as its `MR_SUPER_KEY` to the exact same -// singleton `User.superUser` object (see `getSessionByApiKey` in -// maproulette-backend's `SessionManager.scala`), so a second "super key" -// would still be the identical identity, not a second one. A genuine second -// identity has to be a real user's own per-user API key, in the -// "|" form the backend expects from anyone who isn't using -// the super key. This repo/harness has no OAuth-free way to create that -// user — real users are only ever created via the OSM OAuth callback flow -// (see `SessionManager.getUser`), which this harness doesn't perform — so -// provisioning one is a manual, one-time, out-of-band step: -// 1. Create a real user in the test backend's database (e.g. a one-time -// real OSM OAuth login against the test backend, or a direct DB seed -// by whoever administers the test environment). -// 2. As the super user (already available to this harness), call -// `PUT /user/:userId/apikey` for that user's id to mint their API key. -// 3. Set the returned "|" value as `E2E_REVIEWER_API_KEY` -// in the environment (or `.env.test`) before running specs that use -// the `reviewerRequest` / `reviewerPage` fixtures below. -// Until that's done, those fixtures throw with this same explanation rather -// than silently running as the primary identity. -const REVIEWER_KEY = process.env.E2E_REVIEWER_API_KEY - -function requireReviewerKey(): string { - if (!REVIEWER_KEY) { - throw new Error( - 'The reviewerRequest/reviewerPage fixture requires E2E_REVIEWER_API_KEY to be set to a ' + - 'real user\'s "|" API key. See the comment above REVIEWER_KEY in ' + - 'e2e/fixtures.ts for how to provision one.' - ) + +// The user every request runs as unless a test picks someone else. +// docker-compose.test.yaml lists this OSM id in MR_SUPER_ACCOUNTS, which the +// backend reads only at boot, so e2e/global-setup.ts seeds the user and +// restarts the backend once to have them promoted to super user. +export const SUPER_USER_OSM_ID = 800_000_000 +export const SUPER_USER_TOKEN = 'e2e-super-user-token' + +// Must match APPLICATION_SECRET in docker-compose.test.yaml. +const APPLICATION_SECRET = 'test-application-secret-at-least-32-chars' + +export interface SessionUser { + id: number + osmId: number + token: string +} + +/** + * Logs in as a user the way the backend does after an OSM login. + * + * The backend no longer accepts API keys (maproulette-backend 7a8cdb17), and a + * real login needs OSM itself. What a login leaves behind, though, is just a + * Play session cookie: a JWT signed with the application secret, naming the + * user and carrying a hash of their stored OSM token (see AuthController's + * callback and SessionManager.sessionUser). The test stack's secret is known, + * so the harness signs the same cookie itself for any user it has seeded. + */ +export function sessionCookie(user: SessionUser): string { + const encode = (value: object) => Buffer.from(JSON.stringify(value)).toString('base64url') + const now = Math.floor(Date.now() / 1000) + const unsigned = `${encode({ alg: 'HS256', typ: 'JWT' })}.${encode({ + data: { + tokenHash: createHash('sha256').update(user.token).digest('hex'), + userId: String(user.id), + osmId: String(user.osmId), + userTick: String(Date.now()), + }, + nbf: now, + iat: now, + })}` + const signature = createHmac('sha256', APPLICATION_SECRET).update(unsigned).digest('base64url') + return `${unsigned}.${signature}` +} + +export function storageStateFor(user: SessionUser) { + return { + cookies: [ + { + name: 'PLAY_SESSION', + value: sessionCookie(user), + domain: new URL(BACKEND_URL).hostname, + path: '/', + expires: -1, + httpOnly: true, + secure: false, + sameSite: 'Lax' as const, + }, + ], + origins: [], + } +} + +export function superUser(): SessionUser { + const id = Number(process.env.E2E_SUPER_USER_ID) + if (!Number.isInteger(id)) { + throw new Error('E2E_SUPER_USER_ID is not set; e2e/global-setup.ts should have set it') } - return REVIEWER_KEY + return { id, osmId: SUPER_USER_OSM_ID, token: SUPER_USER_TOKEN } +} + +/** A request context that acts as `user`, for tests that need someone other than the super user. */ +export function newSessionRequest(api: APIRequest, user: SessionUser) { + return api.newContext({ + baseURL: BACKEND_URL, + storageState: storageStateFor(user), + extraHTTPHeaders: { 'Content-Type': 'application/json' }, + }) } const uniqueName = (prefix: string) => @@ -75,7 +114,7 @@ export interface TestTeam { async function createProject(request: APIRequestContext, name: string): Promise { const response = await request.post(`${BACKEND_URL}/api/v2/project`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { name, displayName: name, @@ -92,9 +131,7 @@ async function createProject(request: APIRequestContext, name: string): Promise< async function deleteProject(request: APIRequestContext, id: number): Promise { try { - const response = await request.delete(`${BACKEND_URL}/api/v2/project/${id}?immediate=true`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.delete(`${BACKEND_URL}/api/v2/project/${id}?immediate=true`) if (!response.ok()) { console.warn(`Project ${id} teardown returned ${response.status()}: ${await response.text()}`) } @@ -103,29 +140,40 @@ async function deleteProject(request: APIRequestContext, id: number): Promise { try { - const whoami = await request.get(`${BACKEND_URL}/api/v2/user/whoami`, { - headers: { apiKey: SUPER_KEY }, - }) + const whoami = await request.get(`${BACKEND_URL}/api/v2/user/whoami`) if (!whoami.ok()) { console.warn(`Lock teardown could not identify the acting user: ${whoami.status()}`) return } const { id: userId } = (await whoami.json()) as { id: number } - const locked = await request.get(`${BACKEND_URL}/api/v2/user/${userId}/lockedTasks`, { - headers: { apiKey: SUPER_KEY }, - }) + const locked = await request.get(`${BACKEND_URL}/api/v2/user/${userId}/lockedTasks`) if (!locked.ok()) { console.warn(`Lock teardown could not list held locks: ${locked.status()}`) return } for (const task of (await locked.json()) as { id: number }[]) { - const released = await request.get(`${BACKEND_URL}/api/v2/task/${task.id}/release`, { - headers: { apiKey: SUPER_KEY }, - }) + const released = await request.get(`${BACKEND_URL}/api/v2/task/${task.id}/release`) if (!released.ok()) { console.warn(`Task ${task.id} lock teardown returned ${released.status()}`) } @@ -146,7 +194,7 @@ async function createChallenge( name: string ): Promise { const response = await request.post(`${BACKEND_URL}/api/v2/challenge`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { parent: projectId, name, @@ -173,7 +221,7 @@ async function createTask( coordinates: [number, number] = [-95.454772, 37.6866588] ): Promise { const response = await request.post(`${BACKEND_URL}/api/v2/task`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { name, parent: challengeId, @@ -197,7 +245,7 @@ async function createTask( // delete and need their own teardown below. async function createTeam(request: APIRequestContext, name: string): Promise { const response = await request.post(`${BACKEND_URL}/api/v2/team`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, // `id` and `groupType` are required by the backend's Group writer even on // a create, and the frontend sends the same two placeholders (see // api.team.useCreateTeam). @@ -212,9 +260,7 @@ async function createTeam(request: APIRequestContext, name: string): Promise { try { - const response = await request.delete(`${BACKEND_URL}/api/v2/team/${id}`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.delete(`${BACKEND_URL}/api/v2/team/${id}`) if (!response.ok()) { console.warn(`Team ${id} teardown returned ${response.status()}: ${await response.text()}`) } @@ -234,8 +280,7 @@ export async function grantTeamProjectRole( role = 1 ): Promise { const response = await request.post( - `${BACKEND_URL}/api/v2/team/${teamId}/project/${projectId}/${role}`, - { headers: { apiKey: SUPER_KEY } } + `${BACKEND_URL}/api/v2/team/${teamId}/project/${projectId}/${role}` ) if (!response.ok()) { throw new Error(`Failed to add team to project: ${response.status()} ${await response.text()}`) @@ -252,7 +297,7 @@ export async function giveChallengeToTeam( teamId: number | null ): Promise { const response = await request.put(`${BACKEND_URL}/api/v2/challenge/${challengeId}`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { ownerTeamId: teamId }, }) if (!response.ok()) { @@ -260,45 +305,8 @@ export async function giveChallengeToTeam( } } -export interface TestUser { - id: number +export interface TestUser extends SessionUser { name: string - osmId: number -} - -/** - * A seeded user's own API key, in the "|" form the backend - * expects from anyone who is not using the super key. - * - * Minting it takes two calls, and only the second is useful: PUT .../apikey - * stores an encrypted key and hands back the encrypted value, which is not - * what a client sends. Reading the user back as the super user returns it - * decrypted and prefixed with the id, which is the usable form. - * - * This is what makes a second *acting* identity possible -- a real person who - * is not a superuser, so tests can cover what someone without elevated rights - * may do, and what they are refused. - */ -export async function mintApiKey(request: APIRequestContext, userId: number): Promise { - const minted = await request.put(`${BACKEND_URL}/api/v2/user/${userId}/apikey`, { - headers: { apiKey: SUPER_KEY }, - }) - if (!minted.ok()) { - throw new Error(`Could not mint an API key: ${minted.status()} ${await minted.text()}`) - } - - const reread = await request.get(`${BACKEND_URL}/api/v2/user/${userId}`, { - headers: { apiKey: SUPER_KEY }, - }) - if (!reread.ok()) { - throw new Error(`Could not read the user back: ${reread.status()} ${await reread.text()}`) - } - - const body = (await reread.json()) as { apiKey?: string } - if (!body.apiKey?.includes('|')) { - throw new Error(`User ${userId} came back without a usable API key: ${body.apiKey}`) - } - return body.apiKey } /** @@ -309,41 +317,40 @@ export async function mintApiKey(request: APIRequestContext, userId: number): Pr * to act on has to put one in the database itself. This reaches into the * compose stack to do it, which is why it is confined to this file. */ -function runSql(sql: string): string { - const compose = process.env.MR_COMPOSE_COMMAND ?? 'docker' +export function runSql(sql: string): string { + return compose([ + 'exec', + '-T', + 'db', + 'psql', + '-U', + 'maproulette', + '-d', + 'maproulette', + '-t', + '-A', + '-c', + sql, + ]) +} + +export function compose(args: string[]): string { return execFileSync( - compose, - [ - 'compose', - '-f', - 'docker-compose.test.yaml', - 'exec', - '-T', - 'db', - 'psql', - '-U', - 'maproulette', - '-d', - 'maproulette', - '-t', - '-A', - '-c', - sql, - ], + process.env.MR_COMPOSE_COMMAND ?? 'docker', + ['compose', '-f', 'docker-compose.test.yaml', ...args], { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'] } ).trim() } /** * A real user row, for tests that need somebody other than the super user to - * grant something to. They are only ever a target here, never an actor: this - * harness cannot authenticate as them (that needs an API key, see REVIEWER_KEY - * above). + * grant something to, or to act as (see sessionCookie). */ export function seedUser(name: string, osmId: number): TestUser { + const token = randomUUID() const output = runSql( `INSERT INTO users (osm_id, osm_created, name, oauth_token, oauth_secret) - VALUES (${osmId}, now(), '${name}', '', '') RETURNING id;` + VALUES (${osmId}, now(), '${name}', '${token}', '') RETURNING id;` ) // psql prints the returned row and then its own "INSERT 0 1" status line. const id = Number(output.split('\n')[0]?.trim()) @@ -351,7 +358,7 @@ export function seedUser(name: string, osmId: number): TestUser { // Throwing matters: a NaN id makes every later assertion vacuously pass. throw new Error(`Could not seed a user -- psql returned: ${JSON.stringify(output)}`) } - return { id, name, osmId } + return { id, name, osmId, token } } export function deleteSeededUser(user: TestUser): void { @@ -415,7 +422,7 @@ async function createTagFixTask( ): Promise { const coordinates: [number, number] = [-111.9003158, 40.7630373] const response = await request.post(`${BACKEND_URL}/api/v2/task`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { name, parent: challengeId, @@ -444,7 +451,6 @@ async function createTagFixTask( export interface TestActor { request: APIRequestContext user: TestUser - apiKey: string } export const test = base.extend<{ @@ -457,14 +463,28 @@ export const test = base.extend<{ seededUser: TestUser cooperativeChallenge: TestChallenge tagFixTask: TestTask + reviewer: TestUser reviewerRequest: APIRequestContext reviewerPage: Page }>({ + // Every browser page and request runs as the super user unless a test asks + // for someone else. + storageState: async ({ playwright: _playwright }, use) => { + await use(storageStateFor(superUser())) + }, + + request: async ({ playwright, storageState }, use) => { + const request = await playwright.request.newContext({ storageState }) + await use(request) + await request.dispose() + }, + project: async ({ request }, use) => { const project = await createProject(request, uniqueName('e2e-project')) await use(project) await releaseHeldLocks(request) await deleteProject(request, project.id) + clearOrphanedLocks() }, // Deleting `project` (above) cascades to its challenges and tasks on the @@ -509,20 +529,16 @@ export const test = base.extend<{ deleteSeededUser(user) }, - // A real person who is not a superuser, with their own API key, so a test can - // act as someone with ordinary rights and see what they are refused. - mapperRequest: async ({ playwright, request }, use) => { + // A real person who is not a superuser, logged in as themselves, so a test + // can act as someone with ordinary rights and see what they are refused. + mapperRequest: async ({ playwright }, use) => { const user = seedUser( uniqueName('e2e-actor').replace(/[^a-zA-Z0-9-]/g, ''), 910_000_000 + Math.floor(Math.random() * 80_000_000) ) - const key = await mintApiKey(request, user.id) - const context = await playwright.request.newContext({ - baseURL: BACKEND_URL, - extraHTTPHeaders: { apiKey: key, 'Content-Type': 'application/json' }, - }) + const context = await newSessionRequest(playwright.request, user) - await use({ request: context, user, apiKey: key }) + await use({ request: context, user }) await context.dispose() deleteSeededUser(user) @@ -539,47 +555,30 @@ export const test = base.extend<{ }, cooperativeChallenge: async ({ request, tagFixTask }, use) => { - const response = await request.get( - `${BACKEND_URL}/api/v2/challenge/${tagFixTask.challengeId}`, - { - headers: { apiKey: SUPER_KEY }, - } - ) + const response = await request.get(`${BACKEND_URL}/api/v2/challenge/${tagFixTask.challengeId}`) const body = (await response.json()) as { id: number; name: string; parent: number } await use({ id: body.id, name: body.name, projectId: body.parent }) }, - // Direct-API second identity (see REVIEWER_KEY above). Behaves like the - // built-in `request` fixture, except every call automatically carries the - // reviewer's apiKey header instead of the caller having to pass one. - reviewerRequest: async ({ playwright }, use) => { - const reviewerKey = requireReviewerKey() - const context = await playwright.request.newContext({ - baseURL: BACKEND_URL, - extraHTTPHeaders: { apiKey: reviewerKey, 'Content-Type': 'application/json' }, - }) + // A second person, for tests that need two separate users at once (e.g. a + // mapper who locks a task and another who finds it locked). + reviewer: async ({ request: _request }, use) => { + const user = seedUser( + uniqueName('e2e-reviewer').replace(/[^a-zA-Z0-9-]/g, ''), + 990_000_000 + Math.floor(Math.random() * 9_000_000) + ) + await use(user) + deleteSeededUser(user) + }, + + reviewerRequest: async ({ playwright, reviewer }, use) => { + const context = await newSessionRequest(playwright.request, reviewer) await use(context) await context.dispose() }, - // Browser-driven second identity. The frontend reads its apiKey once per - // page load from a single static /env.json served by the Vite dev server - // (see vite.config.ts's `runtimeEnv` plugin and the boot script in - // index.html) — that file is generated once from `.env.test` when the dev - // server starts, so every `page` in a test run would otherwise fetch the - // exact same apiKey, making a second *browser* identity structurally - // impossible without a frontend change. This fixture works around that by - // intercepting only this context's own /env.json request and swapping in - // the reviewer's key, so pages from this fixture see a different apiKey - // than pages from the default `page` fixture, without touching app code. - reviewerPage: async ({ browser }, use) => { - const reviewerKey = requireReviewerKey() - const context = await browser.newContext() - await context.route('**/env.json', async (route) => { - const response = await route.fetch() - const env = (await response.json()) as Record - await route.fulfill({ response, json: { ...env, VITE_SERVER_API_KEY: reviewerKey } }) - }) + reviewerPage: async ({ browser, reviewer }, use) => { + const context = await browser.newContext({ storageState: storageStateFor(reviewer) }) const page = await context.newPage() await use(page) await context.close() diff --git a/e2e/global-setup.ts b/e2e/global-setup.ts new file mode 100644 index 000000000..2ae739fca --- /dev/null +++ b/e2e/global-setup.ts @@ -0,0 +1,66 @@ +import { + BACKEND_URL, + compose, + runSql, + SUPER_USER_OSM_ID, + SUPER_USER_TOKEN, + sessionCookie, +} from './fixtures' + +const BOOT_TIMEOUT_MS = 180_000 + +/** + * Seeds the super user every test runs as by default, and makes sure the + * backend treats them as one. + * + * The backend only promotes the accounts in MR_SUPER_ACCOUNTS when it boots, + * and on a fresh stack this user does not exist yet at that point, so the + * backend is restarted once after seeding them. A stack reused from an earlier + * run already knows them and is left alone. + */ +export default async function globalSetup() { + const output = runSql( + `INSERT INTO users (osm_id, osm_created, name, oauth_token, oauth_secret) + VALUES (${SUPER_USER_OSM_ID}, now(), 'e2e-super-user', '${SUPER_USER_TOKEN}', '') + ON CONFLICT (osm_id) DO UPDATE SET oauth_token = EXCLUDED.oauth_token, modified = now() + RETURNING id;` + ) + const id = Number(output.split('\n')[0]?.trim()) + if (!Number.isInteger(id)) { + throw new Error(`Could not seed the super user -- psql returned: ${JSON.stringify(output)}`) + } + process.env.E2E_SUPER_USER_ID = String(id) + + const user = { id, osmId: SUPER_USER_OSM_ID, token: SUPER_USER_TOKEN } + if (await isSuperUser(user)) return + + compose(['restart', 'backend']) + await waitForBackend() + if (!(await isSuperUser(user))) { + throw new Error( + `The backend did not promote user ${id} (osm id ${SUPER_USER_OSM_ID}) to super user. ` + + 'Check that MR_SUPER_ACCOUNTS in docker-compose.test.yaml names that osm id.' + ) + } +} + +async function isSuperUser(user: { id: number; osmId: number; token: string }) { + const response = await fetch(`${BACKEND_URL}/api/v2/superusers`, { + headers: { Cookie: `PLAY_SESSION=${sessionCookie(user)}` }, + }) + if (!response.ok) return false + return ((await response.json()) as number[]).includes(user.id) +} + +async function waitForBackend() { + const deadline = Date.now() + BOOT_TIMEOUT_MS + while (Date.now() < deadline) { + try { + if ((await fetch(`${BACKEND_URL}/api/v2/service/info`)).ok) return + } catch { + // Not accepting connections yet. + } + await new Promise((resolve) => setTimeout(resolve, 2_000)) + } + throw new Error(`The backend did not come back within ${BOOT_TIMEOUT_MS / 1000}s of restarting`) +} diff --git a/e2e/profile-settings.spec.ts b/e2e/profile-settings.spec.ts index 936780c0a..4c8f83cc1 100644 --- a/e2e/profile-settings.spec.ts +++ b/e2e/profile-settings.spec.ts @@ -5,14 +5,7 @@ import { expect, test } from './fixtures' // API afterward (an earlier version of this test did both, via direct calls // to GET/PUT /api/v2/user in the spec body — a black-box-testing violation // per TESTING.md, since E2E tests must only observe the UI, not inspect the -// backend directly). The test harness authenticates as a synthetic backend -// user (id -999, via the MR_SUPER_KEY apiKey header) that isn't a real -// persisted DB row — every GET /whoami synthesizes a fresh default user from -// scratch, so a reload always reports defaults regardless of any PUT that -// came before, for any field, and a "restore" PUT is a genuine no-op that -// nothing ever reads back. That's an inherent limitation of this synthetic -// identity, not something a real logged-in user would hit. What IS genuinely -// provable here: submitting the form calls the real update mutation +// backend directly). What this does prove: submitting the form calls the real update mutation // (confirmed via the success toast, which only fires in the mutation's // resolved `.then`), and the query cache it seeds keeps the saved value // showing in the UI immediately afterward, without a reload. @@ -26,7 +19,7 @@ test('a user can update their custom basemap URL setting', async ({ page }) => { await basemapUrlField.fill(newBasemapUrl) - const submit = page.getByRole('button', { name: 'Submit' }) + const submit = page.getByRole('button', { name: 'Save Changes' }) await submit.click() await expect(page.getByText('User settings updated')).toBeVisible({ timeout: 15_000 }) diff --git a/e2e/project-team-ownership.spec.ts b/e2e/project-team-ownership.spec.ts index 4dbdb9926..2eebeaff2 100644 --- a/e2e/project-team-ownership.spec.ts +++ b/e2e/project-team-ownership.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' // Giving a project to a team from the project form, the same way a challenge // is handed over. Ownership is singular and is what puts the team's image on @@ -41,9 +41,7 @@ test('choosing a team on the form gives the project to that team', async ({ await expect(page).toHaveURL(`/manage/project/${project.id}`, { timeout: 15_000 }) // The server is the authority on whether ownership actually took. - const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`) const body = (await response.json()) as { ownerTeamId?: number; avatarUrl?: string } expect(body.ownerTeamId).toBe(team.id) }) @@ -59,9 +57,7 @@ test('a project owned by a team carries that team image url', async ({ await page.getByRole('button', { name: 'Update Project' }).click() await expect(page).toHaveURL(`/manage/project/${project.id}`, { timeout: 15_000 }) - const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`) const body = (await response.json()) as { avatarUrl?: string } // The url is addressed by owning team and answers 404 until that team has an @@ -80,9 +76,7 @@ test('a project can be handed back from a team', async ({ page, project, team, r await page.getByRole('button', { name: 'Update Project' }).click() await expect(page).toHaveURL(`/manage/project/${project.id}`, { timeout: 15_000 }) - const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`, { - headers: { apiKey: SUPER_KEY }, - }) + const response = await request.get(`${BACKEND_URL}/api/v2/project/${project.id}`) const body = (await response.json()) as { ownerTeamId?: number | null; avatarUrl?: string } expect(body.ownerTeamId ?? null).toBeNull() // No owning team means no picture at all, rather than a url that 404s. diff --git a/e2e/super-admin.spec.ts b/e2e/super-admin.spec.ts index 0de32e23d..127eeb9cc 100644 --- a/e2e/super-admin.spec.ts +++ b/e2e/super-admin.spec.ts @@ -1,4 +1,4 @@ -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' interface WhoamiUser { id: number @@ -7,35 +7,14 @@ interface WhoamiUser { } } -// Skipped: the test harness authenticates every request as a synthetic -// backend user (id -999) via the MR_SUPER_KEY apiKey header. That grants the -// backend API full access, and PUT /superuser/-999 does add it to the -// /superusers list — but src/lib/SuperAdminGuard.tsx (the frontend's route -// guard for /super-admin/*) checks for a `grants` entry with role === -1, -// and whoami's `grants` array for this synthetic user stays empty regardless -// (confirmed: no exposed API grants a global role to it). So the frontend -// itself renders "Access Denied" for any /super-admin/* route in this -// environment, independent of anything in this repo — there's currently no -// way to represent a real super-admin-granted user without OSM OAuth or -// direct database access, neither of which the E2E harness uses. -// -// Substitute coverage: src/lib/SuperAdminGuard.test.ts unit-tests -// `isSuperUser` directly (the guard's actual access-control predicate) across -// null/undefined users and grants with/without role === -1, which is the one -// piece of /super-admin/* logic this repo can exercise without a real -// super-admin-granted identity. It does not cover the routed page/table -// behavior below (that remains genuinely untested pending either OSM OAuth -// support or DB-level seeding in the E2E harness). -test.skip('the super-admin users page lists the real authenticated user from the backend', async ({ +test('the super-admin users page lists the real authenticated user from the backend', async ({ page, request, }) => { - // The whole browser session authenticates as this real backend user via the - // apiKey header (see e2e/fixtures.ts / .env.test), so it's a genuine row the - // super-admin users list must render from the database, not a fixture we set up. - const whoamiResponse = await request.get(`${BACKEND_URL}/api/v2/user/whoami`, { - headers: { apiKey: SUPER_KEY }, - }) + // The whole browser session runs as the harness's seeded super user (see + // e2e/global-setup.ts), so it's a genuine row the super-admin users list must + // render from the database. + const whoamiResponse = await request.get(`${BACKEND_URL}/api/v2/user/whoami`) expect(whoamiResponse.ok()).toBeTruthy() const currentUser = (await whoamiResponse.json()) as WhoamiUser diff --git a/e2e/task-feature-collection.spec.ts b/e2e/task-feature-collection.spec.ts index b41426d77..f06561f16 100644 --- a/e2e/task-feature-collection.spec.ts +++ b/e2e/task-feature-collection.spec.ts @@ -1,5 +1,5 @@ import type { APIRequestContext } from '@playwright/test' -import { BACKEND_URL, expect, SUPER_KEY, test } from './fixtures' +import { BACKEND_URL, expect, test } from './fixtures' // A task built from a GeoJSON FeatureCollection holds several features at // once. All of them belong to the one task: they are drawn together, listed @@ -22,7 +22,7 @@ const createTaskWithGeometries = async ( features: unknown[] ): Promise => { const response = await request.post(`${BACKEND_URL}/api/v2/task`, { - headers: { apiKey: SUPER_KEY, 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json' }, data: { name: `fc-task-${Date.now()}-${Math.round(Math.random() * 1e6)}`, parent: challengeId, diff --git a/e2e/task-locking.spec.ts b/e2e/task-locking.spec.ts index c47923206..f756f45b3 100644 --- a/e2e/task-locking.spec.ts +++ b/e2e/task-locking.spec.ts @@ -1,31 +1,25 @@ import { expect, test } from './fixtures' -// Requires E2E_REVIEWER_API_KEY (a second, real backend identity) to exercise -// the two-user lock-conflict path — see the REVIEWER_KEY comment in -// e2e/fixtures.ts for how to provision one. Skipped rather than failing when -// it's not configured, matching the pattern in e2e/super-admin.spec.ts. -if (process.env.E2E_REVIEWER_API_KEY) { - test('a second user is told a task is locked when they open it after the first user', async ({ - page, - reviewerPage, - task, - }) => { - test.setTimeout(30_000) +test('a second user is told a task is locked when they open it after the first user', async ({ + page, + reviewerPage, + task, +}) => { + test.setTimeout(30_000) - // The primary user opens the task and auto-locks it for mapping. - await page.goto(`/tasks/${task.id}`) - await expect(page.getByRole('button', { name: 'Fixed', exact: true })).toBeVisible({ - timeout: 20_000, - }) - - // A second, distinct real user opens the same task while it's still locked. - await reviewerPage.goto(`/tasks/${task.id}`) - await expect( - reviewerPage.getByText( - 'This task is currently locked by another mapper. Try again later or pick a different task.' - ) - ).toBeVisible({ timeout: 20_000 }) + // The primary user opens the task and locks it for mapping. + await page.goto(`/tasks/${task.id}`) + await page.getByRole('button', { name: 'Map this task' }).click() + await expect(page.getByRole('button', { name: 'Fixed', exact: true })).toBeVisible({ + timeout: 20_000, }) -} else { - test.skip('a second user is told a task is locked when they open it after the first user', async () => {}) -} + + // A second, distinct real user opens the same task while it's still locked. + await reviewerPage.goto(`/tasks/${task.id}`) + await reviewerPage.getByRole('button', { name: 'Map this task' }).click() + await expect( + reviewerPage.getByText( + 'This task is currently locked by another mapper. Try again later or pick a different task.' + ) + ).toBeVisible({ timeout: 20_000 }) +}) diff --git a/playwright.config.ts b/playwright.config.ts index d30041a8c..58c845ad9 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -5,6 +5,7 @@ const FRONTEND_URL = 'http://localhost:3005' export default defineConfig({ testDir: './e2e', + globalSetup: './e2e/global-setup.ts', fullyParallel: false, forbidOnly: !!process.env.CI, retries: process.env.CI ? 1 : 0, From b2947ed2a03d369b4492cc278cd6de70b40ef8b0 Mon Sep 17 00:00:00 2001 From: Collin Beczak Date: Wed, 21 Oct 2026 11:03:35 -0500 Subject: [PATCH 2/2] fix unit test --- src/api/index.test.ts | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/api/index.test.ts b/src/api/index.test.ts index ca1507147..c6482e896 100644 --- a/src/api/index.test.ts +++ b/src/api/index.test.ts @@ -111,12 +111,23 @@ describe('apiRequest beforeRequest hook', () => { }) it('sets the apiKey header when an API key is configured', async () => { - const fetchMock = stubFetch(new Response(JSON.stringify({}), { status: 200 })) + const mutableEnv = window.env as MutableEnv + const originalKey = mutableEnv.VITE_SERVER_API_KEY + mutableEnv.VITE_SERVER_API_KEY = 'configured-key' + vi.resetModules() - await apiRequest.get('some/path').json() + try { + const { apiRequest: freshApiRequest } = await import('./index') + const fetchMock = stubFetch(new Response(JSON.stringify({}), { status: 200 })) - const [request] = fetchMock.mock.calls[0] - expect(request.headers.get('apikey')).toBe(apiKey) + await freshApiRequest.get('some/path').json() + + const [request] = fetchMock.mock.calls[0] + expect(request.headers.get('apikey')).toBe('configured-key') + } finally { + mutableEnv.VITE_SERVER_API_KEY = originalKey + vi.resetModules() + } }) it('does not overwrite a content-type header already set on the request', async () => {