spouliot
7576761b70
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 <noreply@anthropic.com>
114 lines
9.0 KiB
Markdown
114 lines
9.0 KiB
Markdown
# Accounting System Audit
|
||
|
||
> Living record of the accounting/GL audit and its remediation status.
|
||
> **Keep this file updated** whenever an accounting finding is opened or closed — the
|
||
> original audit was lost once because it was never written down. Don't let that happen again.
|
||
|
||
Last reconciled against code: **2026-06-19** (branch `dev`, commits through `9532812`).
|
||
Verification at that point: `dotnet build` clean (0 errors); `dotnet test tests/PowderCoating.UnitTests` → **290 passed, 0 failed**.
|
||
|
||
---
|
||
|
||
## How the GL is modeled (context for the findings)
|
||
|
||
Balances are computed two different ways, and they must agree:
|
||
|
||
1. **At posting time** — controllers call `AccountBalanceService.DebitAsync/CreditAsync`, which
|
||
mutate the denormalized `Account.CurrentBalance`.
|
||
2. **At report time** — `FinancialReportService` (Trial Balance, Balance Sheet, P&L, AR/AP aging,
|
||
statements) and `LedgerService` (per-account ledger + `RecalculateBalances`) recompute balances
|
||
from source documents.
|
||
|
||
`LedgerService` also backs the **Balance Reconciliation report** (the detective control added in
|
||
`c2cd19e`), which compares stored `CurrentBalance` vs the ledger-recomputed balance. Any account
|
||
whose recompute path is incomplete will (a) be silently corrupted by a "Recalculate Balances" run
|
||
and (b) produce misleading output in the reconciliation report.
|
||
|
||
---
|
||
|
||
## Resolved findings (this audit batch)
|
||
|
||
| # | Finding | Fix | Commit | Verified |
|
||
|---|---------|-----|--------|----------|
|
||
| **#4** | `Sales Tax Payable` (2200) was credited on every invoice but never relieved — the liability grew forever (no remittance flow). | `JournalEntries/SalesTaxPayment` GET+POST: DR 2200 / CR bank, balanced, in a transaction, honors the period lock and validates amount + bank-account tenancy. | `0c921ba` | Posting reviewed `JournalEntriesController:235-296`. Reporting needs no change (posted JE lines already flow through TB/BS/ledger). |
|
||
| **#7** | Most report queries relied on the global tenant filter, which is **bypassed for SuperAdmin** → cross-company leakage on P&L / Balance Sheet / Trial Balance / aging / statements. | Explicit `CompanyId == companyId` predicate added to every query in `FinancialReportService`. | `9532812` | Confirmed: every query in `FinancialReportService.cs` carries an explicit `CompanyId` predicate. |
|
||
| **#9a** | A cash refund posted **DR AR** (control up) while the customer subledger went down — opposite directions → guaranteed AR drift. | Refund now **reverses the sale**: DR Sales Returns (4960) + DR Sales Tax Payable (tax portion) / CR bank; AR untouched. Split centralized in `Core/Accounting/RefundAllocation.Split`. | `2a82a1d` | Posting (`InvoicesController.IssueRefund`/`CancelRefund`) and both recompute paths (`LedgerService` §5b, `FinancialReportService` TB/BS/P&L) all use `RefundAllocation.Split` — they agree by construction. |
|
||
| **#9b** | Store-credit refunds & credit memos posted nothing to the GL on issue (only a `CreditMemo` + `Customer.CreditBalance`) → outstanding store credit invisible on the balance sheet. | New **2350 Customer Credits** liability. Issue: DR 4950 / CR 2350. Apply: DR 2350 / CR AR. Void remainder: DR 2350 / CR 4950. Updated in all 8 posting sites + TB/BS/P&L + ledger §12b. | `9ce3612` | Posting sites confirmed (`CreditMemosController` 180-185/262-270/314-320; `InvoicesController` store-credit path). Reporting `cmContraRevenue`/`cmIssuedNonVoided` math reviewed. |
|
||
| — | No detective control to catch denormalized-balance drift. | **Balance Reconciliation report** (`Reports/Reconciliation`): per-account stored vs recomputed, plus AR/AP control-vs-subledger. | `c2cd19e` | Reviewed `FinancialReportService.GetBalanceReconciliationAsync`. NOTE: its usefulness is limited by O2 below. |
|
||
|
||
### Original numbering gap
|
||
The commits reference findings **#4, #7, #9a, #9b** — implying a list of at least #1–#9. The original
|
||
audit document was never persisted and is unrecoverable. Findings **#1, #2, #3, #5, #6, #8** cannot be
|
||
matched to commits and their status is **unknown**. If the original list resurfaces, reconcile it here.
|
||
|
||
---
|
||
|
||
## Resolved findings (2026-06-19 remediation)
|
||
|
||
### O1 — Account 2300 mislabeled "Payroll Liabilities" while used as Customer Deposits — **RESOLVED**
|
||
- **Root cause refined:** there is no payroll feature posting to 2300; the deposit GL code has always used
|
||
2300 (resolved by number) as the Customer Deposits liability. Migration `AccountingDepositsGL` already
|
||
renamed it to "Customer Deposits" for tenants that lacked a 2300, but its `NOT EXISTS` guard **skipped**
|
||
pre-existing tenants, leaving them mislabeled. The two seed files still created new tenants with the
|
||
wrong name.
|
||
- **Fix (chosen approach: rename 2300, move payroll to a new 2400):**
|
||
- Seed files now create **2300 = "Customer Deposits" (IsSystem)** and a separate **2400 = "Payroll
|
||
Liabilities"** — `SeedDataService.Accounts.cs`, `SeedData.cs`.
|
||
- `EnsureSystemAccountsAsync` self-heals: renames any 2300 still named "Payroll Liabilities" → "Customer
|
||
Deposits" (preserving user renames) and ensures 2400 exists.
|
||
- Migration `20260619233108_RenameDepositsAccountAddPayroll` does the same for existing tenants (rename
|
||
only where Name is still the default; insert 2400 where missing). Down is best-effort (soft-deletes
|
||
only empty 2400s; does not re-introduce the mislabel).
|
||
- Account **number 2300 is unchanged**, so the deposit posting code (`DepositsController`,
|
||
`InvoicesController`) needed no changes — its lookups are by `"2300"`.
|
||
|
||
### O2 — `LedgerService` never recomputed 4950 Sales Discounts → recalc corrupted it — **RESOLVED**
|
||
- **Fix:** added a 4950 section to both `LedgerService.GetAccountLedgerAsync` and `ComputePriorBalanceAsync`
|
||
that reproduces the *actual postings* so a balance recalc matches the stored balance:
|
||
invoice discounts (DR at invoice date) + credit-memo issuance (DR full amount at issue) − the unapplied
|
||
remainder of voided memos (CR at void). This mirrors what `AccountBalanceService` posts at
|
||
`InvoicesController:849/1108/1629` and `CreditMemosController`, so the Balance Reconciliation report no
|
||
longer shows false drift on 4950.
|
||
- **Note / micro-discrepancy:** the ledger uses `memo.AmountApplied` for the voided remainder (matching the
|
||
true posting), whereas `FinancialReportService` TB/BS derive the voided portion from applications against
|
||
non-voided invoices. These differ only in the rare case of a voided memo applied to a since-voided
|
||
invoice. Left as-is (report-side nuance, not O2's target); documented here so it isn't mistaken for a bug.
|
||
- Regression test: `LedgerServiceTests.GetAccountLedgerAsync_SalesDiscounts4950_IncludesInvoiceDiscountsAndCreditMemoContraRevenue`.
|
||
|
||
Verification of O1+O2: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests` → **291 passed**;
|
||
migration applied to the dev database successfully.
|
||
|
||
---
|
||
|
||
### 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.
|
||
|
||
### 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.
|
||
|
||
Verification of O3+O4: `dotnet build` clean; `dotnet test tests/PowderCoating.UnitTests` → **291 passed**.
|
||
|
||
---
|
||
|
||
## 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.
|