test(e2e): address Alex's review feedback on the PKCE and desk-clash specs

apps/workplace/e2e/local/desk-clash.spec.ts

@@ -10,18 +10,24 @@
  * because a clash check that only looked at your own bookings would still pass a
  * single-user version of this test.
  */
-import { request } from '@playwright/test';
-import { test, expect } from '../../../../e2e/support/fixtures';
-import { APP_URL, BACKEND_URL, WORKERS, deskFor, roleFor } from '../../../../e2e/support/env';
-import { mintToken } from '../../../../e2e/support/auth';
+import type { APIRequestContext } from '@playwright/test';
+import { request, type APIResponse } from '@playwright/test';
 import {
     STAFF_API,
     deleteBooking,
     releaseAsset,
     uniqueTitle,
     zonesWithTag,
 } from '../../../../e2e/support/api';
-import type { APIRequestContext } from '@playwright/test';
+import { mintToken } from '../../../../e2e/support/auth';
+import {
+    APP_URL,
+    BACKEND_URL,
+    WORKERS,
+    deskFor,
+    roleFor,
+} from '../../../../e2e/support/env';
+import { expect, test } from '../../../../e2e/support/fixtures';
 
 const DAY = 86_400;
 const from = () => Math.floor(Date.now() / 1000) - 3 * DAY;
@@ -64,7 +70,10 @@ test.describe('desk double-booking', () => {
     }, testInfo) => {
         const mine = testInfo.parallelIndex;
         const theirs = (mine + 1) % WORKERS;
-        test.skip(theirs === mine, 'needs at least two workers to have two distinct users');
+        test.skip(
+            theirs === mine,
+            'needs at least two workers to have two distinct users',
+        );
 
         const desk = deskFor(mine);
         const zones = await bookingZones(staffApi);
@@ -92,41 +101,73 @@ test.describe('desk double-booking', () => {
         const other = await request.newContext({
             baseURL: BACKEND_URL,
             ignoreHTTPSErrors: true,
-            extraHTTPHeaders: { Authorization: `Bearer ${other_mint.accessToken}` },
+            extraHTTPHeaders: {
+                Authorization: `Bearer ${other_mint.accessToken}`,
+            },
         });
 
+        // Anything the second user unexpectedly succeeds in creating has to be
+        // cleaned up by *them*: `GET /bookings` is caller-scoped, so the owner's
+        // `releaseAsset` in the next run cannot see it, and the desk stays held.
+        const leaked: number[] = [];
+        const capture = async (res: APIResponse) => {
+            if (res.ok()) leaked.push((await res.json()).id);
+            return res;
+        };
+
         try {
             // Exactly the same slot.
-            const exact = await book(other, desk.id, start, end, zones);
+            //
+            // 409 specifically, not "any error". A 5xx from an unhealthy backend
+            // would satisfy `>= 400` while proving nothing about clash detection —
+            // and booking POSTs have a known way of returning 500 under load
+            // (REG-09), which is exactly the failure this spec must not absorb.
+            const exact = await capture(
+                await book(other, desk.id, start, end, zones),
+            );
             expect(
                 exact.status(),
-                `an identical slot must be refused, got ${exact.status()}`,
-            ).toBeGreaterThanOrEqual(400);
+                `an identical slot must be refused with 409, got ${exact.status()}`,
+            ).toBe(409);
 
             // And a partial overlap, which is the case a naive check misses: it
             // starts before the existing booking ends.
-            const partial = await book(other, desk.id, start + 1800, end + 1800, zones);
+            const partial = await capture(
+                await book(other, desk.id, start + 1800, end + 1800, zones),
+            );
             expect(
                 partial.status(),
-                `an overlapping slot must be refused, got ${partial.status()}`,
-            ).toBeGreaterThanOrEqual(400);
+                `an overlapping slot must be refused with 409, got ${partial.status()}`,
+            ).toBe(409);
 
             // Control: a slot that genuinely doesn't overlap is fine. Without this,
             // a backend that rejected everything would pass the two checks above.
-            const clear = await book(other, desk.id, end + 3600, end + 7200, zones);
+            const clear = await book(
+                other,
+                desk.id,
+                end + 3600,
+                end + 7200,
+                zones,
+            );
             expect(
                 clear.ok(),
                 `a non-overlapping slot should be accepted: ${clear.status()} ${await clear.text()}`,
             ).toBeTruthy();
             const clear_booking = await clear.json();
-            await deleteBooking(other, clear_booking.id);
+            leaked.push(clear_booking.id);
         } finally {
+            // Delete as `other`, before the context is disposed — the owner
+            // cannot see these.
+            for (const id of leaked)
+                await deleteBooking(other, id).catch(() => undefined);
             await other.dispose();
             await deleteBooking(staffApi, booking.id);
         }
     });
 
-    test('the desk frees up once the booking is deleted', async ({ staffApi }, testInfo) => {
+    test('the desk frees up once the booking is deleted', async ({
+        staffApi,
+    }, testInfo) => {
         // Guards a nastier version of the same bug: a cancelled booking that still
         // blocks the desk. Users would see it as free and be unable to book it,
         // which is harder to diagnose than a straightforward double-booking.
@@ -142,7 +183,10 @@ test.describe('desk double-booking', () => {
         const booking = await first.json();
 
         const blocked = await book(staffApi, desk.id, start, end, zones);
-        expect(blocked.status(), 'the slot is taken while the booking exists').toBeGreaterThanOrEqual(400);
+        expect(
+            blocked.status(),
+            'the slot is taken while the booking exists',
+        ).toBeGreaterThanOrEqual(400);
 
         await deleteBooking(staffApi, booking.id);
 

apps/workplace/e2e/local/pkce.spec.ts

@@ -12,39 +12,60 @@
  * written during the auth.cr migration. They belong with the suite rather than in
  * a task folder, where nothing runs them.
  */
-import { test, expect } from '../../../../e2e/support/fixtures';
-import { loginViaUI } from '../../../../e2e/support/login';
-import { APP_URL, roleFor } from '../../../../e2e/support/env';
+import { createHash } from 'node:crypto';
+
 import { clientId, redirectUriFor } from '../../../../e2e/support/auth';
+import { APP_URL, roleFor } from '../../../../e2e/support/env';
+import { expect, test } from '../../../../e2e/support/fixtures';
+import { loginViaUI } from '../../../../e2e/support/login';
 
 // The subject is the handshake itself, so start with no credentials.
 test.use({ storageState: undefined });
 
 test.describe('PKCE handshake', () => {
-    test('the browser performs a public-client PKCE exchange with no secret', async ({ page }) => {
+    test('the browser performs a public-client PKCE exchange with no secret', async ({
+        page,
+    }) => {
         const { requests, token } = await loginViaUI(page, roleFor('admin'));
 
-        const authorize = requests.find((r) => r.url.includes('/oauth/authorize'));
+        const authorize = requests.find((r) =>
+            r.url.includes('/oauth/authorize'),
+        );
         const exchange = requests.find(
-            (r) => r.url.includes('/oauth/token') && !r.url.includes('refresh_token'),
+            (r) =>
+                r.url.includes('/oauth/token') &&
+                !r.url.includes('refresh_token'),
         );
 
-        expect(authorize, 'an /oauth/authorize request should have been made').toBeTruthy();
-        expect(exchange, 'an /oauth/token request should have been made').toBeTruthy();
+        expect(
+            authorize,
+            'an /oauth/authorize request should have been made',
+        ).toBeTruthy();
+        expect(
+            exchange,
+            'an /oauth/token request should have been made',
+        ).toBeTruthy();
 
         // --- the authorize leg -------------------------------------------------
         // 302 is the redirect back to the app carrying ?code=. A 200 here would
         // mean we were served a page instead, i.e. the handshake never completed.
-        expect(authorize!.status, 'authorize redirects back with a code').toBe(302);
+        expect(authorize!.status, 'authorize redirects back with a code').toBe(
+            302,
+        );
 
         expect(authorize!.url, 'PKCE must use S256, never "plain"').toContain(
             'code_challenge_method=S256',
         );
 
-        const challenge = new URL(authorize!.url).searchParams.get('code_challenge') ?? '';
-        expect(challenge.length, 'a real code_challenge was sent').toBeGreaterThan(20);
-        // base64url, so it must not contain the standard-base64-only characters.
-        expect(challenge, 'the challenge is base64url encoded').not.toMatch(/[+/=]/);
+        const challenge =
+            new URL(authorize!.url).searchParams.get('code_challenge') ?? '';
+        // SHA-256 is 32 bytes, which is exactly 43 characters of unpadded
+        // base64url. Anything else is not an S256 challenge, whatever the
+        // `code_challenge_method` parameter claims.
+        expect(
+            challenge,
+            'an S256 challenge is 43 base64url characters',
+        ).toMatch(/^[A-Za-z0-9\-_]{43}$/);
 
         // --- the token leg -----------------------------------------------------
         expect(exchange!.status, 'token exchange succeeds').toBe(200);
@@ -53,39 +74,68 @@ test.describe('PKCE handshake', () => {
         // must never send one. If this ever fails, a credential is sitting in a
         // shipped bundle where anyone can read it.
         const wire = `${exchange!.url} ${exchange!.postData ?? ''}`;
-        expect(wire, 'no client_secret anywhere in the token request').not.toContain(
-            'client_secret',
-        );
+        expect(
+            wire,
+            'no client_secret anywhere in the token request',
+        ).not.toContain('client_secret');
 
         // The verifier is what proves this client started the flow. Without it,
         // an intercepted code could be redeemed by anyone.
-        expect(wire, 'the code_verifier was sent').toContain('code_verifier');
+        //
+        // Asserting the two values are *related* is the whole point. Checking
+        // only that a challenge and a verifier were both present would pass even
+        // if they had nothing to do with each other — which is precisely the
+        // state a broken client, or a backend that stopped enforcing PKCE, would
+        // leave things in.
+        // ts-client puts the exchange parameters in the query string, but has
+        // also form-encoded them in the body across versions, so read either.
+        const body = exchange!.postData ?? '';
+        const verifier =
+            new URL(exchange!.url).searchParams.get('code_verifier') ||
+            new URLSearchParams(body).get('code_verifier') ||
+            '';
+        expect(verifier, 'a code_verifier was sent').not.toBe('');
+        const derived = createHash('sha256')
+            .update(verifier)
+            .digest('base64url');
+        expect(derived, 'the challenge is SHA-256(verifier), base64url').toBe(
+            challenge,
+        );
 
         // And it is the client we registered, not something else.
         expect(wire, 'the expected client_id was used').toContain(
             clientId(redirectUriFor(APP_URL)),
         );
 
-        expect(token.access_token, 'the exchange produced a token').toBeTruthy();
+        expect(
+            token.access_token,
+            'the exchange produced a token',
+        ).toBeTruthy();
     });
 
-    test('no credentials are exposed in any /auth request', async ({ page }) => {
+    test('no credentials are exposed in any /auth request', async ({
+        page,
+    }) => {
         const { requests } = await loginViaUI(page, roleFor('admin'));
         const password = roleFor('admin').password;
 
         // The password legitimately appears once, in the sign-in POST body. It must
         // not turn up anywhere else, and above all never in a URL, where it would be
         // captured by browser history, proxies and server logs.
         for (const r of requests) {
-            expect(r.url, `password must never appear in a URL (${r.method} ${r.url})`).not.toContain(
-                password,
-            );
-            expect(r.url, `no client_secret in a URL (${r.method} ${r.url})`).not.toContain(
-                'client_secret',
-            );
+            expect(
+                r.url,
+                `password must never appear in a URL (${r.method} ${r.url})`,
+            ).not.toContain(password);
+            expect(
+                r.url,
+                `no client_secret in a URL (${r.method} ${r.url})`,
+            ).not.toContain('client_secret');
         }
 
-        const bodies_with_password = requests.filter((r) => (r.postData ?? '').includes(password));
+        const bodies_with_password = requests.filter((r) =>
+            (r.postData ?? '').includes(password),
+        );
         expect(
             bodies_with_password.every((r) => r.url.includes('/auth/signin')),
             'the password should only ever be sent to /auth/signin',

apps/workplace/src/app/book/desk-flow/desk-flow-success.component.ts

@@ -3,6 +3,7 @@ import {
     Component,
     computed,
     inject,
+    OnDestroy,
     OnInit,
     signal,
 } from '@angular/core';
@@ -277,7 +278,7 @@ interface GroupBookingListItem {
         UserAvatarComponent,
     ],
 })
-export class NewDeskFlowSuccessComponent implements OnInit {
+export class NewDeskFlowSuccessComponent implements OnInit, OnDestroy {
     private _org = inject(OrganisationService);
     private _state = inject(BookingFormService);
     private _settings = inject(SettingsService);
@@ -369,6 +370,8 @@ export class NewDeskFlowSuccessComponent implements OnInit {
         return this._settings.time_format;
     }
 
+    private _group_bookings_timer?: ReturnType<typeof setTimeout>;
+
     public async ngOnInit() {
         await this._org.waitUntilInitialised();
         this.last_event.set(this._state.last_success);
@@ -384,11 +387,18 @@ export class NewDeskFlowSuccessComponent implements OnInit {
         this.building.set(this._building_pipe.transform(event.zones));
 
         // Load group bookings if this is a group booking
-        setTimeout(async () => {
+        this._group_bookings_timer = setTimeout(async () => {
             if (this.is_group()) await this._loadGroupBookings();
         }, 100);
     }
 
+    public ngOnDestroy() {
+        // The callback reads localStorage and writes to this component's
+        // signals, so leaving it pending past destruction does work on a
+        // component nobody is looking at any more.
+        clearTimeout(this._group_bookings_timer);
+    }
+
     private async _loadGroupBookings() {
         const stored_ids = localStorage.getItem(
             'PLACEOS.last_group_booking_ids',