From 013061c0919d93ccb66279d55f8a14417543b816 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:43:36 -0400 Subject: [PATCH 1/8] feat(rbac): allow toggling employee compliance obligation on built-in roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Customers can now turn the Employee Compliance obligation on/off for built-in roles (owner, admin, etc.) directly from the system role detail page. The toggle was previously locked, so an organization had no way to disable the compliance tasks owners are required to complete by default — or to opt admins into them — without creating a custom role. Storage reuses the existing `organization_role.obligations` JSON column: an upserted row keyed by the built-in role name acts as a per-org override and wins over `BUILT_IN_ROLE_OBLIGATIONS` in every read path (compliance member filter, portal access, frontend permissions hook, policy acknowledgment digest). Permissions stay sourced from the hardcoded defaults — only the obligation is overridable. Override rows are filtered out of `listRoles`'s custom-roles list and surfaced as effective obligations on the built-in entries instead. New API endpoints: - GET /v1/roles/built-in/:name/obligations - PATCH /v1/roles/built-in/:name/obligations Co-Authored-By: Claude Opus 4.7 (1M context) --- .../dto/update-built-in-obligations.dto.ts | 12 ++ apps/api/src/roles/roles.controller.ts | 70 ++++++++ apps/api/src/roles/roles.service.spec.ts | 154 +++++++++++++++++- apps/api/src/roles/roles.service.ts | 120 ++++++++++++-- .../roles/components/PermissionMatrix.tsx | 13 +- .../settings/roles/system/[roleName]/page.tsx | 18 +- .../system/[roleName]/system-role-detail.tsx | 36 +++- apps/app/src/hooks/use-permissions.ts | 32 ++-- apps/app/src/lib/compliance.ts | 34 ++-- ...licy-acknowledgment-digest-helpers.test.ts | 27 +++ .../policy-acknowledgment-digest-helpers.ts | 35 ++-- apps/portal/src/utils/portal-access.ts | 46 ++++-- 12 files changed, 512 insertions(+), 85 deletions(-) create mode 100644 apps/api/src/roles/dto/update-built-in-obligations.dto.ts diff --git a/apps/api/src/roles/dto/update-built-in-obligations.dto.ts b/apps/api/src/roles/dto/update-built-in-obligations.dto.ts new file mode 100644 index 0000000000..21f4655770 --- /dev/null +++ b/apps/api/src/roles/dto/update-built-in-obligations.dto.ts @@ -0,0 +1,12 @@ +import { ApiProperty } from '@nestjs/swagger'; +import { IsObject } from 'class-validator'; + +export class UpdateBuiltInObligationsDto { + @ApiProperty({ + description: 'Obligations override for the built-in role.', + example: { compliance: false }, + required: true, + }) + @IsObject() + obligations!: { compliance?: boolean } & Record; +} diff --git a/apps/api/src/roles/roles.controller.ts b/apps/api/src/roles/roles.controller.ts index 6515a9e0ca..d28ccd3242 100644 --- a/apps/api/src/roles/roles.controller.ts +++ b/apps/api/src/roles/roles.controller.ts @@ -23,6 +23,7 @@ import { PermissionGuard } from '../auth/permission.guard'; import { RequirePermission } from '../auth/require-permission.decorator'; import type { AuthContext as AuthContextType } from '../auth/types'; import { CreateRoleDto } from './dto/create-role.dto'; +import { UpdateBuiltInObligationsDto } from './dto/update-built-in-obligations.dto'; import { UpdateRoleDto } from './dto/update-role.dto'; import { RolesService } from './roles.service'; @@ -172,6 +173,75 @@ export class RolesController { return { permissions, obligations }; } + @Get('built-in/:name/obligations') + @RequirePermission('ac', 'read') + @ApiOperation({ + summary: 'Get obligations for a built-in role', + description: + 'Returns the effective obligations for a built-in role (owner, admin, auditor, employee, contractor) — DB override if present, else the hardcoded default.', + }) + @ApiParam({ name: 'name', description: 'Built-in role name', example: 'owner' }) + @ApiResponse({ + status: 200, + description: 'Effective obligations for the built-in role', + schema: { + type: 'object', + properties: { + name: { type: 'string' }, + obligations: { + type: 'object', + properties: { compliance: { type: 'boolean' } }, + }, + }, + }, + }) + async getBuiltInObligations( + @OrganizationId() organizationId: string, + @Param('name') name: string, + ) { + const obligations = await this.rolesService.getBuiltInObligations( + organizationId, + name, + ); + return { name, obligations }; + } + + @Patch('built-in/:name/obligations') + @RequirePermission('ac', 'update') + @ApiOperation({ + summary: 'Update obligations for a built-in role', + description: + 'Override the obligations for a built-in role (e.g., turn off the compliance obligation for owners). Permissions stay sourced from the hardcoded defaults.', + }) + @ApiParam({ name: 'name', description: 'Built-in role name', example: 'owner' }) + @ApiBody({ type: UpdateBuiltInObligationsDto }) + @ApiResponse({ + status: 200, + description: 'Obligations updated', + schema: { + type: 'object', + properties: { + name: { type: 'string' }, + obligations: { + type: 'object', + properties: { compliance: { type: 'boolean' } }, + }, + }, + }, + }) + @ApiResponse({ status: 400, description: 'Not a built-in role' }) + async updateBuiltInObligations( + @OrganizationId() organizationId: string, + @Param('name') name: string, + @Body() dto: UpdateBuiltInObligationsDto, + ) { + return this.rolesService.updateBuiltInObligations( + organizationId, + name, + dto.obligations, + ); + } + @Get(':roleId') @RequirePermission('ac', 'read') @ApiOperation({ diff --git a/apps/api/src/roles/roles.service.spec.ts b/apps/api/src/roles/roles.service.spec.ts index b5677628a2..cebf6ad700 100644 --- a/apps/api/src/roles/roles.service.spec.ts +++ b/apps/api/src/roles/roles.service.spec.ts @@ -74,7 +74,7 @@ jest.mock('@trycompai/auth', () => { const BUILT_IN_ROLE_OBLIGATIONS: Record> = { owner: { compliance: true }, - admin: { compliance: true }, + admin: {}, auditor: {}, employee: { compliance: true }, contractor: { compliance: true }, @@ -97,6 +97,7 @@ jest.mock('@db', () => ({ count: jest.fn(), create: jest.fn(), update: jest.fn(), + upsert: jest.fn(), delete: jest.fn(), }, member: { @@ -713,4 +714,155 @@ describe('RolesService', () => { expect(result.map((m) => m.id)).toEqual(['m1']); }); }); + + describe('getBuiltInObligations', () => { + const organizationId = 'org_1'; + + it('returns the hardcoded default when no override row exists', async () => { + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue(null); + const result = await service.getBuiltInObligations(organizationId, 'owner'); + expect(result).toEqual({ compliance: true }); + }); + + it('returns the DB override when one exists (string JSON)', async () => { + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue({ + obligations: JSON.stringify({ compliance: false }), + }); + const result = await service.getBuiltInObligations(organizationId, 'owner'); + expect(result).toEqual({ compliance: false }); + }); + + it('returns the DB override when one exists (object JSON)', async () => { + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue({ + obligations: { compliance: false }, + }); + const result = await service.getBuiltInObligations(organizationId, 'owner'); + expect(result).toEqual({ compliance: false }); + }); + + it('returns empty for admin (no default compliance, no override)', async () => { + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue(null); + const result = await service.getBuiltInObligations(organizationId, 'admin'); + expect(result).toEqual({}); + }); + + it('rejects unknown role names', async () => { + await expect( + service.getBuiltInObligations(organizationId, 'not-a-built-in'), + ).rejects.toThrow(BadRequestException); + }); + }); + + describe('updateBuiltInObligations', () => { + const organizationId = 'org_1'; + + it('upserts an organization_role row with the built-in name', async () => { + (mockDb.organizationRole.upsert as jest.Mock).mockResolvedValue({ + name: 'owner', + obligations: JSON.stringify({ compliance: false }), + }); + + const result = await service.updateBuiltInObligations( + organizationId, + 'owner', + { compliance: false }, + ); + + expect(result).toEqual({ name: 'owner', obligations: { compliance: false } }); + expect(mockDb.organizationRole.upsert).toHaveBeenCalledWith({ + where: { organizationId_name: { organizationId, name: 'owner' } }, + create: expect.objectContaining({ + organizationId, + name: 'owner', + obligations: JSON.stringify({ compliance: false }), + }), + update: { obligations: JSON.stringify({ compliance: false }) }, + }); + }); + + it('rejects unknown role names', async () => { + await expect( + service.updateBuiltInObligations(organizationId, 'not-a-built-in', { + compliance: true, + }), + ).rejects.toThrow(BadRequestException); + expect(mockDb.organizationRole.upsert).not.toHaveBeenCalled(); + }); + }); + + describe('getObligationsForRoles with built-in overrides', () => { + const organizationId = 'org_1'; + + it('returns hardcoded defaults when no DB rows match', async () => { + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([]); + const result = await service.getObligationsForRoles(organizationId, [ + 'owner', + 'admin', + ]); + // owner has compliance:true by default, admin has none — union is true + expect(result).toEqual({ compliance: true }); + }); + + it('DB override beats the hardcoded default (owner override → no compliance)', async () => { + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([ + { name: 'owner', obligations: JSON.stringify({ compliance: false }) }, + ]); + const result = await service.getObligationsForRoles(organizationId, [ + 'owner', + ]); + expect(result).toEqual({}); + }); + + it('admin override can opt INTO compliance even though default is none', async () => { + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([ + { name: 'admin', obligations: JSON.stringify({ compliance: true }) }, + ]); + const result = await service.getObligationsForRoles(organizationId, [ + 'admin', + ]); + expect(result).toEqual({ compliance: true }); + }); + }); + + describe('listRoles built-in overrides', () => { + const organizationId = 'org_1'; + + it('hides override rows from customRoles and reflects them on builtInRoles', async () => { + const ownerOverride = { + id: 'rol_owner_override', + name: 'owner', + permissions: '{}', + obligations: JSON.stringify({ compliance: false }), + organizationId, + createdAt: new Date(), + updatedAt: new Date(), + }; + const customRole = { + id: 'rol_custom_1', + name: 'compliance-lead', + permissions: JSON.stringify({ control: ['read'] }), + obligations: '{}', + organizationId, + createdAt: new Date(), + updatedAt: new Date(), + }; + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([ + ownerOverride, + customRole, + ]); + (mockDb.member.count as jest.Mock).mockResolvedValue(0); + + const result = await service.listRoles(organizationId); + // Override row must not appear as a custom role + expect(result.customRoles.map((r) => r.name)).toEqual(['compliance-lead']); + // Built-in entries carry effective obligations — owner reflects the override + const ownerEntry = result.builtInRoles.find((r) => r.name === 'owner'); + expect(ownerEntry?.obligations).toEqual({ compliance: false }); + // Other built-ins still show their hardcoded defaults + const adminEntry = result.builtInRoles.find((r) => r.name === 'admin'); + expect(adminEntry?.obligations).toEqual({}); + const employeeEntry = result.builtInRoles.find((r) => r.name === 'employee'); + expect(employeeEntry?.obligations).toEqual({ compliance: true }); + }); + }); }); diff --git a/apps/api/src/roles/roles.service.ts b/apps/api/src/roles/roles.service.ts index 42cdccf5ff..082c492d51 100644 --- a/apps/api/src/roles/roles.service.ts +++ b/apps/api/src/roles/roles.service.ts @@ -226,11 +226,20 @@ export class RolesService { * List all roles for an organization (built-in + custom) */ async listRoles(organizationId: string) { - // Get custom roles - const customRoles = await db.organizationRole.findMany({ + // Get all organization_role rows; rows named after a built-in role are + // obligation overrides, not custom roles. + const allRows = await db.organizationRole.findMany({ where: { organizationId }, orderBy: { createdAt: 'desc' }, }); + const overrideByName = new Map(); + const customRoles = allRows.filter((r) => { + if (BUILT_IN_ROLES.includes(r.name)) { + overrideByName.set(r.name, r); + return false; + } + return true; + }); // Get member counts for custom roles const memberCounts = await Promise.all( @@ -243,12 +252,21 @@ export class RolesService { ); const countMap = new Map(memberCounts.map((mc) => [mc.roleId, mc.count])); - // Include built-in roles info - const builtInRoles = BUILT_IN_ROLES.map((name) => ({ - name, - isBuiltIn: true, - description: this.getBuiltInRoleDescription(name), - })); + // Include built-in roles info (with effective obligations: override or default) + const builtInRoles = BUILT_IN_ROLES.map((name) => { + const override = overrideByName.get(name); + const obligations = override + ? (typeof override.obligations === 'string' + ? (JSON.parse(override.obligations) as RoleObligations) + : ((override.obligations as RoleObligations) || {})) + : (BUILT_IN_ROLE_OBLIGATIONS[name] ?? {}); + return { + name, + isBuiltIn: true, + description: this.getBuiltInRoleDescription(name), + obligations, + }; + }); return { builtInRoles, @@ -533,8 +551,11 @@ export class RolesService { } /** - * Get merged obligations for a list of custom role names. - * Used by the frontend to resolve effective obligations for custom roles. + * Get merged obligations for a list of role names (custom + built-in). + * + * Built-in roles default to `BUILT_IN_ROLE_OBLIGATIONS[name]`, but an + * organization can override by storing an `organization_role` row with the + * built-in name — the DB row wins when present. */ async getObligationsForRoles( organizationId: string, @@ -542,23 +563,88 @@ export class RolesService { ): Promise { if (roleNames.length === 0) return {}; - const customRoles = await db.organizationRole.findMany({ + const dbRoles = await db.organizationRole.findMany({ where: { organizationId, name: { in: roleNames } }, select: { name: true, obligations: true }, }); - - const combined: RoleObligations = {}; - for (const role of customRoles) { + const overrideByName = new Map(); + for (const role of dbRoles) { const obligations = typeof role.obligations === 'string' - ? JSON.parse(role.obligations) - : role.obligations || {}; - if (obligations.compliance) combined.compliance = true; + ? (JSON.parse(role.obligations) as RoleObligations) + : ((role.obligations as RoleObligations) || {}); + overrideByName.set(role.name, obligations); + } + + const combined: RoleObligations = {}; + for (const name of roleNames) { + const fromDb = overrideByName.get(name); + const effective = + fromDb ?? (BUILT_IN_ROLES.includes(name) ? BUILT_IN_ROLE_OBLIGATIONS[name] ?? {} : {}); + if (effective.compliance) combined.compliance = true; } return combined; } + /** + * Upsert an obligation override for a built-in role. The `organization_role` + * row stores only the obligations JSON; permissions stay sourced from the + * hardcoded `BUILT_IN_ROLE_PERMISSIONS` map. + */ + async updateBuiltInObligations( + organizationId: string, + roleName: string, + obligations: RoleObligations, + ) { + if (!BUILT_IN_ROLES.includes(roleName)) { + throw new BadRequestException(`Not a built-in role: ${roleName}`); + } + + const permissions = JSON.stringify(BUILT_IN_ROLE_PERMISSIONS[roleName] ?? {}); + const obligationsJson = JSON.stringify(obligations); + + const role = await db.organizationRole.upsert({ + where: { organizationId_name: { organizationId, name: roleName } }, + create: { + organizationId, + name: roleName, + permissions, + obligations: obligationsJson, + }, + update: { obligations: obligationsJson }, + }); + + return { + name: role.name, + obligations: JSON.parse(role.obligations) as RoleObligations, + }; + } + + /** + * Read the obligations for a single built-in role for this organization — + * DB override if present, else the hardcoded default. + */ + async getBuiltInObligations( + organizationId: string, + roleName: string, + ): Promise { + if (!BUILT_IN_ROLES.includes(roleName)) { + throw new BadRequestException(`Not a built-in role: ${roleName}`); + } + + const override = await db.organizationRole.findFirst({ + where: { organizationId, name: roleName }, + select: { obligations: true }, + }); + if (override) { + return typeof override.obligations === 'string' + ? (JSON.parse(override.obligations) as RoleObligations) + : ((override.obligations as RoleObligations) || {}); + } + return BUILT_IN_ROLE_OBLIGATIONS[roleName] ?? {}; + } + /** * Get description for built-in roles */ diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/components/PermissionMatrix.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/components/PermissionMatrix.tsx index b499140ee8..d9fac52325 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/components/PermissionMatrix.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/components/PermissionMatrix.tsx @@ -104,7 +104,15 @@ interface PermissionMatrixProps { onChange: (permissions: Record) => void; obligations?: Record; onObligationsChange?: (obligations: Record) => void; + /** Disables the whole matrix. */ disabled?: boolean; + /** + * When true, keeps the permission matrix and access toggles disabled but + * leaves the obligation toggles editable. Used on built-in role pages so + * customers can opt in/out of the compliance obligation without being able + * to edit the role's permissions. + */ + obligationsEditable?: boolean; } /** @@ -215,7 +223,8 @@ function AccessToggle({ ); } -export function PermissionMatrix({ value, onChange, obligations, onObligationsChange, disabled = false }: PermissionMatrixProps) { +export function PermissionMatrix({ value, onChange, obligations, onObligationsChange, disabled = false, obligationsEditable = false }: PermissionMatrixProps) { + const obligationsDisabled = disabled && !obligationsEditable; const handleObligationChange = (key: string, enabled: boolean) => { if (!onObligationsChange) return; const newObligations = { ...obligations }; @@ -306,7 +315,7 @@ export function PermissionMatrix({ value, onChange, obligations, onObligationsCh toggle={toggle} enabled={Boolean(obligations?.[toggle.key])} onToggle={(enabled) => handleObligationChange(toggle.key, enabled)} - disabled={disabled} + disabled={obligationsDisabled} /> ))} diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx index a32fa3b991..7a6a42e4c2 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx @@ -3,6 +3,7 @@ import type { Metadata } from 'next'; import Link from 'next/link'; import { notFound } from 'next/navigation'; import { BUILT_IN_ROLE_OBLIGATIONS } from '@trycompai/auth'; +import { serverApi } from '@/lib/api-server'; import { SYSTEM_ROLES, SYSTEM_ROLE_PERMISSIONS } from '../../constants/system-roles'; import { SystemRoleDetail } from './system-role-detail'; @@ -20,6 +21,16 @@ export default async function SystemRolePage({ notFound(); } + // Resolve the effective obligations for this org — falls back to the + // hardcoded default if the API call fails or no override is set. + const overrideRes = await serverApi.get<{ + name: string; + obligations: Record; + }>(`/v1/roles/built-in/${encodeURIComponent(roleName)}/obligations`); + const effectiveObligations = + overrideRes.data?.obligations ?? + ((BUILT_IN_ROLE_OBLIGATIONS[roleName] || {}) as Record); + return ( - } description={role.description} /> + ); } diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx index 7f146c9e69..85af89abf9 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx @@ -1,16 +1,45 @@ 'use client'; +import { useState } from 'react'; import { Card, CardContent } from '@trycompai/ui/card'; import { Stack, Text } from '@trycompai/design-system'; +import { toast } from 'sonner'; +import { apiClient } from '@/lib/api-client'; import { PermissionMatrix } from '../../components/PermissionMatrix'; interface SystemRoleDetailProps { + roleName: string; permissions: Record; obligations: Record; description: string; } -export function SystemRoleDetail({ permissions, obligations, description }: SystemRoleDetailProps) { +export function SystemRoleDetail({ roleName, permissions, obligations, description }: SystemRoleDetailProps) { + const [currentObligations, setCurrentObligations] = + useState>(obligations); + const [isSaving, setIsSaving] = useState(false); + + const handleObligationsChange = async (next: Record) => { + if (isSaving) return; + const previous = currentObligations; + setCurrentObligations(next); + setIsSaving(true); + try { + const res = await apiClient.patch( + `/v1/roles/built-in/${encodeURIComponent(roleName)}/obligations`, + { obligations: next }, + ); + if (res.error) throw new Error(res.error); + } catch (err) { + setCurrentObligations(previous); + toast.error(err instanceof Error ? err.message : 'Failed to update obligation'); + return; + } finally { + setIsSaving(false); + } + toast.success('Obligation updated'); + }; + return ( @@ -21,9 +50,10 @@ export function SystemRoleDetail({ permissions, obligations, description }: Syst {}} - obligations={obligations} - onObligationsChange={() => {}} + obligations={currentObligations} + onObligationsChange={handleObligationsChange} disabled + obligationsEditable /> diff --git a/apps/app/src/hooks/use-permissions.ts b/apps/app/src/hooks/use-permissions.ts index 95feb67493..254f0feef1 100644 --- a/apps/app/src/hooks/use-permissions.ts +++ b/apps/app/src/hooks/use-permissions.ts @@ -24,10 +24,11 @@ export function usePermissions() { const roleString = activeMember?.role ?? null; // Resolve built-in roles synchronously - const { permissions: builtInPerms, customRoleNames } = - resolveBuiltInPermissions(roleString); + const { permissions: builtInPerms } = resolveBuiltInPermissions(roleString); - // Resolve built-in obligations synchronously + // Resolve built-in obligations synchronously — used as the initial value + // until the server returns the effective obligations (which include any + // per-organization overrides on the built-in roles). const roleNames = parseRolesString(roleString); const builtInObligations: Record = {}; for (const name of roleNames) { @@ -41,14 +42,13 @@ export function usePermissions() { } } - // Fetch custom role permissions (and obligations) if needed (SWR-cached) + // Query the resolver for all role names — custom roles contribute + // permissions, and built-in roles may carry a per-org obligation override. const { data: customData } = useSWR( - customRoleNames.length > 0 - ? ['/v1/roles/permissions', ...customRoleNames] - : null, + roleNames.length > 0 ? ['/v1/roles/permissions', ...roleNames] : null, async () => { const res = await apiClient.get( - `/v1/roles/permissions?roles=${customRoleNames.join(',')}`, + `/v1/roles/permissions?roles=${roleNames.join(',')}`, ); return { permissions: res.data?.permissions ?? {}, @@ -68,13 +68,15 @@ export function usePermissions() { mergePermissions(permissions, customData.permissions); } - // Merge built-in + custom obligations - const obligations: Record = { ...builtInObligations }; - if (customData?.obligations) { - for (const [key, val] of Object.entries(customData.obligations)) { - if (val) obligations[key] = true; - } - } + // Once the server response arrives it represents the effective obligations + // (defaults merged with any overrides). Before it arrives, fall back to the + // synchronous built-in defaults so initial render is correct for the common + // case (no overrides). + const obligations: Record = customData?.obligations + ? Object.fromEntries( + Object.entries(customData.obligations).filter(([, val]) => val), + ) + : { ...builtInObligations }; // CS-189: separate "did a custom role grant this permission" from the // merged permissions, so the Auditor View visibility check can distinguish diff --git a/apps/app/src/lib/compliance.ts b/apps/app/src/lib/compliance.ts index c047b0882b..9ec049513e 100644 --- a/apps/app/src/lib/compliance.ts +++ b/apps/app/src/lib/compliance.ts @@ -82,24 +82,25 @@ export async function filterComplianceMembers( if (members.length === 0) return []; const builtInRoleNames = new Set(Object.keys(BUILT_IN_ROLE_OBLIGATIONS)); - const allCustomRoleNames = new Set(); + const allRoleNames = new Set(); const memberRoles = members.map((member) => { const roleNames = parseRolesString(member.role); - const customNames = roleNames.filter((n) => !builtInRoleNames.has(n)); - for (const name of customNames) allCustomRoleNames.add(name); + for (const name of roleNames) allRoleNames.add(name); return { member, roleNames }; }); - // Single DB query for custom role obligations - let customObligationMap: Record> = {}; - if (allCustomRoleNames.size > 0) { - const customRoles = await db.organizationRole.findMany({ - where: { organizationId, name: { in: [...allCustomRoleNames] } }, + // Single DB query for role obligations (custom + built-in overrides). + // A row in organization_role named after a built-in role overrides the + // hardcoded default for that organization. + let obligationMap: Record> = {}; + if (allRoleNames.size > 0) { + const dbRoles = await db.organizationRole.findMany({ + where: { organizationId, name: { in: [...allRoleNames] } }, select: { name: true, obligations: true }, }); - customObligationMap = Object.fromEntries( - customRoles.map((r) => { + obligationMap = Object.fromEntries( + dbRoles.map((r) => { const obligations = typeof r.obligations === 'string' ? JSON.parse(r.obligations) : (r.obligations || {}); @@ -113,10 +114,15 @@ export async function filterComplianceMembers( // Platform admins are excluded — they join customer orgs to debug if (member.user?.role === 'admin') return false; for (const name of roleNames) { - const builtIn = BUILT_IN_ROLE_OBLIGATIONS[name]; - if (builtIn?.compliance) return true; - const custom = customObligationMap[name]; - if (custom?.compliance) return true; + // DB override wins over the hardcoded built-in default + if (name in obligationMap) { + if (obligationMap[name]?.compliance) return true; + continue; + } + if (builtInRoleNames.has(name)) { + const builtIn = BUILT_IN_ROLE_OBLIGATIONS[name]; + if (builtIn?.compliance) return true; + } } return false; }) diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts index f9a9026e04..09c09313f2 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts @@ -132,4 +132,31 @@ describe('filterDigestMembersByCompliance', () => { ); expect(result.map((m) => m.id)).toEqual(['m4']); }); + + it('respects per-org override that turns OFF compliance on a built-in role', async () => { + // employee defaults to compliance:true — an organization can opt out by + // storing an obligation override row for the built-in role name. + mockDb.organizationRole.findMany.mockResolvedValueOnce([ + { name: 'employee', obligations: { compliance: false } }, + ]); + const member: DigestMember = { + id: 'm6', role: 'employee', department: 'it', + user: { id: 'u6', name: 'F', email: 'f@x', role: null }, + }; + const result = await filterDigestMembersByCompliance(mockDb, [member], 'org_1'); + expect(result).toEqual([]); + }); + + it('respects per-org override that turns ON compliance on a built-in role', async () => { + // admin defaults to no compliance — an org can opt in via override. + mockDb.organizationRole.findMany.mockResolvedValueOnce([ + { name: 'admin', obligations: { compliance: true } }, + ]); + const member: DigestMember = { + id: 'm7', role: 'admin', department: 'it', + user: { id: 'u7', name: 'G', email: 'g@x', role: null }, + }; + const result = await filterDigestMembersByCompliance(mockDb, [member], 'org_1'); + expect(result).toEqual([member]); + }); }); diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts index f35f81604c..686f0394cf 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts @@ -8,7 +8,7 @@ import type { Departments, PolicyVisibility } from '@db'; // Keep in sync with packages/auth/src/permissions.ts BUILT_IN_ROLE_OBLIGATIONS. const BUILT_IN_ROLE_OBLIGATIONS: Record = { owner: { compliance: true }, - admin: { compliance: true }, + admin: {}, auditor: {}, employee: { compliance: true }, contractor: { compliance: true }, @@ -37,26 +37,26 @@ export async function filterDigestMembersByCompliance( ): Promise { if (members.length === 0) return []; - const customRoleNames = new Set(); + const allRoleNames = new Set(); const parsed = members.map((member) => { const roleNames = member.role .split(',') .map((r) => r.trim()) .filter(Boolean); - for (const name of roleNames) { - if (!BUILT_IN_ROLE_NAMES.has(name)) customRoleNames.add(name); - } + for (const name of roleNames) allRoleNames.add(name); return { member, roleNames }; }); - let customObligationMap: Record = {}; - if (customRoleNames.size > 0) { - const customRoles = await db.organizationRole.findMany({ - where: { organizationId, name: { in: [...customRoleNames] } }, + // DB query covers both custom roles AND obligation overrides on built-in + // roles (rows in organization_role named after a built-in role). + let obligationMap: Record = {}; + if (allRoleNames.size > 0) { + const dbRoles = await db.organizationRole.findMany({ + where: { organizationId, name: { in: [...allRoleNames] } }, select: { name: true, obligations: true }, }); - customObligationMap = Object.fromEntries( - customRoles.map((r) => { + obligationMap = Object.fromEntries( + dbRoles.map((r) => { const obligations = typeof r.obligations === 'string' ? JSON.parse(r.obligations) @@ -71,10 +71,15 @@ export async function filterDigestMembersByCompliance( // Platform admins are excluded — matches the @/lib/compliance behavior. if (member.user?.role === 'admin') return false; for (const name of roleNames) { - const builtIn = BUILT_IN_ROLE_OBLIGATIONS[name]; - if (builtIn?.compliance) return true; - const custom = customObligationMap[name]; - if (custom?.compliance) return true; + // DB override wins over the hardcoded built-in default + if (name in obligationMap) { + if (obligationMap[name]?.compliance) return true; + continue; + } + if (BUILT_IN_ROLE_NAMES.has(name)) { + const builtIn = BUILT_IN_ROLE_OBLIGATIONS[name]; + if (builtIn?.compliance) return true; + } } return false; }) diff --git a/apps/portal/src/utils/portal-access.ts b/apps/portal/src/utils/portal-access.ts index 14016f0de7..956179bf77 100644 --- a/apps/portal/src/utils/portal-access.ts +++ b/apps/portal/src/utils/portal-access.ts @@ -16,30 +16,42 @@ export async function hasPortalAccess({ const roles = roleString.split(',').map((r) => r.trim()).filter(Boolean); const permissions: Record = {}; let hasComplianceObligation = false; - const customRoleNames: string[] = []; + // Built-in role permissions come from the hardcoded map. Obligations can be + // overridden per-org by a row in organization_role with the built-in name. for (const role of roles) { const builtInPerms = BUILT_IN_ROLE_PERMISSIONS[role]; - if (builtInPerms) { - mergePermissions(permissions, builtInPerms); - if (BUILT_IN_ROLE_OBLIGATIONS[role]?.compliance) { - hasComplianceObligation = true; - } - } else { - customRoleNames.push(role); - } + if (builtInPerms) mergePermissions(permissions, builtInPerms); } - if (customRoleNames.length > 0) { - const customRoles = await db.organizationRole.findMany({ - where: { organizationId, name: { in: customRoleNames } }, - select: { permissions: true, obligations: true }, + if (roles.length > 0) { + const dbRoles = await db.organizationRole.findMany({ + where: { organizationId, name: { in: roles } }, + select: { name: true, permissions: true, obligations: true }, }); + type DbRoleRow = (typeof dbRoles)[number]; + const overrideByName = new Map( + dbRoles.map((r: DbRoleRow) => [r.name, r] as const), + ); + + for (const role of roles) { + const dbRow = overrideByName.get(role); + const isBuiltIn = Boolean(BUILT_IN_ROLE_PERMISSIONS[role]); + + if (dbRow) { + // For custom roles, the DB row is the source of permissions. + // For built-in roles, we only consult it for the obligations override. + if (!isBuiltIn) { + const perms = parseRolePermissions(dbRow.permissions); + if (perms) mergePermissions(permissions, perms); + } + if (parseRoleObligations(dbRow.obligations).compliance) { + hasComplianceObligation = true; + } + continue; + } - for (const role of customRoles) { - const perms = parseRolePermissions(role.permissions); - if (perms) mergePermissions(permissions, perms); - if (parseRoleObligations(role.obligations).compliance) { + if (isBuiltIn && BUILT_IN_ROLE_OBLIGATIONS[role]?.compliance) { hasComplianceObligation = true; } } From 9e6fe49a6c34323af18e64b07017813f749f6f93 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:49:22 -0400 Subject: [PATCH 2/8] chore(rbac): limit editable built-in obligations to owner + admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The customer request was specifically about Admin and Owner — keep the toggle locked for auditor, employee, and contractor so we don't expand the surface beyond what was asked. Enforced both client-side (`obligationsEditable` only flips on for owner/admin in the system role page) and server-side (`updateBuiltInObligations` rejects the rest with a 400). Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/api/src/roles/roles.service.spec.ts | 23 +++++++++++++++++++ apps/api/src/roles/roles.service.ts | 13 +++++++++++ .../settings/roles/system/[roleName]/page.tsx | 6 +++++ .../system/[roleName]/system-role-detail.tsx | 8 ++++--- 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/apps/api/src/roles/roles.service.spec.ts b/apps/api/src/roles/roles.service.spec.ts index cebf6ad700..aedf2e5624 100644 --- a/apps/api/src/roles/roles.service.spec.ts +++ b/apps/api/src/roles/roles.service.spec.ts @@ -788,6 +788,29 @@ describe('RolesService', () => { ).rejects.toThrow(BadRequestException); expect(mockDb.organizationRole.upsert).not.toHaveBeenCalled(); }); + + it('rejects built-in roles whose obligations are not user-editable', async () => { + for (const locked of ['auditor', 'employee', 'contractor']) { + await expect( + service.updateBuiltInObligations(organizationId, locked, { + compliance: false, + }), + ).rejects.toThrow(BadRequestException); + } + expect(mockDb.organizationRole.upsert).not.toHaveBeenCalled(); + }); + + it('accepts owner and admin', async () => { + (mockDb.organizationRole.upsert as jest.Mock).mockResolvedValue({ + name: 'admin', + obligations: JSON.stringify({ compliance: true }), + }); + await expect( + service.updateBuiltInObligations(organizationId, 'admin', { + compliance: true, + }), + ).resolves.toEqual({ name: 'admin', obligations: { compliance: true } }); + }); }); describe('getObligationsForRoles with built-in overrides', () => { diff --git a/apps/api/src/roles/roles.service.ts b/apps/api/src/roles/roles.service.ts index 082c492d51..7e7665eb1e 100644 --- a/apps/api/src/roles/roles.service.ts +++ b/apps/api/src/roles/roles.service.ts @@ -23,6 +23,11 @@ const VALID_RESOURCES: Record = Object.fromEntries( // Built-in roles that cannot be modified or deleted const BUILT_IN_ROLES = Object.keys(allRoles); +// Subset of built-in roles whose obligations the customer can override. Today +// only owner and admin — the others keep their hardcoded defaults to avoid +// changing behavior for roles the request didn't cover. +const EDITABLE_BUILT_IN_OBLIGATION_ROLES = new Set(['owner', 'admin']); + @Injectable() export class RolesService { /** @@ -591,6 +596,9 @@ export class RolesService { * Upsert an obligation override for a built-in role. The `organization_role` * row stores only the obligations JSON; permissions stay sourced from the * hardcoded `BUILT_IN_ROLE_PERMISSIONS` map. + * + * Only owner and admin obligations are user-overridable today (matches the + * customer request); the other built-in roles keep their hardcoded defaults. */ async updateBuiltInObligations( organizationId: string, @@ -600,6 +608,11 @@ export class RolesService { if (!BUILT_IN_ROLES.includes(roleName)) { throw new BadRequestException(`Not a built-in role: ${roleName}`); } + if (!EDITABLE_BUILT_IN_OBLIGATION_ROLES.has(roleName)) { + throw new BadRequestException( + `Obligations are not editable for role: ${roleName}`, + ); + } const permissions = JSON.stringify(BUILT_IN_ROLE_PERMISSIONS[roleName] ?? {}); const obligationsJson = JSON.stringify(obligations); diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx index 7a6a42e4c2..f606c1d6fa 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/page.tsx @@ -7,6 +7,11 @@ import { serverApi } from '@/lib/api-server'; import { SYSTEM_ROLES, SYSTEM_ROLE_PERMISSIONS } from '../../constants/system-roles'; import { SystemRoleDetail } from './system-role-detail'; +// Customers can opt admins/owners in or out of the Employee Compliance +// obligation; the other built-in roles keep their obligation locked because +// the request only covered admin + owner. +const EDITABLE_OBLIGATION_ROLES = new Set(['owner', 'admin']); + export default async function SystemRolePage({ params, }: { @@ -49,6 +54,7 @@ export default async function SystemRolePage({ permissions={permissions} obligations={effectiveObligations} description={role.description} + obligationsEditable={EDITABLE_OBLIGATION_ROLES.has(roleName)} /> ); diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx index 85af89abf9..05aae0bc93 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx @@ -12,15 +12,17 @@ interface SystemRoleDetailProps { permissions: Record; obligations: Record; description: string; + /** When false, the obligation toggle stays locked along with the rest of the matrix. */ + obligationsEditable?: boolean; } -export function SystemRoleDetail({ roleName, permissions, obligations, description }: SystemRoleDetailProps) { +export function SystemRoleDetail({ roleName, permissions, obligations, description, obligationsEditable = false }: SystemRoleDetailProps) { const [currentObligations, setCurrentObligations] = useState>(obligations); const [isSaving, setIsSaving] = useState(false); const handleObligationsChange = async (next: Record) => { - if (isSaving) return; + if (isSaving || !obligationsEditable) return; const previous = currentObligations; setCurrentObligations(next); setIsSaving(true); @@ -53,7 +55,7 @@ export function SystemRoleDetail({ roleName, permissions, obligations, descripti obligations={currentObligations} onObligationsChange={handleObligationsChange} disabled - obligationsEditable + obligationsEditable={obligationsEditable} /> From 16f6e28d9b30ce9fd640482d3efa670ef22bd710 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 17:54:01 -0400 Subject: [PATCH 3/8] chore(rbac): revert unrelated digest helper snapshot fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverts the change to the inlined BUILT_IN_ROLE_OBLIGATIONS in the digest helper — the stale `admin: { compliance: true }` was pre-existing and not in scope for this PR. The override merge logic stays so the owner/admin toggle still flows through to the digest when an override row exists. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trigger/tasks/task/policy-acknowledgment-digest-helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts index 686f0394cf..737de4e08c 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts @@ -8,7 +8,7 @@ import type { Departments, PolicyVisibility } from '@db'; // Keep in sync with packages/auth/src/permissions.ts BUILT_IN_ROLE_OBLIGATIONS. const BUILT_IN_ROLE_OBLIGATIONS: Record = { owner: { compliance: true }, - admin: {}, + admin: { compliance: true }, auditor: {}, employee: { compliance: true }, contractor: { compliance: true }, From 5fab22f6a51f92b17a3bd67ca25d7d0e10a41e53 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 18:04:42 -0400 Subject: [PATCH 4/8] fix(rbac): sync digest BUILT_IN_ROLE_OBLIGATIONS snapshot with auth package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inlined snapshot still said `admin: { compliance: true }` even though the auth package itself was changed to `admin: {}` in 9316c6d29 — so the toggle on the Admin role page had no effect on digest emails. Sync them so turning the toggle off actually stops admin reminders. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../trigger/tasks/task/policy-acknowledgment-digest-helpers.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts index 737de4e08c..686f0394cf 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts @@ -8,7 +8,7 @@ import type { Departments, PolicyVisibility } from '@db'; // Keep in sync with packages/auth/src/permissions.ts BUILT_IN_ROLE_OBLIGATIONS. const BUILT_IN_ROLE_OBLIGATIONS: Record = { owner: { compliance: true }, - admin: { compliance: true }, + admin: {}, auditor: {}, employee: { compliance: true }, contractor: { compliance: true }, From 6b0fa6cf2e275d14f31493d7ca5d6460b2392d40 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 18:11:54 -0400 Subject: [PATCH 5/8] fix(rbac): tighten obligation DTO + resync state on role navigation Addresses two issues from review: - DTO now uses a nested class with @IsBoolean on `compliance`, so non-boolean values are rejected at validation time instead of being stored in the JSON column. - System role detail resyncs `currentObligations` when `roleName` changes, so navigating between built-in role pages without a hard reload doesn't display stale toggle state from the previous role. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../dto/update-built-in-obligations.dto.ts | 19 +++++++++++++++++-- .../system/[roleName]/system-role-detail.tsx | 10 +++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/apps/api/src/roles/dto/update-built-in-obligations.dto.ts b/apps/api/src/roles/dto/update-built-in-obligations.dto.ts index 21f4655770..f509e74de3 100644 --- a/apps/api/src/roles/dto/update-built-in-obligations.dto.ts +++ b/apps/api/src/roles/dto/update-built-in-obligations.dto.ts @@ -1,12 +1,27 @@ import { ApiProperty } from '@nestjs/swagger'; -import { IsObject } from 'class-validator'; +import { IsBoolean, IsObject, IsOptional, ValidateNested } from 'class-validator'; +import { Type } from 'class-transformer'; + +export class BuiltInObligationsBody { + @ApiProperty({ + description: 'Whether the role must complete compliance tasks.', + example: false, + required: false, + }) + @IsBoolean() + @IsOptional() + compliance?: boolean; +} export class UpdateBuiltInObligationsDto { @ApiProperty({ description: 'Obligations override for the built-in role.', example: { compliance: false }, required: true, + type: BuiltInObligationsBody, }) @IsObject() - obligations!: { compliance?: boolean } & Record; + @ValidateNested() + @Type(() => BuiltInObligationsBody) + obligations!: BuiltInObligationsBody; } diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx index 05aae0bc93..8f0864ee20 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx @@ -1,6 +1,6 @@ 'use client'; -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { Card, CardContent } from '@trycompai/ui/card'; import { Stack, Text } from '@trycompai/design-system'; import { toast } from 'sonner'; @@ -21,6 +21,14 @@ export function SystemRoleDetail({ roleName, permissions, obligations, descripti useState>(obligations); const [isSaving, setIsSaving] = useState(false); + // Resync if the user navigates between built-in role pages without a full + // reload — `useState`'s initial value is only honored on mount. + useEffect(() => { + setCurrentObligations(obligations); + // Keyed on roleName so a re-fetched obligations for the same role + // (e.g., after the user toggles it) doesn't clobber the optimistic value. + }, [roleName]); // eslint-disable-line react-hooks/exhaustive-deps + const handleObligationsChange = async (next: Record) => { if (isSaving || !obligationsEditable) return; const previous = currentObligations; From 3fb0080f1b4e3dcec67a817c18ebc07e484ec30e Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 18:24:46 -0400 Subject: [PATCH 6/8] fix(rbac): exclude override rows from role count + safer obligation fallback Two review fixes: - The 20-custom-role limit now ignores rows that exist only as obligation overrides for built-in roles (name in BUILT_IN_ROLES), so toggling owner or admin doesn't shrink the customer's custom-role budget. - Override semantics tightened: a DB row whose obligations JSON has no `compliance` key falls back to the hardcoded built-in default instead of silently disabling it. Only an explicit true/false on the override now wins over the default. Applied in all 4 read paths (compliance member filter, portal access, digest helper, roles.service.getObligationsForRoles). Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/api/src/roles/roles.service.spec.ts | 37 +++++++++++++++++++ apps/api/src/roles/roles.service.ts | 18 +++++++-- apps/app/src/lib/compliance.ts | 8 ++-- ...licy-acknowledgment-digest-helpers.test.ts | 13 +++++++ .../policy-acknowledgment-digest-helpers.ts | 8 ++-- apps/portal/src/utils/portal-access.ts | 23 ++++++------ 6 files changed, 86 insertions(+), 21 deletions(-) diff --git a/apps/api/src/roles/roles.service.spec.ts b/apps/api/src/roles/roles.service.spec.ts index aedf2e5624..c26667e085 100644 --- a/apps/api/src/roles/roles.service.spec.ts +++ b/apps/api/src/roles/roles.service.spec.ts @@ -230,6 +230,32 @@ describe('RolesService', () => { ).rejects.toThrow('Maximum of 20 custom roles per organization'); }); + it('excludes built-in obligation override rows from the 20-role limit', async () => { + // The count query should filter out override rows so they don't steal + // slots from the customer's custom-role budget. + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue(null); + (mockDb.organizationRole.count as jest.Mock).mockResolvedValue(0); + (mockDb.organizationRole.create as jest.Mock).mockResolvedValue({ + id: 'rol_xyz', + name: validDto.name, + permissions: JSON.stringify(validDto.permissions), + obligations: '{}', + organizationId, + createdAt: new Date(), + updatedAt: new Date(), + }); + + await service.createRole(organizationId, validDto, ['owner']); + expect(mockDb.organizationRole.count).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ + organizationId, + name: { notIn: expect.arrayContaining(['owner', 'admin']) }, + }), + }), + ); + }); + it('should prevent privilege escalation - cannot grant permissions you do not have', async () => { // Employee trying to grant admin-level permissions const dto = { @@ -845,6 +871,17 @@ describe('RolesService', () => { ]); expect(result).toEqual({ compliance: true }); }); + + it('falls back to built-in default when override JSON has no compliance key', async () => { + // A row with `{}` should not silently disable the owner default. + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([ + { name: 'owner', obligations: '{}' }, + ]); + const result = await service.getObligationsForRoles(organizationId, [ + 'owner', + ]); + expect(result).toEqual({ compliance: true }); + }); }); describe('listRoles built-in overrides', () => { diff --git a/apps/api/src/roles/roles.service.ts b/apps/api/src/roles/roles.service.ts index 7e7665eb1e..bad36692f4 100644 --- a/apps/api/src/roles/roles.service.ts +++ b/apps/api/src/roles/roles.service.ts @@ -199,9 +199,11 @@ export class RolesService { throw new BadRequestException(`Role '${dto.name}' already exists`); } - // Check max roles limit + // Check max roles limit — exclude rows that exist solely as obligation + // overrides for built-in roles, since those don't count against the + // customer's 20-custom-role budget. const roleCount = await db.organizationRole.count({ - where: { organizationId }, + where: { organizationId, name: { notIn: BUILT_IN_ROLES } }, }); if (roleCount >= 20) { @@ -584,8 +586,16 @@ export class RolesService { const combined: RoleObligations = {}; for (const name of roleNames) { const fromDb = overrideByName.get(name); - const effective = - fromDb ?? (BUILT_IN_ROLES.includes(name) ? BUILT_IN_ROLE_OBLIGATIONS[name] ?? {} : {}); + // Use the override only when `compliance` is explicitly set in it; + // otherwise fall back to the built-in default. + let effective: RoleObligations; + if (fromDb && 'compliance' in fromDb) { + effective = fromDb; + } else if (BUILT_IN_ROLES.includes(name)) { + effective = BUILT_IN_ROLE_OBLIGATIONS[name] ?? {}; + } else { + effective = fromDb ?? {}; + } if (effective.compliance) combined.compliance = true; } diff --git a/apps/app/src/lib/compliance.ts b/apps/app/src/lib/compliance.ts index 9ec049513e..d916451fb8 100644 --- a/apps/app/src/lib/compliance.ts +++ b/apps/app/src/lib/compliance.ts @@ -114,9 +114,11 @@ export async function filterComplianceMembers( // Platform admins are excluded — they join customer orgs to debug if (member.user?.role === 'admin') return false; for (const name of roleNames) { - // DB override wins over the hardcoded built-in default - if (name in obligationMap) { - if (obligationMap[name]?.compliance) return true; + // DB override wins, but only if `compliance` is explicitly set — + // otherwise fall back to the hardcoded built-in default. + const override = obligationMap[name]; + if (override && 'compliance' in override) { + if (override.compliance) return true; continue; } if (builtInRoleNames.has(name)) { diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts index 09c09313f2..17838c03fc 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.test.ts @@ -159,4 +159,17 @@ describe('filterDigestMembersByCompliance', () => { const result = await filterDigestMembersByCompliance(mockDb, [member], 'org_1'); expect(result).toEqual([member]); }); + + it('falls back to built-in default when override row has no compliance key', async () => { + // A row with `{}` should NOT silently disable the owner default. + mockDb.organizationRole.findMany.mockResolvedValueOnce([ + { name: 'owner', obligations: {} }, + ]); + const member: DigestMember = { + id: 'm8', role: 'owner', department: 'it', + user: { id: 'u8', name: 'H', email: 'h@x', role: null }, + }; + const result = await filterDigestMembersByCompliance(mockDb, [member], 'org_1'); + expect(result).toEqual([member]); + }); }); diff --git a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts index 686f0394cf..bc738efc26 100644 --- a/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts +++ b/apps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.ts @@ -71,9 +71,11 @@ export async function filterDigestMembersByCompliance( // Platform admins are excluded — matches the @/lib/compliance behavior. if (member.user?.role === 'admin') return false; for (const name of roleNames) { - // DB override wins over the hardcoded built-in default - if (name in obligationMap) { - if (obligationMap[name]?.compliance) return true; + // DB override wins, but only if `compliance` is explicitly set — + // otherwise fall back to the hardcoded built-in default. + const override = obligationMap[name]; + if (override && 'compliance' in override) { + if (override.compliance) return true; continue; } if (BUILT_IN_ROLE_NAMES.has(name)) { diff --git a/apps/portal/src/utils/portal-access.ts b/apps/portal/src/utils/portal-access.ts index 956179bf77..1ceab0b028 100644 --- a/apps/portal/src/utils/portal-access.ts +++ b/apps/portal/src/utils/portal-access.ts @@ -38,19 +38,20 @@ export async function hasPortalAccess({ const dbRow = overrideByName.get(role); const isBuiltIn = Boolean(BUILT_IN_ROLE_PERMISSIONS[role]); - if (dbRow) { - // For custom roles, the DB row is the source of permissions. - // For built-in roles, we only consult it for the obligations override. - if (!isBuiltIn) { - const perms = parseRolePermissions(dbRow.permissions); - if (perms) mergePermissions(permissions, perms); - } - if (parseRoleObligations(dbRow.obligations).compliance) { - hasComplianceObligation = true; - } - continue; + // For custom roles, the DB row is the source of permissions. + // For built-in roles, we only consult it for the obligations override. + if (dbRow && !isBuiltIn) { + const perms = parseRolePermissions(dbRow.permissions); + if (perms) mergePermissions(permissions, perms); } + // Obligations: use the override only when `compliance` is explicitly + // set in it; otherwise fall back to the built-in default. + const overrideObligations = dbRow ? parseRoleObligations(dbRow.obligations) : null; + if (overrideObligations && 'compliance' in overrideObligations) { + if (overrideObligations.compliance) hasComplianceObligation = true; + continue; + } if (isBuiltIn && BUILT_IN_ROLE_OBLIGATIONS[role]?.compliance) { hasComplianceObligation = true; } From ba889ca6740da9f43d544ebed2207695f2e90fea Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 18:36:55 -0400 Subject: [PATCH 7/8] fix(rbac): unify obligation fallback across read paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Centralize the override-resolution rule in `resolveEffectiveObligations` so the UI (`getBuiltInObligations`, `listRoles`) and the enforcement path (`getObligationsForRoles`) all apply the same fallback: a DB row only wins when `compliance` is explicitly set, otherwise fall back to the built-in default. Previously a row stored as `{}` would make the UI show the toggle as off while the enforcement layer kept treating the role as carrying the built-in compliance obligation — a divergence cubic flagged. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/api/src/roles/roles.service.spec.ts | 30 +++++++++++ apps/api/src/roles/roles.service.ts | 64 +++++++++++++----------- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/apps/api/src/roles/roles.service.spec.ts b/apps/api/src/roles/roles.service.spec.ts index c26667e085..ee709e6f5f 100644 --- a/apps/api/src/roles/roles.service.spec.ts +++ b/apps/api/src/roles/roles.service.spec.ts @@ -777,6 +777,16 @@ describe('RolesService', () => { service.getBuiltInObligations(organizationId, 'not-a-built-in'), ).rejects.toThrow(BadRequestException); }); + + it('falls back to built-in default when override row exists but has no compliance key', async () => { + // Override row with `{}` should be treated as "no override on compliance" + // — UI must show the built-in default to match what enforcement does. + (mockDb.organizationRole.findFirst as jest.Mock).mockResolvedValue({ + obligations: '{}', + }); + const result = await service.getBuiltInObligations(organizationId, 'owner'); + expect(result).toEqual({ compliance: true }); + }); }); describe('updateBuiltInObligations', () => { @@ -924,5 +934,25 @@ describe('RolesService', () => { const employeeEntry = result.builtInRoles.find((r) => r.name === 'employee'); expect(employeeEntry?.obligations).toEqual({ compliance: true }); }); + + it('shows the built-in default when override row exists with no compliance key', async () => { + // Override row with `{}` should not silently flip the built-in entry off. + (mockDb.organizationRole.findMany as jest.Mock).mockResolvedValue([ + { + id: 'rol_owner_override', + name: 'owner', + permissions: '{}', + obligations: '{}', + organizationId, + createdAt: new Date(), + updatedAt: new Date(), + }, + ]); + (mockDb.member.count as jest.Mock).mockResolvedValue(0); + + const result = await service.listRoles(organizationId); + const ownerEntry = result.builtInRoles.find((r) => r.name === 'owner'); + expect(ownerEntry?.obligations).toEqual({ compliance: true }); + }); }); }); diff --git a/apps/api/src/roles/roles.service.ts b/apps/api/src/roles/roles.service.ts index bad36692f4..12bb421e6f 100644 --- a/apps/api/src/roles/roles.service.ts +++ b/apps/api/src/roles/roles.service.ts @@ -28,6 +28,35 @@ const BUILT_IN_ROLES = Object.keys(allRoles); // changing behavior for roles the request didn't cover. const EDITABLE_BUILT_IN_OBLIGATION_ROLES = new Set(['owner', 'admin']); +function parseObligationsField(value: unknown): RoleObligations { + if (typeof value === 'string') { + try { + return JSON.parse(value) as RoleObligations; + } catch { + return {}; + } + } + return (value as RoleObligations) || {}; +} + +/** + * Resolve the effective obligations for a single role, given an optional DB + * override row. Centralized so every read path applies the same fallback + * rule: a DB row only wins when it explicitly sets `compliance`, otherwise + * we fall back to the hardcoded built-in default. Prevents the UI and the + * enforcement layer from diverging when a row exists with `{}` obligations. + */ +function resolveEffectiveObligations( + roleName: string, + override: RoleObligations | null, +): RoleObligations { + if (override && 'compliance' in override) return override; + if (BUILT_IN_ROLES.includes(roleName)) { + return BUILT_IN_ROLE_OBLIGATIONS[roleName] ?? {}; + } + return override ?? {}; +} + @Injectable() export class RolesService { /** @@ -262,16 +291,12 @@ export class RolesService { // Include built-in roles info (with effective obligations: override or default) const builtInRoles = BUILT_IN_ROLES.map((name) => { const override = overrideByName.get(name); - const obligations = override - ? (typeof override.obligations === 'string' - ? (JSON.parse(override.obligations) as RoleObligations) - : ((override.obligations as RoleObligations) || {})) - : (BUILT_IN_ROLE_OBLIGATIONS[name] ?? {}); + const parsed = override ? parseObligationsField(override.obligations) : null; return { name, isBuiltIn: true, description: this.getBuiltInRoleDescription(name), - obligations, + obligations: resolveEffectiveObligations(name, parsed), }; }); @@ -576,26 +601,13 @@ export class RolesService { }); const overrideByName = new Map(); for (const role of dbRoles) { - const obligations = - typeof role.obligations === 'string' - ? (JSON.parse(role.obligations) as RoleObligations) - : ((role.obligations as RoleObligations) || {}); - overrideByName.set(role.name, obligations); + overrideByName.set(role.name, parseObligationsField(role.obligations)); } const combined: RoleObligations = {}; for (const name of roleNames) { - const fromDb = overrideByName.get(name); - // Use the override only when `compliance` is explicitly set in it; - // otherwise fall back to the built-in default. - let effective: RoleObligations; - if (fromDb && 'compliance' in fromDb) { - effective = fromDb; - } else if (BUILT_IN_ROLES.includes(name)) { - effective = BUILT_IN_ROLE_OBLIGATIONS[name] ?? {}; - } else { - effective = fromDb ?? {}; - } + const fromDb = overrideByName.get(name) ?? null; + const effective = resolveEffectiveObligations(name, fromDb); if (effective.compliance) combined.compliance = true; } @@ -660,12 +672,8 @@ export class RolesService { where: { organizationId, name: roleName }, select: { obligations: true }, }); - if (override) { - return typeof override.obligations === 'string' - ? (JSON.parse(override.obligations) as RoleObligations) - : ((override.obligations as RoleObligations) || {}); - } - return BUILT_IN_ROLE_OBLIGATIONS[roleName] ?? {}; + const parsed = override ? parseObligationsField(override.obligations) : null; + return resolveEffectiveObligations(roleName, parsed); } /** From 32c335334fde304ee6d56c96225096680ba9b5aa Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Thu, 21 May 2026 18:43:47 -0400 Subject: [PATCH 8/8] fix(rbac): sync system role obligations state on prop change Track obligations in the resync effect so the toggle reflects updates when the parent re-fetches (e.g., after router.refresh or a parallel tab mutating the same role). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../roles/system/[roleName]/system-role-detail.tsx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx index 8f0864ee20..dcbf782107 100644 --- a/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx +++ b/apps/app/src/app/(app)/[orgId]/settings/roles/system/[roleName]/system-role-detail.tsx @@ -21,13 +21,12 @@ export function SystemRoleDetail({ roleName, permissions, obligations, descripti useState>(obligations); const [isSaving, setIsSaving] = useState(false); - // Resync if the user navigates between built-in role pages without a full - // reload — `useState`'s initial value is only honored on mount. + // Resync when the parent passes new props — either because the user + // navigated to a different built-in role, or because the page re-fetched + // the same role's obligations. useEffect(() => { setCurrentObligations(obligations); - // Keyed on roleName so a re-fetched obligations for the same role - // (e.g., after the user toggles it) doesn't clobber the optimistic value. - }, [roleName]); // eslint-disable-line react-hooks/exhaustive-deps + }, [roleName, obligations]); const handleObligationsChange = async (next: Record) => { if (isSaving || !obligationsEditable) return;