psa-systems/mokosh-server
Watch
9
0
Fork
You've already forked mokosh-server
 1
Code Issues Pull requests Projects Releases 10 Packages Wiki Activity Actions

fix(rbac): gate Contracts/Rate Cards reads and financial reports like Invoices #271

Merged
nrupard merged 2 commits from fix/PMS-350-rbac-finance-gates into main 2026-06-16 21:43:53 +02:00
Conversation 0 Commits 2 Files changed 3 +167 -5
David commented 2026-06-16 17:36:09 +02:00
Owner
Copy link

External review found a technician could reach Contracts, Rate Cards, and the financial reports that Invoices already restricts to finance roles, plus export the billing CSV through a side-door that skipped the report's own gate. Audit Log, the user list, and invitations were already gated; this closes the remaining server-side holes so every billing / financial surface enforces the same RequireFinance check Invoices uses, returning the identical 403 AppError envelope the SPA already renders.

Contracts: the create/update/delete handlers already carried RequireFinance, but the read handlers (list_contracts, get_contract, list_contract_items, get_hour_balance, list_rate_cards, get_rate_card, list_rate_card_items) only had the module-enable gate, so any reports-enabled tenant member could read contract and rate-card data. Add RequireFinance to each read handler so read and write match.

Reports: switch the billing report from RequireManager to RequireFinance so it matches the Invoices gate exactly (finance, not manager), update its descriptor text, and add the same can_manage_billing() check to the billing branch of the CSV export so the export cannot bypass the report's gate.

Add tests/rbac_route_coverage.rs: an HTTP integration test that logs in as every role (super_admin, admin, manager, technician, dispatcher, sales, finance) and walks the protected routes (invoices, contracts, rate-cards, reports/billing, reports/billing/export, invitations, audit-log, auth/users) asserting the exact allow/deny matrix, so a future feature cannot silently re-open a gap.

The sidebar / nav role-reflection acceptance criterion lives in the mokosh-apps SPA repo, not mokosh-server, so it is out of scope for this change.

#PMS-350

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

External review found a technician could reach Contracts, Rate Cards, and the financial reports that Invoices already restricts to finance roles, plus export the billing CSV through a side-door that skipped the report's own gate. Audit Log, the user list, and invitations were already gated; this closes the remaining server-side holes so every billing / financial surface enforces the same RequireFinance check Invoices uses, returning the identical 403 AppError envelope the SPA already renders. Contracts: the create/update/delete handlers already carried RequireFinance, but the read handlers (list_contracts, get_contract, list_contract_items, get_hour_balance, list_rate_cards, get_rate_card, list_rate_card_items) only had the module-enable gate, so any reports-enabled tenant member could read contract and rate-card data. Add RequireFinance to each read handler so read and write match. Reports: switch the billing report from RequireManager to RequireFinance so it matches the Invoices gate exactly (finance, not manager), update its descriptor text, and add the same can_manage_billing() check to the billing branch of the CSV export so the export cannot bypass the report's gate. Add tests/rbac_route_coverage.rs: an HTTP integration test that logs in as every role (super_admin, admin, manager, technician, dispatcher, sales, finance) and walks the protected routes (invoices, contracts, rate-cards, reports/billing, reports/billing/export, invitations, audit-log, auth/users) asserting the exact allow/deny matrix, so a future feature cannot silently re-open a gap. The sidebar / nav role-reflection acceptance criterion lives in the mokosh-apps SPA repo, not mokosh-server, so it is out of scope for this change. #PMS-350 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(rbac): gate Contracts/Rate Cards reads and financial reports like Invoices
All checks were successful
E2E / Playwright against staging (pull_request) Successful in 36s
Details
Check / fmt + clippy + compile + unit/doc tests (pull_request) Successful in 57s
Details
Integration / integration tests (pull_request) Successful in 7m32s
Details
73cc9af92f 
External review found a technician could reach Contracts, Rate Cards, and the financial reports that Invoices already restricts to finance roles, plus export the billing CSV through a side-door that skipped the report's own gate. Audit Log, the user list, and invitations were already gated; this closes the remaining server-side holes so every billing / financial surface enforces the same RequireFinance check Invoices uses, returning the identical 403 AppError envelope the SPA already renders.

Contracts: the create/update/delete handlers already carried RequireFinance, but the read handlers (list_contracts, get_contract, list_contract_items, get_hour_balance, list_rate_cards, get_rate_card, list_rate_card_items) only had the module-enable gate, so any reports-enabled tenant member could read contract and rate-card data. Add RequireFinance to each read handler so read and write match.

Reports: switch the billing report from RequireManager to RequireFinance so it matches the Invoices gate exactly (finance, not manager), update its descriptor text, and add the same can_manage_billing() check to the billing branch of the CSV export so the export cannot bypass the report's gate.

Add tests/rbac_route_coverage.rs: an HTTP integration test that logs in as every role (super_admin, admin, manager, technician, dispatcher, sales, finance) and walks the protected routes (invoices, contracts, rate-cards, reports/billing, reports/billing/export, invitations, audit-log, auth/users) asserting the exact allow/deny matrix, so a future feature cannot silently re-open a gap.

The sidebar / nav role-reflection acceptance criterion lives in the mokosh-apps SPA repo, not mokosh-server, so it is out of scope for this change.

#PMS-350

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(rbac): gate Client Profitability report + its CSV export like Invoices
All checks were successful
E2E / Playwright against staging (pull_request) Successful in 1m23s
Details
Check / fmt + clippy + compile + unit/doc tests (pull_request) Successful in 2m45s
Details
Create release / Create release from merged PR (pull_request) Has been skipped
Details
Integration / integration tests (pull_request) Successful in 7m20s
Details
d12dbbafaf 
The PMS-350 finance gating missed the clients report (Client Profitability), which the issue's AC explicitly names alongside Revenue Summary and A/R Aging. GET /reports/clients sums invoiced/paid/outstanding from the invoices table, the same financial data the Invoices page and the billing report carry, yet it required only RequireReports, so a technician with reports access could read company financials. The CSV export side-door (/reports/clients/export) had the same gap. Add RequireFinance to clients_report and the same can_manage_billing check to the export's clients arm that the billing arm already enforces, and extend the rbac route-coverage matrix to assert both /reports/clients and its export are FINANCE_ROLES only.

#PMS-350
nrupard deleted branch fix/PMS-350-rbac-finance-gates 2026-06-16 21:43:53 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
psa-systems/mokosh-server!271
No description provided.