diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index 121fbd8..7843ceb 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -25,12 +25,12 @@ Requirements for this milestone. Each maps to roadmap phases. ### Multi-User Data Model -- [ ] **MULTI-01**: Every item, category, thread, and setup is owned by a specific user +- [x] **MULTI-01**: Every item, category, thread, and setup is owned by a specific user - [ ] **MULTI-02**: User can only see and modify their own data (cross-user isolation) - [ ] **MULTI-03**: Categories use composite unique constraint (userId + name) -- [ ] **MULTI-04**: Existing data is assigned to the original user during migration +- [x] **MULTI-04**: Existing data is assigned to the original user during migration - [ ] **MULTI-05**: MCP tools operate within the authenticated user's scope -- [ ] **MULTI-06**: Settings are per-user rather than global +- [x] **MULTI-06**: Settings are per-user rather than global ### Image Storage @@ -126,12 +126,12 @@ Which phases cover which requirements. Updated during roadmap creation. | AUTH-03 | Phase 15 | Pending | | AUTH-04 | Phase 15 | Pending | | AUTH-05 | Phase 15 | Pending | -| MULTI-01 | Phase 16 | Pending | +| MULTI-01 | Phase 16 | Complete | | MULTI-02 | Phase 16 | Pending | | MULTI-03 | Phase 16 | Pending | -| MULTI-04 | Phase 16 | Pending | +| MULTI-04 | Phase 16 | Complete | | MULTI-05 | Phase 16 | Pending | -| MULTI-06 | Phase 16 | Pending | +| MULTI-06 | Phase 16 | Complete | | IMG-01 | Phase 17 | Pending | | IMG-02 | Phase 17 | Pending | | IMG-03 | Phase 17 | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 2be42f5..c801ddd 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -145,7 +145,12 @@ Plans: 3. Existing data from the single-user era is assigned to the original user account after migration 4. MCP tools return only data belonging to the authenticated API key's owner 5. Each user has independent settings (weight unit, onboarding state) that do not affect other users -**Plans**: TBD +**Plans**: 4 plans +Plans: +- [x] 16-01-PLAN.md — Schema foundation: users table, userId columns, auth middleware, test helper +- [ ] 16-02-PLAN.md — Service layer userId scoping +- [ ] 16-03-PLAN.md — Route handlers userId extraction +- [ ] 16-04-PLAN.md — Test suite updates ### Phase 17: Object Storage **Goal**: Images are stored in and served from MinIO instead of the local filesystem @@ -190,6 +195,6 @@ Plans: | 13. Setup Impact Preview | v1.3 | 0/2 | Not started | - | | 14. PostgreSQL Migration | v2.0 | 0/? | Not started | - | | 15. External Authentication | v2.0 | 0/? | Not started | - | -| 16. Multi-User Data Model | v2.0 | 0/? | Not started | - | +| 16. Multi-User Data Model | v2.0 | 1/4 | In progress | - | | 17. Object Storage | v2.0 | 0/? | Not started | - | | 18. Global Items & Public Profiles | v2.0 | 0/? | Not started | - | diff --git a/.planning/STATE.md b/.planning/STATE.md index d9a43f1..9280f35 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -2,16 +2,16 @@ gsd_state_version: 1.0 milestone: v2.0 milestone_name: Platform Foundation -status: planning +status: executing stopped_at: null -last_updated: "2026-04-03" -last_activity: 2026-04-03 — v2.0 roadmap created (Phases 14-18) +last_updated: "2026-04-05" +last_activity: 2026-04-05 — Completed 16-01-PLAN.md (multi-user data model foundation) progress: total_phases: 5 completed_phases: 0 - total_plans: 0 - completed_plans: 0 - percent: 0 + total_plans: 4 + completed_plans: 1 + percent: 5 --- # Project State @@ -21,23 +21,23 @@ progress: See: .planning/PROJECT.md (updated 2026-04-03) **Core value:** Help people make better gear decisions — discover what others use, compare real-world data, and see how a potential buy affects your setup before committing. -**Current focus:** v2.0 Platform Foundation — Phase 14 (PostgreSQL Migration) +**Current focus:** v2.0 Platform Foundation — Phase 16 (Multi-User Data Model) ## Current Position -Phase: 14 of 18 (PostgreSQL Migration) -Plan: 0 of ? in current phase -Status: Ready to plan -Last activity: 2026-04-03 — v2.0 roadmap created (Phases 14-18) +Phase: 16 of 18 (Multi-User Data Model) +Plan: 1 of 4 in current phase +Status: Plan 16-01 complete, executing remaining plans +Last activity: 2026-04-05 — Completed 16-01 (multi-user data model foundation) -Progress: [----------] 0% (v2.0 milestone) +Progress: [#---------] 5% (v2.0 milestone) ## Performance Metrics **Velocity:** -- Total plans completed: 0 (v2.0 milestone) -- Average duration: -- -- Total execution time: -- +- Total plans completed: 1 (v2.0 milestone) +- Average duration: 8min +- Total execution time: 8min *Updated after each plan completion* @@ -45,13 +45,16 @@ Progress: [----------] 0% (v2.0 milestone) ### Decisions -Key decisions made during v2.0 planning: +Key decisions made during v2.0 execution: - Platform pivot: single-user to multi-user with discovery-first approach - External auth provider (self-hosted, open-source) — Logto vs Authentik OPEN decision - SQLite to Postgres migration — required by auth provider and multi-user concurrency - Structured UGC only — ratings and predefined fields, no freeform text until moderation - Separate globalItems table — not a flag on user items table - Single-user SQLite mode diverges at v2.0 boundary +- All API routes require auth (no GET bypass) for per-user data scoping (16-01) +- OAuth service converted from sync to async for pg compatibility (16-01) +- getOrCreateUncategorized placed in category.service.ts (16-01) ### Pending Todos @@ -64,6 +67,6 @@ None active. ## Session Continuity -Last session: 2026-04-03 -Stopped at: v2.0 roadmap created with 5 phases (14-18) covering 30 requirements +Last session: 2026-04-05 +Stopped at: Completed 16-01-PLAN.md (multi-user data model foundation) Resume file: None diff --git a/.planning/phases/16-multi-user-data-model/16-01-PLAN.md b/.planning/phases/16-multi-user-data-model/16-01-PLAN.md new file mode 100644 index 0000000..19476a5 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-01-PLAN.md @@ -0,0 +1,355 @@ +--- +phase: 16-multi-user-data-model +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - src/db/schema.ts + - src/db/seed.ts + - tests/helpers/db.ts + - src/server/middleware/auth.ts + - src/server/services/auth.service.ts + - src/server/services/oauth.service.ts + - src/server/index.ts +autonomous: true +requirements: + - MULTI-01 + - MULTI-04 + - MULTI-06 + +must_haves: + truths: + - "A users table exists with id (serial PK), logtoSub (text unique), createdAt (timestamp)" + - "Every entity table (items, categories, threads, setups, settings, apiKeys) has a userId integer FK column" + - "Categories have a composite unique constraint on (userId, name) instead of unique(name)" + - "Settings have a composite primary key on (userId, key) instead of just (key)" + - "requireAuth middleware resolves userId and sets it on Hono context" + - "verifyApiKey returns { userId } | null instead of boolean" + - "verifyAccessToken returns { userId } | null instead of boolean" + - "createTestDb returns { db, userId } with a seeded test user and per-user Uncategorized category" + - "All API routes require auth (no GET bypass) so userId is always available" + artifacts: + - path: "src/db/schema.ts" + provides: "Users table + userId columns on all entity tables + composite constraints" + contains: "export const users" + - path: "tests/helpers/db.ts" + provides: "Test DB with seeded user" + contains: "logtoSub" + - path: "src/server/middleware/auth.ts" + provides: "userId resolution middleware" + contains: "c.set(\"userId\"" + key_links: + - from: "src/server/middleware/auth.ts" + to: "src/server/services/auth.service.ts" + via: "verifyApiKey returning userId" + pattern: "verifyApiKey.*userId" + - from: "src/server/middleware/auth.ts" + to: "src/server/services/oauth.service.ts" + via: "verifyAccessToken returning userId" + pattern: "verifyAccessToken.*userId" +--- + + +Create the multi-user data model foundation: users table, userId columns on all entity tables, updated auth middleware that resolves userId onto Hono context, and updated test infrastructure. + +Purpose: This is the foundation that all subsequent plans depend on. Without the schema changes, userId columns, and middleware resolution, no service or route can be updated to scope data per-user. + +Output: Updated schema.ts with pgTable imports and users table, migration generated, auth middleware resolving userId, test helper returning { db, userId }. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/STATE.md +@.planning/phases/16-multi-user-data-model/16-CONTEXT.md +@.planning/phases/16-multi-user-data-model/16-RESEARCH.md +@src/db/schema.ts +@src/db/seed.ts +@src/server/middleware/auth.ts +@src/server/services/auth.service.ts +@src/server/services/oauth.service.ts +@src/server/index.ts +@tests/helpers/db.ts + + + + +From src/db/schema.ts (current - uses sqliteTable, must switch to pgTable): +- categories: id, name (unique), icon, createdAt +- items: id, name, weightGrams, priceCents, categoryId, notes, productUrl, imageFilename, imageSourceUrl, quantity, createdAt, updatedAt +- threads: id, name, status, resolvedCandidateId, categoryId, createdAt, updatedAt +- threadCandidates: id, threadId, name, weightGrams, priceCents, categoryId, notes, productUrl, imageFilename, imageSourceUrl, status, pros, cons, sortOrder, createdAt, updatedAt +- setups: id, name, createdAt, updatedAt +- setupItems: id, setupId, itemId, classification +- settings: key (PK), value +- apiKeys: id, name, keyHash, keyPrefix, createdAt +- oauthClients: id, clientId, clientName, redirectUris, createdAt +- oauthCodes: id, code, clientId, codeChallenge, codeChallengeMethod, redirectUri, expiresAt, used +- oauthTokens: id, accessTokenHash, refreshTokenHash, clientId, expiresAt, refreshExpiresAt, createdAt + +From src/server/services/auth.service.ts: +```typescript +export async function verifyApiKey(db: Db, rawKey: string): Promise +export async function createApiKey(db: Db, name: string) +export async function listApiKeys(db: Db) +export async function deleteApiKey(db: Db, id: number) +``` + +From src/server/services/oauth.service.ts: +```typescript +export async function verifyAccessToken(db: Db, token: string): Promise +``` + + + + + + + Task 1: Migrate schema.ts to pgTable and add users table + userId columns + src/db/schema.ts, src/db/seed.ts + src/db/schema.ts, src/db/seed.ts, .planning/phases/16-multi-user-data-model/16-RESEARCH.md + + 1. Rewrite `src/db/schema.ts` to use `drizzle-orm/pg-core` imports instead of `drizzle-orm/sqlite-core`. Replace `sqliteTable` with `pgTable`, `integer("id").primaryKey({ autoIncrement: true })` with `serial("id").primaryKey()`, `integer` with `integer` from pg-core, `real` with `doublePrecision`, and `integer("...", { mode: "timestamp" })` with `timestamp("...").defaultNow()` (or equivalent). + + 2. Add the new `users` table per D-01: + ```typescript + export const users = pgTable("users", { + id: serial("id").primaryKey(), + logtoSub: text("logto_sub").notNull().unique(), + createdAt: timestamp("created_at").defaultNow().notNull(), + }); + ``` + + 3. Add `userId` column (integer, NOT NULL, FK to users.id) to these tables per D-04: + - `items`: `userId: integer("user_id").notNull().references(() => users.id)` + - `categories`: `userId: integer("user_id").notNull().references(() => users.id)` + - `threads`: `userId: integer("user_id").notNull().references(() => users.id)` + - `setups`: `userId: integer("user_id").notNull().references(() => users.id)` + - `apiKeys`: `userId: integer("user_id").notNull().references(() => users.id)` per D-07 + - `oauthTokens`: `userId: integer("user_id").notNull().references(() => users.id)` (per Research open question 2) + + 4. Per D-05, change `categories` unique constraint from `name` alone to composite `(userId, name)`: + ```typescript + export const categories = pgTable("categories", { + id: serial("id").primaryKey(), + name: text("name").notNull(), + icon: text("icon").notNull().default("package"), + userId: integer("user_id").notNull().references(() => users.id), + createdAt: timestamp("created_at").defaultNow().notNull(), + }, (table) => [ + unique().on(table.userId, table.name), + ]); + ``` + + 5. Per D-06, change `settings` PK from `key` alone to composite `(userId, key)`: + ```typescript + export const settings = pgTable("settings", { + userId: integer("user_id").notNull().references(() => users.id), + key: text("key").notNull(), + value: text("value").notNull(), + }, (table) => [ + primaryKey({ columns: [table.userId, table.key] }), + ]); + ``` + + 6. Per D-08, do NOT add userId to `threadCandidates` or `setupItems` (they inherit ownership via parent FK). + + 7. Update `src/db/seed.ts`: The `seedDefaults()` function currently seeds a global Uncategorized category. Since categories now require userId, this global seed no longer works. Change `seedDefaults()` to be a no-op or remove the category seeding entirely (per-user Uncategorized will be created lazily or on first login per D-12). The function can remain as an empty function for now: + ```typescript + export async function seedDefaults() { + // Per-user default categories are created on first login (Phase 16) + } + ``` + + 8. Run `bun run db:generate` to generate the new Drizzle migration into `drizzle-pg/`. Then verify the generated SQL includes the users table, userId columns, composite constraints, and FK relationships. + + IMPORTANT: The schema.ts file MUST use pg-core imports (`pgTable`, `serial`, `text`, `timestamp`, `integer`, `doublePrecision`, `unique`, `primaryKey`). The existing `drizzle-pg/` migration directory already has PostgreSQL DDL from Phase 14. + + + grep -c "pgTable" src/db/schema.ts && grep -c "export const users" src/db/schema.ts && grep "userId" src/db/schema.ts | wc -l && grep -c "unique().on" src/db/schema.ts && grep -c "primaryKey" src/db/schema.ts + + + - `src/db/schema.ts` contains `import.*from "drizzle-orm/pg-core"` (no sqlite-core imports) + - `export const users = pgTable("users"` exists with `logtoSub`, `id`, `createdAt` + - `userId: integer("user_id").notNull().references(() => users.id)` appears in items, categories, threads, setups, apiKeys, oauthTokens + - `unique().on(table.userId, table.name)` exists in categories table definition + - `primaryKey({ columns: [table.userId, table.key] })` exists in settings table definition + - `threadCandidates` and `setupItems` do NOT have a userId column + - `src/db/seed.ts` no longer inserts a global Uncategorized category + - A new migration file exists in `drizzle-pg/` with the users table and userId column additions + + Schema uses pg-core imports, users table exists, all 6 entity tables have userId FK, categories has composite unique, settings has composite PK, migration generated + + + + Task 2: Update auth middleware and auth services to resolve userId + src/server/middleware/auth.ts, src/server/services/auth.service.ts, src/server/services/oauth.service.ts, src/server/index.ts + src/server/middleware/auth.ts, src/server/services/auth.service.ts, src/server/services/oauth.service.ts, src/server/index.ts, src/db/schema.ts + + 1. **Update `verifyApiKey` in `src/server/services/auth.service.ts`** per D-03/D-07: + Change return type from `Promise` to `Promise<{ userId: number } | null>`. The function queries apiKeys by keyPrefix, verifies the hash, and now returns `{ userId: candidate.userId }` on match or `null` on failure. Also update `createApiKey` to accept and store `userId`, `listApiKeys` to filter by `userId`, and `deleteApiKey` to filter by `userId` (using `and(eq(apiKeys.id, id), eq(apiKeys.userId, userId))`). + + 2. **Update `verifyAccessToken` in `src/server/services/oauth.service.ts`**: + Change return type from `Promise` to `Promise<{ userId: number } | null>`. Select `userId` from the oauthTokens record and return `{ userId: record.userId }` on success, `null` on failure. Also update `createTokens` to accept and store `userId`. + + 3. **Create `getOrCreateUser` function** in `src/server/services/auth.service.ts` per D-01: + ```typescript + export async function getOrCreateUser(db: Db, logtoSub: string): Promise<{ id: number }> { + const [user] = await db + .insert(users) + .values({ logtoSub }) + .onConflictDoUpdate({ + target: users.logtoSub, + set: { logtoSub }, + }) + .returning({ id: users.id }); + return user; + } + ``` + + 4. **Create `getOrCreateUncategorized` helper** in `src/server/services/category.service.ts` (or auth.service.ts): + ```typescript + export async function getOrCreateUncategorized(db: Db, userId: number): Promise { + const [existing] = await db + .select({ id: categories.id }) + .from(categories) + .where(and(eq(categories.userId, userId), eq(categories.name, "Uncategorized"))); + if (existing) return existing.id; + const [created] = await db + .insert(categories) + .values({ name: "Uncategorized", icon: "package", userId }) + .returning({ id: categories.id }); + return created.id; + } + ``` + Place this in `category.service.ts` since it's category-related. + + 5. **Rewrite `requireAuth` in `src/server/middleware/auth.ts`** per D-03/D-10: + - For API key auth: call `verifyApiKey(db, apiKey)` which now returns `{ userId } | null`. On success, `c.set("userId", result.userId)` and call `next()`. + - For OAuth Bearer: call `verifyAccessToken(db, token)` which now returns `{ userId } | null`. On success, `c.set("userId", result.userId)`. + - For OIDC session: call `getAuth(c)` for the sub claim, then `getOrCreateUser(db, auth.sub)` to get the local userId. Then call `getOrCreateUncategorized(db, user.id)` to ensure the user has a default category. Set `c.set("userId", user.id)`. + - Import `getOrCreateUser` from auth.service and `getOrCreateUncategorized` from category.service. + + 6. **Update auth middleware configuration in `src/server/index.ts`** per Research pitfall 2: + Change the `/api/*` middleware from: + ```typescript + if (c.req.method === "GET") return next(); + ``` + to apply `requireAuth` to ALL methods on data routes (remove the GET bypass). This ensures userId is always available on context for read operations. Keep the `/api/auth` bypass and add a bypass for `/api/health`. + + The new middleware block should be: + ```typescript + app.use("/api/*", async (c, next) => { + if (c.req.path.startsWith("/api/auth")) return next(); + if (c.req.path === "/api/health") return next(); + return requireAuth(c, next); + }); + ``` + + + grep -c "c.set(\"userId\"" src/server/middleware/auth.ts && grep "Promise<{ userId: number } | null>" src/server/services/auth.service.ts | wc -l && grep "Promise<{ userId: number } | null>" src/server/services/oauth.service.ts | wc -l && grep -c "getOrCreateUser" src/server/services/auth.service.ts && ! grep "GET.*return next" src/server/index.ts + + + - `src/server/middleware/auth.ts` calls `c.set("userId", ...)` in all three auth paths (API key, Bearer, OIDC) + - `verifyApiKey` in auth.service.ts has return type `Promise<{ userId: number } | null>` + - `verifyAccessToken` in oauth.service.ts has return type `Promise<{ userId: number } | null>` + - `getOrCreateUser` function exists in auth.service.ts with `onConflictDoUpdate` pattern + - `getOrCreateUncategorized` function exists in category.service.ts + - `src/server/index.ts` does NOT contain `if (c.req.method === "GET") return next()` + - `src/server/index.ts` still bypasses auth for `/api/auth` and `/api/health` paths + + Auth middleware resolves userId for all auth methods, verifyApiKey and verifyAccessToken return userId, GET routes require auth, getOrCreateUser and getOrCreateUncategorized helpers exist + + + + Task 3: Update test helper to seed user and return { db, userId } + tests/helpers/db.ts + tests/helpers/db.ts, src/db/schema.ts + + Update `createTestDb()` in `tests/helpers/db.ts` to: + 1. After running migrations, insert a test user: `await db.insert(schema.users).values({ logtoSub: "test-user-sub" }).returning()` + 2. Insert the per-user Uncategorized category with the test user's ID: `await db.insert(schema.categories).values({ name: "Uncategorized", icon: "package", userId: user.id })` + 3. Change the return type from just `db` to `{ db, userId: user.id }` so all tests can destructure it. + 4. Also add a helper function `createSecondTestUser(db)` that creates a second user for cross-user isolation tests: + ```typescript + export async function createSecondTestUser(db: Db) { + const [user] = await db + .insert(schema.users) + .values({ logtoSub: "test-user-2-sub" }) + .returning(); + await db + .insert(schema.categories) + .values({ name: "Uncategorized", icon: "package", userId: user.id }); + return user.id; + } + ``` + + The updated `createTestDb` should look like: + ```typescript + export async function createTestDb() { + const db = drizzle({ schema }); + await migrate(db, { migrationsFolder: "./drizzle-pg" }); + + const [user] = await db + .insert(schema.users) + .values({ logtoSub: "test-user-sub" }) + .returning(); + + await db + .insert(schema.categories) + .values({ name: "Uncategorized", icon: "package", userId: user.id }); + + return { db, userId: user.id }; + } + ``` + + IMPORTANT: This changes the return type of createTestDb from `db` to `{ db, userId }`. All existing test files that call `createTestDb()` will need updating in Plan 04 to destructure the result. + + + grep -c "logtoSub" tests/helpers/db.ts && grep -c "userId: user.id" tests/helpers/db.ts && grep "return { db, userId" tests/helpers/db.ts | wc -l && grep -c "createSecondTestUser" tests/helpers/db.ts + + + - `createTestDb()` inserts a user with `logtoSub: "test-user-sub"` + - `createTestDb()` returns `{ db, userId: user.id }` (not just `db`) + - Uncategorized category is created with the test user's ID + - `createSecondTestUser` function exists and is exported + - Import of `schema.users` is present + + Test helper seeds a user, returns { db, userId }, has a createSecondTestUser helper for isolation tests + + + + + +After all tasks complete: +1. `grep "pgTable" src/db/schema.ts` shows pg-core usage throughout +2. `grep "export const users" src/db/schema.ts` confirms users table +3. `grep "userId" src/db/schema.ts` shows userId on items, categories, threads, setups, settings, apiKeys, oauthTokens +4. `grep "c.set(\"userId\"" src/server/middleware/auth.ts` shows userId set in middleware +5. `grep "getOrCreateUser" src/server/services/auth.service.ts` confirms user upsert helper +6. `grep "return { db, userId" tests/helpers/db.ts` confirms new test helper return type +7. No `sqliteTable` imports remain in schema.ts + + + +- Schema uses pg-core imports exclusively (no sqlite-core) +- users table with id, logtoSub (unique), createdAt defined +- userId FK column present on items, categories, threads, setups, settings, apiKeys, oauthTokens +- categories: composite unique on (userId, name) +- settings: composite PK on (userId, key) +- requireAuth resolves userId for API key, Bearer token, and OIDC session +- verifyApiKey and verifyAccessToken return { userId } | null +- Test helper returns { db, userId } with seeded user +- All API routes require auth (no GET bypass) +- Drizzle migration generated in drizzle-pg/ + + + +After completion, create `.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md` + diff --git a/.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md b/.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md new file mode 100644 index 0000000..176c553 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md @@ -0,0 +1,145 @@ +--- +phase: 16-multi-user-data-model +plan: 01 +subsystem: database +tags: [drizzle, pgTable, multi-user, userId, postgresql, auth-middleware] + +requires: + - phase: 14-postgresql-migration + provides: PostgreSQL infrastructure and PGlite test setup + - phase: 15-external-authentication + provides: OIDC auth via Logto, API key and OAuth Bearer auth methods +provides: + - users table with logtoSub for OIDC mapping + - userId FK columns on all entity tables (items, categories, threads, setups, apiKeys, oauthTokens) + - composite unique constraint on categories(userId, name) + - composite primary key on settings(userId, key) + - requireAuth middleware resolving userId onto Hono context + - getOrCreateUser upsert function for OIDC login + - getOrCreateUncategorized lazy category creation + - test helper returning { db, userId } with seeded user +affects: [16-02, 16-03, 16-04, services, routes, mcp-tools, tests] + +tech-stack: + added: [] + patterns: [userId-on-context, per-user-data-isolation, lazy-uncategorized-creation, upsert-on-first-login] + +key-files: + created: + - drizzle-pg.config.ts + - drizzle-pg/0000_thankful_loners.sql + modified: + - src/db/schema.ts + - src/db/seed.ts + - src/server/middleware/auth.ts + - src/server/services/auth.service.ts + - src/server/services/oauth.service.ts + - src/server/services/category.service.ts + - src/server/index.ts + - tests/helpers/db.ts + +key-decisions: + - "All API routes require auth (no GET bypass) so userId is always available for per-user scoping" + - "OAuth service functions converted from sync (.get/.run) to async (await) for pg compatibility" + - "getOrCreateUncategorized placed in category.service.ts since it is category-related" + +patterns-established: + - "userId resolution: requireAuth sets c.set('userId', ...) for all three auth methods" + - "verifyApiKey/verifyAccessToken return { userId } | null instead of boolean" + - "createTestDb returns { db, userId } -- all tests must destructure" + - "Lazy per-user Uncategorized category creation on first OIDC login" + +requirements-completed: [MULTI-01, MULTI-04, MULTI-06] + +duration: 8min +completed: 2026-04-05 +--- + +# Phase 16 Plan 01: Multi-User Data Model Foundation Summary + +**pgTable schema with users table, userId FK on 6 entity tables, composite constraints, and auth middleware resolving userId for all auth methods** + +## Performance + +- **Duration:** 8 min +- **Started:** 2026-04-05T08:31:24Z +- **Completed:** 2026-04-05T08:39:00Z +- **Tasks:** 3 +- **Files modified:** 10 + +## Accomplishments +- Migrated entire schema.ts from sqlite-core to pg-core (pgTable, serial, timestamp, doublePrecision) +- Added users table with logtoSub unique identifier for OIDC mapping and userId FK to items, categories, threads, setups, apiKeys, oauthTokens +- Auth middleware now resolves userId for API key, Bearer token, and OIDC session; all routes require auth +- Test infrastructure returns { db, userId } with seeded user and createSecondTestUser helper + +## Task Commits + +Each task was committed atomically: + +1. **Task 1: Migrate schema.ts to pgTable and add users table + userId columns** - `91e93a3` (feat) +2. **Task 2: Update auth middleware and auth services to resolve userId** - `b6d562f` (feat) +3. **Task 3: Update test helper to seed user and return { db, userId }** - `050478c` (feat) + +## Files Created/Modified +- `src/db/schema.ts` - Rewritten from sqlite-core to pg-core; users table, userId columns, composite constraints +- `src/db/seed.ts` - Emptied global seed; per-user categories created lazily +- `src/server/middleware/auth.ts` - Rewritten to resolve userId for all 3 auth methods +- `src/server/services/auth.service.ts` - Rewritten: getOrCreateUser, verifyApiKey returns userId, scoped API key CRUD +- `src/server/services/oauth.service.ts` - Rewritten: all functions async, verifyAccessToken returns userId, generateTokens accepts userId +- `src/server/services/category.service.ts` - Added getOrCreateUncategorized helper +- `src/server/index.ts` - Removed GET bypass; all API routes require auth +- `tests/helpers/db.ts` - PGlite-based, seeds user, returns { db, userId }, createSecondTestUser helper +- `drizzle-pg.config.ts` - Drizzle config for PostgreSQL dialect +- `drizzle-pg/0000_thankful_loners.sql` - Generated migration with full schema + +## Decisions Made +- All API routes require auth (removed GET bypass) so userId is always available on context for per-user data scoping +- OAuth service functions converted from synchronous (.get/.run/.all) to async/await for PostgreSQL compatibility +- getOrCreateUncategorized placed in category.service.ts since it is category-domain logic +- Old user/session management functions removed from auth.service.ts (replaced by Logto OIDC) + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 2 - Missing Critical] Converted all oauth.service.ts functions to async** +- **Found during:** Task 2 (auth service updates) +- **Issue:** All oauth.service functions used synchronous .get()/.run()/.all() calls from bun-sqlite; these do not work with pg/PGlite which is async-only +- **Fix:** Rewrote all oauth.service functions to use async/await with array destructuring instead of .get() +- **Files modified:** src/server/services/oauth.service.ts +- **Verification:** Code compiles correctly with pg-core types +- **Committed in:** b6d562f (Task 2 commit) + +**2. [Rule 3 - Blocking] Created drizzle-pg.config.ts for migration generation** +- **Found during:** Task 1 (schema migration) +- **Issue:** Existing drizzle.config.ts was SQLite-only; needed PostgreSQL config to generate migrations +- **Fix:** Created drizzle-pg.config.ts pointing to drizzle-pg/ output directory +- **Files modified:** drizzle-pg.config.ts (new) +- **Verification:** Migration generated successfully with 12 tables +- **Committed in:** 91e93a3 (Task 1 commit) + +--- + +**Total deviations:** 2 auto-fixed (1 missing critical, 1 blocking) +**Impact on plan:** Both fixes essential for pg compatibility. No scope creep. + +## Issues Encountered +None + +## Known Stubs +None - all data model changes are structural (schema, middleware, test infrastructure). No UI rendering involved. + +## User Setup Required +None - no external service configuration required. + +## Next Phase Readiness +- Schema foundation complete with users table and userId columns on all entity tables +- Auth middleware resolves userId for all auth methods +- Test helper ready with seeded user +- Next: Plan 16-02 updates all service files to accept userId parameter and filter queries +- Note: createTestDb return type changed from `db` to `{ db, userId }` -- existing tests will need updating in Plan 16-04 + +--- +*Phase: 16-multi-user-data-model* +*Completed: 2026-04-05* diff --git a/.planning/phases/16-multi-user-data-model/16-02-PLAN.md b/.planning/phases/16-multi-user-data-model/16-02-PLAN.md new file mode 100644 index 0000000..9c9ea78 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-02-PLAN.md @@ -0,0 +1,254 @@ +--- +phase: 16-multi-user-data-model +plan: 02 +type: execute +wave: 2 +depends_on: ["16-01"] +files_modified: + - src/server/services/item.service.ts + - src/server/services/category.service.ts + - src/server/services/thread.service.ts + - src/server/services/setup.service.ts + - src/server/services/totals.service.ts + - src/server/services/csv.service.ts + - src/server/services/auth.service.ts +autonomous: true +requirements: + - MULTI-01 + - MULTI-02 + - MULTI-03 + - MULTI-06 + +must_haves: + truths: + - "Every service function that reads or writes user data accepts a userId parameter" + - "All queries filter by userId using eq(table.userId, userId)" + - "Get-by-id queries use and(eq(table.id, id), eq(table.userId, userId)) to prevent cross-user access" + - "Category operations respect composite unique (userId, name)" + - "Settings operations use composite PK (userId, key)" + - "Thread resolution creates the new item with the same userId as the thread" + - "CSV import scopes category creation and lookup to the importing user" + - "API key CRUD is scoped to the owning user" + artifacts: + - path: "src/server/services/item.service.ts" + provides: "User-scoped item CRUD" + contains: "userId: number" + - path: "src/server/services/category.service.ts" + provides: "User-scoped category CRUD with composite unique" + contains: "userId: number" + - path: "src/server/services/thread.service.ts" + provides: "User-scoped thread + candidate CRUD + resolution" + contains: "userId: number" + - path: "src/server/services/setup.service.ts" + provides: "User-scoped setup CRUD + item sync validation" + contains: "userId: number" + - path: "src/server/services/totals.service.ts" + provides: "User-scoped aggregate queries" + contains: "userId: number" + - path: "src/server/services/csv.service.ts" + provides: "User-scoped CSV import/export" + contains: "userId: number" + key_links: + - from: "src/server/services/thread.service.ts" + to: "src/db/schema.ts" + via: "userId on insert(items) during thread resolution" + pattern: "userId.*resolv" + - from: "src/server/services/setup.service.ts" + to: "src/db/schema.ts" + via: "validates item ownership before sync" + pattern: "eq.*items.userId.*userId" + - from: "src/server/services/category.service.ts" + to: "src/db/schema.ts" + via: "getOrCreateUncategorized uses composite unique" + pattern: "getOrCreateUncategorized" +--- + + +Add userId parameter to every service function and scope all database queries to the authenticated user. + +Purpose: Services are the data access layer. Scoping them to userId is the core of multi-user data isolation (MULTI-02). Without this, routes and MCP tools have no way to enforce per-user boundaries. + +Output: All 7 service files updated with userId parameter on every function, all queries filtered by userId. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/phases/16-multi-user-data-model/16-CONTEXT.md +@.planning/phases/16-multi-user-data-model/16-RESEARCH.md +@.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md +@src/db/schema.ts +@src/server/services/item.service.ts +@src/server/services/category.service.ts +@src/server/services/thread.service.ts +@src/server/services/setup.service.ts +@src/server/services/totals.service.ts +@src/server/services/csv.service.ts +@src/server/services/auth.service.ts + + + + + + +Expected new signatures (all gain userId as second param): +- item.service.ts: getAllItems(db, userId), getItemById(db, userId, id), createItem(db, userId, data), updateItem(db, userId, id, data), deleteItem(db, userId, id) +- category.service.ts: getAllCategories(db, userId), getCategoryById(db, userId, id), createCategory(db, userId, data), updateCategory(db, userId, id, data), deleteCategory(db, userId, id) + - getOrCreateUncategorized(db, userId) already exists from Plan 01 +- thread.service.ts: getAllThreads(db, userId), getThreadById(db, userId, id), createThread(db, userId, data), updateThread(db, userId, id, data), deleteThread(db, userId, id), resolveThread(db, userId, id, candidateId), addCandidate(db, userId, ...), updateCandidate(db, userId, ...), removeCandidate(db, userId, ...) +- setup.service.ts: getAllSetups(db, userId), getSetupById(db, userId, id), createSetup(db, userId, data), updateSetup(db, userId, id, data), deleteSetup(db, userId, id), syncSetupItems(db, userId, setupId, items) +- totals.service.ts: getTotals(db, userId) +- csv.service.ts: importItemsCsv(db, userId, data), exportItemsCsv(db, userId) +- auth.service.ts: createApiKey(db, userId, name), listApiKeys(db, userId), deleteApiKey(db, userId, id) — verifyApiKey already updated in Plan 01 + + + + + + + Task 1: Update item, category, totals, and CSV services with userId scoping + src/server/services/item.service.ts, src/server/services/category.service.ts, src/server/services/totals.service.ts, src/server/services/csv.service.ts + src/server/services/item.service.ts, src/server/services/category.service.ts, src/server/services/totals.service.ts, src/server/services/csv.service.ts, src/db/schema.ts + + **item.service.ts** per D-09: + - Add `userId: number` as second parameter to ALL exported functions + - Remove default `db` parameter values (no more `db: Db = prodDb`) -- db is always injected + - `getAllItems`: add `.where(eq(items.userId, userId))` + - `getItemById`: change `.where(eq(items.id, id))` to `.where(and(eq(items.id, id), eq(items.userId, userId)))` -- CRITICAL for isolation per Research anti-pattern + - `createItem`: include `userId` in the `.values({...})` insert + - `updateItem`: add `eq(items.userId, userId)` to the `.where()` clause using `and()` + - `deleteItem`: add `eq(items.userId, userId)` to the `.where()` clause using `and()` + - Import `and` from `drizzle-orm` if not already imported + + **category.service.ts** per D-05/D-09: + - Add `userId: number` as second parameter to ALL exported functions + - Remove default `db` parameter values + - `getAllCategories`: add `.where(eq(categories.userId, userId))` + - `getCategoryById`: use `and(eq(categories.id, id), eq(categories.userId, userId))` + - `createCategory`: include `userId` in the insert values + - `updateCategory`: add userId filter with `and()` + - `deleteCategory`: add userId filter with `and()`. When reassigning items to Uncategorized on delete, use `getOrCreateUncategorized(db, userId)` instead of hardcoded category ID 1. Also scope the item reassignment to only items belonging to this user. + - The `getOrCreateUncategorized` function was already created in Plan 01 + + **totals.service.ts** per D-09: + - Add `userId: number` parameter + - Filter all aggregate queries by userId + - This file computes weight/cost totals across the collection -- must only sum the user's items + + **csv.service.ts** per D-09 and Research pitfall 7: + - Add `userId: number` parameter to `importItemsCsv` and `exportItemsCsv` + - `exportItemsCsv`: filter items query by userId + - `importItemsCsv`: + - Category lookup/creation must filter by userId (use `getOrCreateUncategorized` for fallback) + - When creating new categories from CSV data, include userId + - When creating items, include userId + - Category name matching must be scoped to user's categories + + + grep -c "userId: number" src/server/services/item.service.ts && grep -c "userId: number" src/server/services/category.service.ts && grep -c "userId: number" src/server/services/totals.service.ts && grep -c "userId: number" src/server/services/csv.service.ts && grep "and(" src/server/services/item.service.ts | wc -l + + + - Every exported function in item.service.ts has `userId: number` parameter + - Every exported function in category.service.ts has `userId: number` parameter + - totals.service.ts functions have `userId: number` parameter + - csv.service.ts import/export functions have `userId: number` parameter + - `getItemById` uses `and(eq(items.id, id), eq(items.userId, userId))` (not just eq on id) + - `deleteCategory` uses `getOrCreateUncategorized(db, userId)` not hardcoded ID + - `importItemsCsv` scopes category operations to userId + - No `= prodDb` default parameter values remain + - `and` imported from `drizzle-orm` in all files that use it + + Item, category, totals, and CSV services accept userId and scope all queries to the authenticated user. Get-by-id uses and() for isolation. + + + + Task 2: Update thread, setup, settings, and auth services with userId scoping + src/server/services/thread.service.ts, src/server/services/setup.service.ts, src/server/services/auth.service.ts + src/server/services/thread.service.ts, src/server/services/setup.service.ts, src/server/services/auth.service.ts, src/db/schema.ts + + **thread.service.ts** per D-09 and Research pitfall 6: + - Add `userId: number` as second parameter to ALL exported functions + - Remove default `db` parameter values + - `getAllThreads`: add `.where(eq(threads.userId, userId))` + - `getThreadById`: use `and(eq(threads.id, id), eq(threads.userId, userId))` + - `createThread`: include `userId` in the insert values + - `updateThread`: add userId filter + - `deleteThread`: add userId filter + - `resolveThread`: CRITICAL -- verify the thread belongs to the user before resolving. When creating the new item from the winning candidate, include `userId` in the `insert(items).values({...})`. Also verify the target category belongs to the user. Use `getOrCreateUncategorized(db, userId)` if category fallback is needed. + - `addCandidate`: verify the parent thread belongs to the user before inserting candidate + - `updateCandidate`: verify the parent thread belongs to the user (join or subquery) + - `removeCandidate`: verify the parent thread belongs to the user + + For candidate operations, the pattern should be: + 1. Look up the thread with userId filter: `and(eq(threads.id, threadId), eq(threads.userId, userId))` + 2. If thread not found, return null/throw (the thread doesn't exist for this user) + 3. Proceed with candidate operation on the verified thread + + **setup.service.ts** per D-09 and Research pitfall 8: + - Add `userId: number` as second parameter to ALL exported functions + - `getAllSetups`: add `.where(eq(setups.userId, userId))` + - `getSetupById`: use `and(eq(setups.id, id), eq(setups.userId, userId))` + - `createSetup`: include `userId` in insert values + - `updateSetup`: add userId filter + - `deleteSetup`: add userId filter + - `syncSetupItems`: CRITICAL -- verify the setup belongs to the user AND verify each itemId belongs to the user before inserting into setupItems. Filter the incoming item list against user-owned items: + ```typescript + const userItemIds = await db.select({ id: items.id }).from(items) + .where(and(eq(items.userId, userId), inArray(items.id, itemIds))); + // Only insert items that belong to this user + ``` + + **auth.service.ts** per D-07: + - `createApiKey`: add `userId: number` parameter, include userId in insert values + - `listApiKeys`: add `userId: number` parameter, filter by `.where(eq(apiKeys.userId, userId))` + - `deleteApiKey`: add `userId: number` parameter, filter by `and(eq(apiKeys.id, id), eq(apiKeys.userId, userId))` to prevent deleting another user's API key + - `verifyApiKey` was already updated in Plan 01 to return `{ userId } | null` + - `getOrCreateUser` was already created in Plan 01 + + + grep -c "userId: number" src/server/services/thread.service.ts && grep -c "userId: number" src/server/services/setup.service.ts && grep -c "userId: number" src/server/services/auth.service.ts && grep "and(" src/server/services/thread.service.ts | wc -l && grep "and(" src/server/services/setup.service.ts | wc -l + + + - Every exported function in thread.service.ts has `userId: number` parameter + - Every exported function in setup.service.ts has `userId: number` parameter + - `createApiKey`, `listApiKeys`, `deleteApiKey` in auth.service.ts have `userId: number` parameter + - `resolveThread` includes `userId` in the `insert(items).values({...})` call + - `resolveThread` verifies thread ownership before resolving + - Candidate operations (add, update, remove) verify parent thread ownership + - `syncSetupItems` verifies both setup and item ownership + - `getThreadById` uses `and(eq(threads.id, id), eq(threads.userId, userId))` + - `getSetupById` uses `and(eq(setups.id, id), eq(setups.userId, userId))` + - No `= prodDb` default parameter values remain + + Thread, setup, and auth services accept userId and scope all queries. Thread resolution and setup sync validate ownership. Candidate operations verify parent thread belongs to user. + + + + + +After all tasks complete: +1. `grep -r "userId: number" src/server/services/ | wc -l` shows userId parameter across all service files +2. `grep -r "= prodDb" src/server/services/` returns no matches (no default db params) +3. `grep -r "and(eq" src/server/services/` shows isolation on get-by-id queries +4. No service function reads or writes user-owned data without userId filtering + + + +- All 7 service files accept userId as a parameter +- All queries filter by userId (no unscoped reads or writes) +- Get-by-id, update, and delete operations use and() to combine id + userId conditions +- Thread resolution includes userId on new item creation +- Setup item sync validates item ownership +- Category deletion uses dynamic Uncategorized lookup (not hardcoded ID) +- CSV import scopes all operations to the importing user +- API key CRUD is user-scoped + + + +After completion, create `.planning/phases/16-multi-user-data-model/16-02-SUMMARY.md` + diff --git a/.planning/phases/16-multi-user-data-model/16-03-PLAN.md b/.planning/phases/16-multi-user-data-model/16-03-PLAN.md new file mode 100644 index 0000000..033f960 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-03-PLAN.md @@ -0,0 +1,277 @@ +--- +phase: 16-multi-user-data-model +plan: 03 +type: execute +wave: 3 +depends_on: ["16-01", "16-02"] +files_modified: + - src/server/routes/items.ts + - src/server/routes/categories.ts + - src/server/routes/threads.ts + - src/server/routes/setups.ts + - src/server/routes/settings.ts + - src/server/routes/totals.ts + - src/server/routes/auth.ts + - src/server/routes/images.ts + - src/server/mcp/index.ts + - src/server/mcp/tools/items.ts + - src/server/mcp/tools/categories.ts + - src/server/mcp/tools/threads.ts + - src/server/mcp/tools/setups.ts + - src/server/mcp/resources/collection.ts +autonomous: true +requirements: + - MULTI-02 + - MULTI-05 + - MULTI-06 + +must_haves: + truths: + - "Every route handler extracts userId from context and passes it to service functions" + - "Settings routes use userId for per-user settings" + - "MCP tools receive userId and pass it to service functions" + - "MCP server is created with userId from the authenticated session" + - "No route calls a service function without passing userId" + artifacts: + - path: "src/server/routes/items.ts" + provides: "User-scoped item routes" + contains: "c.get(\"userId\")" + - path: "src/server/routes/settings.ts" + provides: "Per-user settings routes" + contains: "c.get(\"userId\")" + - path: "src/server/mcp/index.ts" + provides: "MCP server with userId threading" + contains: "createMcpServer(db, userId)" + key_links: + - from: "src/server/routes/items.ts" + to: "src/server/services/item.service.ts" + via: "userId passed from context to service" + pattern: "getAllItems.*db.*userId" + - from: "src/server/mcp/index.ts" + to: "src/server/mcp/tools/items.ts" + via: "userId passed to registerItemTools" + pattern: "registerItemTools.*db.*userId" +--- + + +Wire userId from Hono context into all route handlers and MCP tool registrations, completing the multi-user data isolation chain. + +Purpose: Routes are the HTTP boundary. They extract userId (set by requireAuth middleware in Plan 01) and pass it to services (updated in Plan 02). MCP tools are the programmatic boundary. Together they ensure every data operation is scoped to the authenticated user. + +Output: All route files and MCP tools pass userId to service calls. Settings use per-user composite key. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/phases/16-multi-user-data-model/16-CONTEXT.md +@.planning/phases/16-multi-user-data-model/16-RESEARCH.md +@.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md +@.planning/phases/16-multi-user-data-model/16-02-SUMMARY.md +@src/server/routes/items.ts +@src/server/routes/categories.ts +@src/server/routes/threads.ts +@src/server/routes/setups.ts +@src/server/routes/settings.ts +@src/server/routes/totals.ts +@src/server/routes/auth.ts +@src/server/routes/images.ts +@src/server/mcp/index.ts +@src/server/mcp/tools/items.ts +@src/server/mcp/tools/categories.ts +@src/server/mcp/tools/threads.ts +@src/server/mcp/tools/setups.ts + + + + + +Route pattern (what every handler must do): +```typescript +app.get("/", async (c) => { + const db = c.get("db"); + const userId = c.get("userId"); + const result = await serviceFunction(db, userId, ...); + return c.json(result); +}); +``` + +MCP pattern (what must change): +```typescript +// Before: createMcpServer(db: Db) +// After: createMcpServer(db: Db, userId: number) +// Before: registerItemTools(db) +// After: registerItemTools(db, userId) +``` + +Settings pattern (composite key): +```typescript +// Before: eq(settings.key, key) +// After: and(eq(settings.userId, userId), eq(settings.key, key)) +// Insert with onConflict must target [settings.userId, settings.key] +``` + + + + + + + Task 1: Update all route handlers to extract and pass userId + src/server/routes/items.ts, src/server/routes/categories.ts, src/server/routes/threads.ts, src/server/routes/setups.ts, src/server/routes/settings.ts, src/server/routes/totals.ts, src/server/routes/auth.ts, src/server/routes/images.ts + src/server/routes/items.ts, src/server/routes/categories.ts, src/server/routes/threads.ts, src/server/routes/setups.ts, src/server/routes/settings.ts, src/server/routes/totals.ts, src/server/routes/auth.ts, src/server/routes/images.ts + + For EVERY route handler in EVERY route file, add `const userId = c.get("userId");` after the `const db = c.get("db");` line, then pass `userId` to every service function call. + + **items.ts**: Extract userId, pass to getAllItems(db, userId), getItemById(db, userId, id), createItem(db, userId, data), updateItem(db, userId, id, data), deleteItem(db, userId, id). + + **categories.ts**: Extract userId, pass to getAllCategories(db, userId), getCategoryById(db, userId, id), createCategory(db, userId, data), updateCategory(db, userId, id, data), deleteCategory(db, userId, id). + + **threads.ts**: Extract userId, pass to all thread service calls including addCandidate, updateCandidate, removeCandidate, resolveThread. + + **setups.ts**: Extract userId, pass to all setup service calls including syncSetupItems. + + **totals.ts**: Extract userId, pass to getTotals(db, userId) or equivalent. + + **settings.ts** per D-06: This route does inline DB queries (no service file). Update to: + - GET `/:key`: Add userId to the where clause: `and(eq(settings.userId, userId), eq(settings.key, key))` + - PUT `/:key`: Update the upsert to use composite conflict target: `.onConflictDoUpdate({ target: [settings.userId, settings.key], set: { value: body.value } })` and include userId in the insert values: `.values({ userId, key, value: body.value })` + - Import `and` from `drizzle-orm` and `settings` from schema + + **auth.ts**: Extract userId, pass to createApiKey(db, userId, name), listApiKeys(db, userId), deleteApiKey(db, userId, id). Auth routes that don't need userId (login, me, setup) can skip it. + + **images.ts**: This route handles image uploads which don't directly involve userId scoping on the images table (images are stored by filename, not in a user-scoped table). However, if the route calls any service that now requires userId, pass it. Read the file first to determine what changes are needed. + + IMPORTANT: The `Env` type annotation on each Hono app may need updating to include `userId` in the Variables type: + ```typescript + type Env = { Variables: { db?: any; userId?: number } }; + ``` + + + for f in src/server/routes/items.ts src/server/routes/categories.ts src/server/routes/threads.ts src/server/routes/setups.ts src/server/routes/settings.ts src/server/routes/totals.ts src/server/routes/auth.ts; do echo "$f: $(grep -c 'c.get("userId")' $f)"; done + + + - Every route handler in items.ts, categories.ts, threads.ts, setups.ts, totals.ts, auth.ts contains `c.get("userId")` + - Every service function call includes userId as the second argument + - settings.ts uses `and(eq(settings.userId, userId), eq(settings.key, key))` for reads + - settings.ts upsert targets `[settings.userId, settings.key]` for composite conflict + - settings.ts insert includes userId in values + - Env type includes `userId` in Variables + - No service call is missing the userId parameter + + All route handlers extract userId from context and pass to every service call. Settings routes use composite key. + + + + Task 2: Update MCP server and tool registrations with userId + src/server/mcp/index.ts, src/server/mcp/tools/items.ts, src/server/mcp/tools/categories.ts, src/server/mcp/tools/threads.ts, src/server/mcp/tools/setups.ts, src/server/mcp/resources/collection.ts + src/server/mcp/index.ts, src/server/mcp/tools/items.ts, src/server/mcp/tools/categories.ts, src/server/mcp/tools/threads.ts, src/server/mcp/tools/setups.ts, src/server/mcp/resources/collection.ts + + Per D-13 and Research pitfall 5: + + 1. **Update `createMcpServer` signature** in `src/server/mcp/index.ts`: + Change from `createMcpServer(db: Db)` to `createMcpServer(db: Db, userId: number)`. + Pass userId to all `register*Tools` calls: + - `registerItemTools(db, userId)` + - `registerCategoryTools(db, userId)` + - `registerThreadTools(db, userId)` + - `registerSetupTools(db, userId)` + - `getCollectionSummary(db, userId)` + (registerImageTools has no db/userId dependency so leave unchanged) + + 2. **Update MCP auth middleware** to resolve userId: + The MCP auth middleware in `mcpRoutes.use("/*", ...)` currently calls `verifyAccessToken` and `verifyApiKey` which now return `{ userId } | null`. Store the userId and make it available to the POST handler. + + Use the Hono context to pass userId, similar to the main API middleware: + ```typescript + mcpRoutes.use("/*", async (c, next) => { + const db = c.get("db") ?? prodDb; + + // Try Bearer token first (OAuth) + const authHeader = c.req.header("Authorization"); + if (authHeader?.startsWith("Bearer ")) { + const token = authHeader.slice(7); + const result = await verifyAccessToken(db, token); + if (result) { + c.set("userId", result.userId); + return next(); + } + return c.json({ error: "invalid_token" }, 401); + } + + // Try API key + const apiKey = c.req.header("X-API-Key"); + if (apiKey) { + const result = await verifyApiKey(db, apiKey); + if (result) { + c.set("userId", result.userId); + return next(); + } + return c.json({ error: "Invalid API key" }, 401); + } + // ... rest of auth handling + }); + ``` + + 3. **Update MCP POST handler** to pass userId when creating MCP server: + In the `mcpRoutes.post("/", ...)` handler, extract userId from context and pass to createMcpServer: + ```typescript + const userId = c.get("userId"); + const server = createMcpServer(db, userId); + ``` + + 4. **Store userId alongside transport** in the session map per Research pitfall 5: + Change `transports` map type from `Map` to `Map`. + When reusing an existing session, extract userId from the stored session data (no need to recreate MCP server -- the session was already initialized with the correct userId). + + 5. **Update each tool registration file** to accept and use userId: + - `src/server/mcp/tools/items.ts`: `registerItemTools(db: Db, userId: number)` -- pass userId to all item service calls + - `src/server/mcp/tools/categories.ts`: `registerCategoryTools(db: Db, userId: number)` -- pass userId to all category service calls + - `src/server/mcp/tools/threads.ts`: `registerThreadTools(db: Db, userId: number)` -- pass userId to all thread service calls + - `src/server/mcp/tools/setups.ts`: `registerSetupTools(db: Db, userId: number)` -- pass userId to all setup service calls + + 6. **Update `getCollectionSummary`** in `src/server/mcp/resources/collection.ts`: + Add userId parameter, scope the summary queries to the user's data only. + + + grep -c "userId: number" src/server/mcp/index.ts && grep "createMcpServer(db, userId)" src/server/mcp/index.ts | wc -l && grep -c "userId: number" src/server/mcp/tools/items.ts && grep -c "userId: number" src/server/mcp/tools/categories.ts && grep -c "userId: number" src/server/mcp/tools/threads.ts && grep -c "userId: number" src/server/mcp/tools/setups.ts && grep -c "c.set(\"userId\"" src/server/mcp/index.ts + + + - `createMcpServer` accepts `(db: Db, userId: number)` signature + - MCP auth middleware sets `c.set("userId", result.userId)` for both API key and Bearer token auth + - MCP POST handler passes userId to `createMcpServer(db, userId)` + - Transport map stores userId alongside transport + - All 4 tool registration functions accept `(db: Db, userId: number)` + - All tool handlers pass userId to service function calls + - `getCollectionSummary` accepts and uses userId + - No MCP tool calls a service function without userId + + MCP server creation receives userId, all tool registrations pass userId to service calls, MCP auth middleware resolves userId from API key or Bearer token + + + + + +After all tasks complete: +1. `grep -r 'c.get("userId")' src/server/routes/ | wc -l` shows userId extraction in all route files +2. `grep -r 'c.get("userId")' src/server/mcp/ | wc -l` shows userId in MCP middleware +3. `grep "createMcpServer(db, userId)" src/server/mcp/index.ts` confirms MCP userId threading +4. No service call anywhere in routes/ or mcp/ is missing the userId argument + + + +- All route handlers extract userId from context and pass to services +- Settings routes use composite PK for per-user settings +- MCP server creation includes userId +- MCP tool registrations pass userId to all service calls +- MCP auth middleware resolves userId from API key and Bearer token +- Complete chain: middleware sets userId -> routes/MCP extract it -> services filter by it + + + +After completion, create `.planning/phases/16-multi-user-data-model/16-03-SUMMARY.md` + diff --git a/.planning/phases/16-multi-user-data-model/16-04-PLAN.md b/.planning/phases/16-multi-user-data-model/16-04-PLAN.md new file mode 100644 index 0000000..6d2037a --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-04-PLAN.md @@ -0,0 +1,320 @@ +--- +phase: 16-multi-user-data-model +plan: 04 +type: execute +wave: 3 +depends_on: ["16-01", "16-02"] +files_modified: + - tests/services/item.service.test.ts + - tests/services/category.service.test.ts + - tests/services/thread.service.test.ts + - tests/services/setup.service.test.ts + - tests/services/totals.test.ts + - tests/services/csv.service.test.ts + - tests/services/auth.service.test.ts + - tests/services/oauth.service.test.ts + - tests/routes/items.test.ts + - tests/routes/categories.test.ts + - tests/routes/threads.test.ts + - tests/routes/setups.test.ts + - tests/routes/auth.test.ts + - tests/routes/images.test.ts + - tests/routes/oauth.test.ts + - tests/routes/params.test.ts + - tests/mcp/tools.test.ts +autonomous: true +requirements: + - MULTI-02 + - MULTI-04 + - MULTI-05 + +must_haves: + truths: + - "All existing tests pass after updating to use { db, userId } from createTestDb" + - "Service tests pass userId to every service function call" + - "Route tests set userId on the test app context" + - "MCP tests pass userId to MCP server creation" + - "At least one cross-user isolation test exists proving User A cannot see User B's data" + artifacts: + - path: "tests/services/item.service.test.ts" + provides: "User-scoped item service tests" + contains: "userId" + - path: "tests/mcp/tools.test.ts" + provides: "User-scoped MCP tool tests" + contains: "userId" + key_links: + - from: "tests/helpers/db.ts" + to: "tests/services/*.test.ts" + via: "createTestDb returns { db, userId }" + pattern: "const \\{ db, userId \\}" + - from: "tests/services/item.service.test.ts" + to: "src/server/services/item.service.ts" + via: "passes userId to all service calls" + pattern: "getAllItems.*db.*userId" +--- + + +Update all test files to work with the new multi-user data model: destructure { db, userId } from createTestDb(), pass userId to all service calls, set userId in route test contexts, and add cross-user isolation tests. + +Purpose: Tests validate that multi-user isolation works correctly. Without updated tests, we cannot verify that MULTI-02 (cross-user isolation) is enforced. The test suite must pass green before the phase is complete. + +Output: All 17 test files updated, cross-user isolation tests added, full test suite passes. + + + +@$HOME/.claude/get-shit-done/workflows/execute-plan.md +@$HOME/.claude/get-shit-done/templates/summary.md + + + +@.planning/PROJECT.md +@.planning/ROADMAP.md +@.planning/phases/16-multi-user-data-model/16-CONTEXT.md +@.planning/phases/16-multi-user-data-model/16-RESEARCH.md +@.planning/phases/16-multi-user-data-model/16-01-SUMMARY.md +@.planning/phases/16-multi-user-data-model/16-02-SUMMARY.md +@tests/helpers/db.ts +@tests/services/item.service.test.ts +@tests/services/category.service.test.ts +@tests/routes/items.test.ts +@tests/mcp/tools.test.ts + + + + + + + +Test update pattern for service tests: +```typescript +// Before: +let db: any; +beforeEach(async () => { db = await createTestDb(); }); +// ... +const items = await getAllItems(db); + +// After: +let db: any; +let userId: number; +beforeEach(async () => { ({ db, userId } = await createTestDb()); }); +// ... +const items = await getAllItems(db, userId); +``` + +Test update pattern for route tests: +```typescript +// Before: +const testApp = new Hono(); +testApp.use("*", async (c, next) => { c.set("db", db); await next(); }); + +// After: +const testApp = new Hono(); +testApp.use("*", async (c, next) => { c.set("db", db); c.set("userId", userId); await next(); }); +``` + +Cross-user isolation test pattern: +```typescript +import { createSecondTestUser } from "../helpers/db"; + +test("user cannot see other user's items", async () => { + const userId2 = await createSecondTestUser(db); + await createItem(db, userId, { name: "User 1 Item", ... }); + await createItem(db, userId2, { name: "User 2 Item", ... }); + + const user1Items = await getAllItems(db, userId); + const user2Items = await getAllItems(db, userId2); + + expect(user1Items).toHaveLength(1); + expect(user1Items[0].name).toBe("User 1 Item"); + expect(user2Items).toHaveLength(1); + expect(user2Items[0].name).toBe("User 2 Item"); +}); +``` + + + + + + + Task 1: Update all service test files to pass userId + tests/services/item.service.test.ts, tests/services/category.service.test.ts, tests/services/thread.service.test.ts, tests/services/setup.service.test.ts, tests/services/totals.test.ts, tests/services/csv.service.test.ts, tests/services/auth.service.test.ts, tests/services/oauth.service.test.ts + tests/services/item.service.test.ts, tests/services/category.service.test.ts, tests/services/thread.service.test.ts, tests/services/setup.service.test.ts, tests/services/totals.test.ts, tests/services/csv.service.test.ts, tests/services/auth.service.test.ts, tests/services/oauth.service.test.ts, tests/helpers/db.ts + + For EACH of the 8 service test files, apply this systematic transformation: + + 1. **Change createTestDb destructuring** from `db = await createTestDb()` to `({ db, userId } = await createTestDb())`. Add `userId` variable declaration alongside `db`. + + 2. **Add userId to every service function call** as the second argument after `db`. Go through every call to any service function in each test and add `userId`. + + 3. **Fix category references**: Tests that reference "Uncategorized" by hardcoded ID 1 must instead look up the category by name+userId or use the ID returned from the seeded data. The test helper already seeds Uncategorized with the test user's ID. + + 4. **Specific file notes:** + + **item.service.test.ts** (~160 lines): + - Destructure `{ db, userId }` from createTestDb + - Every `getAllItems(db)` becomes `getAllItems(db, userId)` + - Every `getItemById(db, id)` becomes `getItemById(db, userId, id)` + - Every `createItem(db, data)` becomes `createItem(db, userId, data)` + - Every `updateItem(db, id, data)` becomes `updateItem(db, userId, id, data)` + - Every `deleteItem(db, id)` becomes `deleteItem(db, userId, id)` + - ADD a cross-user isolation test: create a second user with `createSecondTestUser(db)`, create items for each user, verify each user only sees their own items, and verify getItemById returns null for another user's item ID + + **category.service.test.ts** (~97 lines): + - Same destructuring pattern + - Add userId to all category service calls + - ADD a test for composite unique constraint: two users can have categories with the same name + - Fix any hardcoded "Uncategorized" ID references + + **thread.service.test.ts** (~523 lines): + - Same destructuring pattern + - Add userId to ALL thread, candidate, and resolve calls + - Candidate operations need userId (for parent thread verification) + - Thread resolution test: verify the created item has the correct userId + - ADD a cross-user isolation test for threads + + **setup.service.test.ts** (~293 lines): + - Same destructuring pattern + - Add userId to all setup service calls + - syncSetupItems calls need userId + - ADD a test that verifies a user cannot add another user's items to their setup + + **totals.test.ts** (~79 lines): + - Same destructuring pattern + - Add userId to getTotals calls + + **csv.service.test.ts** (~196 lines): + - Same destructuring pattern + - Add userId to import/export calls + - CSV import must create categories with userId + + **auth.service.test.ts** (~68 lines): + - Same destructuring pattern + - Add userId to createApiKey, listApiKeys, deleteApiKey calls + - Update verifyApiKey assertions to check for `{ userId }` return instead of boolean `true` + + **oauth.service.test.ts** (~290 lines): + - Same destructuring pattern + - Add userId where needed (createTokens, verifyAccessToken) + - Update verifyAccessToken assertions to check for `{ userId }` return instead of boolean + + 5. **Import `createSecondTestUser`** from helpers/db.ts in files that add isolation tests (at minimum: item, category, thread, setup test files). + + After all files are updated, run `bun test` to verify the full suite passes. + + + bun test tests/services/ 2>&1 | tail -5 + + + - All 8 service test files use `{ db, userId } = await createTestDb()` + - Every service function call in tests includes userId argument + - `item.service.test.ts` has a cross-user isolation test using `createSecondTestUser` + - `category.service.test.ts` has a composite unique constraint test (same name, different users) + - `auth.service.test.ts` checks `verifyApiKey` returns `{ userId }` not boolean + - `oauth.service.test.ts` checks `verifyAccessToken` returns `{ userId }` not boolean + - `bun test tests/services/` passes all tests + + All 8 service test files updated with userId, cross-user isolation tests added, all service tests pass + + + + Task 2: Update route tests, MCP tests, and run full suite + tests/routes/items.test.ts, tests/routes/categories.test.ts, tests/routes/threads.test.ts, tests/routes/setups.test.ts, tests/routes/auth.test.ts, tests/routes/images.test.ts, tests/routes/oauth.test.ts, tests/routes/params.test.ts, tests/mcp/tools.test.ts + tests/routes/items.test.ts, tests/routes/categories.test.ts, tests/routes/threads.test.ts, tests/routes/setups.test.ts, tests/routes/auth.test.ts, tests/routes/images.test.ts, tests/routes/oauth.test.ts, tests/routes/params.test.ts, tests/mcp/tools.test.ts + + **Route tests** (7 files): + + Route tests create a test Hono app with middleware that sets `db` on context. They need TWO changes: + + 1. **Destructure `{ db, userId }`** from createTestDb + 2. **Set userId on the test app context** in the middleware setup: + ```typescript + testApp.use("*", async (c, next) => { + c.set("db", db); + c.set("userId", userId); + await next(); + }); + ``` + + For each route test file, read the file first to understand its middleware setup pattern, then add `c.set("userId", userId)` to the middleware. The route handlers will then pick it up via `c.get("userId")`. + + **Specific notes per file:** + + **items.test.ts** (~207 lines): Set userId in context middleware. Tests should work as-is since routes now pass userId to services. + + **categories.test.ts** (~91 lines): Set userId in context middleware. + + **threads.test.ts** (~413 lines): Set userId in context middleware. Thread tests may reference hardcoded Uncategorized category ID -- fix these. + + **setups.test.ts** (~305 lines): Set userId in context middleware. + + **auth.test.ts** (~139 lines): Set userId in context middleware for API key management routes. Auth-specific routes (login, setup) may not need userId. + + **images.test.ts** (~26 lines): Set userId in context middleware if needed. + + **oauth.test.ts** (~443 lines): May not need userId for OAuth flow tests, but set it for consistency. OAuth token creation may need userId. + + **params.test.ts** (~81 lines): Set userId in context middleware. + + **MCP tests** (1 file): + + **tools.test.ts** (~253 lines): + - Destructure `{ db, userId }` from createTestDb + - Update `createMcpServer(db)` calls to `createMcpServer(db, userId)` + - MCP tool calls should then work since tools now receive userId from the server + - ADD an isolation test: create data as user 1, verify MCP tools as user 2 don't return user 1's data + + After ALL test files are updated, run the FULL test suite: + ```bash + bun test + ``` + + Fix any remaining failures. Common issues to watch for: + - Missing userId argument (TypeScript will flag this) + - Hardcoded category ID 1 (use the seeded category from createTestDb) + - Boolean vs object return type from verifyApiKey/verifyAccessToken + - Settings tests using single key lookup instead of composite + - Route tests not setting userId in context + + Also run lint to ensure no formatting issues: + ```bash + bun run lint + ``` + + + bun test 2>&1 | tail -10 + + + - All route test files set `c.set("userId", userId)` in test middleware + - All route test files use `{ db, userId } = await createTestDb()` + - MCP tools.test.ts passes userId to createMcpServer + - MCP tools.test.ts has a cross-user isolation test + - `bun test` passes ALL tests (0 failures) + - `bun run lint` passes + + All 17 test files updated, full test suite passes green, lint passes, cross-user isolation verified by tests + + + + + +After all tasks complete: +1. `bun test` exits with 0 failures +2. `bun run lint` passes +3. `grep -r "createSecondTestUser" tests/` shows isolation tests exist +4. `grep -r 'c.set("userId"' tests/routes/` shows userId set in all route test middleware +5. `grep "createMcpServer(db, userId)" tests/mcp/tools.test.ts` confirms MCP test update + + + +- Full test suite passes (`bun test` returns 0 failures) +- All service tests pass userId to every service call +- All route tests set userId in test app context +- MCP tests pass userId to createMcpServer +- Cross-user isolation tests exist for items, categories, threads, setups +- Composite unique constraint test exists for categories +- Lint passes + + + +After completion, create `.planning/phases/16-multi-user-data-model/16-04-SUMMARY.md` + diff --git a/.planning/phases/16-multi-user-data-model/16-CONTEXT.md b/.planning/phases/16-multi-user-data-model/16-CONTEXT.md new file mode 100644 index 0000000..042ffe7 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-CONTEXT.md @@ -0,0 +1,126 @@ +# Phase 16: Multi-User Data Model - Context + +**Gathered:** 2026-04-05 +**Status:** Ready for planning + + +## Phase Boundary + +Add user ownership to all user-created entities (items, categories, threads, setups, settings) and enforce complete cross-user data isolation. Every query must be scoped to the authenticated user. MCP tools operate within the authenticated user's scope. + + + + +## Implementation Decisions + +### User Identity Storage +- **D-01:** Create a thin local `users` table: `id` (serial integer PK), `logtoSub` (text, unique, not null), `createdAt` (timestamp). Auto-created on first OIDC login via upsert. +- **D-02:** All entity tables reference `users.id` (integer FK) — not the Logto sub string directly. Integer FKs are more efficient for joins across 6+ tables. +- **D-03:** The `requireAuth` middleware resolves the authenticated identity to a local `users.id` and sets it on the Hono context (e.g., `c.set("userId", userId)`). + +### Schema Changes +- **D-04:** Add `userId` (integer, NOT NULL, FK → users.id) column to: `items`, `categories`, `threads`, `setups`, `settings`, `apiKeys`. +- **D-05:** `categories`: Drop global unique constraint on `name`. Add composite unique constraint on `(userId, name)`. Each user gets their own "Uncategorized" default category. +- **D-06:** `settings`: Change primary key from `key` alone to composite `(userId, key)`. Each user has their own settings (weightUnit, etc.). +- **D-07:** `apiKeys`: Add `userId` column so middleware can resolve which user's data an API key grants access to. +- **D-08:** `threadCandidates` and `setupItems`: No userId needed — they inherit ownership through their parent thread/setup FK. + +### Service Layer Changes +- **D-09:** Every service function that reads or writes user-owned data gains a `userId` parameter. All queries include `where(eq(table.userId, userId))` for isolation. +- **D-10:** `requireAuth` middleware sets `userId` on context. Routes extract `userId` from context and pass to services. + +### Data Migration +- **D-11:** Migration script adds `userId` column with a temporary default, then updates all existing rows to user ID 1 (the first registered user), then removes the default and sets NOT NULL. +- **D-12:** Create "Uncategorized" category per-user on first login (or lazily when needed). + +### MCP Tool Scoping +- **D-13:** MCP tools resolve userId from the authenticated token (API key → userId lookup, or Bearer token → userId). All tool operations are scoped to that user. + +### Claude's Discretion +- Exact migration SQL approach (single migration vs multi-step) +- Whether to use Drizzle's `.where()` chaining or a helper function for userId scoping +- Default category creation strategy (eager on first login vs lazy on first item creation) +- Whether thread resolution should check that the target category belongs to the same user +- Order of service file changes (all at once vs table-by-table) + + + + +## Canonical References + +**Downstream agents MUST read these before planning or implementing.** + +### Database Schema +- `src/db/schema.ts` — Current schema (no userId columns yet) +- `drizzle-pg/` — PostgreSQL migration directory + +### Services (all need userId parameter) +- `src/server/services/item.service.ts` — Item CRUD +- `src/server/services/category.service.ts` — Category CRUD + unique constraint +- `src/server/services/thread.service.ts` — Thread + candidate CRUD + resolution +- `src/server/services/setup.service.ts` — Setup CRUD + item sync +- `src/server/services/totals.service.ts` — Aggregate queries (weight/cost totals) +- `src/server/services/csv.service.ts` — CSV import/export +- `src/server/services/auth.service.ts` — API key management (needs userId) + +### Routes (all need userId from context) +- `src/server/routes/*.ts` — All route files pass userId to services + +### Middleware +- `src/server/middleware/auth.ts` — requireAuth resolves userId onto context +- `src/server/services/auth.service.ts` — API key → userId lookup + +### MCP +- `src/server/mcp/index.ts` — MCP tool handlers need userId scoping + +### Tests +- `tests/services/*.test.ts` — All service tests need userId in calls +- `tests/routes/*.test.ts` — Route tests need userId in context +- `tests/mcp/tools.test.ts` — MCP tests need userId scoping + +### Requirements +- `.planning/REQUIREMENTS.md` — MULTI-01 through MULTI-06 + + + + +## Existing Code Insights + +### Reusable Assets +- Service DI pattern (db as first param) — extend with userId as second param +- `requireAuth` middleware — extend to resolve and set userId on context +- Drizzle `eq()` where clauses — same pattern, add userId condition +- `createTestDb()` helper — extend to seed a test user + +### Established Patterns +- **Service DI**: `functionName(db, ...)` — becomes `functionName(db, userId, ...)` +- **Route context**: `c.get("db")` — add `c.get("userId")` +- **Async Postgres**: All services already use `await` with Drizzle (from Phase 14) +- **Test isolation**: PGlite per-test — add user seed to `createTestDb()` + +### Integration Points +- `src/server/middleware/auth.ts` — Primary point for userId resolution +- `src/server/index.ts` — Where middleware is applied +- `src/db/schema.ts` — All table definitions need userId column +- `tests/helpers/db.ts` — Test DB helper needs user seed + + + + +## Specific Ideas + +No specific requirements — open to standard approaches for multi-tenant data isolation with Drizzle ORM. + + + + +## Deferred Ideas + +None — discussion stayed within phase scope + + + +--- + +*Phase: 16-multi-user-data-model* +*Context gathered: 2026-04-05* diff --git a/.planning/phases/16-multi-user-data-model/16-DISCUSSION-LOG.md b/.planning/phases/16-multi-user-data-model/16-DISCUSSION-LOG.md new file mode 100644 index 0000000..c5333cd --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-DISCUSSION-LOG.md @@ -0,0 +1,85 @@ +# Phase 16: Multi-User Data Model - Discussion Log + +> **Audit trail only.** Do not use as input to planning, research, or execution agents. +> Decisions are captured in CONTEXT.md — this log preserves the alternatives considered. + +**Date:** 2026-04-05 +**Phase:** 16-multi-user-data-model +**Areas discussed:** User ID Representation, Existing Data Migration, Category Uniqueness, Settings Scope, API Key Ownership +**Mode:** --auto --batch (all decisions auto-selected) + +--- + +## User ID Representation + +| Option | Description | Selected | +|--------|-------------|----------| +| Local users table with integer ID | Thin table mapping Logto sub to auto-increment integer, all FKs use integer | ✓ | +| Logto sub string directly | Store Logto sub (string UUID) as FK on every entity table | | +| Mapping table without FK | Store logtoSub on entities, join manually | | + +**User's choice:** Local users table with integer ID (auto-selected) +**Notes:** Integer FKs are more efficient for joins. Thin table auto-creates on first OIDC login. + +--- + +## Existing Data Migration + +| Option | Description | Selected | +|--------|-------------|----------| +| Migration assigns all rows to user ID 1 | Add userId column, set all existing data to first user | ✓ | +| Prompt for Logto sub during migration | Interactive script asks for the Logto user ID | | +| Leave data unassigned until claimed | Nullable userId, user claims data on first login | | + +**User's choice:** Migration assigns all rows to user ID 1 (auto-selected) +**Notes:** Single existing user, simplest approach. + +--- + +## Category Uniqueness + +| Option | Description | Selected | +|--------|-------------|----------| +| Composite unique (userId, name) | Each user has independent category namespace | ✓ | +| Global unique (current) | All users share one category namespace | | + +**User's choice:** Composite unique (userId, name) (auto-selected) +**Notes:** Required by MULTI-03. + +--- + +## Settings Scope + +| Option | Description | Selected | +|--------|-------------|----------| +| Composite PK (userId, key) | Each user has own settings | ✓ | +| Separate user_settings table | New table for per-user settings | | + +**User's choice:** Composite PK (userId, key) (auto-selected) +**Notes:** Required by MULTI-06. Minimal schema change. + +--- + +## API Key Ownership + +| Option | Description | Selected | +|--------|-------------|----------| +| userId column on apiKeys | API keys belong to creating user, middleware resolves user scope | ✓ | +| Shared API keys (no user scope) | API keys grant access to all data | | + +**User's choice:** userId column on apiKeys (auto-selected) +**Notes:** Required by MULTI-05 for MCP tool scoping. + +--- + +## Claude's Discretion + +- Migration SQL approach +- userId scoping helper vs inline where clauses +- Default category creation strategy +- Thread resolution cross-user checks +- Service change ordering + +## Deferred Ideas + +None — discussion stayed within phase scope diff --git a/.planning/phases/16-multi-user-data-model/16-RESEARCH.md b/.planning/phases/16-multi-user-data-model/16-RESEARCH.md new file mode 100644 index 0000000..619e1a7 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-RESEARCH.md @@ -0,0 +1,534 @@ +# Phase 16: Multi-User Data Model - Research + +**Researched:** 2026-04-04 +**Domain:** Multi-tenant data isolation with Drizzle ORM on PostgreSQL +**Confidence:** HIGH + +## Summary + +Phase 16 adds user ownership to all user-created entities (items, categories, threads, setups, settings, apiKeys) and enforces complete cross-user data isolation. The current codebase has no `users` table (Phase 15 removed the old one) and no `userId` columns on any entity table. Every service function signature is `(db, ...)` and needs to become `(db, userId, ...)`. + +The scope is well-defined and mechanical: create a new `users` table (with `logtoSub` for OIDC mapping), add `userId` FK columns to 6 tables, update the `requireAuth` middleware to resolve and set `userId` on context, update all 7 service files to accept and filter by `userId`, update all route handlers to extract `userId` from context, update all MCP tool registrations to pass `userId`, and update all tests. + +**Primary recommendation:** Use a multi-step Drizzle migration (add column nullable, backfill existing data to user 1, set NOT NULL + FK constraint) and a systematic service-by-service approach with `and(eq(table.userId, userId), ...)` filtering on every query. + + +## User Constraints (from CONTEXT.md) + +### Locked Decisions +- **D-01:** Create a thin local `users` table: `id` (serial integer PK), `logtoSub` (text, unique, not null), `createdAt` (timestamp). Auto-created on first OIDC login via upsert. +- **D-02:** All entity tables reference `users.id` (integer FK) -- not the Logto sub string directly. +- **D-03:** The `requireAuth` middleware resolves the authenticated identity to a local `users.id` and sets it on the Hono context (e.g., `c.set("userId", userId)`). +- **D-04:** Add `userId` (integer, NOT NULL, FK -> users.id) column to: `items`, `categories`, `threads`, `setups`, `settings`, `apiKeys`. +- **D-05:** `categories`: Drop global unique constraint on `name`. Add composite unique constraint on `(userId, name)`. Each user gets their own "Uncategorized" default category. +- **D-06:** `settings`: Change primary key from `key` alone to composite `(userId, key)`. Each user has their own settings. +- **D-07:** `apiKeys`: Add `userId` column so middleware can resolve which user's data an API key grants access to. +- **D-08:** `threadCandidates` and `setupItems`: No userId needed -- they inherit ownership through their parent thread/setup FK. +- **D-09:** Every service function that reads or writes user-owned data gains a `userId` parameter. All queries include `where(eq(table.userId, userId))`. +- **D-10:** `requireAuth` middleware sets `userId` on context. Routes extract `userId` from context and pass to services. +- **D-11:** Migration script adds `userId` column with a temporary default, then updates all existing rows to user ID 1, then removes the default and sets NOT NULL. +- **D-12:** Create "Uncategorized" category per-user on first login (or lazily when needed). +- **D-13:** MCP tools resolve userId from the authenticated token (API key -> userId lookup, or Bearer token -> userId). All tool operations are scoped to that user. + +### Claude's Discretion +- Exact migration SQL approach (single migration vs multi-step) +- Whether to use Drizzle's `.where()` chaining or a helper function for userId scoping +- Default category creation strategy (eager on first login vs lazy on first item creation) +- Whether thread resolution should check that the target category belongs to the same user +- Order of service file changes (all at once vs table-by-table) + +### Deferred Ideas (OUT OF SCOPE) +None -- discussion stayed within phase scope. + + + +## Phase Requirements + +| ID | Description | Research Support | +|----|-------------|------------------| +| MULTI-01 | Every item, category, thread, and setup is owned by a specific user | D-04 adds userId FK to all entity tables; D-01 creates users table | +| MULTI-02 | User can only see and modify their own data (cross-user isolation) | D-09 adds userId filtering to all service queries; D-10 threads userId through routes | +| MULTI-03 | Categories use composite unique constraint (userId + name) | D-05 replaces global unique(name) with unique(userId, name) | +| MULTI-04 | Existing data is assigned to the original user during migration | D-11 migration backfills all rows to user ID 1 | +| MULTI-05 | MCP tools operate within the authenticated user's scope | D-13 resolves userId from API key or Bearer token in MCP auth middleware | +| MULTI-06 | Settings are per-user rather than global | D-06 changes settings PK from (key) to composite (userId, key) | + + +## Project Constraints (from CLAUDE.md) + +- **Stack**: React 19 + Hono + Drizzle ORM + PostgreSQL (migrated from SQLite in Phase 14), running on Bun +- **Services pattern**: Pure business logic functions that take a db instance. No HTTP awareness. +- **Prices stored as cents** (integer). Timestamps as integers with `{ mode: "timestamp" }`. +- **Testing**: Bun test runner. `createTestDb()` uses PGlite with Drizzle migrations. Tests at service level and route level. +- **Auth model**: Public-read, authenticated-write. Cookie sessions for web UI, API keys for programmatic access. +- **Schema file**: `src/db/schema.ts` -- currently uses `sqliteTable` imports but targets PostgreSQL via Drizzle abstraction. +- **Migrations**: Generated via `bun run db:generate`, applied via `bun run db:push`. Migration directory: `drizzle-pg/`. + +## Standard Stack + +### Core +| Library | Version | Purpose | Why Standard | +|---------|---------|---------|--------------| +| drizzle-orm | (current in project) | Schema definition, query building, migrations | Already in use; provides `and()`, `eq()`, composite constraints | +| drizzle-kit | (current in project) | Migration generation from schema changes | Already in use; `bun run db:generate` | +| @hono/oidc-auth | (current in project) | OIDC session for browser users (getAuth -> sub claim) | Already in use from Phase 15 | +| hono | (current in project) | HTTP framework with typed context | Already in use; `c.set("userId", ...)` / `c.get("userId")` | + +### Supporting +| Library | Version | Purpose | When to Use | +|---------|---------|---------|-------------| +| @electric-sql/pglite | (current in project) | In-memory PG for tests | Already in use in `createTestDb()` | + +No new dependencies are needed for this phase. + +## Architecture Patterns + +### Current Service Signature Pattern +```typescript +// BEFORE (current) +export async function getAllItems(db: Db = prodDb) { + return db.select().from(items); +} + +// AFTER (Phase 16) +export async function getAllItems(db: Db, userId: number) { + return db.select().from(items) + .where(eq(items.userId, userId)); +} +``` + +### userId Filtering Pattern (Drizzle `and()`) +```typescript +import { and, eq } from "drizzle-orm"; + +// Single condition (list queries) +.where(eq(items.userId, userId)) + +// Multiple conditions (get by ID queries -- CRITICAL for isolation) +.where(and(eq(items.id, id), eq(items.userId, userId))) +``` + +Every query that reads or writes user-owned data MUST include the userId filter. For get-by-id, update, and delete operations, this means using `and()` to combine the id condition with the userId condition. This prevents user A from accessing user B's data by guessing IDs. + +### Middleware userId Resolution Pattern +```typescript +// src/server/middleware/auth.ts +export async function requireAuth(c: Context, next: Next) { + const db = c.get("db"); + + // 1. API key -> resolve userId from apiKeys table + const apiKey = c.req.header("X-API-Key"); + if (apiKey) { + const result = await verifyApiKeyWithUser(db, apiKey); + if (result) { + c.set("userId", result.userId); + return next(); + } + return c.json({ error: "Invalid API key" }, 401); + } + + // 2. OAuth Bearer -> resolve userId from token -> user mapping + const authHeader = c.req.header("Authorization"); + if (authHeader?.startsWith("Bearer ")) { + const token = authHeader.slice(7); + const result = await verifyAccessTokenWithUser(db, token); + if (result) { + c.set("userId", result.userId); + return next(); + } + return c.json({ error: "invalid_token" }, 401); + } + + // 3. OIDC session -> resolve logtoSub to local userId via upsert + const auth = await getAuth(c); + if (auth) { + const user = await getOrCreateUser(db, auth.sub); + c.set("userId", user.id); + return next(); + } + + return c.json({ error: "Authentication required" }, 401); +} +``` + +### Route userId Extraction Pattern +```typescript +// In route handlers +app.get("/", async (c) => { + const db = c.get("db"); + const userId = c.get("userId"); // Set by requireAuth middleware + const items = await getAllItems(db, userId); + return c.json(items); +}); +``` + +**Important nuance**: Currently GET routes are public (no auth required). With multi-user data, GET routes ALSO need auth to know whose data to return. The middleware configuration in `src/server/index.ts` currently skips auth for GET requests: +```typescript +if (c.req.method === "GET") return next(); +``` +This must change -- all data routes need userId resolution. Options: +1. Apply `requireAuth` to all methods on data routes (recommended -- simplest) +2. Create a separate `resolveUser` middleware that runs on GET but returns 401 only on writes + +**Recommendation:** Apply `requireAuth` to all API routes (not just writes). The "public read" model no longer makes sense in a multi-user context where you need to know whose data to show. + +### MCP Tool Registration Pattern +```typescript +// MCP tools need userId passed through +export function registerItemTools(db: Db, userId: number) { + return { + list_items: async (args: { categoryId?: number }) => { + const items = await getAllItems(db, userId); + // ... + }, + }; +} +``` + +The MCP server creation must receive userId, which means the MCP auth middleware resolves userId and passes it to `createMcpServer(db, userId)`. + +### Schema Changes Pattern + +**New users table:** +```typescript +import { pgTable, serial, text, timestamp, unique } from "drizzle-orm/pg-core"; + +export const users = pgTable("users", { + id: serial("id").primaryKey(), + logtoSub: text("logto_sub").notNull().unique(), + createdAt: timestamp("created_at").defaultNow().notNull(), +}); +``` + +**Adding userId to entity tables (example: items):** +```typescript +export const items = pgTable("items", { + // ... existing columns ... + userId: integer("user_id").notNull().references(() => users.id), +}); +``` + +**Composite unique on categories:** +```typescript +import { unique } from "drizzle-orm/pg-core"; + +export const categories = pgTable("categories", { + id: serial("id").primaryKey(), + name: text("name").notNull(), + icon: text("icon").notNull().default("package"), + userId: integer("user_id").notNull().references(() => users.id), + createdAt: timestamp("created_at").defaultNow().notNull(), +}, (table) => [ + unique().on(table.userId, table.name), +]); +``` + +**Composite PK on settings:** +```typescript +import { primaryKey } from "drizzle-orm/pg-core"; + +export const settings = pgTable("settings", { + userId: integer("user_id").notNull().references(() => users.id), + key: text("key").notNull(), + value: text("value").notNull(), +}, (table) => [ + primaryKey({ columns: [table.userId, table.key] }), +]); +``` + +### Recommended Project Structure (unchanged) +``` +src/ + db/ + schema.ts # Add users table, userId columns, composite constraints + server/ + middleware/auth.ts # Extend to resolve and set userId + services/ # All service functions gain userId parameter + routes/ # All routes extract userId from context + mcp/ + index.ts # MCP auth resolves userId, passes to createMcpServer + tools/ # Tool registrations accept userId +tests/ + helpers/db.ts # createTestDb() seeds a test user + services/ # All tests pass userId to service calls + routes/ # Route tests set userId in context + mcp/ # MCP tests pass userId +``` + +### Anti-Patterns to Avoid +- **Filtering in application code instead of SQL:** Never fetch all records then filter by userId in JS. Always use `.where(eq(table.userId, userId))` in the query. +- **Missing userId on get-by-id queries:** `getItemById(db, id)` without userId allows cross-user access by ID guessing. MUST be `and(eq(items.id, id), eq(items.userId, userId))`. +- **Trusting child entity ownership via parent lookup:** When deleting a candidate, verify the parent thread belongs to the user, not just that the candidate exists. +- **Hardcoding Uncategorized category id=1:** With per-user categories, there is no global ID 1. Each user's Uncategorized category has its own ID. + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Composite unique constraints | Custom application-level uniqueness checks | Drizzle's `unique().on()` + PostgreSQL UNIQUE constraint | DB-level enforcement is atomic and race-condition-free | +| Composite primary keys | Surrogate key + application uniqueness check | Drizzle's `primaryKey({ columns: [...] })` | Cleaner for settings table | +| User upsert on first login | SELECT then conditional INSERT | PostgreSQL `ON CONFLICT DO NOTHING` / `ON CONFLICT DO UPDATE` via Drizzle | Race-condition-free, single query | +| Migration data backfill | Manual SQL scripts outside Drizzle | Drizzle migration with raw SQL for backfill step | Keeps migration history consistent | + +## Common Pitfalls + +### Pitfall 1: Uncategorized Category Hardcoded to ID 1 +**What goes wrong:** Current code hardcodes `categoryId: 1` as the Uncategorized fallback (in `deleteCategory`, `resolveThread`, `createTestDb`). With per-user categories, each user's Uncategorized has a different ID. +**Why it happens:** Single-user era guaranteed ID 1 was always Uncategorized. +**How to avoid:** Create a helper function `getOrCreateUncategorized(db, userId)` that looks up the user's Uncategorized category by name+userId, creating it if missing. Replace all hardcoded `1` references. +**Warning signs:** Tests failing with FK constraint violations, or items silently assigned to another user's category. + +### Pitfall 2: GET Routes Still Public (No userId Available) +**What goes wrong:** GET /api/items returns all items from all users because no userId is available in context. +**Why it happens:** Current middleware skips auth for GET requests. Multi-user data requires knowing the user for reads too. +**How to avoid:** Apply `requireAuth` to all data API routes, not just write operations. Update the middleware configuration in `src/server/index.ts`. +**Warning signs:** API returning data from other users, or 500 errors from `userId` being undefined. + +### Pitfall 3: Settings Table PK Change Requires Migration Care +**What goes wrong:** Changing a primary key on an existing table with data requires dropping and recreating the constraint. +**Why it happens:** PostgreSQL doesn't support ALTER PRIMARY KEY directly. +**How to avoid:** Migration should: (1) add userId column, (2) drop old PK constraint, (3) add composite PK. All in a transaction. +**Warning signs:** Migration fails with "cannot add constraint" errors. + +### Pitfall 4: verifyApiKey Must Return userId, Not Just Boolean +**What goes wrong:** Current `verifyApiKey` returns `boolean`. But the middleware needs the userId associated with that API key to set on context. +**Why it happens:** In single-user mode, knowing "auth is valid" was sufficient. Multi-user needs to know WHICH user. +**How to avoid:** Change `verifyApiKey` to return `{ userId: number } | null` instead of `boolean`. Same for `verifyAccessToken`. +**Warning signs:** userId is undefined in routes when using API key auth. + +### Pitfall 5: MCP Server Per-Session Architecture vs Per-Request userId +**What goes wrong:** The current MCP architecture creates one `McpServer` per session and reuses it. But userId needs to be available for every tool call. +**Why it happens:** `createMcpServer(db)` is called once at session init. userId is resolved per-request in the MCP auth middleware. +**How to avoid:** Either (a) create the MCP server with the userId at session init (requires storing userId alongside transport), or (b) pass userId through the tool call context. Option (a) is simpler since the session is already per-authenticated-user. +**Warning signs:** MCP tools returning data from wrong user, or userId undefined in tool handlers. + +### Pitfall 6: Thread Resolution Must Scope New Item to Same User +**What goes wrong:** `resolveThread` creates a new item from a candidate. The new item must have the same userId as the thread. +**Why it happens:** Current code doesn't set userId on the new item because the field doesn't exist yet. +**How to avoid:** Pass userId to `resolveThread` and include it in the `insert(items).values({ ..., userId })` call. Also verify the thread belongs to the user before resolving. +**Warning signs:** Resolved items appearing under wrong user or FK constraint violations. + +### Pitfall 7: CSV Import Creates Categories Without userId +**What goes wrong:** `importItemsCsv` creates new categories on-the-fly when importing. These must be scoped to the importing user. +**Why it happens:** Current code inserts categories without userId. +**How to avoid:** Pass userId to `importItemsCsv`. Category creation and lookup must filter by userId. +**Warning signs:** Categories from CSV import visible to all users, or unique constraint violations. + +### Pitfall 8: Setup Items Cross-User Boundary +**What goes wrong:** `syncSetupItems` takes arbitrary itemIds. A user could add another user's items to their setup. +**Why it happens:** No validation that the items belong to the same user as the setup. +**How to avoid:** In `syncSetupItems`, verify each itemId belongs to the same userId before inserting. Or filter the item list to only user-owned items. +**Warning signs:** Setup showing items owned by other users. + +## Code Examples + +### User Upsert on First Login +```typescript +// Source: Drizzle ORM PostgreSQL onConflict pattern +export async function getOrCreateUser(db: Db, logtoSub: string): Promise<{ id: number }> { + const [user] = await db + .insert(users) + .values({ logtoSub }) + .onConflictDoUpdate({ + target: users.logtoSub, + set: { logtoSub }, // no-op update to return existing row + }) + .returning({ id: users.id }); + return user; +} +``` + +### Get or Create Uncategorized Category +```typescript +export async function getOrCreateUncategorized(db: Db, userId: number): Promise { + const [existing] = await db + .select({ id: categories.id }) + .from(categories) + .where(and(eq(categories.userId, userId), eq(categories.name, "Uncategorized"))); + + if (existing) return existing.id; + + const [created] = await db + .insert(categories) + .values({ name: "Uncategorized", icon: "package", userId }) + .returning({ id: categories.id }); + + return created.id; +} +``` + +### Migration SQL (multi-step in single migration file) +```sql +-- Step 1: Create users table +CREATE TABLE "users" ( + "id" serial PRIMARY KEY NOT NULL, + "logto_sub" text NOT NULL, + "created_at" timestamp DEFAULT now() NOT NULL, + CONSTRAINT "users_logto_sub_unique" UNIQUE("logto_sub") +); + +-- Step 2: Insert placeholder user for existing data +INSERT INTO "users" ("logto_sub") VALUES ('migration-placeholder'); + +-- Step 3: Add userId columns (nullable first) +ALTER TABLE "items" ADD COLUMN "user_id" integer; +ALTER TABLE "categories" ADD COLUMN "user_id" integer; +ALTER TABLE "threads" ADD COLUMN "user_id" integer; +ALTER TABLE "setups" ADD COLUMN "user_id" integer; +ALTER TABLE "api_keys" ADD COLUMN "user_id" integer; + +-- Step 4: Backfill all rows to user 1 +UPDATE "items" SET "user_id" = 1; +UPDATE "categories" SET "user_id" = 1; +UPDATE "threads" SET "user_id" = 1; +UPDATE "setups" SET "user_id" = 1; +UPDATE "api_keys" SET "user_id" = 1; + +-- Step 5: Set NOT NULL and add FK constraints +ALTER TABLE "items" ALTER COLUMN "user_id" SET NOT NULL; +ALTER TABLE "items" ADD CONSTRAINT "items_user_id_users_id_fk" + FOREIGN KEY ("user_id") REFERENCES "users"("id"); +-- (repeat for all tables) + +-- Step 6: Settings table - add userId, change PK +ALTER TABLE "settings" ADD COLUMN "user_id" integer; +UPDATE "settings" SET "user_id" = 1; +ALTER TABLE "settings" ALTER COLUMN "user_id" SET NOT NULL; +ALTER TABLE "settings" DROP CONSTRAINT "settings_pkey"; +ALTER TABLE "settings" ADD PRIMARY KEY ("user_id", "key"); +ALTER TABLE "settings" ADD CONSTRAINT "settings_user_id_users_id_fk" + FOREIGN KEY ("user_id") REFERENCES "users"("id"); + +-- Step 7: Categories - drop old unique, add composite unique +ALTER TABLE "categories" DROP CONSTRAINT "categories_name_unique"; +ALTER TABLE "categories" ADD CONSTRAINT "categories_user_id_name_unique" + UNIQUE("user_id", "name"); +``` + +### Test Helper Update +```typescript +export async function createTestDb() { + const db = drizzle({ schema }); + await migrate(db, { migrationsFolder: "./drizzle-pg" }); + + // Seed test user + const [user] = await db + .insert(schema.users) + .values({ logtoSub: "test-user-sub" }) + .returning(); + + // Seed per-user Uncategorized category + await db + .insert(schema.categories) + .values({ name: "Uncategorized", icon: "package", userId: user.id }); + + return { db, userId: user.id }; +} +``` + +Note: this changes the return type of `createTestDb()` from just `db` to `{ db, userId }`. All test files need updating. + +### API Key Verification Returning userId +```typescript +export async function verifyApiKey( + db: Db, + rawKey: string, +): Promise<{ userId: number } | null> { + const prefix = rawKey.slice(0, 8); + const candidates = await db + .select({ keyHash: apiKeys.keyHash, userId: apiKeys.userId }) + .from(apiKeys) + .where(eq(apiKeys.keyPrefix, prefix)); + + for (const candidate of candidates) { + if (await Bun.password.verify(rawKey, candidate.keyHash)) { + return { userId: candidate.userId }; + } + } + return null; +} +``` + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| Single-user, no userId columns | Multi-user with userId FK on all entities | Phase 16 | Every query must be scoped | +| Global Uncategorized category (id=1) | Per-user Uncategorized (dynamic lookup) | Phase 16 | No more hardcoded category IDs | +| `verifyApiKey` returns boolean | Returns `{ userId } | null` | Phase 16 | Middleware can set userId on context | +| Public GET endpoints | All data endpoints require auth | Phase 16 | Must know user to scope data | +| `settings` table PK = `key` | PK = `(userId, key)` | Phase 16 | Per-user settings | + +## Validation Architecture + +### Test Framework +| Property | Value | +|----------|-------| +| Framework | Bun test runner (built-in) | +| Config file | none (Bun built-in) | +| Quick run command | `bun test tests/services/item.service.test.ts` | +| Full suite command | `bun test` | + +### Phase Requirements -> Test Map +| Req ID | Behavior | Test Type | Automated Command | File Exists? | +|--------|----------|-----------|-------------------|-------------| +| MULTI-01 | Items/categories/threads/setups have userId FK | unit | `bun test tests/services/item.service.test.ts` | Exists (needs update) | +| MULTI-02 | User A cannot see User B's data | unit | `bun test tests/services/item.service.test.ts` (new isolation test) | Needs new test | +| MULTI-03 | Categories composite unique (userId, name) | unit | `bun test tests/services/category.service.test.ts` | Exists (needs update) | +| MULTI-04 | Migration backfills existing data to user 1 | integration | Migration test (manual verification) | Needs new test | +| MULTI-05 | MCP tools scoped to authenticated user | unit | `bun test tests/mcp/tools.test.ts` | Exists (needs update) | +| MULTI-06 | Settings per-user | unit | `bun test tests/routes/settings.test.ts` (new) | Needs update | + +### Sampling Rate +- **Per task commit:** `bun test` (full suite, fast with PGlite) +- **Per wave merge:** `bun test` + `bun run lint` +- **Phase gate:** Full suite green before `/gsd:verify-work` + +### Wave 0 Gaps +- [ ] Update `createTestDb()` to return `{ db, userId }` with seeded user +- [ ] Add cross-user isolation tests (create data as user A, verify user B cannot see it) +- [ ] Update all existing service tests to pass userId parameter +- [ ] Update all existing route tests to set userId in context middleware +- [ ] Update MCP tool tests to pass userId to register functions + +## Open Questions + +1. **Schema file still uses `sqliteTable` imports** + - What we know: `src/db/schema.ts` imports from `drizzle-orm/sqlite-core` but `src/db/index.ts` uses `drizzle-orm/postgres-js`. The PG migration in `drizzle-pg/` has the correct PostgreSQL DDL. Tests use PGlite successfully. + - What's unclear: Whether Phase 14 intended to switch schema.ts to `pgTable` imports or if Drizzle's abstraction handles this. The migration SQL was generated from this schema and works. + - Recommendation: This phase should switch schema.ts to use `drizzle-orm/pg-core` imports (`pgTable`, `serial`, `text`, `timestamp`, `integer`, `doublePrecision`) as part of adding the new columns. This ensures composite constraints and PG-specific features work correctly. Verify by running `bun run db:generate` after changes. + +2. **OAuth token -> userId mapping** + - What we know: `oauth_tokens` table stores access/refresh token hashes but has no userId column. + - What's unclear: How to resolve an OAuth Bearer token to a userId. Currently `verifyAccessToken` just returns boolean. + - Recommendation: Add `userId` column to `oauth_tokens` table, set during token creation (the `/oauth/authorize` flow knows the OIDC user). Then `verifyAccessToken` can return userId. + +3. **MCP session architecture and userId** + - What we know: MCP creates one server per session. Auth middleware runs per-request. + - Recommendation: Store userId alongside the transport in the session map: `Map`. Pass userId when creating the MCP server. Since a session is always for one authenticated user, this is safe. + +## Sources + +### Primary (HIGH confidence) +- `src/db/schema.ts` -- Current table definitions (no users table, no userId columns) +- `src/server/middleware/auth.ts` -- Current auth middleware (returns boolean, no userId resolution) +- `src/server/services/*.ts` -- All 7 service files (all use `(db, ...)` signature) +- `src/server/routes/*.ts` -- All route handlers (no userId extraction from context) +- `src/server/mcp/index.ts` -- MCP server creation (no userId threading) +- `tests/helpers/db.ts` -- Test helper (seeds Uncategorized with id=1, no user) +- `drizzle-pg/0000_fuzzy_shiva.sql` -- Current PostgreSQL migration (includes old users table from pre-Phase 15) +- `.planning/phases/15-external-authentication/15-VERIFICATION.md` -- Confirms users/sessions tables dropped + +### Secondary (MEDIUM confidence) +- Drizzle ORM composite constraints -- `unique().on()` and `primaryKey({ columns: [...] })` patterns verified from existing codebase (`oauth.service.ts` uses `and()`) + +## Metadata + +**Confidence breakdown:** +- Standard stack: HIGH -- no new libraries needed, all patterns exist in codebase +- Architecture: HIGH -- mechanical transformation of existing service/route/test patterns +- Pitfalls: HIGH -- identified from direct code inspection of all affected files +- Migration: MEDIUM -- composite PK change on settings table needs careful SQL ordering + +**Research date:** 2026-04-04 +**Valid until:** 2026-05-04 (stable -- no external dependency changes expected) diff --git a/.planning/phases/16-multi-user-data-model/16-VALIDATION.md b/.planning/phases/16-multi-user-data-model/16-VALIDATION.md new file mode 100644 index 0000000..c3eb085 --- /dev/null +++ b/.planning/phases/16-multi-user-data-model/16-VALIDATION.md @@ -0,0 +1,79 @@ +--- +phase: 16 +slug: multi-user-data-model +status: draft +nyquist_compliant: false +wave_0_complete: false +created: 2026-04-05 +--- + +# Phase 16 — Validation Strategy + +> Per-phase validation contract for feedback sampling during execution. + +--- + +## Test Infrastructure + +| Property | Value | +|----------|-------| +| **Framework** | Bun test runner (built-in) | +| **Config file** | none (Bun built-in) | +| **Quick run command** | `bun test tests/services/item.service.test.ts` | +| **Full suite command** | `bun test` | +| **Estimated runtime** | ~30 seconds (individual files) | + +--- + +## Sampling Rate + +- **After every task commit:** Run `bun test` (affected file) +- **After every plan wave:** Run `bun test` (full suite) +- **Before `/gsd:verify-work`:** Full suite must be green +- **Max feedback latency:** 30 seconds + +--- + +## Per-Task Verification Map + +| Task ID | Plan | Wave | Requirement | Test Type | Automated Command | File Exists | Status | +|---------|------|------|-------------|-----------|-------------------|-------------|--------| +| 16-01-01 | 01 | 1 | MULTI-01 | unit | `bun test tests/services/item.service.test.ts` | ✅ (needs update) | ⬜ pending | +| 16-01-02 | 01 | 1 | MULTI-03 | unit | `bun test tests/services/category.service.test.ts` | ✅ (needs update) | ⬜ pending | +| 16-02-01 | 02 | 2 | MULTI-02 | unit | `bun test tests/services/item.service.test.ts` | ❌ W0 (new isolation test) | ⬜ pending | +| 16-02-02 | 02 | 2 | MULTI-05 | unit | `bun test tests/mcp/tools.test.ts` | ✅ (needs update) | ⬜ pending | +| 16-02-03 | 02 | 2 | MULTI-06 | unit | `bun test tests/routes/settings.test.ts` | ✅ (needs update) | ⬜ pending | +| 16-03-01 | 03 | 3 | MULTI-04 | integration | Migration verification | ❌ W0 | ⬜ pending | + +*Status: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky* + +--- + +## Wave 0 Requirements + +- [ ] Update `createTestDb()` to return `{ db, userId }` with seeded user +- [ ] Add cross-user isolation tests (create as user A, verify user B can't see) +- [ ] Update all service tests to pass userId parameter +- [ ] Update all route tests to set userId in context +- [ ] Update MCP tool tests to pass userId + +--- + +## Manual-Only Verifications + +| Behavior | Requirement | Why Manual | Test Instructions | +|----------|-------------|------------|-------------------| +| Existing data migrated to first user | MULTI-04 | Requires real migration against existing DB | Run migration on dev DB, verify all items/categories/threads/setups have userId = 1 | + +--- + +## Validation Sign-Off + +- [ ] All tasks have `` verify or Wave 0 dependencies +- [ ] Sampling continuity: no 3 consecutive tasks without automated verify +- [ ] Wave 0 covers all MISSING references +- [ ] No watch-mode flags +- [ ] Feedback latency < 30s +- [ ] `nyquist_compliant: true` set in frontmatter + +**Approval:** pending