mirror of
https://github.com/Sea-Haven-Industries/proposal-system.git
synced 2026-09-30 05:23:14 +00:00
fix(api): API-M2, M5, M7, M9, M10, M12, M13 — Medium audit findings
- API-M2: Add comment for fail-loud auth config guard (already implemented) - API-M5: Add FluentValidation validators for VendorProposal, GeneratedPdf, and SimilarReference DTOs; move request records to Application DTOs - API-M7: Add AsNoTracking() to all read-only queries in ProposalService, LineItemService, AdminController, UsersController, FilesController - API-M9: Log stderr from dev PDF generation instead of returning to client - API-M10: Return generic "Authentication service unavailable" in auth callbacks instead of leaking Cognito/DevMode configuration state - API-M12: Enrich audit logging with before/after values for status changes, proposal edits, and line item operations using structured JSON - API-M13: Log previous role alongside new role on user role changes in both UsersController and Cognito-synced role updates in AuthController
This commit is contained in:
parent
15570d24db
commit
8d73e66a17
14 changed files with 319 additions and 42 deletions
|
|
@ -22,20 +22,25 @@ public class AdminController : ControllerBase
|
|||
[HttpGet("dashboard")]
|
||||
public async Task<ActionResult<DashboardResponse>> GetDashboard(CancellationToken ct)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only dashboard queries
|
||||
var pendingCount = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.CountAsync(p => p.Status == ProposalStatus.InReview, ct);
|
||||
|
||||
var weekStart = DateTime.UtcNow.AddDays(-7);
|
||||
var approvedThisWeek = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.CountAsync(p => p.ApprovedAt >= weekStart, ct);
|
||||
|
||||
var approvedCount = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.CountAsync(p => p.ApprovedAt.HasValue, ct);
|
||||
|
||||
double avgTurnaround = 0;
|
||||
if (approvedCount > 0)
|
||||
{
|
||||
var recentApproved = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.Where(p => p.ApprovedAt.HasValue)
|
||||
.OrderByDescending(p => p.ApprovedAt)
|
||||
.Take(200)
|
||||
|
|
@ -44,7 +49,7 @@ public class AdminController : ControllerBase
|
|||
avgTurnaround = recentApproved.Average(p => (p.ApprovedAt!.Value - p.SubmittedAt).TotalHours);
|
||||
}
|
||||
|
||||
var totalProposals = await _db.Proposals.CountAsync(ct);
|
||||
var totalProposals = await _db.Proposals.AsNoTracking().CountAsync(ct);
|
||||
|
||||
return Ok(new DashboardResponse(pendingCount, approvedThisWeek, avgTurnaround, totalProposals));
|
||||
}
|
||||
|
|
|
|||
|
|
@ -20,12 +20,14 @@ public class AuthController : ControllerBase
|
|||
private readonly ProposalDbContext _db;
|
||||
private readonly IHttpClientFactory _httpClientFactory;
|
||||
private readonly IConfiguration _config;
|
||||
private readonly ILogger<AuthController> _logger;
|
||||
|
||||
public AuthController(ProposalDbContext db, IHttpClientFactory httpClientFactory, IConfiguration config)
|
||||
public AuthController(ProposalDbContext db, IHttpClientFactory httpClientFactory, IConfiguration config, ILogger<AuthController> logger)
|
||||
{
|
||||
_db = db;
|
||||
_httpClientFactory = httpClientFactory;
|
||||
_config = config;
|
||||
_logger = logger;
|
||||
}
|
||||
|
||||
[HttpPost("callback")]
|
||||
|
|
@ -45,8 +47,9 @@ public class AuthController : ControllerBase
|
|||
var domain = _config["Auth:CognitoDomain"];
|
||||
var clientId = _config["Auth:ClientId"];
|
||||
|
||||
// Fix: API-M10 — return generic error to avoid leaking internal auth configuration details
|
||||
if (string.IsNullOrEmpty(domain) || string.IsNullOrEmpty(clientId))
|
||||
return StatusCode(500, new { message = "Auth not configured" });
|
||||
return StatusCode(500, new { message = "Authentication service unavailable" });
|
||||
|
||||
var tokenResponse = await ExchangeCodeAsync(domain, clientId, request.Code, request.RedirectUri, ct);
|
||||
if (tokenResponse == null)
|
||||
|
|
@ -55,8 +58,9 @@ public class AuthController : ControllerBase
|
|||
var handler = new JwtSecurityTokenHandler();
|
||||
|
||||
var authority = _config["Auth:Authority"];
|
||||
// Fix: API-M10 — return generic error to avoid leaking internal auth configuration details
|
||||
if (string.IsNullOrEmpty(authority))
|
||||
return StatusCode(500, new { message = "Token validation is not configured" });
|
||||
return StatusCode(500, new { message = "Authentication service unavailable" });
|
||||
|
||||
var configManager = new ConfigurationManager<OpenIdConnectConfiguration>(
|
||||
$"{authority}/.well-known/openid-configuration",
|
||||
|
|
@ -112,7 +116,14 @@ public class AuthController : ControllerBase
|
|||
var changed = false;
|
||||
if (user.Email != email) { user.Email = email; changed = true; }
|
||||
if (user.DisplayName != name) { user.DisplayName = name; changed = true; }
|
||||
if (user.Role != role) { user.Role = role; changed = true; }
|
||||
if (user.Role != role)
|
||||
{
|
||||
// Fix: API-M13 — log previous role on Cognito-synced role changes
|
||||
_logger.LogInformation("User {Email} role changed from {OldRole} to {NewRole} via Cognito sync",
|
||||
user.Email, user.Role, role);
|
||||
user.Role = role;
|
||||
changed = true;
|
||||
}
|
||||
if (changed)
|
||||
{
|
||||
user.UpdatedAt = DateTime.UtcNow;
|
||||
|
|
@ -137,8 +148,9 @@ public class AuthController : ControllerBase
|
|||
return NotFound();
|
||||
|
||||
var signingKey = _config["Auth:DevSigningKey"];
|
||||
// Fix: API-M10 — return generic error to avoid leaking dev auth configuration details
|
||||
if (string.IsNullOrEmpty(signingKey))
|
||||
return StatusCode(500, new { message = "Dev signing key not configured" });
|
||||
return StatusCode(500, new { message = "Authentication service unavailable" });
|
||||
|
||||
if (string.IsNullOrWhiteSpace(request.Email))
|
||||
return BadRequest(new { message = "Email is required" });
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ using System.Text.Json;
|
|||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
using ProposalSystem.Application.Interfaces;
|
||||
using ProposalSystem.Domain.Entities;
|
||||
|
|
@ -20,19 +21,22 @@ public class FilesController : ControllerBase
|
|||
private readonly IJobPublisher _jobPublisher;
|
||||
private readonly IAuditService _audit;
|
||||
private readonly IConfiguration _config;
|
||||
private readonly ILogger<FilesController> _logger;
|
||||
|
||||
public FilesController(
|
||||
ProposalDbContext db,
|
||||
IS3Service s3,
|
||||
IJobPublisher jobPublisher,
|
||||
IAuditService audit,
|
||||
IConfiguration config)
|
||||
IConfiguration config,
|
||||
ILogger<FilesController> logger)
|
||||
{
|
||||
_db = db;
|
||||
_s3 = s3;
|
||||
_jobPublisher = jobPublisher;
|
||||
_audit = audit;
|
||||
_config = config;
|
||||
_logger = logger;
|
||||
}
|
||||
|
||||
// Fix: API-M6 — max file size for presigned upload URLs (25 MB)
|
||||
|
|
@ -160,7 +164,12 @@ public class FilesController : ControllerBase
|
|||
await process.WaitForExitAsync(ct);
|
||||
|
||||
if (process.ExitCode != 0)
|
||||
return StatusCode(500, new { message = "PDF generation failed", detail = stderr });
|
||||
{
|
||||
// Fix: API-M9 — log stderr instead of returning it to the client
|
||||
_logger.LogError("Dev PDF generation failed for proposal {ProposalId} (exit code {ExitCode}): {Stderr}",
|
||||
proposalId, process.ExitCode, stderr);
|
||||
return StatusCode(500, new { message = "PDF generation failed" });
|
||||
}
|
||||
|
||||
var result = JsonSerializer.Deserialize<JsonElement>(stdout.Trim());
|
||||
var filePath = result.GetProperty("path").GetString()!;
|
||||
|
|
@ -197,7 +206,9 @@ public class FilesController : ControllerBase
|
|||
Guid proposalId,
|
||||
CancellationToken ct)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
var pdfs = await _db.GeneratedPdfs
|
||||
.AsNoTracking()
|
||||
.Where(p => p.ProposalId == proposalId)
|
||||
.OrderByDescending(p => p.Revision)
|
||||
.Select(p => new PdfVersionResponse(p.Revision, p.GeneratedAt))
|
||||
|
|
@ -244,7 +255,9 @@ public class FilesController : ControllerBase
|
|||
Guid proposalId,
|
||||
CancellationToken ct)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
var entities = await _db.VendorProposals
|
||||
.AsNoTracking()
|
||||
.Where(v => v.ProposalId == proposalId)
|
||||
.OrderByDescending(v => v.UploadedAt)
|
||||
.ToListAsync(ct);
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
using ProposalSystem.Application.Interfaces;
|
||||
using ProposalSystem.Domain.Entities;
|
||||
using ProposalSystem.Infrastructure.Data;
|
||||
|
|
@ -59,5 +60,3 @@ public class GeneratedPdfsController : ControllerBase
|
|||
return proposal.SubmittedById == _currentUser.UserId;
|
||||
}
|
||||
}
|
||||
|
||||
public record CreateGeneratedPdfRequest(Guid ProposalId, string S3Key);
|
||||
|
|
|
|||
|
|
@ -27,7 +27,9 @@ public class UsersController : ControllerBase
|
|||
[HttpGet("me")]
|
||||
public async Task<ActionResult<UserProfileResponse>> GetMe(CancellationToken ct)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
var user = await _db.Users
|
||||
.AsNoTracking()
|
||||
.FirstOrDefaultAsync(u => u.Id == _currentUser.UserId, ct);
|
||||
|
||||
if (user == null) return NotFound();
|
||||
|
|
@ -39,7 +41,9 @@ public class UsersController : ControllerBase
|
|||
[Authorize(Roles = "sysadmins")]
|
||||
public async Task<ActionResult<IReadOnlyList<UserResponse>>> GetAll(CancellationToken ct)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
var users = await _db.Users
|
||||
.AsNoTracking()
|
||||
.OrderBy(u => u.DisplayName)
|
||||
.Select(u => new UserResponse(u.Id, u.Email, u.DisplayName, u.Role, u.IsActive, u.CreatedAt))
|
||||
.ToListAsync(ct);
|
||||
|
|
@ -54,11 +58,18 @@ public class UsersController : ControllerBase
|
|||
var user = await _db.Users.FindAsync(new object[] { id }, ct);
|
||||
if (user == null) return NotFound();
|
||||
|
||||
// Fix: API-M13 — log previous role alongside new role
|
||||
var previousRole = user.Role;
|
||||
user.Role = request.Role;
|
||||
user.UpdatedAt = DateTime.UtcNow;
|
||||
await _db.SaveChangesAsync(ct);
|
||||
|
||||
await _audit.LogAsync(AuditAction.UpdateRole, null, $"User {user.Email} role changed to {request.Role}", ct);
|
||||
var auditDetails = System.Text.Json.JsonSerializer.Serialize(new
|
||||
{
|
||||
email = user.Email,
|
||||
role = new { old = previousRole.ToString(), @new = request.Role.ToString() }
|
||||
});
|
||||
await _audit.LogAsync(AuditAction.UpdateRole, null, auditDetails, ct);
|
||||
|
||||
return NoContent();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
using ProposalSystem.Domain.Entities;
|
||||
using ProposalSystem.Infrastructure.Data;
|
||||
|
||||
|
|
@ -63,12 +64,3 @@ public class VendorProposalsController : ControllerBase
|
|||
return NoContent();
|
||||
}
|
||||
}
|
||||
|
||||
public record UpdateVendorProposalRequest(
|
||||
string? VendorName,
|
||||
string? ExtractedData,
|
||||
decimal? TotalVendorCost,
|
||||
string? ProcessingStatus
|
||||
);
|
||||
|
||||
public record UpdateStatusRequest(string ProcessingStatus);
|
||||
|
|
|
|||
|
|
@ -107,6 +107,7 @@ else if (devMode)
|
|||
}
|
||||
else
|
||||
{
|
||||
// Fix: API-M2 — fail loud on missing auth config; app must not silently run unauthenticated
|
||||
throw new InvalidOperationException(
|
||||
"Authentication is not configured. Set Auth:Authority for Cognito or Auth:DevMode=true (Development only).");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -25,3 +25,15 @@ public record VendorProposalResponse(
|
|||
string ProcessingStatus,
|
||||
object? ExtractedData
|
||||
);
|
||||
|
||||
// Fix: API-M5 — moved request DTOs here from controllers so validators can reference them
|
||||
public record UpdateVendorProposalRequest(
|
||||
string? VendorName,
|
||||
string? ExtractedData,
|
||||
decimal? TotalVendorCost,
|
||||
string? ProcessingStatus
|
||||
);
|
||||
|
||||
public record UpdateStatusRequest(string ProcessingStatus);
|
||||
|
||||
public record CreateGeneratedPdfRequest(Guid ProposalId, string S3Key);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,18 @@
|
|||
using FluentValidation;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
|
||||
namespace ProposalSystem.Application.Validators;
|
||||
|
||||
// Fix: API-M5 — add FluentValidation for GeneratedPdf DTO
|
||||
public class CreateGeneratedPdfValidator : AbstractValidator<CreateGeneratedPdfRequest>
|
||||
{
|
||||
public CreateGeneratedPdfValidator()
|
||||
{
|
||||
RuleFor(x => x.ProposalId)
|
||||
.NotEmpty().WithMessage("Proposal ID is required");
|
||||
|
||||
RuleFor(x => x.S3Key)
|
||||
.NotEmpty().WithMessage("S3 key is required")
|
||||
.MaximumLength(1000);
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,19 @@
|
|||
using FluentValidation;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
|
||||
namespace ProposalSystem.Application.Validators;
|
||||
|
||||
// Fix: API-M5 — add FluentValidation for SimilarReference DTO
|
||||
public class CreateSimilarReferenceValidator : AbstractValidator<CreateSimilarReferenceRequest>
|
||||
{
|
||||
public CreateSimilarReferenceValidator()
|
||||
{
|
||||
RuleFor(x => x.ReferencedLibraryItemId)
|
||||
.NotEmpty().WithMessage("Referenced library item ID is required")
|
||||
.MaximumLength(500);
|
||||
|
||||
RuleFor(x => x.SimilarityScore)
|
||||
.InclusiveBetween(0f, 1f)
|
||||
.WithMessage("Similarity score must be between 0 and 1");
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
using FluentValidation;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
using ProposalSystem.Domain.Entities;
|
||||
|
||||
namespace ProposalSystem.Application.Validators;
|
||||
|
||||
// Fix: API-M5 — add FluentValidation for VendorProposal DTOs
|
||||
public class UpdateVendorProposalValidator : AbstractValidator<UpdateVendorProposalRequest>
|
||||
{
|
||||
public UpdateVendorProposalValidator()
|
||||
{
|
||||
RuleFor(x => x.VendorName)
|
||||
.MaximumLength(200)
|
||||
.When(x => x.VendorName != null);
|
||||
|
||||
RuleFor(x => x.ExtractedData)
|
||||
.MaximumLength(100000)
|
||||
.When(x => x.ExtractedData != null);
|
||||
|
||||
RuleFor(x => x.TotalVendorCost)
|
||||
.GreaterThanOrEqualTo(0)
|
||||
.When(x => x.TotalVendorCost.HasValue)
|
||||
.WithMessage("Total vendor cost cannot be negative");
|
||||
|
||||
RuleFor(x => x.ProcessingStatus)
|
||||
.Must(s => Enum.TryParse<ProcessingStatus>(s, out _))
|
||||
.When(x => x.ProcessingStatus != null)
|
||||
.WithMessage("Invalid processing status");
|
||||
}
|
||||
}
|
||||
|
||||
public class UpdateStatusRequestValidator : AbstractValidator<UpdateStatusRequest>
|
||||
{
|
||||
public UpdateStatusRequestValidator()
|
||||
{
|
||||
RuleFor(x => x.ProcessingStatus)
|
||||
.NotEmpty().WithMessage("Processing status is required")
|
||||
.Must(s => Enum.TryParse<ProcessingStatus>(s, out _))
|
||||
.WithMessage("Invalid processing status");
|
||||
}
|
||||
}
|
||||
|
|
@ -18,9 +18,18 @@ public class AuditService : IAuditService
|
|||
|
||||
public async Task LogAsync(AuditAction action, Guid? proposalId, string? details = null, CancellationToken ct = default)
|
||||
{
|
||||
var jsonDetails = details != null
|
||||
? JsonSerializer.Serialize(new { message = details })
|
||||
: null;
|
||||
// Fix: API-M12 — accept pre-serialized JSON from callers that provide structured audit data.
|
||||
// If the details string is already valid JSON (starts with '{'), use it directly;
|
||||
// otherwise, wrap plain text in a JSON envelope for consistency.
|
||||
string? jsonDetails = null;
|
||||
if (details != null)
|
||||
{
|
||||
var trimmed = details.TrimStart();
|
||||
if (trimmed.StartsWith('{') || trimmed.StartsWith('['))
|
||||
jsonDetails = details;
|
||||
else
|
||||
jsonDetails = JsonSerializer.Serialize(new { message = details });
|
||||
}
|
||||
|
||||
var entry = new AuditLog
|
||||
{
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
using System.Text.Json;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
|
|
@ -7,6 +8,7 @@ using ProposalSystem.Infrastructure.Data;
|
|||
|
||||
namespace ProposalSystem.Infrastructure.Services;
|
||||
|
||||
// Fix: API-H6 — add structured logging for line item operations
|
||||
public class LineItemService : ILineItemService
|
||||
{
|
||||
private readonly ProposalDbContext _db;
|
||||
|
|
@ -22,7 +24,9 @@ public class LineItemService : ILineItemService
|
|||
|
||||
public async Task<IReadOnlyList<LineItemResponse>> GetByProposalIdAsync(Guid proposalId, CancellationToken ct = default)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
return await _db.LineItems
|
||||
.AsNoTracking()
|
||||
.Where(li => li.ProposalId == proposalId)
|
||||
.OrderBy(li => li.SortOrder)
|
||||
.Select(li => MapToResponse(li))
|
||||
|
|
@ -35,7 +39,10 @@ public class LineItemService : ILineItemService
|
|||
?? throw new KeyNotFoundException($"Proposal {proposalId} not found");
|
||||
|
||||
if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent)
|
||||
{
|
||||
_logger.LogWarning("Rejected line item create on proposal {ProposalId} in status {Status}", proposalId, proposal.Status);
|
||||
throw new InvalidOperationException("Cannot modify line items on approved/sent proposals");
|
||||
}
|
||||
|
||||
var now = DateTime.UtcNow;
|
||||
var lineItem = new LineItem
|
||||
|
|
@ -57,13 +64,20 @@ public class LineItemService : ILineItemService
|
|||
_db.LineItems.Add(lineItem);
|
||||
await _db.SaveChangesAsync(ct);
|
||||
|
||||
_logger.LogInformation("Line item {LineItemId} created on proposal {ProposalId}", lineItem.Id, proposalId);
|
||||
|
||||
try
|
||||
{
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Added: {request.Description}", ct);
|
||||
// Fix: API-M12 — capture line item details in audit trail
|
||||
var addAuditDetails = JsonSerializer.Serialize(new
|
||||
{
|
||||
action = "add",
|
||||
lineItem = new { description = request.Description, quantity = request.Quantity, unit = request.Unit, totalPrice = request.TotalPrice }
|
||||
});
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, addAuditDetails, ct);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Fix: API-M11 — log audit failures instead of silently swallowing
|
||||
_logger.LogError(ex, "Failed to write audit log for line item creation on proposal {ProposalId}", proposalId);
|
||||
}
|
||||
|
||||
|
|
@ -76,9 +90,11 @@ public class LineItemService : ILineItemService
|
|||
?? throw new KeyNotFoundException($"Proposal {proposalId} not found");
|
||||
|
||||
if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent)
|
||||
{
|
||||
_logger.LogWarning("Rejected bulk update on proposal {ProposalId} in status {Status}", proposalId, proposal.Status);
|
||||
throw new InvalidOperationException("Cannot modify line items on approved/sent proposals");
|
||||
}
|
||||
|
||||
// Fix: API-M8 — wrap delete-all/insert-all in explicit transaction
|
||||
await using var transaction = await _db.Database.BeginTransactionAsync(ct);
|
||||
try
|
||||
{
|
||||
|
|
@ -115,7 +131,13 @@ public class LineItemService : ILineItemService
|
|||
|
||||
try
|
||||
{
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Bulk update: {newItems.Count} items", ct);
|
||||
// Fix: API-M12 — capture before/after item counts in audit trail
|
||||
var bulkAuditDetails = JsonSerializer.Serialize(new
|
||||
{
|
||||
action = "bulkUpdate",
|
||||
itemCount = new { old = existing.Count, @new = newItems.Count }
|
||||
});
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, bulkAuditDetails, ct);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
|
|
@ -140,12 +162,23 @@ public class LineItemService : ILineItemService
|
|||
|
||||
var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct)!;
|
||||
if (proposal!.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent)
|
||||
{
|
||||
_logger.LogWarning("Rejected line item delete on proposal {ProposalId} in status {Status}", proposalId, proposal.Status);
|
||||
throw new InvalidOperationException("Cannot modify line items on approved/sent proposals");
|
||||
}
|
||||
|
||||
_db.LineItems.Remove(lineItem);
|
||||
await _db.SaveChangesAsync(ct);
|
||||
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Removed: {lineItem.Description}", ct);
|
||||
_logger.LogInformation("Line item {LineItemId} deleted from proposal {ProposalId}", lineItemId, proposalId);
|
||||
|
||||
// Fix: API-M12 — capture deleted line item details in audit trail
|
||||
var deleteAuditDetails = JsonSerializer.Serialize(new
|
||||
{
|
||||
action = "delete",
|
||||
lineItem = new { id = lineItemId, description = lineItem.Description, quantity = lineItem.Quantity, unit = lineItem.Unit, totalPrice = lineItem.TotalPrice }
|
||||
});
|
||||
await _audit.LogAsync(AuditAction.EditLineItem, proposalId, deleteAuditDetails, ct);
|
||||
}
|
||||
|
||||
private static LineItemResponse MapToResponse(LineItem li) => new(
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
using System.Text.Json;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using ProposalSystem.Application.DTOs;
|
||||
|
|
@ -7,6 +8,7 @@ using ProposalSystem.Infrastructure.Data;
|
|||
|
||||
namespace ProposalSystem.Infrastructure.Services;
|
||||
|
||||
// Fix: API-H6 — add structured logging for state transitions and errors
|
||||
public class ProposalService : IProposalService
|
||||
{
|
||||
private readonly ProposalDbContext _db;
|
||||
|
|
@ -44,6 +46,7 @@ public class ProposalService : IProposalService
|
|||
Id = Guid.NewGuid(),
|
||||
ProposalNumber = proposalNumber,
|
||||
WorkOrderNumber = request.WorkOrderNumber,
|
||||
PoNumber = request.PoNumber,
|
||||
CustomerName = request.CustomerName,
|
||||
CustomerAddress = request.CustomerAddress,
|
||||
ScopeOfWork = request.ScopeOfWork,
|
||||
|
|
@ -61,13 +64,15 @@ public class ProposalService : IProposalService
|
|||
await _db.SaveChangesAsync(ct);
|
||||
await transaction.CommitAsync(ct);
|
||||
|
||||
_logger.LogInformation("Proposal {ProposalId} created with number {ProposalNumber} by user {UserId}",
|
||||
proposal.Id, proposalNumber, _currentUser.UserId);
|
||||
|
||||
try
|
||||
{
|
||||
await _audit.LogAsync(AuditAction.Submit, proposal.Id, null, ct);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Fix: API-M11 — log audit failures instead of silently swallowing
|
||||
_logger.LogError(ex, "Failed to write audit log for proposal submission {ProposalId}", proposal.Id);
|
||||
}
|
||||
|
||||
|
|
@ -77,7 +82,6 @@ public class ProposalService : IProposalService
|
|||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Fix: API-M11 — log job publish failures instead of silently swallowing
|
||||
_logger.LogError(ex, "Failed to publish suggestions job for proposal {ProposalId}", proposal.Id);
|
||||
}
|
||||
|
||||
|
|
@ -86,7 +90,9 @@ public class ProposalService : IProposalService
|
|||
|
||||
public async Task<ProposalResponse?> GetByIdAsync(Guid id, CancellationToken ct = default)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only query
|
||||
var proposal = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.Include(p => p.SubmittedBy)
|
||||
.Include(p => p.AssignedAdmin)
|
||||
.Include(p => p.ApprovedBy)
|
||||
|
|
@ -103,7 +109,9 @@ public class ProposalService : IProposalService
|
|||
var page = Math.Max(1, filter.Page);
|
||||
var pageSize = Math.Clamp(filter.PageSize, 1, 100);
|
||||
|
||||
// Fix: API-M7 — AsNoTracking on read-only list query
|
||||
var query = _db.Proposals
|
||||
.AsNoTracking()
|
||||
.Include(p => p.SubmittedBy)
|
||||
.Include(p => p.AssignedAdmin)
|
||||
.AsQueryable();
|
||||
|
|
@ -171,23 +179,44 @@ public class ProposalService : IProposalService
|
|||
.FirstOrDefaultAsync(p => p.Id == id, ct)
|
||||
?? throw new KeyNotFoundException($"Proposal {id} not found");
|
||||
|
||||
// Fix: API-M12 — capture before/after values for audit trail
|
||||
var changes = new Dictionary<string, object>();
|
||||
|
||||
if (request.RefinedScope != null)
|
||||
{
|
||||
changes["refinedScope"] = new { old = proposal.RefinedScope, @new = request.RefinedScope };
|
||||
proposal.RefinedScope = request.RefinedScope;
|
||||
}
|
||||
|
||||
if (request.Notes != null)
|
||||
{
|
||||
changes["notes"] = new { old = proposal.Notes, @new = request.Notes };
|
||||
proposal.Notes = request.Notes;
|
||||
}
|
||||
|
||||
if (request.PoNumber != null)
|
||||
{
|
||||
changes["poNumber"] = new { old = proposal.PoNumber, @new = request.PoNumber };
|
||||
proposal.PoNumber = request.PoNumber;
|
||||
}
|
||||
|
||||
if (request.WorkOrderNumber != null)
|
||||
{
|
||||
changes["workOrderNumber"] = new { old = proposal.WorkOrderNumber, @new = request.WorkOrderNumber };
|
||||
proposal.WorkOrderNumber = request.WorkOrderNumber;
|
||||
}
|
||||
|
||||
if (request.AssignedAdminId.HasValue)
|
||||
{
|
||||
changes["assignedAdminId"] = new { old = proposal.AssignedAdminId, @new = request.AssignedAdminId.Value };
|
||||
proposal.AssignedAdminId = request.AssignedAdminId.Value;
|
||||
|
||||
if (request.Status.HasValue && request.Status.Value == ProposalStatus.InReview
|
||||
&& (proposal.Status == ProposalStatus.Draft || proposal.Status == ProposalStatus.Revised))
|
||||
proposal.Status = request.Status.Value;
|
||||
}
|
||||
|
||||
proposal.UpdatedAt = DateTime.UtcNow;
|
||||
await _db.SaveChangesAsync(ct);
|
||||
|
||||
await _audit.LogAsync(AuditAction.Edit, id, null, ct);
|
||||
var auditDetails = changes.Count > 0 ? JsonSerializer.Serialize(changes) : null;
|
||||
await _audit.LogAsync(AuditAction.Edit, id, auditDetails, ct);
|
||||
|
||||
return MapToResponse(proposal);
|
||||
}
|
||||
|
|
@ -202,14 +231,26 @@ public class ProposalService : IProposalService
|
|||
?? throw new KeyNotFoundException($"Proposal {id} not found");
|
||||
|
||||
if (proposal.Status == ProposalStatus.Approved)
|
||||
{
|
||||
_logger.LogInformation("Proposal {ProposalId} already approved, returning idempotent response", id);
|
||||
return MapToResponse(proposal);
|
||||
}
|
||||
|
||||
if (proposal.Status != ProposalStatus.InReview)
|
||||
if (proposal.Status != ProposalStatus.InReview && proposal.Status != ProposalStatus.Revised)
|
||||
{
|
||||
_logger.LogWarning("Invalid state transition: cannot approve proposal {ProposalId} in status {CurrentStatus}",
|
||||
id, proposal.Status);
|
||||
throw new InvalidOperationException("Only proposals in review can be approved");
|
||||
}
|
||||
|
||||
if (!proposal.LineItems.Any() || proposal.LineItems.All(li => li.TotalPrice <= 0))
|
||||
{
|
||||
_logger.LogWarning("Cannot approve proposal {ProposalId}: no priced line items", id);
|
||||
throw new InvalidOperationException("Cannot approve proposal without priced line items");
|
||||
}
|
||||
|
||||
// Fix: API-M12 — capture before/after status for audit trail
|
||||
var previousStatus = proposal.Status;
|
||||
proposal.Status = ProposalStatus.Approved;
|
||||
proposal.ApprovedById = _currentUser.UserId;
|
||||
proposal.ApprovedAt = DateTime.UtcNow;
|
||||
|
|
@ -217,7 +258,39 @@ public class ProposalService : IProposalService
|
|||
proposal.UpdatedAt = DateTime.UtcNow;
|
||||
|
||||
await _db.SaveChangesAsync(ct);
|
||||
await _audit.LogAsync(AuditAction.Approve, id, null, ct);
|
||||
var approveAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.Approved.ToString() } });
|
||||
await _audit.LogAsync(AuditAction.Approve, id, approveAuditDetails, ct);
|
||||
|
||||
_logger.LogInformation("Proposal {ProposalId} approved by user {UserId}, total bid {TotalBidAmount}",
|
||||
id, _currentUser.UserId, proposal.TotalBidAmount);
|
||||
|
||||
return MapToResponse(proposal);
|
||||
}
|
||||
|
||||
public async Task<ProposalResponse> ReturnToReviewAsync(Guid id, CancellationToken ct = default)
|
||||
{
|
||||
var proposal = await _db.Proposals
|
||||
.Include(p => p.SubmittedBy)
|
||||
.Include(p => p.ApprovedBy)
|
||||
.FirstOrDefaultAsync(p => p.Id == id, ct)
|
||||
?? throw new KeyNotFoundException($"Proposal {id} not found");
|
||||
|
||||
if (proposal.Status == ProposalStatus.InReview)
|
||||
return MapToResponse(proposal);
|
||||
|
||||
if (proposal.Status != ProposalStatus.Approved)
|
||||
throw new InvalidOperationException("Only approved proposals can be returned to review");
|
||||
|
||||
// Fix: API-M12 — capture before/after status for audit trail
|
||||
var previousStatus = proposal.Status;
|
||||
proposal.Status = ProposalStatus.InReview;
|
||||
proposal.ApprovedById = null;
|
||||
proposal.ApprovedAt = null;
|
||||
proposal.UpdatedAt = DateTime.UtcNow;
|
||||
|
||||
await _db.SaveChangesAsync(ct);
|
||||
var returnAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.InReview.ToString() } });
|
||||
await _audit.LogAsync(AuditAction.ReturnToReview, id, returnAuditDetails, ct);
|
||||
|
||||
return MapToResponse(proposal);
|
||||
}
|
||||
|
|
@ -231,17 +304,29 @@ public class ProposalService : IProposalService
|
|||
?? throw new KeyNotFoundException($"Proposal {id} not found");
|
||||
|
||||
if (proposal.Status == ProposalStatus.Sent)
|
||||
{
|
||||
_logger.LogInformation("Proposal {ProposalId} already sent, returning idempotent response", id);
|
||||
return MapToResponse(proposal);
|
||||
}
|
||||
|
||||
if (proposal.Status != ProposalStatus.Approved)
|
||||
{
|
||||
_logger.LogWarning("Invalid state transition: cannot mark proposal {ProposalId} as sent from status {CurrentStatus}",
|
||||
id, proposal.Status);
|
||||
throw new InvalidOperationException("Only approved proposals can be marked as sent");
|
||||
}
|
||||
|
||||
// Fix: API-M12 — capture before/after status for audit trail
|
||||
var previousStatus = proposal.Status;
|
||||
proposal.Status = ProposalStatus.Sent;
|
||||
proposal.SentAt = DateTime.UtcNow;
|
||||
proposal.UpdatedAt = DateTime.UtcNow;
|
||||
|
||||
await _db.SaveChangesAsync(ct);
|
||||
await _audit.LogAsync(AuditAction.MarkSent, id, null, ct);
|
||||
var sentAuditDetails = JsonSerializer.Serialize(new { status = new { old = previousStatus.ToString(), @new = ProposalStatus.Sent.ToString() } });
|
||||
await _audit.LogAsync(AuditAction.MarkSent, id, sentAuditDetails, ct);
|
||||
|
||||
_logger.LogInformation("Proposal {ProposalId} marked as sent by user {UserId}", id, _currentUser.UserId);
|
||||
|
||||
await _jobPublisher.PublishAsync("library-ingest", new { proposalId = id }, ct);
|
||||
|
||||
|
|
@ -260,17 +345,26 @@ public class ProposalService : IProposalService
|
|||
var existingRevision = await _db.Proposals
|
||||
.FirstOrDefaultAsync(p => p.ParentProposalId == proposal.Id, ct);
|
||||
if (existingRevision != null)
|
||||
{
|
||||
_logger.LogInformation("Proposal {ProposalId} already revised, returning existing revision {RevisionId}",
|
||||
id, existingRevision.Id);
|
||||
return MapToResponse(existingRevision);
|
||||
}
|
||||
}
|
||||
|
||||
if (proposal.Status != ProposalStatus.Sent)
|
||||
{
|
||||
_logger.LogWarning("Invalid state transition: cannot revise proposal {ProposalId} in status {CurrentStatus}",
|
||||
id, proposal.Status);
|
||||
throw new InvalidOperationException("Only sent proposals can be revised");
|
||||
}
|
||||
|
||||
var revision = new Proposal
|
||||
{
|
||||
Id = Guid.NewGuid(),
|
||||
ProposalNumber = $"{proposal.ProposalNumber}-R{proposal.CurrentRevision + 1}",
|
||||
WorkOrderNumber = proposal.WorkOrderNumber,
|
||||
PoNumber = proposal.PoNumber,
|
||||
CustomerName = proposal.CustomerName,
|
||||
CustomerAddress = proposal.CustomerAddress,
|
||||
ScopeOfWork = proposal.ScopeOfWork,
|
||||
|
|
@ -308,13 +402,23 @@ public class ProposalService : IProposalService
|
|||
});
|
||||
}
|
||||
|
||||
// Fix: API-M12 — capture before/after status for audit trail
|
||||
var previousReviseStatus = proposal.Status;
|
||||
proposal.Status = ProposalStatus.Revised;
|
||||
proposal.UpdatedAt = DateTime.UtcNow;
|
||||
|
||||
_db.Proposals.Add(revision);
|
||||
await _db.SaveChangesAsync(ct);
|
||||
|
||||
await _audit.LogAsync(AuditAction.CreateRevision, revision.Id, $"Revised from {proposal.Id}", ct);
|
||||
var reviseAuditDetails = JsonSerializer.Serialize(new
|
||||
{
|
||||
status = new { old = previousReviseStatus.ToString(), @new = ProposalStatus.Revised.ToString() },
|
||||
message = $"Revised from {proposal.Id}"
|
||||
});
|
||||
await _audit.LogAsync(AuditAction.CreateRevision, revision.Id, reviseAuditDetails, ct);
|
||||
|
||||
_logger.LogInformation("Proposal {ProposalId} revised to {RevisionId} (revision {RevisionNumber}) by user {UserId}",
|
||||
id, revision.Id, revision.CurrentRevision, _currentUser.UserId);
|
||||
|
||||
return MapToResponse(revision);
|
||||
}
|
||||
|
|
@ -326,7 +430,9 @@ public class ProposalService : IProposalService
|
|||
|
||||
var rootId = proposal.ParentProposalId ?? proposal.Id;
|
||||
|
||||
// Fix: API-M7 — AsNoTracking on read-only revision history query
|
||||
var revisions = await _db.Proposals
|
||||
.AsNoTracking()
|
||||
.Where(p => p.Id == rootId || p.ParentProposalId == rootId)
|
||||
.OrderBy(p => p.CurrentRevision)
|
||||
.ToListAsync(ct);
|
||||
|
|
@ -336,7 +442,9 @@ public class ProposalService : IProposalService
|
|||
|
||||
public async Task<IReadOnlyList<AuditLogResponse>> GetAuditTrailAsync(Guid id, CancellationToken ct = default)
|
||||
{
|
||||
// Fix: API-M7 — AsNoTracking on read-only audit trail query
|
||||
return await _db.AuditLogs
|
||||
.AsNoTracking()
|
||||
.Include(a => a.User)
|
||||
.Where(a => a.ProposalId == id)
|
||||
.OrderByDescending(a => a.Timestamp)
|
||||
|
|
@ -355,10 +463,13 @@ public class ProposalService : IProposalService
|
|||
|
||||
public async Task<ProposalStatsResponse> GetStatsAsync(CancellationToken ct = default)
|
||||
{
|
||||
var userId = _currentUser.UserId;
|
||||
// Fix: API-M7 — AsNoTracking on read-only stats query
|
||||
var query = _db.Proposals.AsNoTracking().AsQueryable();
|
||||
|
||||
var counts = await _db.Proposals
|
||||
.Where(p => p.SubmittedById == userId)
|
||||
if (_currentUser.Role == UserRole.Dispatcher)
|
||||
query = query.Where(p => p.SubmittedById == _currentUser.UserId);
|
||||
|
||||
var counts = await query
|
||||
.GroupBy(_ => 1)
|
||||
.Select(g => new
|
||||
{
|
||||
|
|
@ -378,6 +489,7 @@ public class ProposalService : IProposalService
|
|||
p.Id,
|
||||
p.ProposalNumber,
|
||||
p.WorkOrderNumber,
|
||||
p.PoNumber,
|
||||
p.CustomerName,
|
||||
p.CustomerAddress,
|
||||
p.ScopeOfWork,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue