From 7576761b7023f1bc2e5ae2c02bef7264de6b6823 Mon Sep 17 00:00:00 2001 From: Scott Pouliot Date: Fri, 19 Jun 2026 19:48:53 -0400 Subject: [PATCH] Scope GL posting account lookups by CompanyId; cap sales-tax remittance (audit O3, O4) O3: defense-in-depth on the write/posting path. Finding #7 scoped the report (read) path; this scopes every GL posting-path account lookup that determines where money lands, so a SuperAdmin acting in a company context can never post to another tenant's account: - InvoicesController: all account-resolver helpers (checking, customer deposits, sales returns, customer credits, AR, bad debt, sales tax, sales discount, GC liability) plus the bank-account and write-off expense dropdowns - CreditMemosController: Create/Apply/Void GL lookups (scoped via the in-scope customer/invoice/memo) - GiftCertificatesController: Create/BulkCreate/Void GL lookups + GC liability helper - BillsController: AP/expense account resolution that pre-fills APAccountId DepositsController and JournalEntriesController.SalesTaxPayment were already scoped. O4: SalesTaxPayment now rejects a remittance greater than the outstanding Sales Tax Payable balance (0.005 rounding tolerance), so a typo can no longer drive 2200 into an abnormal debit balance. Remaining pure read-path dropdown lookups (app-wide, lower risk) are documented in docs/ACCOUNTING_AUDIT.md as a separate follow-up. All audit findings O1-O4 are now resolved. Build clean; 291 unit tests pass. Co-Authored-By: Claude Opus 4.8 --- docs/ACCOUNTING_AUDIT.md | 47 ++++++++++--------- .../Controllers/BillsController.cs | 7 +-- .../Controllers/CreditMemosController.cs | 12 ++--- .../Controllers/GiftCertificatesController.cs | 12 ++--- .../Controllers/InvoicesController.cs | 28 ++++++----- .../Controllers/JournalEntriesController.cs | 8 ++++ 6 files changed, 65 insertions(+), 49 deletions(-) diff --git a/docs/ACCOUNTING_AUDIT.md b/docs/ACCOUNTING_AUDIT.md index 62a0a1a..4b05905 100644 --- a/docs/ACCOUNTING_AUDIT.md +++ b/docs/ACCOUNTING_AUDIT.md @@ -80,29 +80,34 @@ migration applied to the dev database successfully. --- -## Open findings (deferred — to do after O1/O2 per user) +### O3 — Write-path account lookups omitted explicit `CompanyId` — **RESOLVED** +- Added `CompanyId` to every **GL posting-path** account lookup that determines where money posts: + - `InvoicesController` — all account-resolver helpers (`GetCheckingAccountIdAsync`, + `GetCustomerDepositsAccountIdAsync`, `GetSalesReturnsAccountIdAsync`, `GetCustomerCreditsAccountIdAsync`, + `GetArAccountIdAsync`, `GetBadDebtAccountIdAsync`, `ResolveSalesTaxAccountIdAsync`, + `GetSalesDiscountAccountIdAsync`, `GetGcLiabilityAccountIdAsync`) plus the bank-account and write-off + expense dropdowns (scoped via `_tenantContext`). + - `CreditMemosController` — Create/Apply/Void GL lookups (scoped via the in-scope `customer`/`invoice`/`memo`). + - `GiftCertificatesController` — Create, BulkCreate, Void GL lookups + `GetGcLiabilityAccountIdAsync`. + - `BillsController` — AP/expense account resolution that pre-fills `APAccountId` (Create + CreateFromPO). +- `DepositsController` and `JournalEntriesController.SalesTaxPayment` already scoped correctly. -### O3 — Write-path account lookups omit explicit `CompanyId` (defense-in-depth gap) — **LOW-MEDIUM** -- Finding #7 added explicit `CompanyId` predicates to the **read** path, but several **posting** lookups - still rely on the global filter alone, e.g. `CreditMemosController` (182-184, 268, 317-318) and - `InvoicesController` refund/credit account lookups use `FirstOrDefaultAsync(a => a.AccountNumber == "2350" && a.IsActive)` - with no `CompanyId`. `JournalEntriesController.SalesTaxPayment` (246) does it correctly. -- **Impact:** low in practice (these run under a non-SuperAdmin company context where the global filter - applies), but it violates the repo's standing rule and would post to the wrong tenant's account if ever - executed in a SuperAdmin/multi-company context. -- **Fix direction:** add `a.CompanyId == companyId` to every account lookup on the posting path. +### O4 — Sales-tax remittance could over-remit (drive 2200 negative) — **RESOLVED** +- `JournalEntriesController.SalesTaxPayment` (POST) now rejects any amount exceeding `taxAcct.CurrentBalance` + (0.005 rounding tolerance), so a typo can no longer push Sales Tax Payable into an abnormal debit balance. -### O4 — Sales-tax remittance can over-remit (drive 2200 negative) — **LOW** -- `SalesTaxPayment` validates amount > 0 but does **not** cap the payment at the outstanding 2200 balance. -- **Impact:** a typo can push Sales Tax Payable negative (a debit-balance liability), which is an - abnormal balance that then surfaces oddly on the balance sheet. -- **Fix direction:** show the current 2200 balance (already shown on the GET form) and reject/​warn when - `amount` exceeds it. +Verification of O3+O4: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests` → **291 passed**. --- -## Suggested priority order -1. **O1** (mislabeled/commingled liability — real balance-sheet correctness issue) -2. **O2** (recalc corruption + reconciliation report reliability) -3. **O3** (defense-in-depth on write path) -4. **O4** (input guard) +## Remaining (non-O1–O4) — known lower-risk follow-up +Pure **read-path dropdown/display** account lookups still rely on the global tenant filter (correct for +non-SuperAdmin; only a SuperAdmin acting inside a company could see another tenant's accounts in a picker). +These are app-wide, not specific to the accounting posting path, e.g. `VendorCreditsController` +PopulateDropdowns/Details, `BillsController` PopulateDropdowns and Index/edit account lists, +`ExpensesController` filter + categorization pickers. Tracked here so they aren't mistaken for a regression; +fold into a broader defense-in-depth pass if/when desired. + +## Status +All audit findings **O1–O4 are resolved** on `dev`. Original audit numbering #1–3/#5/#6/#8 remains +unrecoverable (see top). The accounting batch has **not** been merged to `master`/production yet. diff --git a/src/PowderCoating.Web/Controllers/BillsController.cs b/src/PowderCoating.Web/Controllers/BillsController.cs index 91aa41f..a276713 100644 --- a/src/PowderCoating.Web/Controllers/BillsController.cs +++ b/src/PowderCoating.Web/Controllers/BillsController.cs @@ -202,14 +202,14 @@ public class BillsController : Controller } var apAccount = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountSubType == AccountSubType.AccountsPayable); + a => a.CompanyId == po.CompanyId && a.AccountSubType == AccountSubType.AccountsPayable); // Vendor default expense account, fall back to first expense/COGS account int? defaultExpenseAccountId = po.Vendor?.DefaultExpenseAccountId; if (!defaultExpenseAccountId.HasValue) { var fallbackAccount = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && (a.AccountType == AccountType.Expense || a.AccountType == AccountType.CostOfGoods)); + a => a.CompanyId == po.CompanyId && a.IsActive && (a.AccountType == AccountType.Expense || a.AccountType == AccountType.CostOfGoods)); defaultExpenseAccountId = fallbackAccount?.Id; } @@ -272,8 +272,9 @@ public class BillsController : Controller }; // Pre-fill AP account + var companyId = _tenantContext.GetCurrentCompanyId() ?? 0; var apAccount = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountSubType == AccountSubType.AccountsPayable); + a => a.CompanyId == companyId && a.AccountSubType == AccountSubType.AccountsPayable); dto.APAccountId = apAccount?.Id ?? 0; // Pre-fill default expense account for vendor diff --git a/src/PowderCoating.Web/Controllers/CreditMemosController.cs b/src/PowderCoating.Web/Controllers/CreditMemosController.cs index 2842253..fa96324 100644 --- a/src/PowderCoating.Web/Controllers/CreditMemosController.cs +++ b/src/PowderCoating.Web/Controllers/CreditMemosController.cs @@ -179,8 +179,8 @@ public class CreditMemosController : Controller // GL: the credit is a liability owed to the customer — DR Sales Discounts (contra-revenue) // / CR Customer Credits (2350). Relieved when the memo is applied to an invoice. - var discountAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.AccountNumber == "4950" && a.IsActive); - var customerCreditsAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.AccountNumber == "2350" && a.IsActive); + var discountAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.CompanyId == customer.CompanyId && a.AccountNumber == "4950" && a.IsActive); + var customerCreditsAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.CompanyId == customer.CompanyId && a.AccountNumber == "2350" && a.IsActive); await _accountBalanceService.DebitAsync(discountAcct?.Id, vm.Amount); await _accountBalanceService.CreditAsync(customerCreditsAcct?.Id, vm.Amount); @@ -263,9 +263,9 @@ public class CreditMemosController : Controller // The contra-revenue (Sales Discounts) was recognized when the credit was issued. // Keeps Account.CurrentBalance in sync for RecalculateAllAsync and direct readers. var arAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountSubType == AccountSubType.AccountsReceivable && a.IsActive); + a => a.CompanyId == invoice.CompanyId && a.AccountSubType == AccountSubType.AccountsReceivable && a.IsActive); var customerCreditsAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountNumber == "2350" && a.IsActive); + a => a.CompanyId == invoice.CompanyId && a.AccountNumber == "2350" && a.IsActive); await _accountBalanceService.DebitAsync(customerCreditsAcct?.Id, applyAmount); await _accountBalanceService.CreditAsync(arAcct?.Id, applyAmount); @@ -314,8 +314,8 @@ public class CreditMemosController : Controller // GL: reverse the unapplied issuance — DR Customer Credits (2350) / CR Sales Discounts. if (remaining > 0) { - var ccAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.AccountNumber == "2350" && a.IsActive); - var sdAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.AccountNumber == "4950" && a.IsActive); + var ccAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.CompanyId == memo.CompanyId && a.AccountNumber == "2350" && a.IsActive); + var sdAcct = await _unitOfWork.Accounts.FirstOrDefaultAsync(a => a.CompanyId == memo.CompanyId && a.AccountNumber == "4950" && a.IsActive); await _accountBalanceService.DebitAsync(ccAcct?.Id, remaining); await _accountBalanceService.CreditAsync(sdAcct?.Id, remaining); } diff --git a/src/PowderCoating.Web/Controllers/GiftCertificatesController.cs b/src/PowderCoating.Web/Controllers/GiftCertificatesController.cs index badaa9f..c7df7c3 100644 --- a/src/PowderCoating.Web/Controllers/GiftCertificatesController.cs +++ b/src/PowderCoating.Web/Controllers/GiftCertificatesController.cs @@ -254,14 +254,14 @@ public class GiftCertificatesController : Controller if (dto.IssuedReason == GiftCertificateIssuedReason.Sold) { var checkingAcctId = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && (a.AccountSubType == AccountSubTypeEnum.Checking + a => a.CompanyId == companyId && a.IsActive && (a.AccountSubType == AccountSubTypeEnum.Checking || a.AccountSubType == AccountSubTypeEnum.Cash)); await _accountBalanceService.DebitAsync(checkingAcctId?.Id, cert.OriginalAmount); } else { var discountAcctId = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "4950"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "4950"); await _accountBalanceService.DebitAsync(discountAcctId?.Id, cert.OriginalAmount); } @@ -310,7 +310,7 @@ public class GiftCertificatesController : Controller var companyId = currentUser?.CompanyId ?? 0; var gcLiabilityAcctId = await GetGcLiabilityAccountIdAsync(companyId); var otherIncomeAcctId = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountSubType == AccountSubTypeEnum.OtherIncome); + a => a.CompanyId == companyId && a.IsActive && a.AccountSubType == AccountSubTypeEnum.OtherIncome); await _accountBalanceService.DebitAsync(gcLiabilityAcctId, remaining); await _accountBalanceService.CreditAsync(otherIncomeAcctId?.Id, remaining); } @@ -437,7 +437,7 @@ public class GiftCertificatesController : Controller private async Task GetGcLiabilityAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "2500"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "2500"); return acct?.Id; } @@ -477,14 +477,14 @@ public class GiftCertificatesController : Controller if (dto.IssuedReason == GiftCertificateIssuedReason.Sold) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && (a.AccountSubType == AccountSubTypeEnum.Checking + a => a.CompanyId == companyId && a.IsActive && (a.AccountSubType == AccountSubTypeEnum.Checking || a.AccountSubType == AccountSubTypeEnum.Cash)); checkingAcctId = acct?.Id; } else { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "4950"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "4950"); discountAcctId = acct?.Id; } diff --git a/src/PowderCoating.Web/Controllers/InvoicesController.cs b/src/PowderCoating.Web/Controllers/InvoicesController.cs index d723a37..6d41848 100644 --- a/src/PowderCoating.Web/Controllers/InvoicesController.cs +++ b/src/PowderCoating.Web/Controllers/InvoicesController.cs @@ -305,8 +305,9 @@ public class InvoicesController : Controller && company?.StripeConnectStatus == StripeConnectStatus.Active; // Expense accounts for the write-off bad-debt modal + var expenseCompanyId = _tenantContext.GetCurrentCompanyId() ?? 0; var expenseAccounts = await _unitOfWork.Accounts.FindAsync( - a => a.IsActive && a.AccountType == AccountType.Expense); + a => a.CompanyId == expenseCompanyId && a.IsActive && a.AccountType == AccountType.Expense); ViewBag.ExpenseAccounts = expenseAccounts .OrderBy(a => a.AccountNumber).ThenBy(a => a.Name) .Select(a => new SelectListItem($"{a.AccountNumber} – {a.Name}", a.Id.ToString())) @@ -2458,7 +2459,8 @@ public class InvoicesController : Controller /// private async Task PopulateBankAccountsAsync() { - var accounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive + var companyId = _tenantContext.GetCurrentCompanyId() ?? 0; + var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive && (a.AccountSubType == AccountSubType.Cash || a.AccountSubType == AccountSubType.Checking || a.AccountSubType == AccountSubType.Savings)); @@ -2473,7 +2475,7 @@ public class InvoicesController : Controller private async Task GetCheckingAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && (a.AccountSubType == AccountSubType.Checking + a => a.CompanyId == companyId && a.IsActive && (a.AccountSubType == AccountSubType.Checking || a.AccountSubType == AccountSubType.Cash)); return acct?.Id; } @@ -2482,7 +2484,7 @@ public class InvoicesController : Controller private async Task GetCustomerDepositsAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "2300"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "2300"); return acct?.Id; } @@ -2490,7 +2492,7 @@ public class InvoicesController : Controller private async Task GetSalesReturnsAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "4960"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "4960"); return acct?.Id; } @@ -2498,7 +2500,7 @@ public class InvoicesController : Controller private async Task GetCustomerCreditsAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "2350"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "2350"); return acct?.Id; } @@ -2506,7 +2508,7 @@ public class InvoicesController : Controller private async Task GetArAccountIdAsync(int companyId) { var accounts = await _unitOfWork.Accounts.FindAsync( - a => a.IsActive && a.AccountSubType == AccountSubType.AccountsReceivable); + a => a.CompanyId == companyId && a.IsActive && a.AccountSubType == AccountSubType.AccountsReceivable); return accounts.FirstOrDefault()?.Id; } @@ -2517,7 +2519,7 @@ public class InvoicesController : Controller private async Task GetBadDebtAccountIdAsync(int companyId) { var expenses = await _unitOfWork.Accounts.FindAsync( - a => a.IsActive && a.AccountType == AccountType.Expense); + a => a.CompanyId == companyId && a.IsActive && a.AccountType == AccountType.Expense); return expenses.FirstOrDefault(a => a.Name.Contains("bad", StringComparison.OrdinalIgnoreCase) || a.Name.Contains("debt", StringComparison.OrdinalIgnoreCase))?.Id ?? expenses.FirstOrDefault()?.Id; @@ -2548,9 +2550,9 @@ public class InvoicesController : Controller private async Task ResolveSalesTaxAccountIdAsync(int companyId) { var taxAccount = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountNumber == "2200" && a.IsActive); + a => a.CompanyId == companyId && a.AccountNumber == "2200" && a.IsActive); taxAccount ??= await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountType == AccountType.Liability && a.IsActive && a.Name.ToLower().Contains("tax")); + a => a.CompanyId == companyId && a.AccountType == AccountType.Liability && a.IsActive && a.Name.ToLower().Contains("tax")); return taxAccount?.Id; } @@ -2562,9 +2564,9 @@ public class InvoicesController : Controller private async Task GetSalesDiscountAccountIdAsync(int companyId) { var discountAccount = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountNumber == "4950" && a.IsActive); + a => a.CompanyId == companyId && a.AccountNumber == "4950" && a.IsActive); discountAccount ??= await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.AccountType == AccountType.Revenue && a.IsActive && a.Name.ToLower().Contains("discount")); + a => a.CompanyId == companyId && a.AccountType == AccountType.Revenue && a.IsActive && a.Name.ToLower().Contains("discount")); return discountAccount?.Id; } @@ -2572,7 +2574,7 @@ public class InvoicesController : Controller private async Task GetGcLiabilityAccountIdAsync(int companyId) { var acct = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.IsActive && a.AccountNumber == "2500"); + a => a.CompanyId == companyId && a.IsActive && a.AccountNumber == "2500"); return acct?.Id; } diff --git a/src/PowderCoating.Web/Controllers/JournalEntriesController.cs b/src/PowderCoating.Web/Controllers/JournalEntriesController.cs index 1446119..5686756 100644 --- a/src/PowderCoating.Web/Controllers/JournalEntriesController.cs +++ b/src/PowderCoating.Web/Controllers/JournalEntriesController.cs @@ -251,6 +251,14 @@ public class JournalEntriesController : Controller return RedirectToAction(nameof(SalesTaxPayment)); } + // Don't let a remittance exceed the outstanding liability — overpaying would push Sales Tax + // Payable into an abnormal (debit) balance. The 0.005 tolerance absorbs decimal rounding. + if (amount > taxAcct.CurrentBalance + 0.005m) + { + TempData["Error"] = $"Payment of {amount:C} exceeds the Sales Tax Payable balance of {taxAcct.CurrentBalance:C}. Enter an amount up to the outstanding liability."; + return RedirectToAction(nameof(SalesTaxPayment)); + } + var bankAcct = await _unitOfWork.Accounts.GetByIdAsync(bankAccountId); if (bankAcct == null || bankAcct.CompanyId != companyId) {