Scope all controller account lookups by CompanyId (defense-in-depth sweep)
Completes the read-path defense-in-depth pass flagged in the accounting audit:
every Accounts lookup in a controller now carries an explicit CompanyId predicate,
matching the standing rule in CLAUDE.md ("every FindAsync/GetAllAsync must include
an explicit CompanyId"). ~19 lookups across 12 controllers:
- Tier 1 (write-path): AccountsController duplicate account-number check (Create/Edit)
- Tier 2 (dropdowns/lists): Accounts (Index/year-end/parent), BankReconciliations,
Bills (bank list + receipt scan + suggest), Budgets, CatalogItems, Expenses,
FixedAssets, Inventory, JournalEntries chart dropdown, Vendors
- Tier 3 (accountIds.Contains display maps): JournalEntries/Reports/VendorCredits
detail views, scoped via the in-scope entity's CompanyId for uniformity
companyId source per controller: _tenantContext where available, else the in-scope
entity's CompanyId, else the current user. Build clean; 291 unit tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -118,13 +118,17 @@ Verification of O3+O4: `dotnet build` clean; `dotnet test tests/PowderCoating.Un
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Remaining (non-O1–O4) — known lower-risk follow-up
|
## Read-path account lookup sweep (Tier 1–3) — **RESOLVED**
|
||||||
Pure **read-path dropdown/display** account lookups still rely on the global tenant filter (correct for
|
Completed the app-wide defense-in-depth pass: every `Accounts` lookup in a controller now carries an
|
||||||
non-SuperAdmin; only a SuperAdmin acting inside a company could see another tenant's accounts in a picker).
|
explicit `CompanyId` predicate, matching the standing rule in CLAUDE.md. ~19 lookups across 12 controllers:
|
||||||
These are app-wide, not specific to the accounting posting path, e.g. `VendorCreditsController`
|
- **Tier 1 (write-path validation):** `AccountsController` duplicate account-number check on Create/Edit.
|
||||||
PopulateDropdowns/Details, `BillsController` PopulateDropdowns and Index/edit account lists,
|
- **Tier 2 (dropdowns/lists):** `AccountsController` (Index/year-end/parent dropdown), `BankReconciliations`,
|
||||||
`ExpensesController` filter + categorization pickers. Tracked here so they aren't mistaken for a regression;
|
`Bills` (bank list + receipt-scan + suggest), `Budgets`, `CatalogItems`, `Expenses`, `FixedAssets`,
|
||||||
fold into a broader defense-in-depth pass if/when desired.
|
`Inventory`, `JournalEntries` (chart dropdown), `Vendors`.
|
||||||
|
- **Tier 3 (`accountIds.Contains` display maps):** `JournalEntries` Details, `Reports` budget vs actual,
|
||||||
|
`VendorCredits` Details — scoped via the in-scope entity's `CompanyId` for uniformity.
|
||||||
|
`companyId` source per controller: `_tenantContext.GetCurrentCompanyId()` where available, else the
|
||||||
|
in-scope entity's `CompanyId`, else `_userManager` current user. Build clean; 291 unit tests pass.
|
||||||
|
|
||||||
## Status
|
## Status
|
||||||
All audit findings **O1–O4 are resolved** on `dev`. Original audit numbering #1–3/#5/#6/#8 remains
|
All audit findings **O1–O4 are resolved** on `dev`. Original audit numbering #1–3/#5/#6/#8 remains
|
||||||
|
|||||||
@@ -55,7 +55,8 @@ public class AccountsController : Controller
|
|||||||
// GET: /Accounts
|
// GET: /Accounts
|
||||||
public async Task<IActionResult> Index()
|
public async Task<IActionResult> Index()
|
||||||
{
|
{
|
||||||
var accounts = await _unitOfWork.Accounts.GetAllAsync(false, a => a.ParentAccount);
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
|
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId, false, a => a.ParentAccount);
|
||||||
|
|
||||||
var dtos = _mapper.Map<List<AccountListDto>>(accounts.OrderBy(a => a.AccountNumber).ToList());
|
var dtos = _mapper.Map<List<AccountListDto>>(accounts.OrderBy(a => a.AccountNumber).ToList());
|
||||||
|
|
||||||
@@ -134,7 +135,7 @@ public class AccountsController : Controller
|
|||||||
var currentUser = await _userManager.GetUserAsync(User);
|
var currentUser = await _userManager.GetUserAsync(User);
|
||||||
|
|
||||||
// Check for duplicate account number
|
// Check for duplicate account number
|
||||||
var existing = await _unitOfWork.Accounts.FindAsync(a => a.AccountNumber == dto.AccountNumber);
|
var existing = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == currentUser!.CompanyId && a.AccountNumber == dto.AccountNumber);
|
||||||
if (existing.Any())
|
if (existing.Any())
|
||||||
{
|
{
|
||||||
ModelState.AddModelError(nameof(dto.AccountNumber), "An account with this number already exists.");
|
ModelState.AddModelError(nameof(dto.AccountNumber), "An account with this number already exists.");
|
||||||
@@ -213,7 +214,7 @@ public class AccountsController : Controller
|
|||||||
|
|
||||||
// Check duplicate number (excluding self)
|
// Check duplicate number (excluding self)
|
||||||
var existing = await _unitOfWork.Accounts.FindAsync(
|
var existing = await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.AccountNumber == dto.AccountNumber && a.Id != id);
|
a => a.CompanyId == account.CompanyId && a.AccountNumber == dto.AccountNumber && a.Id != id);
|
||||||
if (existing.Any())
|
if (existing.Any())
|
||||||
{
|
{
|
||||||
ModelState.AddModelError(nameof(dto.AccountNumber), "An account with this number already exists.");
|
ModelState.AddModelError(nameof(dto.AccountNumber), "An account with this number already exists.");
|
||||||
@@ -472,7 +473,7 @@ public class AccountsController : Controller
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Load all active accounts with balances
|
// Load all active accounts with balances
|
||||||
var accounts = (await _unitOfWork.Accounts.FindAsync(a => a.IsActive)).ToList();
|
var accounts = (await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive)).ToList();
|
||||||
|
|
||||||
var revenueAccounts = accounts.Where(a => a.AccountType == AccountType.Revenue).ToList();
|
var revenueAccounts = accounts.Where(a => a.AccountType == AccountType.Revenue).ToList();
|
||||||
var expenseAccounts = accounts.Where(a =>
|
var expenseAccounts = accounts.Where(a =>
|
||||||
@@ -616,7 +617,8 @@ public class AccountsController : Controller
|
|||||||
/// </summary>
|
/// </summary>
|
||||||
private async Task PopulateDropdownsAsync(int? excludeId = null)
|
private async Task PopulateDropdownsAsync(int? excludeId = null)
|
||||||
{
|
{
|
||||||
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => excludeId == null || a.Id != excludeId.Value);
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
|
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && (excludeId == null || a.Id != excludeId.Value));
|
||||||
|
|
||||||
ViewBag.ParentAccounts = allAccounts
|
ViewBag.ParentAccounts = allAccounts
|
||||||
.OrderBy(a => a.AccountNumber)
|
.OrderBy(a => a.AccountNumber)
|
||||||
|
|||||||
@@ -365,8 +365,9 @@ public class BankReconciliationsController : Controller
|
|||||||
|
|
||||||
private async Task PopulateAccountDropdownAsync()
|
private async Task PopulateAccountDropdownAsync()
|
||||||
{
|
{
|
||||||
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(
|
var accounts = await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.IsActive
|
a => a.CompanyId == companyId && a.IsActive
|
||||||
&& (a.AccountSubType == AccountSubType.Checking
|
&& (a.AccountSubType == AccountSubType.Checking
|
||||||
|| a.AccountSubType == AccountSubType.Savings
|
|| a.AccountSubType == AccountSubType.Savings
|
||||||
|| a.AccountSubType == AccountSubType.Cash));
|
|| a.AccountSubType == AccountSubType.Cash));
|
||||||
|
|||||||
@@ -453,10 +453,11 @@ public class BillsController : Controller
|
|||||||
|
|
||||||
// Payment form defaults
|
// Payment form defaults
|
||||||
var bankAccounts = (await _unitOfWork.Accounts.FindAsync(
|
var bankAccounts = (await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.AccountSubType == AccountSubType.Cash ||
|
a => a.CompanyId == bill.CompanyId &&
|
||||||
|
(a.AccountSubType == AccountSubType.Cash ||
|
||||||
a.AccountSubType == AccountSubType.Checking ||
|
a.AccountSubType == AccountSubType.Checking ||
|
||||||
a.AccountSubType == AccountSubType.Savings ||
|
a.AccountSubType == AccountSubType.Savings ||
|
||||||
a.AccountSubType == AccountSubType.CreditCard))
|
a.AccountSubType == AccountSubType.CreditCard)))
|
||||||
.OrderBy(a => a.AccountNumber)
|
.OrderBy(a => a.AccountNumber)
|
||||||
.ToList();
|
.ToList();
|
||||||
|
|
||||||
@@ -1077,7 +1078,8 @@ public class BillsController : Controller
|
|||||||
return Json(new { success = false, error = "File must be under 10 MB." });
|
return Json(new { success = false, error = "File must be under 10 MB." });
|
||||||
|
|
||||||
// Load expense accounts for matching
|
// Load expense accounts for matching
|
||||||
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive);
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
|
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive);
|
||||||
var expenseAccounts = allAccounts
|
var expenseAccounts = allAccounts
|
||||||
.Where(a => a.AccountType == AccountType.Expense ||
|
.Where(a => a.AccountType == AccountType.Expense ||
|
||||||
a.AccountType == AccountType.CostOfGoods ||
|
a.AccountType == AccountType.CostOfGoods ||
|
||||||
@@ -1097,7 +1099,6 @@ public class BillsController : Controller
|
|||||||
var imageBytes = ms.ToArray();
|
var imageBytes = ms.ToArray();
|
||||||
|
|
||||||
var result = await _accountingAi.ScanReceiptAsync(imageBytes, receiptImage.ContentType, expenseAccounts);
|
var result = await _accountingAi.ScanReceiptAsync(imageBytes, receiptImage.ContentType, expenseAccounts);
|
||||||
var companyId = int.TryParse(User.FindFirst("CompanyId")?.Value, out var cid) ? cid : 0;
|
|
||||||
var userId = User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value ?? "";
|
var userId = User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)?.Value ?? "";
|
||||||
await _usageLogger.LogAsync(companyId, userId, AppConstants.AiFeatures.ReceiptScan, inputLength: (int)receiptImage.Length);
|
await _usageLogger.LogAsync(companyId, userId, AppConstants.AiFeatures.ReceiptScan, inputLength: (int)receiptImage.Length);
|
||||||
return Json(result);
|
return Json(result);
|
||||||
@@ -1124,7 +1125,8 @@ public class BillsController : Controller
|
|||||||
// Load expense accounts if not supplied
|
// Load expense accounts if not supplied
|
||||||
if (!request.AvailableAccounts.Any())
|
if (!request.AvailableAccounts.Any())
|
||||||
{
|
{
|
||||||
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive);
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
|
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive);
|
||||||
request.AvailableAccounts = allAccounts
|
request.AvailableAccounts = allAccounts
|
||||||
.Where(a => a.AccountType == AccountType.Expense ||
|
.Where(a => a.AccountType == AccountType.Expense ||
|
||||||
a.AccountType == AccountType.CostOfGoods ||
|
a.AccountType == AccountType.CostOfGoods ||
|
||||||
|
|||||||
@@ -246,8 +246,9 @@ public class BudgetsController : Controller
|
|||||||
|
|
||||||
private async Task<List<Account>> GetBudgetableAccountsAsync()
|
private async Task<List<Account>> GetBudgetableAccountsAsync()
|
||||||
{
|
{
|
||||||
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(
|
var accounts = await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.IsActive && (a.AccountType == AccountType.Revenue || a.AccountType == AccountType.Expense));
|
a => a.CompanyId == companyId && a.IsActive && (a.AccountType == AccountType.Revenue || a.AccountType == AccountType.Expense));
|
||||||
return accounts.OrderBy(a => a.AccountNumber).ToList();
|
return accounts.OrderBy(a => a.AccountNumber).ToList();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -670,7 +670,8 @@ namespace PowderCoating.Web.Controllers
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
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);
|
||||||
|
|
||||||
var revenueAccounts = accounts
|
var revenueAccounts = accounts
|
||||||
.Where(a => a.AccountType == PowderCoating.Core.Enums.AccountType.Revenue)
|
.Where(a => a.AccountType == PowderCoating.Core.Enums.AccountType.Revenue)
|
||||||
|
|||||||
@@ -105,8 +105,9 @@ public class ExpensesController : Controller
|
|||||||
ViewBag.To = to?.ToString("yyyy-MM-dd");
|
ViewBag.To = to?.ToString("yyyy-MM-dd");
|
||||||
ViewBag.TotalAmount = dtos.Sum(e => e.Amount);
|
ViewBag.TotalAmount = dtos.Sum(e => e.Amount);
|
||||||
|
|
||||||
|
var legacyUser = await _userManager.GetUserAsync(User);
|
||||||
var expenseAccounts = (await _unitOfWork.Accounts.FindAsync(
|
var expenseAccounts = (await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.IsActive &&
|
a => a.CompanyId == legacyUser!.CompanyId && a.IsActive &&
|
||||||
(a.AccountType == AccountType.Expense || a.AccountType == AccountType.CostOfGoods)))
|
(a.AccountType == AccountType.Expense || a.AccountType == AccountType.CostOfGoods)))
|
||||||
.OrderBy(a => a.AccountNumber)
|
.OrderBy(a => a.AccountNumber)
|
||||||
.ToList();
|
.ToList();
|
||||||
@@ -479,7 +480,8 @@ public class ExpensesController : Controller
|
|||||||
|
|
||||||
if (!request.AvailableAccounts.Any())
|
if (!request.AvailableAccounts.Any())
|
||||||
{
|
{
|
||||||
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive);
|
var currentUser = await _userManager.GetUserAsync(User);
|
||||||
|
var allAccounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == currentUser!.CompanyId && a.IsActive);
|
||||||
request.AvailableAccounts = allAccounts
|
request.AvailableAccounts = allAccounts
|
||||||
.Where(a => a.AccountType == AccountType.Expense ||
|
.Where(a => a.AccountType == AccountType.Expense ||
|
||||||
a.AccountType == AccountType.CostOfGoods)
|
a.AccountType == AccountType.CostOfGoods)
|
||||||
|
|||||||
@@ -313,7 +313,8 @@ public class FixedAssetsController : Controller
|
|||||||
|
|
||||||
private async Task PopulateAccountsAsync()
|
private async Task PopulateAccountsAsync()
|
||||||
{
|
{
|
||||||
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);
|
||||||
var list = accounts.OrderBy(a => a.AccountNumber).ThenBy(a => a.Name).ToList();
|
var list = accounts.OrderBy(a => a.AccountNumber).ThenBy(a => a.Name).ToList();
|
||||||
|
|
||||||
ViewBag.AssetAccounts = list
|
ViewBag.AssetAccounts = list
|
||||||
|
|||||||
@@ -1640,7 +1640,7 @@ public class InventoryController : Controller
|
|||||||
new SelectListItem { Value = "rolls", Text = "Rolls" }
|
new SelectListItem { Value = "rolls", Text = "Rolls" }
|
||||||
};
|
};
|
||||||
|
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive);
|
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive);
|
||||||
|
|
||||||
ViewBag.InventoryAccounts = accounts
|
ViewBag.InventoryAccounts = accounts
|
||||||
.Where(a => a.AccountType == AccountType.Asset
|
.Where(a => a.AccountType == AccountType.Asset
|
||||||
|
|||||||
@@ -118,7 +118,7 @@ public class JournalEntriesController : Controller
|
|||||||
|
|
||||||
// Load account names for lines
|
// Load account names for lines
|
||||||
var accountIds = je.Lines.Select(l => l.AccountId).Distinct().ToList();
|
var accountIds = je.Lines.Select(l => l.AccountId).Distinct().ToList();
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(a => accountIds.Contains(a.Id));
|
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == je.CompanyId && accountIds.Contains(a.Id));
|
||||||
ViewBag.AccountMap = accounts.ToDictionary(a => a.Id, a => $"{a.AccountNumber} – {a.Name}");
|
ViewBag.AccountMap = accounts.ToDictionary(a => a.Id, a => $"{a.AccountNumber} – {a.Name}");
|
||||||
|
|
||||||
// Reversal metadata
|
// Reversal metadata
|
||||||
@@ -474,7 +474,8 @@ public class JournalEntriesController : Controller
|
|||||||
|
|
||||||
private async Task PopulateAccountDropdownAsync()
|
private async Task PopulateAccountDropdownAsync()
|
||||||
{
|
{
|
||||||
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);
|
||||||
ViewBag.AccountSelectList = accounts
|
ViewBag.AccountSelectList = accounts
|
||||||
.OrderBy(a => a.AccountNumber)
|
.OrderBy(a => a.AccountNumber)
|
||||||
.Select(a => new SelectListItem
|
.Select(a => new SelectListItem
|
||||||
|
|||||||
@@ -2558,7 +2558,7 @@ public class ReportsController : Controller
|
|||||||
|
|
||||||
// Load account metadata for budget lines
|
// Load account metadata for budget lines
|
||||||
var accountIds = budget.Lines.Select(l => l.AccountId).Distinct().ToList();
|
var accountIds = budget.Lines.Select(l => l.AccountId).Distinct().ToList();
|
||||||
var accounts = (await _unitOfWork.Accounts.FindAsync(a => accountIds.Contains(a.Id)))
|
var accounts = (await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == budget.CompanyId && accountIds.Contains(a.Id)))
|
||||||
.ToDictionary(a => a.Id);
|
.ToDictionary(a => a.Id);
|
||||||
|
|
||||||
var rows = new List<BudgetVsActualRow>();
|
var rows = new List<BudgetVsActualRow>();
|
||||||
|
|||||||
@@ -132,7 +132,7 @@ public class VendorCreditsController : Controller
|
|||||||
.Select(l => l.AccountId!.Value)
|
.Select(l => l.AccountId!.Value)
|
||||||
.Distinct()
|
.Distinct()
|
||||||
.ToList();
|
.ToList();
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(a => accountIds.Contains(a.Id));
|
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == vc.CompanyId && accountIds.Contains(a.Id));
|
||||||
ViewBag.AccountMap = accounts.ToDictionary(a => a.Id, a => $"{a.AccountNumber} – {a.Name}");
|
ViewBag.AccountMap = accounts.ToDictionary(a => a.Id, a => $"{a.AccountNumber} – {a.Name}");
|
||||||
|
|
||||||
// Load bills referenced by applications
|
// Load bills referenced by applications
|
||||||
@@ -357,8 +357,9 @@ public class VendorCreditsController : Controller
|
|||||||
|
|
||||||
private async Task PopulateDropdownsAsync()
|
private async Task PopulateDropdownsAsync()
|
||||||
{
|
{
|
||||||
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
var vendors = await _unitOfWork.Vendors.FindAsync(v => v.IsActive);
|
var vendors = await _unitOfWork.Vendors.FindAsync(v => v.IsActive);
|
||||||
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.IsActive);
|
var accounts = await _unitOfWork.Accounts.FindAsync(a => a.CompanyId == companyId && a.IsActive);
|
||||||
|
|
||||||
ViewBag.VendorList = vendors
|
ViewBag.VendorList = vendors
|
||||||
.OrderBy(v => v.CompanyName)
|
.OrderBy(v => v.CompanyName)
|
||||||
|
|||||||
@@ -463,8 +463,9 @@ public class VendorsController : Controller
|
|||||||
|
|
||||||
private async Task PopulateExpenseAccountsAsync()
|
private async Task PopulateExpenseAccountsAsync()
|
||||||
{
|
{
|
||||||
|
var companyId = _tenantContext.GetCurrentCompanyId() ?? 0;
|
||||||
var accounts = (await _unitOfWork.Accounts.FindAsync(
|
var accounts = (await _unitOfWork.Accounts.FindAsync(
|
||||||
a => a.IsActive && (a.AccountType == AccountType.Expense ||
|
a => a.CompanyId == companyId && a.IsActive && (a.AccountType == AccountType.Expense ||
|
||||||
a.AccountType == AccountType.CostOfGoods ||
|
a.AccountType == AccountType.CostOfGoods ||
|
||||||
a.AccountType == AccountType.Asset)))
|
a.AccountType == AccountType.Asset)))
|
||||||
.OrderBy(a => a.AccountNumber)
|
.OrderBy(a => a.AccountNumber)
|
||||||
|
|||||||
Reference in New Issue
Block a user