From 26a0df835fd0e4ae803bb3ede59ee66c0cccb331 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 29 Jan 2026 20:14:27 -0600 Subject: [PATCH] fix(api): fix RLS context, DTO validation, and error handling - Wrap SET LOCAL in transactions for proper connection pooling - Make workspaceId optional in query DTOs (derived from guards) - Replace Error throws with UnauthorizedException in activity controller - Update workspace guard to remove RLS context setting - Document that services should use withUserContext/withUserTransaction --- apps/api/src/activity/activity.controller.ts | 14 +++- .../activity/dto/query-activity-log.dto.ts | 3 +- apps/api/src/common/guards/workspace.guard.ts | 10 ++- apps/api/src/domains/dto/query-domains.dto.ts | 3 +- apps/api/src/events/dto/query-events.dto.ts | 3 +- apps/api/src/ideas/dto/query-ideas.dto.ts | 3 +- apps/api/src/lib/db-context.ts | 71 ++++++++++--------- .../src/projects/dto/query-projects.dto.ts | 3 +- apps/api/src/tasks/dto/query-tasks.dto.ts | 3 +- 9 files changed, 63 insertions(+), 50 deletions(-) diff --git a/apps/api/src/activity/activity.controller.ts b/apps/api/src/activity/activity.controller.ts index f648a1d..49b656e 100644 --- a/apps/api/src/activity/activity.controller.ts +++ b/apps/api/src/activity/activity.controller.ts @@ -1,4 +1,12 @@ -import { Controller, Get, Query, Param, UseGuards, Request } from "@nestjs/common"; +import { + Controller, + Get, + Query, + Param, + UseGuards, + Request, + UnauthorizedException +} from "@nestjs/common"; import { ActivityService } from "./activity.service"; import { EntityType } from "@prisma/client"; import type { QueryActivityLogDto } from "./dto"; @@ -34,7 +42,7 @@ export class ActivityController { async findOne(@Param("id") id: string, @Request() req: any) { const workspaceId = req.user?.workspaceId; if (!workspaceId) { - throw new Error("User workspaceId not found"); + throw new UnauthorizedException("User workspaceId not found"); } return this.activityService.findOne(id, workspaceId); } @@ -52,7 +60,7 @@ export class ActivityController { ) { const workspaceId = req.user?.workspaceId; if (!workspaceId) { - throw new Error("User workspaceId not found"); + throw new UnauthorizedException("User workspaceId not found"); } return this.activityService.getAuditTrail(workspaceId, entityType, entityId); } diff --git a/apps/api/src/activity/dto/query-activity-log.dto.ts b/apps/api/src/activity/dto/query-activity-log.dto.ts index 3ec1c88..b5afbba 100644 --- a/apps/api/src/activity/dto/query-activity-log.dto.ts +++ b/apps/api/src/activity/dto/query-activity-log.dto.ts @@ -14,8 +14,9 @@ import { Type } from "class-transformer"; * DTO for querying activity logs with filters and pagination */ export class QueryActivityLogDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsUUID("4", { message: "userId must be a valid UUID" }) diff --git a/apps/api/src/common/guards/workspace.guard.ts b/apps/api/src/common/guards/workspace.guard.ts index 305b52e..26d049c 100644 --- a/apps/api/src/common/guards/workspace.guard.ts +++ b/apps/api/src/common/guards/workspace.guard.ts @@ -7,13 +7,11 @@ import { Logger, } from "@nestjs/common"; import { PrismaService } from "../../prisma/prisma.service"; -import { setCurrentUser } from "../../lib/db-context"; /** * WorkspaceGuard ensures that: * 1. A workspace is specified in the request (header, param, or body) * 2. The authenticated user is a member of that workspace - * 3. The user context is set for Row-Level Security (RLS) * * This guard should be used in combination with AuthGuard: * @@ -25,7 +23,7 @@ import { setCurrentUser } from "../../lib/db-context"; * @Get() * async getTasks(@Workspace() workspaceId: string) { * // workspaceId is verified and available - * // RLS context is automatically set + * // Service layer must use withUserContext() for RLS * } * } * ``` @@ -36,6 +34,9 @@ import { setCurrentUser } from "../../lib/db-context"; * - Request body: `workspaceId` field * * Priority: Header > Param > Body + * + * Note: RLS context must be set at the service layer using withUserContext() + * or withUserTransaction() to ensure proper transaction scoping with connection pooling. */ @Injectable() export class WorkspaceGuard implements CanActivate { @@ -75,9 +76,6 @@ export class WorkspaceGuard implements CanActivate { ); } - // Set RLS context for this request - await setCurrentUser(user.id, this.prisma); - // Attach workspace info to request for convenience request.workspace = { id: workspaceId, diff --git a/apps/api/src/domains/dto/query-domains.dto.ts b/apps/api/src/domains/dto/query-domains.dto.ts index 1270973..3cdfe86 100644 --- a/apps/api/src/domains/dto/query-domains.dto.ts +++ b/apps/api/src/domains/dto/query-domains.dto.ts @@ -12,8 +12,9 @@ import { Type } from "class-transformer"; * DTO for querying domains with filters and pagination */ export class QueryDomainsDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsString({ message: "search must be a string" }) diff --git a/apps/api/src/events/dto/query-events.dto.ts b/apps/api/src/events/dto/query-events.dto.ts index 0814825..27a65eb 100644 --- a/apps/api/src/events/dto/query-events.dto.ts +++ b/apps/api/src/events/dto/query-events.dto.ts @@ -13,8 +13,9 @@ import { Type } from "class-transformer"; * DTO for querying events with filters and pagination */ export class QueryEventsDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsUUID("4", { message: "projectId must be a valid UUID" }) diff --git a/apps/api/src/ideas/dto/query-ideas.dto.ts b/apps/api/src/ideas/dto/query-ideas.dto.ts index 7d2f0bb..92cda3f 100644 --- a/apps/api/src/ideas/dto/query-ideas.dto.ts +++ b/apps/api/src/ideas/dto/query-ideas.dto.ts @@ -14,8 +14,9 @@ import { Type } from "class-transformer"; * DTO for querying ideas with filters and pagination */ export class QueryIdeasDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsEnum(IdeaStatus, { message: "status must be a valid IdeaStatus" }) diff --git a/apps/api/src/lib/db-context.ts b/apps/api/src/lib/db-context.ts index d2e67d6..41dddbd 100644 --- a/apps/api/src/lib/db-context.ts +++ b/apps/api/src/lib/db-context.ts @@ -22,51 +22,58 @@ function getPrismaInstance(): PrismaClient { } /** - * Sets the current user ID for RLS policies. + * Sets the current user ID for RLS policies within a transaction context. * Must be called before executing any queries that rely on RLS. * + * Note: SET LOCAL must be used within a transaction to ensure it's scoped + * correctly with connection pooling. This is a low-level function - prefer + * using withUserContext or withUserTransaction for most use cases. + * * @param userId - The UUID of the current user - * @param client - Optional Prisma client (defaults to global prisma) + * @param client - Prisma client (required - must be a transaction client) * * @example * ```typescript - * await setCurrentUser(userId); - * const tasks = await prisma.task.findMany(); // Automatically filtered by RLS + * await prisma.$transaction(async (tx) => { + * await setCurrentUser(userId, tx); + * const tasks = await tx.task.findMany(); // Automatically filtered by RLS + * }); * ``` */ export async function setCurrentUser( userId: string, - client?: PrismaClient + client: PrismaClient ): Promise { - const prismaClient = client || getPrismaInstance(); - await prismaClient.$executeRaw`SET LOCAL app.current_user_id = ${userId}`; + await client.$executeRaw`SET LOCAL app.current_user_id = ${userId}`; } /** - * Clears the current user context. + * Clears the current user context within a transaction. * Use this to reset the session or when switching users. * - * @param client - Optional Prisma client (defaults to global prisma) + * Note: SET LOCAL is automatically cleared at transaction end, + * so explicit clearing is typically unnecessary. + * + * @param client - Prisma client (required - must be a transaction client) */ export async function clearCurrentUser( - client?: PrismaClient + client: PrismaClient ): Promise { - const prismaClient = client || getPrismaInstance(); - await prismaClient.$executeRaw`SET LOCAL app.current_user_id = NULL`; + await client.$executeRaw`SET LOCAL app.current_user_id = NULL`; } /** - * Executes a function with the current user context set. - * Automatically sets and clears the user context. + * Executes a function with the current user context set within a transaction. + * Automatically sets the user context and ensures it's properly scoped. * * @param userId - The UUID of the current user - * @param fn - The function to execute with user context + * @param fn - The function to execute with user context (receives transaction client) * @returns The result of the function * * @example * ```typescript - * const tasks = await withUserContext(userId, async () => { - * return prisma.task.findMany({ + * const tasks = await withUserContext(userId, async (tx) => { + * return tx.task.findMany({ * where: { workspaceId } * }); * }); @@ -74,16 +81,13 @@ export async function clearCurrentUser( */ export async function withUserContext( userId: string, - fn: () => Promise + fn: (tx: any) => Promise ): Promise { - await setCurrentUser(userId); - try { - return await fn(); - } finally { - // Note: LOCAL settings are automatically cleared at transaction end - // but we explicitly clear here for consistency - await clearCurrentUser(); - } + const prismaClient = getPrismaInstance(); + return prismaClient.$transaction(async (tx) => { + await setCurrentUser(userId, tx as PrismaClient); + return fn(tx); + }); } /** @@ -168,9 +172,8 @@ export async function verifyWorkspaceAccess( userId: string, workspaceId: string ): Promise { - const prismaClient = getPrismaInstance(); - return withUserContext(userId, async () => { - const member = await prismaClient.workspaceMember.findUnique({ + return withUserContext(userId, async (tx) => { + const member = await tx.workspaceMember.findUnique({ where: { workspaceId_userId: { workspaceId, @@ -195,9 +198,8 @@ export async function verifyWorkspaceAccess( * ``` */ export async function getUserWorkspaces(userId: string) { - const prismaClient = getPrismaInstance(); - return withUserContext(userId, async () => { - return prismaClient.workspace.findMany({ + return withUserContext(userId, async (tx) => { + return tx.workspace.findMany({ include: { members: { where: { userId }, @@ -219,9 +221,8 @@ export async function isWorkspaceAdmin( userId: string, workspaceId: string ): Promise { - const prismaClient = getPrismaInstance(); - return withUserContext(userId, async () => { - const member = await prismaClient.workspaceMember.findUnique({ + return withUserContext(userId, async (tx) => { + const member = await tx.workspaceMember.findUnique({ where: { workspaceId_userId: { workspaceId, diff --git a/apps/api/src/projects/dto/query-projects.dto.ts b/apps/api/src/projects/dto/query-projects.dto.ts index c108813..f8e24d1 100644 --- a/apps/api/src/projects/dto/query-projects.dto.ts +++ b/apps/api/src/projects/dto/query-projects.dto.ts @@ -14,8 +14,9 @@ import { Type } from "class-transformer"; * DTO for querying projects with filters and pagination */ export class QueryProjectsDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsEnum(ProjectStatus, { message: "status must be a valid ProjectStatus" }) diff --git a/apps/api/src/tasks/dto/query-tasks.dto.ts b/apps/api/src/tasks/dto/query-tasks.dto.ts index cb0fda0..e4f56b9 100644 --- a/apps/api/src/tasks/dto/query-tasks.dto.ts +++ b/apps/api/src/tasks/dto/query-tasks.dto.ts @@ -14,8 +14,9 @@ import { Type } from "class-transformer"; * DTO for querying tasks with filters and pagination */ export class QueryTasksDto { + @IsOptional() @IsUUID("4", { message: "workspaceId must be a valid UUID" }) - workspaceId!: string; + workspaceId?: string; @IsOptional() @IsEnum(TaskStatus, { message: "status must be a valid TaskStatus" })