diff --git a/backend/server.js b/backend/server.js index dd0d9ee2..fbcdb2f1 100644 --- a/backend/server.js +++ b/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')); diff --git a/backend/src/__tests__/customerAccountsService.test.js b/backend/src/__tests__/customerAccountsService.test.js index 42380a74..4a236c4d 100644 --- a/backend/src/__tests__/customerAccountsService.test.js +++ b/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]); + }); +}); diff --git a/backend/src/__tests__/noStoreCache.test.js b/backend/src/__tests__/noStoreCache.test.js new file mode 100644 index 00000000..048cc480 --- /dev/null +++ b/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'); + }); +}); diff --git a/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js b/backend/src/__tests__/verifyGalleryAccess.customerRevoke.test.js new file mode 100644 index 00000000..1ebbefef --- /dev/null +++ b/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'); + }); +}); diff --git a/backend/src/middleware/noStoreCache.js b/backend/src/middleware/noStoreCache.js new file mode 100644 index 00000000..f95de501 --- /dev/null +++ b/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 };