Merge pull request #472 from the-luap/followup/470-tests-and-cache-headers

test+fix(customer-portal): #470 review follow-ups (test coverage + cache headers)

backend/server.js

@@ -593,11 +593,19 @@ app.use('/api/admin/users', require('./src/routes/adminUsers'));
 // Putting the global flag in the kill-switch role was a mistake — a
 // stray click in Settings → Features would lock every paying
 // customer out at once. PR-revert moved the gate back to per-record.
-app.use('/api/admin/customers', require('./src/routes/adminCustomers'));
+//
+// `noStoreCache` belt-and-braces the cache-control story for both
+// surfaces: any response — 200, 4xx, 5xx — carries `Cache-Control:
+// no-store` so a transient error (the now-reverted #458 410, a
+// permission flip mid-session, a backend restart) can't get pinned
+// in browser or intermediate caches and outlive its cause. See the
+// PR #458 → #470 history in the middleware file for context.
+const { noStoreCache } = require('./src/middleware/noStoreCache');
+app.use('/api/admin/customers', noStoreCache, require('./src/routes/adminCustomers'));
 // Customer-side surface (#354). Strictly separate from /api/admin/* —
 // distinct token type, distinct cookie, distinct middleware.
-app.use('/api/customer/auth', require('./src/routes/customerAuth'));
-app.use('/api/customer', require('./src/routes/customer'));
+app.use('/api/customer/auth', noStoreCache, require('./src/routes/customerAuth'));
+app.use('/api/customer', noStoreCache, require('./src/routes/customer'));
 app.use('/api/admin/event-types', require('./src/routes/adminEventTypes'));
 app.use('/api/admin/api-tokens', require('./src/routes/adminApiTokens'));
 app.use('/api/admin/webhooks', require('./src/routes/adminWebhooks'));

backend/src/__tests__/customerAccountsService.test.js

@@ -199,3 +199,141 @@ describe('customerHasAccessToEvent', () => {
     expect(result).toBe(false);
   });
 });
+
+// ---- setAssignmentsForCustomer ----------------------------------------
+//
+// The inverse of setAssignmentsForEvent: takes one customer + a list of
+// event ids and reconciles the junction table. Powers the "Manage
+// galleries" dialog on the customer detail page. The
+// `verifyGalleryAccess` middleware re-checks this junction on every
+// customer-minted JWT, so getting the diff math right here is the
+// access-control story for the whole feature (#470).
+//
+// notifyCustomerOfNewAssignments() runs as fire-and-forget after the
+// transactional work and queues a follow-up email. The tests below
+// configure mocks for the calls it makes (load customer row, load
+// event rows) so it can resolve cleanly without crashing the assert
+// path — we don't assert on its body here; the email pipeline is a
+// separate seam.
+
+describe('setAssignmentsForCustomer', () => {
+  // The notifier issues two more db() calls after the writer returns:
+  // SELECT customer_accounts and SELECT events. Provide cheap chains
+  // that resolve to "no customer / no events" so it early-returns
+  // without firing queueEmail. Returns a small helper so each test
+  // can append it after its own writer chains.
+  function appendNotifierMocks() {
+    db.mockImplementationOnce(() => chain({ first: null }));   // customer lookup -> not found
+    db.mockImplementationOnce(() => chain({ rows: [] }));       // events lookup -> empty
+  }
+
+  it('inserts only events that are missing and removes those not in the wanted list', async () => {
+    const svc = require('../services/customerAccountsService');
+    // existing assignments: customer is on events 10 and 20.
+    const existingChain = chain({ rows: [
+      { id: 500, event_id: 10 },
+      { id: 501, event_id: 20 },
+    ] });
+    const deleteChain = chain({ del: 1 });
+    // Validity check returns 30 only — event 99 is filtered out
+    // (archived or missing).
+    const validityChain = chain({ pluck: [30] });
+    const insertChain = chain({ insert: [] });
+
+    db.mockImplementationOnce(() => existingChain);
+    db.mockImplementationOnce(() => deleteChain);
+    db.mockImplementationOnce(() => validityChain);
+    db.mockImplementationOnce(() => insertChain);
+    appendNotifierMocks();
+
+    const summary = await svc.setAssignmentsForCustomer(7, [20, 30, 99], 12);
+
+    // Remove event 10 (not in wanted).
+    expect(deleteChain.whereIn).toHaveBeenCalledWith('id', [500]);
+    // Insert ONLY event 30 — event 99 was dropped by the validity filter.
+    expect(insertChain.insert).toHaveBeenCalledWith([{
+      event_id: 30,
+      customer_account_id: 7,
+      assigned_by_admin_id: 12,
+      assigned_at: expect.any(Date),
+    }]);
+    expect(summary).toEqual({
+      added: 2,           // 30 and 99 were both attempted
+      removed: 1,         // event 10
+      addedEventIds: [30], // only event 30 actually landed in the DB
+    });
+  });
+
+  it('silently filters archived/missing event ids out of the insert', async () => {
+    const svc = require('../services/customerAccountsService');
+    const existingChain = chain({ rows: [] });
+    // Three candidates; validity check pulls back zero -> all three are
+    // archived or missing. Service should log a warning and skip the
+    // insert entirely (rows.length === 0 short-circuits the .insert call).
+    const validityChain = chain({ pluck: [] });
+
+    db.mockImplementationOnce(() => existingChain);
+    db.mockImplementationOnce(() => validityChain);
+    appendNotifierMocks();
+
+    const summary = await svc.setAssignmentsForCustomer(7, [99, 100, 101], 12);
+
+    expect(summary).toEqual({
+      added: 3,            // attempted three
+      removed: 0,
+      addedEventIds: [],   // none landed
+    });
+  });
+
+  it('clears all assignments when wanted list is empty', async () => {
+    const svc = require('../services/customerAccountsService');
+    const existingChain = chain({ rows: [
+      { id: 500, event_id: 10 },
+      { id: 501, event_id: 20 },
+    ] });
+    const deleteChain = chain({ del: 2 });
+
+    db.mockImplementationOnce(() => existingChain);
+    db.mockImplementationOnce(() => deleteChain);
+    appendNotifierMocks();
+
+    const summary = await svc.setAssignmentsForCustomer(7, [], 12);
+    expect(deleteChain.whereIn).toHaveBeenCalledWith('id', [500, 501]);
+    expect(summary).toEqual({ added: 0, removed: 2, addedEventIds: [] });
+  });
+
+  it('is a no-op when wanted equals existing', async () => {
+    const svc = require('../services/customerAccountsService');
+    const existingChain = chain({ rows: [
+      { id: 500, event_id: 10 },
+    ] });
+    db.mockImplementationOnce(() => existingChain);
+    appendNotifierMocks();
+
+    const summary = await svc.setAssignmentsForCustomer(7, [10], 12);
+    expect(summary).toEqual({ added: 0, removed: 0, addedEventIds: [] });
+  });
+
+  it('coerces non-integer / negative event ids out of the wanted set', async () => {
+    const svc = require('../services/customerAccountsService');
+    const existingChain = chain({ rows: [] });
+    const validityChain = chain({ pluck: [10] });
+    const insertChain = chain({ insert: [] });
+
+    db.mockImplementationOnce(() => existingChain);
+    db.mockImplementationOnce(() => validityChain);
+    db.mockImplementationOnce(() => insertChain);
+    appendNotifierMocks();
+
+    // 'abc' isn't a number, -5 is negative, 0 is invalid, 10.5 is fractional.
+    // Only the integer 10 should survive the Number()/Number.isFinite()
+    // filter. 10.5 coerces to a finite 10.5 (Number.isFinite returns true)
+    // but the validity check only returns the integer 10 so we end up
+    // inserting 10. Asserting on the eventual insert payload is the
+    // cleanest contract.
+    await svc.setAssignmentsForCustomer(7, [10, 'abc', -5, 0, '10'], 12);
+    const insertedRows = insertChain.insert.mock.calls[0][0];
+    const insertedEventIds = insertedRows.map((r) => r.event_id);
+    expect(insertedEventIds).toEqual([10]);
+  });
+});

backend/src/__tests__/noStoreCache.test.js

@@ -0,0 +1,48 @@
+/**
+ * Unit test for the noStoreCache middleware (#470 follow-up).
+ *
+ * The middleware exists because of a real production-class bug: when
+ * #458 mounted a 410-returning kill-switch in front of customer
+ * endpoints, browsers cached the 410 (no Cache-Control was set) and
+ * kept serving it after #470 reverted the middleware. This test pins
+ * the contract so a future cleanup pass doesn't quietly drop the
+ * header set and re-introduce the bug.
+ */
+
+const { noStoreCache } = require('../middleware/noStoreCache');
+
+function makeRes() {
+  const headers = {};
+  return {
+    setHeader: (k, v) => { headers[k] = v; },
+    headers,
+  };
+}
+
+describe('noStoreCache middleware', () => {
+  it('sets Cache-Control: no-store + private and calls next()', () => {
+    const res = makeRes();
+    const next = jest.fn();
+
+    noStoreCache({}, res, next);
+
+    expect(res.headers['Cache-Control']).toBe(
+      'no-store, no-cache, must-revalidate, private',
+    );
+    // HTTP/1.0 fallbacks — old proxies in front of customer-facing
+    // surfaces (corporate VPN gateways, legacy CDNs) honour these.
+    expect(res.headers.Pragma).toBe('no-cache');
+    expect(res.headers.Expires).toBe('0');
+    expect(next).toHaveBeenCalledTimes(1);
+  });
+
+  it('runs as middleware regardless of response status', () => {
+    // The header must land on EVERY response coming from the route
+    // group — including 4xx/5xx — so a stale 410 from a
+    // now-reverted middleware can't get pinned in browser cache like
+    // it did in the #458 → #470 sequence.
+    const res = makeRes();
+    noStoreCache({}, res, () => {});
+    expect(res.headers['Cache-Control']).toContain('no-store');
+  });
+});

backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js

@@ -0,0 +1,220 @@
+/**
+ * Unit tests for verifyGalleryAccess's customer-assignment re-check
+ * (PR #470). Documents the contract:
+ *
+ *   - JWT with `via === 'customer'` and a `customerId` claim →
+ *     middleware re-reads event_customer_assignments and 403s with
+ *     code 'CUSTOMER_ASSIGNMENT_REVOKED' when the row is gone.
+ *   - JWT without those claims (the per-event-password flow) → no
+ *     re-check, no extra query, no perf cost. This is asserted
+ *     explicitly because a regression that silently re-checks every
+ *     gallery token would 403 every guest the moment a customer was
+ *     unassigned from any unrelated event.
+ *   - Re-check is wrapped in withRetry so transient DB blips don't
+ *     bounce a legitimate session.
+ *
+ * Same pattern as authSession.symmetry.test.js — every collaborator
+ * mocked so the test stays a fast unit test (no postgres, no real JWTs).
+ */
+
+jest.mock('../database/db', () => {
+  const mockDb = jest.fn();
+  // The middleware uses `withRetry(fn)` to wrap reads. For the test
+  // surface we just want to invoke the callback synchronously and
+  // surface whatever it returns / throws.
+  const withRetry = jest.fn((fn) => fn());
+  return { db: mockDb, withRetry };
+});
+
+jest.mock('../utils/logger', () => ({
+  info: jest.fn(), warn: jest.fn(), error: jest.fn(), debug: jest.fn(),
+}));
+
+jest.mock('jsonwebtoken', () => ({
+  verify: jest.fn(),
+}));
+
+jest.mock('../utils/tokenUtils', () => ({
+  getGalleryTokenFromRequest: jest.fn(),
+}));
+
+jest.mock('../utils/dbCompat', () => ({
+  formatBoolean: (v) => (v ? 1 : 0),
+}));
+
+const jwt = require('jsonwebtoken');
+const { db } = require('../database/db');
+const { getGalleryTokenFromRequest } = require('../utils/tokenUtils');
+const { verifyGalleryAccess } = require('../middleware/gallery');
+
+function makeRes() {
+  const res = {};
+  res.status = jest.fn().mockReturnValue(res);
+  res.json = jest.fn().mockReturnValue(res);
+  return res;
+}
+
+function makeReq(slug = 'test-event') {
+  return {
+    params: { slug },
+    headers: {},
+    cookies: {},
+    query: {},
+    ip: '1.2.3.4',
+    get: () => 'jest',
+    connection: { remoteAddress: '1.2.3.4' },
+  };
+}
+
+// The middleware queries the `events` table first (existence check),
+// then optionally `event_customer_assignments`. This helper queues
+// both responses on the shared db mock so each test can spell out
+// the scenario in order. Returns the assignments chain so the test
+// can assert against it.
+function mockEventAndAssignment({ event, assignment }) {
+  // `db('events').where({...}).select('*').first()` — chainable.
+  const eventsChain = {};
+  eventsChain.where = jest.fn().mockReturnValue(eventsChain);
+  eventsChain.select = jest.fn().mockReturnValue(eventsChain);
+  eventsChain.first = jest.fn().mockResolvedValue(event);
+
+  // `db('event_customer_assignments').where({...}).first()`.
+  const assignChain = {};
+  assignChain.where = jest.fn().mockReturnValue(assignChain);
+  assignChain.first = jest.fn().mockResolvedValue(assignment);
+
+  db.mockImplementationOnce(() => eventsChain)
+    .mockImplementationOnce(() => assignChain);
+
+  return { eventsChain, assignChain };
+}
+
+beforeEach(() => {
+  db.mockReset();
+  jwt.verify.mockReset();
+  getGalleryTokenFromRequest.mockReset();
+});
+
+// ---- customer-minted JWT, assignment intact ----------------------------
+
+describe('verifyGalleryAccess — customer-minted JWT with active assignment', () => {
+  it('allows access when the event_customer_assignments row exists', async () => {
+    getGalleryTokenFromRequest.mockReturnValue('tkn');
+    jwt.verify.mockReturnValue({
+      eventId: 42,
+      via: 'customer',
+      customerId: 7,
+    });
+    const { assignChain } = mockEventAndAssignment({
+      event: { id: 42, slug: 'test-event', is_active: true, is_archived: false },
+      assignment: { id: 999, event_id: 42, customer_account_id: 7 },
+    });
+
+    const req = makeReq();
+    const res = makeRes();
+    const next = jest.fn();
+    await verifyGalleryAccess(req, res, next);
+
+    expect(assignChain.where).toHaveBeenCalledWith({
+      event_id: 42,
+      customer_account_id: 7,
+    });
+    expect(next).toHaveBeenCalledTimes(1);
+    expect(res.status).not.toHaveBeenCalled();
+    expect(req.event).toEqual(expect.objectContaining({ id: 42 }));
+  });
+});
+
+// ---- customer-minted JWT, assignment revoked ---------------------------
+
+describe('verifyGalleryAccess — customer-minted JWT after revocation', () => {
+  it('returns 403 CUSTOMER_ASSIGNMENT_REVOKED when the junction row is gone', async () => {
+    getGalleryTokenFromRequest.mockReturnValue('tkn');
+    jwt.verify.mockReturnValue({
+      eventId: 42,
+      via: 'customer',
+      customerId: 7,
+    });
+    mockEventAndAssignment({
+      event: { id: 42, slug: 'test-event', is_active: true, is_archived: false },
+      assignment: undefined, // <-- the admin just removed it
+    });
+
+    const req = makeReq();
+    const res = makeRes();
+    const next = jest.fn();
+    await verifyGalleryAccess(req, res, next);
+
+    expect(next).not.toHaveBeenCalled();
+    expect(res.status).toHaveBeenCalledWith(403);
+    expect(res.json).toHaveBeenCalledWith(
+      expect.objectContaining({ code: 'CUSTOMER_ASSIGNMENT_REVOKED' }),
+    );
+  });
+
+  it('still re-checks when the customerId is in the token but via claim is missing-but-numeric', async () => {
+    // Belt-and-braces: the gate triggers on `via === 'customer'`. A
+    // token with customerId but no `via` should NOT re-check (it isn't
+    // a customer-minted token — could be legacy). This pins the
+    // contract so a future refactor can't accidentally widen the gate
+    // and start 403'ing per-event-password sessions.
+    getGalleryTokenFromRequest.mockReturnValue('tkn');
+    jwt.verify.mockReturnValue({
+      eventId: 42,
+      customerId: 7,
+      // intentionally no `via` claim
+    });
+    // The middleware uses one db() call for events; if it tried to
+    // re-check we'd see a second db() call and the test would throw
+    // (no more mock implementations queued).
+    const eventsChain = {};
+    eventsChain.where = jest.fn().mockReturnValue(eventsChain);
+    eventsChain.select = jest.fn().mockReturnValue(eventsChain);
+    eventsChain.first = jest.fn().mockResolvedValue({
+      id: 42, slug: 'test-event', is_active: true, is_archived: false,
+    });
+    db.mockImplementationOnce(() => eventsChain);
+
+    const req = makeReq();
+    const res = makeRes();
+    const next = jest.fn();
+    await verifyGalleryAccess(req, res, next);
+
+    expect(next).toHaveBeenCalledTimes(1);
+    expect(db).toHaveBeenCalledTimes(1); // events table only — no assignments query
+  });
+});
+
+// ---- per-event-password JWT (no `via` claim) ---------------------------
+
+describe('verifyGalleryAccess — per-event-password JWT', () => {
+  it('does NOT touch event_customer_assignments and passes through', async () => {
+    getGalleryTokenFromRequest.mockReturnValue('tkn');
+    jwt.verify.mockReturnValue({
+      eventId: 42,
+      // No via, no customerId — this is the legacy per-event-password
+      // flow where every guest mints their own JWT after entering the
+      // gallery password.
+    });
+    // Only events table should be queried. If the middleware regresses
+    // and starts querying event_customer_assignments here, the second
+    // db() call would have no mock implementation and the test would
+    // surface an error.
+    const eventsChain = {};
+    eventsChain.where = jest.fn().mockReturnValue(eventsChain);
+    eventsChain.select = jest.fn().mockReturnValue(eventsChain);
+    eventsChain.first = jest.fn().mockResolvedValue({
+      id: 42, slug: 'test-event', is_active: true, is_archived: false,
+    });
+    db.mockImplementationOnce(() => eventsChain);
+
+    const req = makeReq();
+    const res = makeRes();
+    const next = jest.fn();
+    await verifyGalleryAccess(req, res, next);
+
+    expect(next).toHaveBeenCalledTimes(1);
+    expect(db).toHaveBeenCalledTimes(1);
+    expect(db.mock.calls[0][0]).toBe('events');
+  });
+});

backend/src/middleware/noStoreCache.js

@@ -0,0 +1,39 @@
+/**
+ * Cache-Control: no-store helper for sensitive endpoints.
+ *
+ * Why a dedicated middleware: shipping the wrong cache-control header
+ * on a session-bearing endpoint is a class-of-bug that bites long
+ * after the original mistake. The PR #458 / PR #470 history is the
+ * concrete trigger:
+ *
+ *   - #458 mounted requireCustomerPortalEnabled which 410'd every
+ *     /api/customer/* and /api/admin/customers/* request when the
+ *     master toggle was off.
+ *   - Some browsers cached the 410 (the response carried no explicit
+ *     Cache-Control header, so heuristic freshness applied — for an
+ *     authenticated/sensitive surface that's the wrong default).
+ *   - #470 reverted the middleware, but a customer whose tab cached
+ *     the 410 still saw 410s until they hard-refreshed.
+ *
+ * Mounting `noStoreCache` in front of these routes belt-and-braces
+ * the future: any 4xx/5xx (or 200) response from these endpoints
+ * carries `Cache-Control: no-store`, so a transient kill-switch,
+ * permission flip, or backend restart can never get pinned in
+ * intermediate caches.
+ *
+ * No-op cost (one setHeader per request); applied per route group
+ * rather than globally so static assets + galleries keep their
+ * own caching strategy.
+ */
+
+function noStoreCache(req, res, next) {
+  // `no-store` is the strongest signal — no cache, no revalidation,
+  // no offline retention. Pair with `private` so any well-behaved
+  // intermediate proxy treats the response as user-specific.
+  res.setHeader('Cache-Control', 'no-store, no-cache, must-revalidate, private');
+  res.setHeader('Pragma', 'no-cache'); // HTTP/1.0 fallback for older proxies
+  res.setHeader('Expires', '0');
+  next();
+}
+
+module.exports = { noStoreCache };