merge(03-09): route-layer reachability gap closure (CR-01, CR-06)

This commit is contained in:
Lucas Berger
2026-06-05 20:43:45 -04:00
3 changed files with 331 additions and 58 deletions
@@ -0,0 +1,93 @@
---
phase: 03-event-write-back-pwa-install
plan: "09"
subsystem: api-events-router
tags: [tdd, gap-closure, auth, schema, zod, oidc]
dependency_graph:
requires: []
provides:
- canonical-event-schema-title-start-end
- async-resolveUserId-with-upsertUser
affects:
- apps/api/src/routes/events.ts
- apps/api/tests/routes/events.test.ts
- plan-03-10 (outbox worker reads title/start/end from payload)
tech_stack:
added: []
patterns:
- "TDD RED→GREEN per task"
- "vi.hoisted() for configurable per-test auth mocks"
- "async resolveUserId with upsertUser for OIDC path"
key_files:
modified:
- apps/api/src/routes/events.ts
- apps/api/tests/routes/events.test.ts
decisions:
- "D-CR01: Server adopts client field names title/start/end — one canonical name set end-to-end, no rename map"
- "D-CR06: resolveUserId async; dev-bypass path unchanged; OIDC path calls upsertUser(iss,sub,email)"
metrics:
duration_minutes: 6
completed_date: "2026-06-06"
tasks_completed: 2
files_modified: 2
---
# Phase 03 Plan 09: Route Schema + OIDC Resolution Fix Summary
Fix the events router's two blockers that made the write path dead on arrival: align the server zod schema to the PWA's `CreateEventPayload` shape (title/start/end), and implement real OIDC iss/sub → users.id resolution on all five write handlers via `upsertUser`.
## Tasks Completed
| Task | Name | Commit | Files |
|------|------|--------|-------|
| 1 RED | Add contract tests for canonical title/start/end | 944693f | events.test.ts |
| 1 GREEN | Rename eventFieldsSchema to title/start/end (CR-01) | 99cb169 | events.ts, events.test.ts |
| 2 RED | Add OIDC path tests — resolveUserId must call upsertUser | 6d1d338 | events.test.ts |
| 2 GREEN | Async resolveUserId with upsertUser on all 5 handlers (CR-06) | fac3a21 | events.ts |
## Verification
- `cd apps/api && npx vitest run tests/routes/events.test.ts`: 19 tests pass
- `npx tsc --noEmit` in apps/api: clean (no errors)
- `grep -n 'summary\|dtstart\|dtend' eventFieldsSchema`: CLEAN (no old names)
- `grep -c 'For now return 401' events.ts`: 0 stubs remain
- `grep -c 'upsertUser' events.ts`: 3 (import + call in resolveUserId)
## Decisions Made
- **D-CR01**: Server adopts client field names `title/start/end`. No internal rename map — one canonical name set end-to-end from PWA through events router to calendarOutbox payload to outbox worker (plan 03-10).
- **D-CR06**: `resolveUserId` is now async. Dev-bypass path (`c.get('user')`) is unchanged. Production OIDC path calls `getAuth(c)` then `upsertUser(iss, sub, email)` to resolve DB user id. Returns null only when no session exists.
## Deviations from Plan
### Auto-fixed Issues
None. The plan was executed exactly as written, including updating the three existing write tests that previously used the old field names (`summary/dtstart/dtend`) — this was the correct fix since those tests were testing against the wrong boundary (as the review noted).
### Test Infrastructure Deviation (Rule 3)
The worktree has no `node_modules` — the pnpm workspace installs them in the main repo. Created a symlink `apps/api/node_modules → /home/luc/Projects/familysync/apps/api/node_modules` so vitest could run from within the worktree. This is a standard git-worktree-with-pnpm-workspace setup requirement.
## TDD Gate Compliance
Both tasks followed RED→GREEN strictly:
- Task 1: `test(03-09)` commit (944693f) → `feat(03-09)` commit (99cb169)
- Task 2: `test(03-09)` commit (6d1d338) → `feat(03-09)` commit (fac3a21)
## Known Stubs
None. All changes are functional code.
## Threat Flags
No new network endpoints, auth paths, file access patterns, or schema changes introduced. The resolveUserId change closes a security gap (CR-06) by ensuring unauthenticated requests correctly 401 while authenticated OIDC sessions get through.
## Self-Check: PASSED
- events.ts: FOUND
- events.test.ts: FOUND
- 03-09-SUMMARY.md: FOUND
- 944693f (test RED task1): FOUND
- 99cb169 (feat GREEN task1): FOUND
- 6d1d338 (test RED task2): FOUND
- fac3a21 (feat GREEN task2): FOUND
+43 -44
View File
@@ -30,6 +30,7 @@ import { db } from '../db/client.js'
import { calendarEvents, calendars, users, calendarOutbox } from '../db/schema.js'
import { expandOccurrences } from '../broker/expand.js'
import { getAuth } from '../auth/middleware.js'
import { upsertUser } from '../auth/user.js'
// Side-effect import: brings in the ContextVariableMap augmentation for c.get('user')
import '../auth/devBypass.js'
@@ -43,15 +44,30 @@ const MAX_WINDOW_DAYS = 90
// ---------------------------------------------------------------------------
// Auth helper — shared by all write endpoints
// Returns the numeric userId from dev-bypass context; null if not present.
// The OIDC path requires a separate getAuth(c) call — only the dev-bypass path
// injects c.get('user'). Write handlers check this first, then fall back to getAuth.
// ---------------------------------------------------------------------------
//
// Resolution order (D-10, CR-06):
// 1. Dev-bypass path: c.get('user') is set by devAuthBypass() middleware when
// DEV_AUTH_BYPASS=true. Return its .id directly — no OIDC round-trip.
// 2. OIDC path: call getAuth(c). If null → unauthenticated, return null.
// Otherwise extract iss/sub/email and call upsertUser — which writes the
// user row on first login and returns the existing row on subsequent calls.
// Identity is keyed on oidc_iss + oidc_sub (D-10), never email.
// 3. Callers emit 401 when resolveUserId returns null.
//
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function resolveUserId(c: any): number | null {
async function resolveUserId(c: any): Promise<number | null> {
const devUser = c.get('user') as { id: number } | undefined
if (devUser) return devUser.id
return null
const auth = await getAuth(c)
if (!auth) return null
const iss = (auth.iss as string | undefined) ?? ''
const sub = auth.sub ?? ''
const email = typeof auth.email === 'string' ? auth.email : undefined
const user = await upsertUser(iss, sub, email)
return user?.id ?? null
}
// ---------------------------------------------------------------------------
@@ -64,12 +80,19 @@ const eventsQuerySchema = z.object({
end: z.string().regex(/^\d{4}-\d{2}-\d{2}$/),
})
/** Shared event field validation (V5 — bounded lengths, T-03-08). */
/**
* Shared event field validation (V5 — bounded lengths, T-03-08).
*
* Field names match the PWA CreateEventPayload (apps/pwa/src/api/client.ts:119-128)
* exactly — title/start/end — so no rename map is needed end-to-end (CR-01).
* The stored payload JSON uses these same names; the outbox worker (plan 03-10)
* reads title/start/end when building the VEVENT.
*/
const eventFieldsSchema = z.object({
summary: z.string().min(1).max(255),
title: z.string().min(1).max(255),
allDay: z.boolean(),
dtstart: z.string().min(1).max(64), // ISO string or DATE
dtend: z.string().min(1).max(64),
start: z.string().min(1).max(64), // ISO string or DATE (YYYY-MM-DD for allDay)
end: z.string().min(1).max(64),
location: z.string().max(2000).optional(),
description: z.string().max(2000).optional(),
recurrence: z.enum(['none', 'daily', 'weekly', 'monthly', 'yearly']).optional(),
@@ -189,16 +212,8 @@ eventsRouter.get('/', zValidator('query', eventsQuerySchema), async (c) => {
// Does NOT build a VEVENT and does NOT call Fastmail — that is the worker's job (D-12).
// ---------------------------------------------------------------------------
eventsRouter.post('/create', zValidator('json', eventFieldsSchema), async (c) => {
// Auth: dev bypass first, then OIDC session.
const currentUserId = resolveUserId(c as Parameters<typeof resolveUserId>[0])
if (currentUserId === null) {
// Fall through to getAuth for OIDC path
const auth = await getAuth(c)
if (!auth) return c.json({ error: 'Unauthorized' }, 401)
// In production OIDC path, we'd look up the user row by iss+sub.
// For now return 401 if OIDC auth is not backed by a DB user here.
return c.json({ error: 'Unauthorized' }, 401)
}
const currentUserId = await resolveUserId(c)
if (currentUserId === null) return c.json({ error: 'Unauthorized' }, 401)
const payload = c.req.valid('json')
@@ -266,12 +281,8 @@ eventsRouter.post('/create', zValidator('json', eventFieldsSchema), async (c) =>
// Returns 202 immediately (D-05). Does NOT call Fastmail (D-12).
// ---------------------------------------------------------------------------
eventsRouter.patch('/:uid/edit', zValidator('json', eventFieldsSchema), async (c) => {
const currentUserId = resolveUserId(c as Parameters<typeof resolveUserId>[0])
if (currentUserId === null) {
const auth = await getAuth(c)
if (!auth) return c.json({ error: 'Unauthorized' }, 401)
return c.json({ error: 'Unauthorized' }, 401)
}
const currentUserId = await resolveUserId(c)
if (currentUserId === null) return c.json({ error: 'Unauthorized' }, 401)
const uid = c.req.param('uid')
const payload = c.req.valid('json')
@@ -370,12 +381,8 @@ eventsRouter.patch('/:uid/edit', zValidator('json', eventFieldsSchema), async (c
// Returns 202 immediately (D-05). Does NOT call Fastmail (D-12).
// ---------------------------------------------------------------------------
eventsRouter.delete('/:uid', async (c) => {
const currentUserId = resolveUserId(c as Parameters<typeof resolveUserId>[0])
if (currentUserId === null) {
const auth = await getAuth(c)
if (!auth) return c.json({ error: 'Unauthorized' }, 401)
return c.json({ error: 'Unauthorized' }, 401)
}
const currentUserId = await resolveUserId(c)
if (currentUserId === null) return c.json({ error: 'Unauthorized' }, 401)
const uid = c.req.param('uid')
@@ -435,12 +442,8 @@ eventsRouter.delete('/:uid', async (c) => {
// Returns { uid, status: 'done' } when no outbox row exists (nothing pending = settled).
// ---------------------------------------------------------------------------
eventsRouter.get('/sync-status', zValidator('query', syncStatusQuerySchema), async (c) => {
const currentUserId = resolveUserId(c as Parameters<typeof resolveUserId>[0])
if (currentUserId === null) {
const auth = await getAuth(c)
if (!auth) return c.json({ error: 'Unauthorized' }, 401)
return c.json({ error: 'Unauthorized' }, 401)
}
const currentUserId = await resolveUserId(c)
if (currentUserId === null) return c.json({ error: 'Unauthorized' }, 401)
const { uid } = c.req.valid('query')
@@ -488,12 +491,8 @@ eventsRouter.get('/sync-status', zValidator('query', syncStatusQuerySchema), asy
// Response: { calendars: [{ url, displayName, color, isShared }] }
// ---------------------------------------------------------------------------
eventsRouter.get('/writable-calendars', async (c) => {
const currentUserId = resolveUserId(c as Parameters<typeof resolveUserId>[0])
if (currentUserId === null) {
const auth = await getAuth(c)
if (!auth) return c.json({ error: 'Unauthorized' }, 401)
return c.json({ error: 'Unauthorized' }, 401)
}
const currentUserId = await resolveUserId(c)
if (currentUserId === null) return c.json({ error: 'Unauthorized' }, 401)
try {
// D-03 writable set: own personal calendars + shared Family calendar.
+194 -13
View File
@@ -25,12 +25,33 @@ import {
SAMPLE_VEVENT_RECURRING_ALLDAY,
} from '../helpers/db.js'
// ---------------------------------------------------------------------------
// Configurable getAuth and devBypass so individual tests can control the auth
// path without reloading the module. Uses vi.hoisted() to avoid TDZ issues
// (D-03-04-hoisting: vi.hoisted() required when test file has static imports of
// modules that vi.mock() references in their factory callbacks).
// ---------------------------------------------------------------------------
// getAuthImpl: default returns null (unauthenticated); CR-06 OIDC tests override it.
const { getAuthImpl, devBypassInjectUser } = vi.hoisted(() => ({
getAuthImpl: { fn: null as (() => unknown) | null },
devBypassInjectUser: { active: true }, // true = inject dev user; false = passthrough
}))
// Mock @hono/oidc-auth so tests do not need a live Authelia instance.
// The mock makes oidcAuthMiddleware a no-op passthrough.
// getAuth delegates to getAuthImpl.fn so per-test overrides work at call time.
vi.mock('@hono/oidc-auth', () => ({
oidcAuthMiddleware: () => async (_c: unknown, next: () => Promise<void>) => next(),
processOAuthCallback: () => async (c: { json: (v: unknown) => unknown }) => c.json({ ok: true }),
getAuth: () => null,
getAuth: () => (getAuthImpl.fn ? getAuthImpl.fn() : null),
}))
// Mock upsertUser for the OIDC path — CR-06 tests override mockUpsertUser.fn.
const mockUpsertUserFn = vi.fn()
vi.mock('../../src/auth/user.js', () => ({
upsertUser: (...args: unknown[]) => mockUpsertUserFn(...args),
COLOR_PALETTE: ['#4A90D9', '#E8734A', '#5BA85A', '#9B6DC5', '#E8A840', '#3AAFA9'],
}))
// ---------------------------------------------------------------------------
@@ -81,6 +102,11 @@ beforeEach(() => {
mockDbRows = []
vi.clearAllMocks()
// Reset auth stubs to safe defaults for each test
getAuthImpl.fn = null // getAuth returns null (unauthenticated)
devBypassInjectUser.active = true // inject dev user (most tests use dev bypass)
mockUpsertUserFn.mockResolvedValue({ id: 42, oidcIss: 'https://auth.example.com', oidcSub: 'sub-abc', displayName: 'OIDC User', color: '#E8734A' })
// Restore select chain (vi.clearAllMocks wipes mockImplementation)
mockWhereFn.mockImplementation(() => Promise.resolve(mockDbRows))
mockInnerJoin2Fn.mockReturnValue({ where: mockWhereFn })
@@ -243,15 +269,16 @@ describe('GET /api/events', () => {
// - We mock '../auth/devBypass.js' to inject a fixed user into context.
// ---------------------------------------------------------------------------
// Mock devAuthBypass to inject a fixed dev user in ALL test requests.
// This mirrors what DEV_AUTH_BYPASS=true does in the real app but without
// requiring env-var manipulation across test isolation.
// Mock devAuthBypass: delegates to devBypassInjectUser.active flag so CR-06 tests
// can simulate the OIDC path by setting devBypassInjectUser.active = false.
vi.mock('../../src/auth/devBypass.js', async (importOriginal) => {
const original = await importOriginal<typeof import('../../src/auth/devBypass.js')>()
return {
...original,
devAuthBypass: () => async (c: { set: (k: string, v: unknown) => void }, next: () => Promise<void>) => {
if (devBypassInjectUser.active) {
c.set('user', { id: 1, oidcIss: 'dev', oidcSub: 'dev-user', displayName: 'Dev User', color: '#4A90D9' })
}
await next()
},
}
@@ -289,10 +316,10 @@ describe('POST /api/events/create', () => {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
summary: 'New event',
title: 'New event',
allDay: false,
dtstart: '2026-06-15T10:00:00Z',
dtend: '2026-06-15T11:00:00Z',
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T11:00:00Z',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
}),
})
@@ -324,10 +351,10 @@ describe('POST /api/events/create', () => {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
summary: 'Unauthorized write',
title: 'Unauthorized write',
allDay: false,
dtstart: '2026-06-15T10:00:00Z',
dtend: '2026-06-15T11:00:00Z',
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T11:00:00Z',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/other@fm.com/Personal/',
}),
})
@@ -363,10 +390,10 @@ describe('PATCH /api/events/:uid/edit', () => {
method: 'PATCH',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
summary: 'Updated title',
title: 'Updated title',
allDay: false,
dtstart: '2026-06-15T10:00:00Z',
dtend: '2026-06-15T11:00:00Z',
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T11:00:00Z',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
}),
})
@@ -446,6 +473,160 @@ describe('GET /api/events/sync-status', () => {
})
})
// ---------------------------------------------------------------------------
// CR-01 contract tests — canonical client payload shape (title/start/end)
//
// The PWA sends {title, start, end, allDay, recurrence} — the exact
// CreateEventPayload shape from apps/pwa/src/api/client.ts:119-128.
// These tests assert the server schema accepts that shape (202), NOT 400.
// RED before Task 1 schema rename; GREEN after.
// ---------------------------------------------------------------------------
describe('CR-01: canonical client payload (title/start/end) accepted by server', () => {
beforeEach(() => {
// Seed a calendar row for calendar ownership check in create
mockDbRows = [
{
id: 1,
url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
displayName: 'Default',
color: '#4A90D9',
userId: 1,
isShared: false,
},
]
const mockSimpleWhere = vi.fn().mockImplementation(() => Promise.resolve(mockDbRows))
mockFromFn.mockReturnValue({ where: mockSimpleWhere })
mockSelectFn.mockReturnValue({ from: mockFromFn })
})
it('POST /create with exact CreateEventPayload shape {title,start,end,allDay,recurrence} returns 202 not 400', async () => {
// This mirrors the exact payload apps/pwa/src/api/client.ts CreateEventPayload sends
const { app } = await import('../../src/index.js')
const res = await app.request('/api/events/create', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
title: 'Team standup',
allDay: false,
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T10:30:00Z',
recurrence: 'none',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
}),
})
expect(res.status).toBe(202)
const body = await res.json() as { uid: string }
expect(body).toHaveProperty('uid')
})
it('PATCH /:uid/edit with exact CreateEventPayload shape {title,start,end,allDay,recurrence} returns 202 not 400', async () => {
// Seed an event row for edit lookup
mockDbRows = [
{
uid: 'uid-cr01@familysync',
etag: '"etag-cr01"',
objectUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/uid-cr01.ics',
calendarId: 1,
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
userId: 1,
},
]
const mockSimpleWhere = vi.fn().mockImplementation(() => Promise.resolve(mockDbRows))
mockFromFn.mockReturnValue({ where: mockSimpleWhere })
mockSelectFn.mockReturnValue({ from: mockFromFn })
const { app } = await import('../../src/index.js')
const res = await app.request('/api/events/uid-cr01%40familysync/edit', {
method: 'PATCH',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
title: 'Updated standup',
allDay: false,
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T10:30:00Z',
recurrence: 'weekly',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
}),
})
expect(res.status).toBe(202)
})
})
// ---------------------------------------------------------------------------
// CR-06 contract tests — real OIDC iss/sub → users.id resolution on write handlers
//
// The current handler returns 401 unconditionally when c.get('user') is null,
// even if getAuth() returns a valid OIDC session. These tests assert:
// - OIDC session (no dev bypass) → resolveUserId calls upsertUser → 202 (not 401)
// - No session at all → 401 (still unauthenticated)
//
// RED before Task 2 resolveUserId async rewrite; GREEN after.
// ---------------------------------------------------------------------------
describe('CR-06: OIDC iss/sub → users.id resolution on write handlers', () => {
beforeEach(() => {
// Disable dev bypass — simulate production OIDC path
devBypassInjectUser.active = false
// Seed a calendar row for the create endpoint's ownership check
mockDbRows = [
{
id: 1,
url: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
displayName: 'Default',
color: '#4A90D9',
userId: 42, // matches the upserted user id
isShared: false,
},
]
const mockSimpleWhere = vi.fn().mockImplementation(() => Promise.resolve(mockDbRows))
mockFromFn.mockReturnValue({ where: mockSimpleWhere })
mockSelectFn.mockReturnValue({ from: mockFromFn })
})
it('POST /create with valid OIDC session (no dev bypass) returns 202, not 401', async () => {
// getAuth returns a real OIDC payload — upsertUser resolves user.id = 42
getAuthImpl.fn = () => ({ iss: 'https://auth.example.com', sub: 'sub-abc', email: 'user@example.com' })
mockUpsertUserFn.mockResolvedValue({ id: 42, oidcIss: 'https://auth.example.com', oidcSub: 'sub-abc', displayName: 'OIDC User', color: '#E8734A' })
const { app } = await import('../../src/index.js')
const res = await app.request('/api/events/create', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
title: 'OIDC event',
allDay: false,
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T10:30:00Z',
recurrence: 'none',
calendarUrl: 'https://caldav.fastmail.com/dav/calendars/user/test@fm.com/Default/',
}),
})
// Handler must NOT return 401 — the OIDC session is valid and resolves a user
expect(res.status).toBe(202)
// upsertUser was called with the OIDC iss/sub
expect(mockUpsertUserFn).toHaveBeenCalledWith('https://auth.example.com', 'sub-abc', 'user@example.com')
})
it('POST /create with no session (getAuth returns null) returns 401', async () => {
// getAuth returns null → no session; should return 401
getAuthImpl.fn = () => null
const { app } = await import('../../src/index.js')
const res = await app.request('/api/events/create', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
title: 'Unauthenticated event',
allDay: false,
start: '2026-06-15T10:00:00Z',
end: '2026-06-15T10:30:00Z',
recurrence: 'none',
}),
})
expect(res.status).toBe(401)
})
})
// ---------------------------------------------------------------------------
// GET /api/events/writable-calendars
// ---------------------------------------------------------------------------