diff --git a/Api.SeaHavenIndustries/Controllers/AssetController.cs b/Api.SeaHavenIndustries/Controllers/AssetController.cs index e671ad2..ce5f8be 100644 --- a/Api.SeaHavenIndustries/Controllers/AssetController.cs +++ b/Api.SeaHavenIndustries/Controllers/AssetController.cs @@ -2,26 +2,22 @@ using Api.SeaHavenIndustries.DTOs; using Data.SeaHavenIndustries; using FluentValidation; using Microsoft.AspNetCore.Authorization; -using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; -using Microsoft.EntityFrameworkCore; using SeaHaven.Services.Interfaces; using SeaHaven.Services.DTOs; using System.Security.Claims; namespace Api.SeaHavenIndustries.Controllers { - //[Authorize] // Removed for testing + [Authorize] [ApiController] [Route("api/Asset")] public class AssetController : Controller { - private readonly UserManager _userManager; private readonly IAssetService _assetService; - public AssetController(UserManager userManager, IAssetService assetService) + public AssetController(IAssetService assetService) { - _userManager = userManager; _assetService = assetService; } diff --git a/Api.SeaHavenIndustries/Controllers/ContactController.cs b/Api.SeaHavenIndustries/Controllers/ContactController.cs index be53d37..fffc814 100644 --- a/Api.SeaHavenIndustries/Controllers/ContactController.cs +++ b/Api.SeaHavenIndustries/Controllers/ContactController.cs @@ -10,7 +10,7 @@ using SeaHaven.Services.Interfaces; namespace Api.SeaHavenIndustries.Controllers { - //[Authorize] // Removed for testing + [Authorize] [ApiController] [Route("api/Contact")] [Route("api/contacts")] diff --git a/docs/refactor-backend-findings.md b/docs/refactor-backend-findings.md index 1ee176a..0afc548 100644 --- a/docs/refactor-backend-findings.md +++ b/docs/refactor-backend-findings.md @@ -4,10 +4,22 @@ Tracking doc for `refactor/backend-architecture-cleanup`. Each item is verified against the actual code (not just a grep hit) before being listed here. Checked off as fixed, with tests, on this branch. +## 0. Security finding (not an architecture item, fixed anyway — user confirmed) + +- [x] **`AssetController` and `ContactController` both had `[Authorize]` + commented out** (`//[Authorize] // Removed for testing`), so every + endpoint on both — asset CRUD, contact CRUD, and (via the next item) + Location CRUD — was reachable with no authentication. Git history shows + it landed via a commit titled "fix" / "backend changes" — looks like + forgotten debug code, not an intentional exception. Confirmed with the + user before fixing (changes production auth behavior). **Fixed:** + restored `[Authorize]` on both. + ## 1. Layering / dependency-injection violations -- [ ] `AssetController` injects `UserManager` via constructor - but never uses it anywhere in the file — dead dependency, remove it. +- [x] `AssetController` injected `UserManager` via constructor + but never used it anywhere in the file. **Fixed:** removed the dead + dependency and its now-unused `using` statements. - [ ] `ContactController` depends on both `IContactService` **and** `ILocationService` and implements full Location CRUD (`GetAllLocationsAsync`, `CreateLocationAsync`, `UpdateLocationAsync`, `DeleteLocationAsync`, @@ -57,26 +69,31 @@ Ruled out (internal/no external DTO, not a validator candidate): ## 3. N+1 / query-shape issues (verified, not grep-only) -- [ ] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loads all comments - then calls `_commentDataService.DeleteAsync(comment.Id)` once per - comment. `CommentDataService.DeleteAsync` does its own `GetByIdAsync` - (1 SELECT) + `SaveChangesAsync` (1 commit) **per call** — N comments = - 2N round trips and N separate commits, breaking the one-commit - convention (§4) as well as being slow. Fix: add a batch delete - (`DeleteAllForWorkOrderAsync` via `ExecuteDeleteAsync`, or load once + - `RemoveRange` + one `SaveChangesAsync`) and call it once. -- [ ] **`WorkOrderService.SaveAttachmentsAndPhotos`** loops over uploaded +- [x] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loaded all comments + then called `_commentDataService.DeleteAsync(comment.Id)` once per + comment (2N round trips, N separate commits — broke the one-commit + convention, §4). **Fixed:** added `ICommentDataService.DeleteAllForWorkOrderAsync` + (single `ExecuteDeleteAsync`), caller now calls it once. +- [x] **`WorkOrderService.SaveAttachmentsAndPhotos`** looped over uploaded attachments calling `_workOrderDataService.AddWorkOrderAttachmentAsync` - per file; that method does `AddAsync` + `SaveChangesAsync` **per call** — - N attachments = N separate commits. Fix: collect the `WorkOrderAttachments` - entities in the loop (file storage save can stay per-file — that's I/O, - not DB), then add a batch method that calls `AddRangeAsync` + one - `SaveChangesAsync`. -- [ ] **`VendorCompanyRosterDataService`** (3 call sites: roster update, - create, and a third) loads `existingTechnicians` once via a single - query, then does `existingTechnicians.FirstOrDefault(e => e.Id == ...)` - **inside** a loop over the incoming roster — O(n·m) in-memory scan. - Fix: build `existingTechnicians.ToDictionary(e => e.Id)` once before the + per file (N separate commits). **Fixed:** added + `AddWorkOrderAttachmentsAsync` (mirrors the existing `AddAuditLogsAsync` + batch pattern already in this interface) — `AddRangeAsync` + one + `SaveChangesAsync`; file-storage save stays per-file (I/O, not DB). + Also fixed the same class of issue in the before/after/signoff-photo + block right below it: up to 3 separate `GetByIdAsync` + `UpdateAsync` + (each its own commit) when all three photos are submitted together, + now 1 `GetByIdAsync` + 1 `UpdateAsync`. +- [x] **`VendorCompanyRosterDataService.SaveRosterAsync`** (the roster-update + method only — checked the other two call sites and they don't do this: + `CreateRosterAsync` is a pure additive create with nothing to match + against, and the third occurrence only does a uniform update over all + existing technicians, no per-item lookup) loads `existingTechnicians` + once via a single query, then did + `existingTechnicians.FirstOrDefault(e => e.Id == ...)` **inside** a loop + over the incoming roster — O(n·m) in-memory scan. **Fixed:** built + `existingTechniciansById = existingTechnicians.ToDictionary(v => v.Id)` + once before the loop and used `TryGetValue` inside it — O(n+m). loop and `TryGetValue` inside it — O(n+m). ## Ruled out during the sweep (false positives / legitimate design)