feat(pairing): denormalize user_email onto pairing_codes#45
Merged
Conversation
Removes a gotrue round-trip from the device-status poll path. Devices poll /api/pair/status/* every 3 s; on the success path the prior shape did: 1. SELECT claimed/expires_at/user_id FROM pairing_codes 2. supabase.auth.admin.getUserById(user_id) -- gotrue round-trip Step (2) existed solely to fetch users.email so the device could show the claimer's address in the Cloud submenu for confirmation. Stamping user_email onto pairing_codes at claim time collapses (2) into (1). ## Why the denorm is safe Stale only if the user mutates their auth email AFTER claim but BEFORE the device finishes polling — bounded window (max 5 min, the pairing TTL) and harmless for the one-time confirmation UX (the user sees the email they had at claim time, which is the email they expect). ## Why the email comes from the session, not auth.admin Audit P4 originally suggested an `auth.admin.getUserById` call inside claim_pairing_atomic (claim-time, not poll-time). The route handler already has `user.email` from `safeGetSession()`, so we feed it through directly — saves the gotrue round-trip entirely instead of just shifting it. `user.email` can be null/undefined for some auth flows; we coerce to "" at the route boundary so the column gets a stable string (NOT NULL violation can't fire because the column is nullable anyway, but the contract is cleaner this way). ## RPC signature change `claim_pairing_atomic(p_user_id, p_pairing_id, p_token_hash)` → `claim_pairing_atomic(p_user_id, p_pairing_id, p_token_hash, p_user_email)`. Postgres treats overloaded signatures as distinct objects, so the migration DROPs the 3-arg version explicitly before CREATE-ing the 4-arg version. Existing GRANT in 20260430000002 was tied to the 3-arg signature and dies with the DROP; new GRANT for 4-arg lives in this migration. CLI is pinned >= 2.95.4 (PR #43) so the function name's "atomic" substring no longer trips the v2.90 parser bug — bare REVOKE+GRANT statements are safe. ## Rollback symmetry `rollback_claim_pairing` updated to clear `user_email` alongside `claimed` and `user_id` so the row's denorm fields stay consistent with the claim flag. Signature unchanged → existing GRANT in 20260430000004 survives. ## Test plan - [x] `npx vitest run` — 268/268 pass (claimPairingCode signature update propagated through 12 tests; new defensive test for the `user_email IS NULL` case in checkPairingStatus) - [x] `npm run check` — 0 errors - [x] `supabase db reset --local` at CLI 2.95.4 — 40 migrations apply clean including 20260430000006 Refs: docs/audits/2026-04-29-server-helpers.md issue P4 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Address review feedback on the email denorm:
- Switch claimPairingCode to an options-bag signature
({ userId, userEmail, code }) so adjacent same-type string args
cannot be silently swapped at the call site. WS-B will add more
denorm fields here; the bag scales without further re-ordering.
- Type userEmail as `string | null` and pass `user.email ?? null`
from the route. Empty string is a bad sentinel for "unknown
email"; NULL expresses it honestly through the stack. Operators
wiring phone-only or OAuth-without-email-scope auth keep working.
- Collapse the dual `?? ""` fallback to a single conversion site
in checkPairingStatus; pairing_codes.user_email being NULL is
now a documented, valid state rather than a defensive defence.
- Note in claim/+server.ts that `user.email` must come from the
server-validated session, never from the request body — locks
the security-adjacent wiring against future refactors.
- Trim two over-long comments to align with the project rule of
comments only when WHY is non-obvious.
- Test polish: introduce a callClaim helper to defang signature
drift across 12 repeated invocations (the wire-shape test keeps
its explicit literals so a future arg-order change is loud); set
user_email: null on the unclaimed mock to mirror real DB shape.
- Backfill user_email for any pairing_codes rows already claimed before the migration applies. Self-heals within 5 minutes via TTL regardless, but the UPDATE closes the seam for a deploy that lands during an active pairing window. - Replace `CREATE OR REPLACE FUNCTION claim_pairing_atomic` with plain `CREATE FUNCTION` after the `DROP IF EXISTS` of the 3-arg signature. The 4-arg version is brand new — `OR REPLACE` obscured intent and would silently swallow a re-run that found the function still present. - Update the user_email COMMENT to acknowledge that NULL post-claim is a valid steady state (operator may wire phone or OAuth-no-email auth flows). The read side continues to surface NULL as empty string at the API boundary. - Point the rollback_claim_pairing redeclaration at the canonical asymmetry rationale in 20260430000004_create_rollback_claim_pairing_fn.sql so a contributor reading 000006 first via grep does not reinvent the "do NOT delete the device row" reasoning.
Cover the two wiring guarantees that the unit-tested business logic in src/lib/server/pairing.ts cannot enforce alone: - The route handler MUST forward `user.email` from the server-validated session, NEVER from the request body. A future refactor that read body.email would let a malicious browser display any email on the device's pairing-confirmation screen (security-adjacent UX). The test passes a body with a different email and asserts the session email wins. - A session user without an email (phone signup, OAuth without email scope) flows through as `null`, not as `""`. This is the hand-off point for the read-side NULL → "" conversion in checkPairingStatus. - Plus a 401 baseline so the auth gate is regression-proof. Adopts the existing tests/routes/transfer-confirm.test.ts mocking pattern (vi.mock the route's deps before importing it).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements audit issue P4. Stamps the claimer's email onto
pairing_codes.user_emailat claim time socheckPairingStatuscan return it without a gotrue round-trip per poll. Removes theauth.admin.getUserByIdcall from the device-poll hot path.The email is sourced from
safeGetSession()in the route handler, so the round-trip is eliminated entirely (not just shifted from poll-time to claim-time as the audit originally suggested).Test plan
npx vitest run— 268/268 pass (claimPairingCode signature update propagated; new defensive test for null user_email)npm run check— 0 errorssupabase db reset --localat CLI 2.95.4 — 40 migrations apply cleansupabase/migrations/**)Notes for reviewer
claim_pairing_atomicsignature changes 3-arg → 4-arg. MigrationDROP IF EXISTSthe old signature beforeCREATEof the new — Postgres treats overloads as distinct objects. Existing GRANTs from20260430000002die with the drop; new GRANT for the 4-arg signature ships in this migration.rollback_claim_pairingclearsuser_emailalongsideclaimedanduser_idfor symmetry. Signature unchanged → grant in20260430000004carries over.