diff --git a/docs/ACCOUNTING_AUDIT.md b/docs/ACCOUNTING_AUDIT.md index d433aea..be83e51 100644 --- a/docs/ACCOUNTING_AUDIT.md +++ b/docs/ACCOUNTING_AUDIT.md @@ -230,16 +230,21 @@ deposit account. **No ledger-drift bugs found.** Notes: - **Deposit account picker ↔ recompute:** consistent. Live posting debits the chosen `DepositAccountId`, and `LedgerService` reproduces the debit by `DepositAccountId == accountId` (lines ~78/724). Picking a non-default deposit account recomputes correctly. -- **Bank / "pay-from" / bank-rec pickers** now list all Asset + Liability accounts with **no server-side type - guard** — a user could pick a nonsensical source (e.g. A/R). Postings stay sign-correct - (`AccountBalanceService` keys sign off `AccountSubType`), and this is the intended "trust the operator" - tradeoff, but there is no longer a guardrail. Accepted; noted here for visibility. +- **Bank / "pay-from" / bank-rec pickers — server-side guard added (2026-06-20):** the submitted account is + now validated via `AccountGuard.IsValidMoneyAccountAsync` (active, company-owned, AccountType Asset or + Liability) before any posting, at bill `RecordPayment` / `Create(payNow)` / `EditPayment`, + `BankReconciliation.Create`, and deposit `Record`. Defense in depth against tampered/stale POSTs. Per the + "trust the operator" decision this still allows e.g. A/R (an Asset) as a source — it only rejects + non-money types (Revenue/Expense/Equity/COGS). - **Latent deposit imbalance — RESOLVED (2026-06-20):** a deposit saved with a null `DepositAccountId` posted `CR 2300` with no offsetting debit → unbalanced. `DepositsController.Record` now blocks recording when the `2300` Customer Deposits account exists but no deposit/bank account resolves (user must pick one). When `2300` doesn't exist (company not using accounting), no GL posts at all, so the deposit is still allowed through. -- **Pre-existing type/sub-type mismatch risk:** account create does not enforce a valid type↔sub-type pairing, - and sign convention keys off sub-type — a mis-paired account would post with the wrong sign. Backlog item. +- **Type/sub-type mismatch risk — RESOLVED (2026-06-20):** account `AccountType` is now **derived** from the + chosen `AccountSubType` on create/edit via `AccountClassification.TypeForSubType` (single source of truth, + also used by the Create pre-select), so the two can never disagree and the sub-type-based sign convention is + always consistent with the displayed type. A read-only sweep of the dev DB (109 accounts) found **0** existing + mismatches, so no repair tool was needed. ## Status **All findings O1–O9 + the read-path sweep are resolved** on `dev` (O9 by policy decision — expense at diff --git a/src/PowderCoating.Core/Enums/AccountClassification.cs b/src/PowderCoating.Core/Enums/AccountClassification.cs new file mode 100644 index 0000000..20ee752 --- /dev/null +++ b/src/PowderCoating.Core/Enums/AccountClassification.cs @@ -0,0 +1,31 @@ +namespace PowderCoating.Core.Enums; + +/// +/// Single source of truth mapping an to its parent +/// . Each sub-type belongs to exactly one type, so the type can always +/// be derived from the sub-type. Used on account create/edit to keep the two fields consistent +/// (a mismatched pair would post with the wrong debit/credit sign, since the sign convention keys +/// off the sub-type) and anywhere else that needs the canonical pairing. +/// +public static class AccountClassification +{ + /// Returns the parent for a given . + public static AccountType TypeForSubType(AccountSubType subType) => subType switch + { + AccountSubType.Cash or AccountSubType.Checking or AccountSubType.Savings + or AccountSubType.AccountsReceivable or AccountSubType.Inventory or AccountSubType.FixedAsset + or AccountSubType.OtherCurrentAsset or AccountSubType.OtherAsset => AccountType.Asset, + + AccountSubType.AccountsPayable or AccountSubType.CreditCard + or AccountSubType.OtherCurrentLiability or AccountSubType.LongTermLiability => AccountType.Liability, + + AccountSubType.OwnersEquity or AccountSubType.RetainedEarnings => AccountType.Equity, + + AccountSubType.Sales or AccountSubType.ServiceRevenue or AccountSubType.OtherIncome => AccountType.Revenue, + + AccountSubType.CostOfGoodsSold => AccountType.CostOfGoods, + + // All expense sub-types (enum values >= 50) and any future additions default to Expense. + _ => AccountType.Expense + }; +} diff --git a/src/PowderCoating.Web/Controllers/AccountsController.cs b/src/PowderCoating.Web/Controllers/AccountsController.cs index c070c9f..7982bc0 100644 --- a/src/PowderCoating.Web/Controllers/AccountsController.cs +++ b/src/PowderCoating.Web/Controllers/AccountsController.cs @@ -91,18 +91,7 @@ public class AccountsController : Controller if (preSubType.HasValue) { dto.AccountSubType = preSubType.Value; - dto.AccountType = preSubType.Value switch - { - AccountSubType.Cash or AccountSubType.Checking or AccountSubType.Savings - or AccountSubType.AccountsReceivable or AccountSubType.Inventory or AccountSubType.FixedAsset - or AccountSubType.OtherCurrentAsset or AccountSubType.OtherAsset => AccountType.Asset, - AccountSubType.AccountsPayable or AccountSubType.CreditCard - or AccountSubType.OtherCurrentLiability or AccountSubType.LongTermLiability => AccountType.Liability, - AccountSubType.OwnersEquity or AccountSubType.RetainedEarnings => AccountType.Equity, - AccountSubType.Sales or AccountSubType.ServiceRevenue or AccountSubType.OtherIncome => AccountType.Revenue, - AccountSubType.CostOfGoodsSold => AccountType.CostOfGoods, - _ => AccountType.Expense - }; + dto.AccountType = AccountClassification.TypeForSubType(preSubType.Value); } ViewBag.Inline = inline; if (inline) @@ -151,6 +140,9 @@ public class AccountsController : Controller var account = _mapper.Map(dto); account.CompanyId = currentUser!.CompanyId; account.CreatedBy = currentUser.Email; + // Derive the parent type from the chosen sub-type so the two can never disagree — + // a mismatch would post with the wrong debit/credit sign (sign keys off sub-type). + account.AccountType = AccountClassification.TypeForSubType(account.AccountSubType); await _unitOfWork.Accounts.AddAsync(account); await _unitOfWork.CompleteAsync(); @@ -226,6 +218,9 @@ public class AccountsController : Controller } _mapper.Map(dto, account); + // Keep type consistent with the chosen sub-type (see Create) so the sign convention, + // which keys off sub-type, can never be at odds with the displayed account type. + account.AccountType = AccountClassification.TypeForSubType(account.AccountSubType); account.UpdatedAt = DateTime.UtcNow; account.UpdatedBy = (await _userManager.GetUserAsync(User))?.Email; diff --git a/src/PowderCoating.Web/Controllers/BankReconciliationsController.cs b/src/PowderCoating.Web/Controllers/BankReconciliationsController.cs index 49cd142..a1d2282 100644 --- a/src/PowderCoating.Web/Controllers/BankReconciliationsController.cs +++ b/src/PowderCoating.Web/Controllers/BankReconciliationsController.cs @@ -8,6 +8,7 @@ using PowderCoating.Core.Entities; using PowderCoating.Core.Enums; using PowderCoating.Core.Interfaces; using PowderCoating.Shared.Constants; +using PowderCoating.Web.Helpers; namespace PowderCoating.Web.Controllers; @@ -73,6 +74,13 @@ public class BankReconciliationsController : Controller var companyId = _tenantContext.GetCurrentCompanyId() ?? 0; + // The account being reconciled must be a real money account (Asset/Liability). + if (!await AccountGuard.IsValidMoneyAccountAsync(_unitOfWork, model.AccountId, companyId)) + { + TempData["Error"] = "Select a valid bank, cash, or credit account to reconcile."; + return RedirectToAction(nameof(Create)); + } + // Set beginning balance from last completed reconciliation for this account, or 0 var lastCompleted = (await _unitOfWork.BankReconciliations.FindAsync( br => br.CompanyId == companyId diff --git a/src/PowderCoating.Web/Controllers/BillsController.cs b/src/PowderCoating.Web/Controllers/BillsController.cs index 7536f41..57970fe 100644 --- a/src/PowderCoating.Web/Controllers/BillsController.cs +++ b/src/PowderCoating.Web/Controllers/BillsController.cs @@ -340,6 +340,16 @@ public class BillsController : Controller } } + // Validate the pay-from account before entering the transaction so an invalid + // selection rejects the whole request rather than saving a bill with no payment. + if (payNow && paymentMethod.HasValue && bankAccountId.HasValue && currentUser != null + && !await AccountGuard.IsValidMoneyAccountAsync(_unitOfWork, bankAccountId, currentUser.CompanyId)) + { + ModelState.AddModelError(string.Empty, "Choose a valid bank or credit account to record the payment."); + await PopulateDropdownsAsync(); + return View(dto); + } + Bill? bill = null; // Bill entity, PO back-reference, and optional immediate payment all commit @@ -718,6 +728,14 @@ public class BillsController : Controller return RedirectToAction(nameof(Details), new { id = dto.BillId }); } + // The pay-from account must be a real money account (Asset/Liability) — defense in depth + // against a tampered or stale selection before we post to it. + if (!await AccountGuard.IsValidMoneyAccountAsync(_unitOfWork, dto.BankAccountId, bill.CompanyId)) + { + TempData["Error"] = "Select a valid bank or credit account to pay from."; + return RedirectToAction(nameof(Details), new { id = dto.BillId }); + } + var currentUser = await _userManager.GetUserAsync(User); var payment = _mapper.Map(dto); @@ -843,6 +861,13 @@ public class BillsController : Controller var payment = await _unitOfWork.BillPayments.GetByIdAsync(dto.PaymentId); if (payment == null) return NotFound(); + // Reject an invalid new pay-from account before we move the balance to it. + if (!await AccountGuard.IsValidMoneyAccountAsync(_unitOfWork, dto.BankAccountId, payment.CompanyId)) + { + TempData["Error"] = "Select a valid bank or credit account."; + return RedirectToAction(nameof(Details), new { id = dto.BillId }); + } + var currentUser = await _userManager.GetUserAsync(User); // If the bank account changed, reverse the old balance entry and apply the new one diff --git a/src/PowderCoating.Web/Controllers/DepositsController.cs b/src/PowderCoating.Web/Controllers/DepositsController.cs index 10ced5d..e7f8128 100644 --- a/src/PowderCoating.Web/Controllers/DepositsController.cs +++ b/src/PowderCoating.Web/Controllers/DepositsController.cs @@ -9,6 +9,7 @@ using PowderCoating.Core.Entities; using PowderCoating.Core.Enums; using PowderCoating.Core.Interfaces; using PowderCoating.Shared.Constants; +using PowderCoating.Web.Helpers; using QuestPDF.Fluent; using QuestPDF.Helpers; using QuestPDF.Infrastructure; @@ -87,13 +88,10 @@ public class DepositsController : Controller // auto-pick of the first Checking/Cash account. Validate any user-supplied id // belongs to this company (defense in depth — the global filter alone isn't enough). int? depositAcctId = null; - if (depositAccountId.HasValue) + if (depositAccountId.HasValue && + await AccountGuard.IsValidMoneyAccountAsync(_unitOfWork, depositAccountId, currentUser.CompanyId)) { - var chosen = await _unitOfWork.Accounts.FirstOrDefaultAsync( - a => a.Id == depositAccountId.Value - && a.CompanyId == currentUser.CompanyId - && a.IsActive); - depositAcctId = chosen?.Id; + depositAcctId = depositAccountId; } depositAcctId ??= await GetCheckingAccountIdAsync(currentUser.CompanyId); diff --git a/src/PowderCoating.Web/Helpers/AccountGuard.cs b/src/PowderCoating.Web/Helpers/AccountGuard.cs new file mode 100644 index 0000000..dd906f4 --- /dev/null +++ b/src/PowderCoating.Web/Helpers/AccountGuard.cs @@ -0,0 +1,28 @@ +using PowderCoating.Core.Enums; +using PowderCoating.Core.Interfaces; + +namespace PowderCoating.Web.Helpers; + +/// +/// Server-side validation for account selections that must be a "money" account — a payment +/// source (bill payment), deposit target, or reconcilable account. The dropdowns already limit +/// the choices, so this is defense in depth against tampered or stale POSTs (e.g. an account +/// deleted/retyped between page load and submit): it rejects anything that isn't an active, +/// company-owned Asset or Liability account before a GL posting is made against it. +/// +internal static class AccountGuard +{ + /// + /// Returns true when identifies an active account belonging to + /// whose top-level type is Asset or Liability. Filters CompanyId + /// explicitly (defense in depth alongside the global tenant filter). + /// + internal static async Task IsValidMoneyAccountAsync(IUnitOfWork unitOfWork, int? accountId, int companyId) + { + if (accountId == null) return false; + var account = await unitOfWork.Accounts.FirstOrDefaultAsync( + a => a.Id == accountId.Value && a.CompanyId == companyId && a.IsActive); + return account != null + && (account.AccountType == AccountType.Asset || account.AccountType == AccountType.Liability); + } +}