Merge pull request #196 from Sea-Haven-Industries/fix/ab/sh-406-no-revoke-auto-approved

fix(uplifts): refuse admin revoke of an auto-approved uplift

Api.SeaHavenIndustries.Tests/UpliftRevokeEndpointRulesTests.cs

@@ -0,0 +1,276 @@
+using System.Security.Claims;
+using Api.SeaHavenIndustries.Controllers;
+using Data.SeaHavenIndustries;
+using Data.SeaHavenIndustries.Enums;
+using FluentAssertions;
+using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging;
+using Moq;
+using SeaHaven.DataServices.Implementation;
+using SeaHaven.Services.Configuration;
+using SeaHaven.Services.DTOs;
+using SeaHaven.Services.Helpers;
+using SeaHaven.Services.Implementation;
+using SeaHaven.Services.Interfaces;
+using Xunit;
+
+namespace Api.SeaHavenIndustries.Tests;
+
+/// <summary>
+/// Revoke rules exercised end to end through both revoke endpoints (the Uplift
+/// Approvals route and the work-order route) against the real services, so a
+/// refusal is observed as the HTTP result and the unchanged stored state.
+/// </summary>
+public sealed class UpliftRevokeEndpointRulesTests
+{
+    private const int WorkOrderId = 1;
+    private const int DispatchId = 10;
+    private const int AutoApprovedId = 100;
+    private const int AdminApprovedId = 101;
+
+    public enum RevokeRoute
+    {
+        UpliftApprovals,
+        WorkOrder,
+    }
+
+    private static ApplicationDbContext CreateContext()
+    {
+        var options = new DbContextOptionsBuilder<ApplicationDbContext>()
+            .UseInMemoryDatabase(Guid.NewGuid().ToString())
+            .Options;
+        return new ApplicationDbContext(options);
+    }
+
+    private static ClaimsPrincipal OrgWideUser(string userId, string role) =>
+        new(new ClaimsIdentity(
+            new[]
+            {
+                new Claim(ClaimTypes.NameIdentifier, userId),
+                new Claim(ClaimTypes.Role, role),
+                new Claim(SeaHavenClaimTypes.OrgScope, SeaHavenClaimTypes.OrgScopeAll),
+            },
+            "test"));
+
+    private static async Task SeedAsync(ApplicationDbContext context)
+    {
+        context.Accounts.Add(new Accounts { Id = 1, Name = "Acme Corp", IsDeleted = false });
+        context.Users.Add(new ApplicationUser { Id = "admin-1", UserName = "admin-1", FirstName = "Ada", LastName = "Admin" });
+        context.Users.Add(new ApplicationUser { Id = "dispatcher-1", UserName = "dispatcher-1", FirstName = "Dee", LastName = "Dispatcher" });
+        context.Vendors.Add(new Vendor { Id = 1, CompanyName = "Acme HVAC" });
+        context.Dispatches.Add(new Dispatch
+        {
+            Id = DispatchId,
+            VendorId = 1,
+            WorkOrderId = WorkOrderId,
+            NTEAmount = 2200m,
+            DispatchNumber = "DIS-10",
+            Status = "Scheduled",
+        });
+        context.workOrders.Add(new WorkOrder
+        {
+            Id = WorkOrderId,
+            InternalWONumber = "10000000001",
+            PrimaryDispatchId = DispatchId,
+            AccountId = 1,
+            WorkOrderType = WorkOrderType.PM,
+            LifecycleStatus = LifecycleStatus.Scheduled,
+        });
+        // The admin filed this one themselves and it auto-approved within the allowance.
+        context.DispatchUpliftRequests.Add(new DispatchUpliftRequest
+        {
+            Id = AutoApprovedId,
+            DispatchId = DispatchId,
+            CurrentNTE = 1000m,
+            RequestedNTE = 400m,
+            Status = UpliftStatus.NoApprovalRequired,
+            RequiredTier = 0,
+            NotificationStatus = "Sent",
+            createdby = "admin-1",
+            CreatedDate = DateTime.UtcNow.AddHours(-2),
+        });
+        context.DispatchUpliftRequests.Add(new DispatchUpliftRequest
+        {
+            Id = AdminApprovedId,
+            DispatchId = DispatchId,
+            CurrentNTE = 1400m,
+            RequestedNTE = 800m,
+            Status = UpliftStatus.Approved,
+            RequiredTier = 1,
+            NotificationStatus = "Sent",
+            createdby = "dispatcher-1",
+            CreatedDate = DateTime.UtcNow.AddHours(-1),
+            DecidedAt = DateTime.UtcNow.AddMinutes(-30),
+            DecidedByUserId = "admin-1",
+        });
+        await context.SaveChangesAsync();
+    }
+
+    private static WorkOrderUpliftService NewWorkOrderUpliftService(ApplicationDbContext context) =>
+        new(
+            new UpliftDataService(context),
+            new DispatchDataService(context),
+            new WorkOrderDetailDataService(context),
+            new WorkOrderAccountResolver(new AccountDataService(context), new LocationDataService(context)),
+            new UserDataService(context),
+            new TeamPermissionOverrideDataService(context),
+            new TeamPermissionPolicy(),
+            TimeProvider.System,
+            Microsoft.Extensions.Options.Options.Create(new ApprovalsOptions()));
+
+    private static async Task<IActionResult> RevokeAsync(
+        ApplicationDbContext context,
+        RevokeRoute route,
+        ClaimsPrincipal user,
+        int upliftId,
+        string? reason)
+    {
+        var workOrderFlow = NewWorkOrderUpliftService(context);
+        var httpContext = new DefaultHttpContext { User = user };
+
+        if (route == RevokeRoute.WorkOrder)
+        {
+            var controller = new WorkOrderDetailController(
+                Mock.Of<IWorkOrderDetailService>(),
+                Mock.Of<IWorkOrderCommentService>(),
+                workOrderFlow,
+                Mock.Of<ILogger<WorkOrderDetailController>>())
+            {
+                ControllerContext = new ControllerContext { HttpContext = httpContext },
+            };
+            return await controller.RevokeUplift(
+                WorkOrderId,
+                upliftId,
+                new RevokeWorkOrderUpliftRequestDto { Reason = reason },
+                CancellationToken.None);
+        }
+
+        var upliftService = new UpliftService(
+            new UpliftDataService(context),
+            new DispatchDataService(context),
+            Mock.Of<IVendorDocumentStoragePort>(),
+            TimeProvider.System,
+            Microsoft.Extensions.Options.Options.Create(new ApprovalsOptions()),
+            workOrderFlow);
+        var approvals = new UpliftController(upliftService, Mock.Of<ILogger<UpliftController>>())
+        {
+            ControllerContext = new ControllerContext { HttpContext = httpContext },
+        };
+        return await approvals.Revoke(
+            upliftId,
+            new UpliftController.DecisionRequest { Note = reason },
+            CancellationToken.None);
+    }
+
+    private static async Task AssertUnchangedAsync(ApplicationDbContext context, int upliftId, string status)
+    {
+        var stored = await context.DispatchUpliftRequests.AsNoTracking().SingleAsync(u => u.Id == upliftId);
+        stored.Status.Should().Be(status);
+        stored.DecisionNote.Should().BeNull();
+        (await context.Dispatches.AsNoTracking().SingleAsync(d => d.Id == DispatchId)).NTEAmount.Should().Be(2200m);
+        (await context.WorkOrderAuditLogs.AsNoTracking().AnyAsync(a => a.Action == "uplift_revoke")).Should().BeFalse();
+    }
+
+    [Fact]
+    public async Task WorkOrderRoute_AdminRevokingAutoApprovedUpliftTheyRequested_IsForbiddenAndChangesNothing()
+    {
+        await using var context = CreateContext();
+        await SeedAsync(context);
+
+        var result = await RevokeAsync(
+            context,
+            RevokeRoute.WorkOrder,
+            OrgWideUser("admin-1", "Admin"),
+            AutoApprovedId,
+            "Wrong quote attached");
+
+        var refused = result.Should().BeOfType<ObjectResult>().Subject;
+        refused.StatusCode.Should().Be(StatusCodes.Status403Forbidden);
+        refused.Value.Should().BeOfType<Response>().Which.Message
+            .Should().StartWith("You are not authorized to perform this action");
+        await AssertUnchangedAsync(context, AutoApprovedId, UpliftStatus.NoApprovalRequired);
+    }
+
+    [Fact]
+    public async Task ApprovalsRoute_AdminRevokingAutoApprovedUplift_IsRefusedAndChangesNothing()
+    {
+        await using var context = CreateContext();
+        await SeedAsync(context);
+
+        var result = await RevokeAsync(
+            context,
+            RevokeRoute.UpliftApprovals,
+            OrgWideUser("admin-1", "Admin"),
+            AutoApprovedId,
+            "Wrong quote attached");
+
+        var refused = result.Should().BeOfType<BadRequestObjectResult>().Subject;
+        refused.Value.Should().BeOfType<Response>().Which.Message
+            .Should().StartWith("This uplift request cannot be revoked");
+        await AssertUnchangedAsync(context, AutoApprovedId, UpliftStatus.NoApprovalRequired);
+    }
+
+    [Theory]
+    [InlineData(RevokeRoute.UpliftApprovals)]
+    [InlineData(RevokeRoute.WorkOrder)]
+    public async Task AdminRevokingAdminApprovedUpliftWithReason_Succeeds(RevokeRoute route)
+    {
+        await using var context = CreateContext();
+        await SeedAsync(context);
+
+        var result = await RevokeAsync(
+            context,
+            route,
+            OrgWideUser("admin-1", "Admin"),
+            AdminApprovedId,
+            "  Approved against the wrong quote  ");
+
+        result.Should().BeOfType<OkObjectResult>();
+        var stored = await context.DispatchUpliftRequests.AsNoTracking().SingleAsync(u => u.Id == AdminApprovedId);
+        stored.Status.Should().Be(UpliftStatus.Revoked);
+        stored.DecisionNote.Should().Be("Approved against the wrong quote");
+        (await context.Dispatches.AsNoTracking().SingleAsync(d => d.Id == DispatchId)).NTEAmount.Should().Be(1400m);
+        var audit = await context.WorkOrderAuditLogs.AsNoTracking().SingleAsync(a => a.Action == "uplift_revoke");
+        audit.OldValue.Should().Be(UpliftStatus.Approved);
+        audit.NewValue.Should().Be(UpliftStatus.Revoked);
+    }
+
+    [Theory]
+    [InlineData(RevokeRoute.UpliftApprovals)]
+    [InlineData(RevokeRoute.WorkOrder)]
+    public async Task AdminRevokingAdminApprovedUpliftWithoutReason_IsRefused(RevokeRoute route)
+    {
+        await using var context = CreateContext();
+        await SeedAsync(context);
+
+        var result = await RevokeAsync(context, route, OrgWideUser("admin-1", "Admin"), AdminApprovedId, "   ");
+
+        result.Should().BeOfType<BadRequestObjectResult>();
+        await AssertUnchangedAsync(context, AdminApprovedId, UpliftStatus.Approved);
+    }
+
+    [Fact]
+    public async Task WorkOrderRoute_DispatcherRevokingOwnAutoApprovedUpliftWithoutReason_Succeeds()
+    {
+        await using var context = CreateContext();
+        await SeedAsync(context);
+        var own = await context.DispatchUpliftRequests.SingleAsync(u => u.Id == AutoApprovedId);
+        own.createdby = "dispatcher-1";
+        await context.SaveChangesAsync();
+
+        var result = await RevokeAsync(
+            context,
+            RevokeRoute.WorkOrder,
+            OrgWideUser("dispatcher-1", "Dispatcher"),
+            AutoApprovedId,
+            null);
+
+        result.Should().BeOfType<OkObjectResult>();
+        var stored = await context.DispatchUpliftRequests.AsNoTracking().SingleAsync(u => u.Id == AutoApprovedId);
+        stored.Status.Should().Be(UpliftStatus.Revoked);
+        stored.DecisionNote.Should().BeNull();
+        (await context.Dispatches.AsNoTracking().SingleAsync(d => d.Id == DispatchId)).NTEAmount.Should().Be(1000m);
+    }
+}

SeaHaven.Services/Implementation/WorkOrderUpliftService.cs

@@ -273,6 +273,11 @@ namespace SeaHaven.Services.Implementation
 
             if (canonical == UpliftStatus.NoApprovalRequired)
             {
+                // An admin revoke overturns a human decision, and an auto-approval has none,
+                // so admins are refused here even when they filed the request themselves.
+                if (user.IsInRole("Admin"))
+                    throw new UpliftForbiddenException("Admins can revoke only admin-approved uplifts");
+
                 if (string.IsNullOrWhiteSpace(userId)
                     || !string.Equals(req.createdby, userId, StringComparison.Ordinal))
                 {

SeaHavenIndustries.Tests/WorkOrderUpliftDispatchOwnershipTests.cs

@@ -212,7 +212,7 @@ public sealed class WorkOrderUpliftDispatchOwnershipTests
             row.Id,
             created!.Id,
             new RevokeWorkOrderUpliftRequestDto(),
-            WorkOrderAccountTestHelpers.AccountUser(),
+            WorkOrderAccountTestHelpers.AccountUser("actor-1", 1, "Dispatcher"),
             CancellationToken.None);
 
         Assert.Equal("revoked", revoked!.Status);

SeaHavenIndustries.Tests/WorkOrderUpliftServiceTests.cs

@@ -48,6 +48,11 @@ public sealed class WorkOrderUpliftServiceTests
     private static ClaimsPrincipal Dispatcher(string userId = "dispatcher-1")
         => WorkOrderAccountTestHelpers.OrgWideAdmin(userId);
 
+    // Same user as Dispatcher(), without the Admin role: the only kind of caller that
+    // may revoke an auto-approved uplift, and only their own.
+    private static ClaimsPrincipal DispatcherRoleOnly(string userId = "dispatcher-1")
+        => WorkOrderAccountTestHelpers.AccountUser(userId, 1, "Dispatcher");
+
     private static async Task<(WorkOrder WorkOrder, Dispatch Dispatch)> SeedWorkOrderAsync(
         ApplicationDbContext context,
         WorkOrderType type = WorkOrderType.PM,
@@ -585,7 +590,7 @@ public sealed class WorkOrderUpliftServiceTests
             workOrder.Id,
             created.Id,
             new RevokeWorkOrderUpliftRequestDto(),
-            Dispatcher(),
+            DispatcherRoleOnly(),
             CancellationToken.None);
 
         // SH-196: revoking frees the allowance, so it must release the NTE too. Otherwise
@@ -724,7 +729,7 @@ public sealed class WorkOrderUpliftServiceTests
             workOrder.Id,
             created!.Id,
             new RevokeWorkOrderUpliftRequestDto(),
-            Dispatcher(),
+            DispatcherRoleOnly(),
             CancellationToken.None);
 
         Assert.Equal("revoked", revoked!.Status);
@@ -733,6 +738,35 @@ public sealed class WorkOrderUpliftServiceTests
             .SumAutoApprovedAmountForWorkOrderAsync(workOrder.Id, CancellationToken.None));
     }
 
+    [Fact]
+    public async Task RevokeAsync_AdminOwnerRevokingAutoApproved_IsForbiddenAndKeepsAllowanceConsumed()
+    {
+        await using var context = CreateContext();
+        var (workOrder, _) = await SeedWorkOrderAsync(context);
+        var service = NewService(context);
+
+        var created = await service.CreateAsync(
+            workOrder.Id,
+            new CreateWorkOrderUpliftRequestDto { Amount = 400m, Notes = "Within limit" },
+            Dispatcher(),
+            CancellationToken.None);
+        Assert.Equal("auto_approved", created!.Status);
+
+        await Assert.ThrowsAsync<UpliftForbiddenException>(() => service.RevokeAsync(
+            workOrder.Id,
+            created.Id,
+            new RevokeWorkOrderUpliftRequestDto { Reason = "Wrong quote" },
+            Dispatcher(),
+            CancellationToken.None));
+
+        var stored = context.DispatchUpliftRequests.Single(u => u.Id == created.Id);
+        Assert.Equal(UpliftStatus.NoApprovalRequired, stored.Status);
+        Assert.Null(stored.DecisionNote);
+        Assert.Equal(1400m, context.Dispatches.Single(d => d.Id == 10).NTEAmount);
+        Assert.Equal(400m, await new UpliftDataService(context)
+            .SumAutoApprovedAmountForWorkOrderAsync(workOrder.Id, CancellationToken.None));
+    }
+
     [Theory]
     [InlineData(LifecycleStatus.Completed)]
     [InlineData(LifecycleStatus.Canceled)]
@@ -760,7 +794,7 @@ public sealed class WorkOrderUpliftServiceTests
                 workOrder.Id,
                 100,
                 new RevokeWorkOrderUpliftRequestDto(),
-                Dispatcher(),
+                DispatcherRoleOnly(),
                 CancellationToken.None));
         Assert.Contains("work order", ex.Message, StringComparison.OrdinalIgnoreCase);
     }