fix(auth): migrate the six billing controllers onto @RequirePermissions()

README.md

@@ -14,7 +14,7 @@
   <img src="https://img.shields.io/badge/NestJS-framework-E0234E?logo=nestjs&logoColor=white" alt="NestJS" />
   <img src="https://img.shields.io/badge/PostgreSQL-database-4169E1?logo=postgresql&logoColor=white" alt="PostgreSQL" />
   <img src="https://img.shields.io/badge/License-Apache%202.0-blue" alt="Apache 2.0 License" />
-  <img src="https://img.shields.io/badge/Tests-2222%20passing-brightgreen" alt="2222 Tests Passing" />  <img src="https://img.shields.io/badge/AI%20Agents-12%20built--in-blueviolet" alt="12 AI Agents" />
+  <img src="https://img.shields.io/badge/Tests-2237%20passing-brightgreen" alt="2237 Tests Passing" />  <img src="https://img.shields.io/badge/AI%20Agents-12%20built--in-blueviolet" alt="12 AI Agents" />
 </p>
 
 <p align="center">
@@ -510,7 +510,7 @@ Operator notes for activating existing adapters, metasearch landings on the dire
 | OTA Channels | Booking.com + Expedia (EQC) + SiteMinder + DerbySoft | Direct + aggregated OTA connectivity (ARI + content) |
 | XML Processing | fast-xml-parser | Booking.com OTA XML protocol |
 | Package Manager | pnpm workspaces | Monorepo management |
-| Testing | Vitest (2222 passing tests across 266 files with passing tests) | Unit and integration tests |
+| Testing | Vitest (2237 passing tests across 267 files with passing tests) | Unit and integration tests |
 | Build | tsup (packages) + Vite (dashboard) + nest build (API) | Fast builds |
 | Containers | Docker + docker-compose | Local dev and production deployment |
 | CI/CD | GitHub Actions | Automated testing, builds, and releases |
@@ -648,7 +648,7 @@ Before going live, verify the items in [`docs/deployment.md`](./docs/deployment.
 ### Run tests
 
 ```bash
-# Passing-test count: 2222 test cases across 266 files (skipped excluded)
+# Passing-test count: 2237 test cases across 267 files (skipped excluded)
 
 # API tests only
 pnpm --filter @telivityhaip/api test
@@ -1197,7 +1197,7 @@ HAIP is built in public and contributions are welcome.
 pnpm install          # Install dependencies
 pnpm build            # Build all workspace packages
 pnpm dev              # Start API in dev mode (hot reload)
-pnpm test             # Run all tests (2222 passing, 266 files with passes; skipped excluded)
+pnpm test             # Run all tests (2237 passing, 267 files with passes; skipped excluded)
 pnpm lint             # ESLint
 ```
 

apps/api/src/modules/accounting/accounting.controller.ts

@@ -9,7 +9,7 @@ import {
   ParseUUIDPipe,
 } from '@nestjs/common';
 import { ApiTags, ApiOperation, ApiResponse, ApiQuery } from '@nestjs/swagger';
-import { Roles } from '../auth/roles.decorator';
+import { RequirePermissions } from '../auth/permissions.decorator';
 import { DepositService } from './deposit.service';
 import { ArService } from './ar.service';
 import { AccountingCodeService } from './accounting-code.service';
@@ -44,7 +44,7 @@ export class AccountingController {
   // --- Deposit Ledger (KB 10) ---
 
   @Post('deposits')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Record an advance deposit (held liability)' })
   @ApiResponse({ status: 201, description: 'Deposit recorded' })
   recordDeposit(@Body() dto: RecordDepositDto) {
@@ -71,7 +71,7 @@ export class AccountingController {
   }
 
   @Post('deposits/:id/apply')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Apply a held deposit to a folio (KB 10.3)' })
   @ApiResponse({ status: 200, description: 'Deposit applied' })
   applyDeposit(
@@ -82,7 +82,7 @@ export class AccountingController {
   }
 
   @Post('deposits/:id/refund')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Refund a held refundable deposit (KB 10.4)' })
   @ApiResponse({ status: 200, description: 'Deposit refunded' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -94,7 +94,7 @@ export class AccountingController {
   }
 
   @Post('deposits/:id/forfeit')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Forfeit a held deposit as earned revenue (KB 10.4)' })
   @ApiResponse({ status: 200, description: 'Deposit forfeited' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -108,7 +108,7 @@ export class AccountingController {
   // --- Accounts Receivable (KB 11) ---
 
   @Post('ar/ledgers')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Create an A/R ledger' })
   @ApiResponse({ status: 201, description: 'A/R ledger created' })
   createArLedger(@Body() dto: CreateArLedgerDto) {
@@ -135,7 +135,7 @@ export class AccountingController {
   }
 
   @Patch('ar/ledgers/:id')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Update an A/R ledger' })
   @ApiResponse({ status: 200, description: 'A/R ledger updated' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -148,7 +148,7 @@ export class AccountingController {
   }
 
   @Post('ar/ledgers/:id/close')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Close an A/R ledger (KB 11.2)' })
   @ApiResponse({ status: 200, description: 'A/R ledger closed' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -171,15 +171,15 @@ export class AccountingController {
   }
 
   @Post('ar/transfer')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Transfer an outstanding folio balance into an A/R ledger (KB 11.3)' })
   @ApiResponse({ status: 201, description: 'Balance transferred to A/R' })
   transferToAr(@Body() dto: TransferToArDto) {
     return this.arService.transferFolioToAR(dto);
   }
 
   @Post('ar/transactions/:id/reverse')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Reverse an A/R transfer (KB 11.4)' })
   @ApiResponse({ status: 200, description: 'Transfer reversed' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -191,7 +191,7 @@ export class AccountingController {
   }
 
   @Post('ar/ledgers/:id/payments')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Record a payment against an A/R ledger (KB 11.5)' })
   @ApiResponse({ status: 201, description: 'A/R payment recorded' })
   recordArPayment(
@@ -223,7 +223,7 @@ export class AccountingController {
   // --- Custom Accounting / GL Codes (KB 5) ---
 
   @Post('accounting/codes')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Create a custom accounting / GL code' })
   @ApiResponse({ status: 201, description: 'Accounting code created' })
   createAccountingCode(@Body() dto: CreateAccountingCodeDto) {
@@ -250,7 +250,7 @@ export class AccountingController {
   }
 
   @Patch('accounting/codes/:id')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Update an accounting code' })
   @ApiResponse({ status: 200, description: 'Accounting code updated' })
   @ApiQuery({ name: 'propertyId', type: String })
@@ -263,7 +263,7 @@ export class AccountingController {
   }
 
   @Post('accounting/codes/:id/archive')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('accounting.manage')
   @ApiOperation({ summary: 'Archive an accounting code (soft delete)' })
   @ApiResponse({ status: 200, description: 'Accounting code archived' })
   @ApiQuery({ name: 'propertyId', type: String })

apps/api/src/modules/auth/billing-role-controllers-migration.spec.ts

@@ -0,0 +1,63 @@
+import { describe, it, expect } from 'vitest';
+import { Reflector } from '@nestjs/core';
+import { ROLES_KEY } from './roles.decorator';
+import { PERMISSIONS_KEY } from './permissions.decorator';
+import { PaymentController } from '../payment/payment.controller';
+import { FolioController } from '../folio/folio.controller';
+import { NotificationsController } from '../notifications/notifications.controller';
+import { HouseAccountController } from '../house-account/house-account.controller';
+import { CashierController } from '../cashier/cashier.controller';
+import { AccountingController } from '../accounting/accounting.controller';
+
+/**
+ * The six billing-adjacent controllers left behind by #340, which migrated
+ * eighteen others off the legacy @Roles() decorator. These were deferred
+ * because they gated every route -- read and write alike -- with one identical
+ * realm-role list, so no existing permission key matched it without either
+ * widening or narrowing who could record a payment or post a charge.
+ *
+ * The rule applied here is to PRESERVE the access the realm-role gate was
+ * granting in practice: reservations, night_auditor and accounting keep the
+ * billing-write ability the old list already gave them, and the matching
+ * ROLE_DEFAULT_PERMISSIONS entries plus migration
+ * 0023_billing_write_access_grants.sql backfill the keys for properties that
+ * already have RBAC rows.
+ *
+ * One side effect worth disclosing rather than burying: communications.manage
+ * (notifications.controller.ts) is already held by revenue_manager for an
+ * unrelated existing route (groups.controller.ts), so reusing that key here
+ * also lets revenue_manager send guest SMS/WhatsApp/Telegram messages, which
+ * the old gate did not. Reusing the existing key seemed better than inventing
+ * a near-duplicate one, but say the word and it can be split.
+ */
+const reflector = new Reflector();
+
+// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type
+function rolesOf(target: Function) {
+  return reflector.get<string[] | undefined>(ROLES_KEY, target);
+}
+// eslint-disable-next-line @typescript-eslint/no-unsafe-function-type
+function permsOf(target: Function) {
+  return reflector.get<string[] | undefined>(PERMISSIONS_KEY, target);
+}
+
+describe('billing-write routes migrated off @Roles() onto @RequirePermissions', () => {
+  it.each([
+    [PaymentController.prototype.recordPayment, 'folios.manage'],
+    [PaymentController.prototype.voidPayment, 'folios.manage'],
+    [FolioController.prototype.createFolio, 'folios.manage'],
+    [FolioController.prototype.settleFolio, 'folios.manage'],
+    [FolioController.prototype.postCharge, 'folios.manage'],
+    [NotificationsController.prototype.sendSms, 'communications.manage'],
+    [HouseAccountController.prototype.openHouseAccount, 'houseaccounts.manage'],
+    [HouseAccountController.prototype.addPayment, 'houseaccounts.manage'],
+    [CashierController.prototype.createDrawer, 'cashier.access'],
+    [CashierController.prototype.closeSession, 'cashier.access'],
+    [AccountingController.prototype.recordDeposit, 'accounting.manage'],
+    [AccountingController.prototype.createArLedger, 'accounting.manage'],
+    [AccountingController.prototype.createAccountingCode, 'accounting.manage'],
+  ])('%s: no leftover @Roles, gated on @RequirePermissions(%s)', (method, expectedKey) => {
+    expect(rolesOf(method)).toBeUndefined();
+    expect(permsOf(method)).toEqual([expectedKey]);
+  });
+});

apps/api/src/modules/auth/permissions.catalog.spec.ts

@@ -55,11 +55,34 @@ describe('permissions catalog', () => {
     expect(ROLE_DEFAULT_PERMISSIONS.general_manager).toContain('revenue.manage');
   });
 
-  it('reservations can book but not post folios or run cashier', () => {
+  // Was 'reservations can book but not post folios or run cashier' -- that
+  // split was never real. The legacy @Roles() gate on every billing controller
+  // (folio / payment / cashier / house-account / accounting) already included
+  // 'reservations'; only the newer @RequirePermissions catalog enforced the
+  // split, and only by omission. Migrating those controllers preserves the
+  // access the realm-role gate was granting rather than narrowing it silently.
+  it('reservations can book and post folios/cashier -- matches what the old realm-role gate already granted', () => {
     expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('reservations.write');
     expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('folios.read');
-    expect(ROLE_DEFAULT_PERMISSIONS.reservations).not.toContain('folios.manage');
-    expect(ROLE_DEFAULT_PERMISSIONS.reservations).not.toContain('cashier.access');
+    expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('folios.manage');
+    expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('cashier.access');
+    expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('houseaccounts.manage');
+    expect(ROLE_DEFAULT_PERMISSIONS.reservations).toContain('accounting.manage');
+  });
+
+  it('night_auditor and accounting also gained the billing-write access the old realm-role gate granted', () => {
+    for (const role of ['night_auditor', 'accounting']) {
+      expect(ROLE_DEFAULT_PERMISSIONS[role], role).toContain('folios.manage');
+      expect(ROLE_DEFAULT_PERMISSIONS[role], role).toContain('houseaccounts.manage');
+      expect(ROLE_DEFAULT_PERMISSIONS[role], role).toContain('accounting.manage');
+      expect(ROLE_DEFAULT_PERMISSIONS[role], role).toContain('communications.manage');
+    }
+    expect(ROLE_DEFAULT_PERMISSIONS.night_auditor).toContain('cashier.access');
+  });
+
+  it('front_desk gained cashier.access and accounting.manage', () => {
+    expect(ROLE_DEFAULT_PERMISSIONS.front_desk).toContain('cashier.access');
+    expect(ROLE_DEFAULT_PERMISSIONS.front_desk).toContain('accounting.manage');
   });
 
   it('revenue_manager cannot view folios (nav and list API gated)', () => {

apps/api/src/modules/auth/permissions.catalog.ts

@@ -38,6 +38,7 @@ export const PERMISSIONS: readonly PermissionDef[] = [
   { key: 'houseaccounts.read', label: 'View house accounts', group: 'House Accounts', navKey: '/house-accounts' },
   { key: 'houseaccounts.manage', label: 'Manage house accounts', group: 'House Accounts' },
   { key: 'accounting.view', label: 'Accounting & deposits', group: 'Accounting', navKey: '/accounting' },
+  { key: 'accounting.manage', label: 'Post deposits, A/R ledgers & accounting codes', group: 'Accounting' },
   { key: 'tax.manage', label: 'Tax profiles & rules', group: 'Tax', navKey: '/tax' },
   { key: 'rateplans.read', label: 'View rate plans', group: 'Rate Plans', navKey: '/rate-plans' },
   { key: 'rateplans.manage', label: 'Manage rate plans', group: 'Rate Plans' },
@@ -97,6 +98,8 @@ export const ROLE_DEFAULT_PERMISSIONS: Record<string, readonly string[]> = {
     'reviews.manage',
     'ops.read',
     'ops.manage',
+    'cashier.access',
+    'accounting.manage',
   ],
   housekeeping: ['dashboard.view', 'rooms.read', 'housekeeping.read', 'ops.read'],
   housekeeping_manager: [
@@ -112,11 +115,15 @@ export const ROLE_DEFAULT_PERMISSIONS: Record<string, readonly string[]> = {
     'dashboard.view',
     'reservations.read',
     'folios.read',
+    'folios.manage',
     'nightaudit.run',
     'reports.view',
     'cashier.access',
     'houseaccounts.read',
+    'houseaccounts.manage',
     'accounting.view',
+    'accounting.manage',
+    'communications.manage',
     'commercial.read',
   ],
   readonly: [
@@ -162,12 +169,21 @@ export const ROLE_DEFAULT_PERMISSIONS: Record<string, readonly string[]> = {
     'houseaccounts.manage',
     'cashier.access',
     'accounting.view',
+    'accounting.manage',
     'tax.manage',
     'nightaudit.run',
+    'communications.manage',
     'reports.view',
     'commercial.read',
   ],
-  /** Booking desk — reservations without cashier / folio posting. */
+  /**
+   * Booking desk. The doc comment used to read "reservations without cashier /
+   * folio posting", but the legacy @Roles() gate never enforced that split:
+   * `reservations` was already in the allow-list of every billing controller
+   * below. These keys record the access the realm-role gate was granting in
+   * practice, so migrating to permissions preserves behaviour rather than
+   * silently narrowing it.
+   */
   reservations: [
     'dashboard.view',
     'frontdesk.access',
@@ -178,6 +194,10 @@ export const ROLE_DEFAULT_PERMISSIONS: Record<string, readonly string[]> = {
     'rooms.read',
     'media.manage',
     'folios.read',
+    'folios.manage',
+    'houseaccounts.manage',
+    'cashier.access',
+    'accounting.manage',
     'groups.read',
     'groups.manage',
     'commercial.read',

apps/api/src/modules/cashier/cashier.controller.ts

@@ -8,7 +8,7 @@ import {
   ParseUUIDPipe,
 } from '@nestjs/common';
 import { ApiTags, ApiOperation, ApiResponse, ApiQuery } from '@nestjs/swagger';
-import { Roles } from '../auth/roles.decorator';
+import { RequirePermissions } from '../auth/permissions.decorator';
 import { CashierService } from './cashier.service';
 import { CreateDrawerDto } from './dto/create-drawer.dto';
 import { OpenSessionDto } from './dto/open-session.dto';
@@ -28,7 +28,7 @@ export class CashierController {
   }
 
   @Post('drawers')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('cashier.access')
   @ApiOperation({ summary: 'Create a cash drawer (KB 12.1)' })
   @ApiResponse({ status: 201, description: 'Cash drawer created' })
   createDrawer(@Body() dto: CreateDrawerDto) {
@@ -61,7 +61,7 @@ export class CashierController {
   }
 
   @Post('sessions')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('cashier.access')
   @ApiOperation({ summary: 'Open a cash drawer session/shift (KB 12.2)' })
   @ApiResponse({ status: 201, description: 'Session opened' })
   openSession(@Body() dto: OpenSessionDto) {
@@ -98,7 +98,7 @@ export class CashierController {
   }
 
   @Post('sessions/:id/movements')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('cashier.access')
   @ApiOperation({ summary: 'Record a cash movement (KB 12.3)' })
   @ApiResponse({ status: 201, description: 'Movement recorded' })
   recordMovement(
@@ -109,7 +109,7 @@ export class CashierController {
   }
 
   @Post('sessions/:id/close')
-  @Roles('admin', 'general_manager', 'front_desk', 'reservations', 'night_auditor', 'accounting')
+  @RequirePermissions('cashier.access')
   @ApiOperation({ summary: 'Close a session and compute variance (KB 12.4)' })
   @ApiResponse({ status: 200, description: 'Session closed' })
   closeSession(