fix(security): restore authorization on asset and contact endpoints

AssetController and ContactController had [Authorize] commented out with
"Removed for testing", leaving every asset and contact endpoint reachable
without a token. Restore the attribute. Also drop AssetController's unused
UserManager dependency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

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<ApplicationUser> _userManager;
         private readonly IAssetService _assetService;
 
-        public AssetController(UserManager<ApplicationUser> userManager, IAssetService assetService)
+        public AssetController(IAssetService assetService)
         {
-            _userManager = userManager;
             _assetService = assetService;
         }
 

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")]

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<ApplicationUser>` via constructor
+- [x] `AssetController` injected `UserManager<ApplicationUser>` via constructor
-      but never uses it anywhere in the file — dead dependency, remove it.
+      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
+- [x] **`WorkOrderService.DeleteWorkOrderCascadeAsync`** loaded all comments
-      then calls `_commentDataService.DeleteAsync(comment.Id)` once per
+      then called `_commentDataService.DeleteAsync(comment.Id)` once per
-      comment. `CommentDataService.DeleteAsync` does its own `GetByIdAsync`
+      comment (2N round trips, N separate commits — broke the one-commit
-      (1 SELECT) + `SaveChangesAsync` (1 commit) **per call** — N comments =
+      convention, §4). **Fixed:** added `ICommentDataService.DeleteAllForWorkOrderAsync`
-      2N round trips and N separate commits, breaking the one-commit
+      (single `ExecuteDeleteAsync`), caller now calls it once.
-      convention (§4) as well as being slow. Fix: add a batch delete
+- [x] **`WorkOrderService.SaveAttachmentsAndPhotos`** looped over uploaded
-      (`DeleteAllForWorkOrderAsync` via `ExecuteDeleteAsync`, or load once +
-      `RemoveRange` + one `SaveChangesAsync`) and call it once.
-- [ ] **`WorkOrderService.SaveAttachmentsAndPhotos`** loops over uploaded
       attachments calling `_workOrderDataService.AddWorkOrderAttachmentAsync`
-      per file; that method does `AddAsync` + `SaveChangesAsync` **per call** —
+      per file (N separate commits). **Fixed:** added
-      N attachments = N separate commits. Fix: collect the `WorkOrderAttachments`
+      `AddWorkOrderAttachmentsAsync` (mirrors the existing `AddAuditLogsAsync`
-      entities in the loop (file storage save can stay per-file — that's I/O,
+      batch pattern already in this interface) — `AddRangeAsync` + one
-      not DB), then add a batch method that calls `AddRangeAsync` + one
+      `SaveChangesAsync`; file-storage save stays per-file (I/O, not DB).
-      `SaveChangesAsync`.
+      Also fixed the same class of issue in the before/after/signoff-photo
-- [ ] **`VendorCompanyRosterDataService`** (3 call sites: roster update,
+      block right below it: up to 3 separate `GetByIdAsync` + `UpdateAsync`
-      create, and a third) loads `existingTechnicians` once via a single
+      (each its own commit) when all three photos are submitted together,
-      query, then does `existingTechnicians.FirstOrDefault(e => e.Id == ...)`
+      now 1 `GetByIdAsync` + 1 `UpdateAsync`.
-      **inside** a loop over the incoming roster — O(n·m) in-memory scan.
+- [x] **`VendorCompanyRosterDataService.SaveRosterAsync`** (the roster-update
-      Fix: build `existingTechnicians.ToDictionary(e => e.Id)` once before the
+      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)