From 440ab2b3129b80b5abbb26b9950c8abc399b03f6 Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:45:56 -0400 Subject: [PATCH] fix(poracle): report a refused human, profile, area or place as the caller's mistake PoracleHumanProxy read 404 and one 409 on the delete-place path and let every other non-success reach EnsureSuccessStatusCode(), whose HttpRequestException the global handler flattens into 500 "An unexpected error occurred". So "state is required (true/false)", "profile_no must be specified" and "invalid latitude" -- all of which PoracleNG had explained -- arrived as a server fault and were logged as one. #539 closed exactly this on the tracking proxy and never reached the other half of the surface. Fourteen call sites now go through EnsureAcceptedAsync, which reads 400 and 422 through the existing PoracleProblemDetails and throws PoracleRequestRefusedException. A new filter answers it, rather than reusing AlarmValidationException: that type is documented as an alarm rejection and shares an alarm-worded fallback, so a profile rename would have been told "Poracle rejected the alarm." Describe() gained a fallback overload for the same reason. GetHumanAsync and CheckLocationAsync keep returning null on failure -- HumanService, ProfileService and TestAlertService all branch on it. Also stops a 404 that is not a dead account signing the user out. PoracleNG answers 404 "Profile not found" for a missing profile number and gin answers a plaintext "404 page not found" for a route an older build lacks; both were read as AccountGoneException and produced a 401, and inside ProfileOverviewService's restore-the-profile finally that happened while swallowing the real failure. Account-gone now needs PoracleNG to say so. Statuses and bodies verified live against 5.1.0 (:3040) and 5.2.1 (:3042): the v1 routes answer 400 identically on both, and 5.2.1's /api/v2/humans surface answers RFC 9457 at 422 for the same mistakes, which is why both are matched. No test data was created. --- .../PoracleRequestRefusedExceptionFilter.cs | 37 ++ .../Pgan.PoracleWebNet.Api/Program.cs | 1 + CHANGELOG.md | 2 + .../PoracleRequestRefusedException.cs | 50 +++ .../PoracleHumanProxy.cs | 118 ++++-- .../PoracleProblemDetails.cs | 16 +- ...racleRequestRefusedExceptionFilterTests.cs | 84 ++++ .../Services/PoracleHumanProxyRefusalTests.cs | 365 ++++++++++++++++++ 8 files changed, 630 insertions(+), 43 deletions(-) create mode 100644 Applications/Pgan.PoracleWebNet.Api/Filters/PoracleRequestRefusedExceptionFilter.cs create mode 100644 Core/Pgan.PoracleWebNet.Core.Models/PoracleRequestRefusedException.cs create mode 100644 Tests/Pgan.PoracleWebNet.Tests/Filters/PoracleRequestRefusedExceptionFilterTests.cs create mode 100644 Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs diff --git a/Applications/Pgan.PoracleWebNet.Api/Filters/PoracleRequestRefusedExceptionFilter.cs b/Applications/Pgan.PoracleWebNet.Api/Filters/PoracleRequestRefusedExceptionFilter.cs new file mode 100644 index 00000000..71cc2f17 --- /dev/null +++ b/Applications/Pgan.PoracleWebNet.Api/Filters/PoracleRequestRefusedExceptionFilter.cs @@ -0,0 +1,37 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; +using Pgan.PoracleWebNet.Core.Models; + +namespace Pgan.PoracleWebNet.Api.Filters; + +/// +/// Reports a refusal from PoracleNG's human, profile, area and location routes as the caller's problem. +/// +/// +/// Registered globally beside , which does the same job for the +/// tracking routes. Same body shape as every other refusal in this API -- { "error": "..." } -- so +/// the SPA's interceptor needs nothing new to show it. +/// +public sealed class PoracleRequestRefusedExceptionFilter : IActionFilter +{ + public void OnActionExecuting(ActionExecutingContext context) + { + } + + public void OnActionExecuted(ActionExecutedContext context) + { + if (context.Exception is not PoracleRequestRefusedException ex) + { + return; + } + + context.Result = new ObjectResult(new + { + error = ex.Message, + }) + { + StatusCode = ex.StatusCode, + }; + context.ExceptionHandled = true; + } +} diff --git a/Applications/Pgan.PoracleWebNet.Api/Program.cs b/Applications/Pgan.PoracleWebNet.Api/Program.cs index 7e4ae63c..1096326e 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Program.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Program.cs @@ -188,6 +188,7 @@ options.Filters.Add(); options.Filters.Add(); options.Filters.Add(); + options.Filters.Add(); options.Filters.Add(); options.Filters.Add(); options.Filters.Add(); diff --git a/CHANGELOG.md b/CHANGELOG.md index 509a7ba2..c9510ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **A refused profile, area, place or account change now says what was wrong with it.** Saving a profile with no number, a location that is not a location, an area list Poracle could not read, or banning a user through an older path all came back as *An unexpected error occurred* -- the wording this site uses when it genuinely does not know -- and were recorded as server faults. Poracle had explained every one of them; the explanation was being thrown away one layer below the screen. It now reaches the dialog that asked, in Poracle's own words, whether the server answered in its long-standing format or the newer one 5.2.1 introduced. The alarm pages were fixed this way some time ago; this is the same fix on the half of the site it never reached. +- **Switching to a profile that is no longer there stops signing you out.** Any *not found* answer from Poracle was read as *your account has been deleted*, which is what one of them means. A missing profile is not, and neither is a route an older Poracle does not have -- but both ended the session and sent you back to the login page, and one of them did it while a profile duplicate was tidying up after itself, hiding whatever had actually gone wrong. A deleted account still signs you out, exactly as before. - **The PVP rank range is readable again on a dark-themed alarm card.** The band under a PVP alarm showed its league and nothing else, so the ranks you had set looked like they had been dropped. They were being drawn, in white, on a band that stays light in both themes. The league name gained some contrast on the way past ([#800](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/800)). - **A delegate configured in Poracle's own config can see their webhooks again.** Poracle reports a delegated webhook by the name the operator wrote in `webhook_admins`, and this site matched those strings against the webhook's URL, so a name matched nothing: the *My Webhooks* item appeared in the sidebar, the page it led to was empty, and the button on it would have been refused. Grants are now resolved to the webhook they name, whether the config names it by name or by URL, and a grant that names no webhook at all no longer puts an item in the sidebar ([#797](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/797)). - **Impersonating a delegate shows what the delegate sees.** *My Webhooks* was hidden inside an impersonation session -- the one place an admin looks to find out why someone is complaining -- while the page and its actions would both have answered. Impersonating a second account from inside that session is refused, since only one token can be held for the way back out ([#797](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/797)). diff --git a/Core/Pgan.PoracleWebNet.Core.Models/PoracleRequestRefusedException.cs b/Core/Pgan.PoracleWebNet.Core.Models/PoracleRequestRefusedException.cs new file mode 100644 index 00000000..4bc6c037 --- /dev/null +++ b/Core/Pgan.PoracleWebNet.Core.Models/PoracleRequestRefusedException.cs @@ -0,0 +1,50 @@ +namespace Pgan.PoracleWebNet.Core.Models; + +/// +/// PoracleNG refused a human, profile, area or location request and said why. +/// +/// +/// +/// PoracleHumanProxy handled 404 and one 409 and let everything else reach +/// EnsureSuccessStatusCode(), so a 400 such as "state is required (true/false)" or "profile_no must +/// be specified" arrived at the browser as 500 "An unexpected error occurred" and was logged as a server +/// fault. This is the same defect #539 closed on the tracking proxy, on the half of the surface that fix +/// did not reach. +/// +/// +/// Deliberately not : that type is documented as an alarm rejection +/// and shares an alarm-worded fallback with it, so reusing it would answer "Poracle rejected the alarm." +/// to someone renaming a profile or saving a place. One narrowly-named exception per failure shape is the +/// pattern the rest of the filters already follow. +/// +/// +/// carries the answer through because not every refusal is a 400: PoracleNG +/// answers 404 "Profile not found" for a profile number that does not exist, which is the caller naming +/// something absent, not the account being gone. +/// +/// +public sealed class PoracleRequestRefusedException : Exception +{ + public PoracleRequestRefusedException(string message) + : base(message) + { + } + + public PoracleRequestRefusedException(string message, int statusCode) + : base(message) => this.StatusCode = statusCode; + + public PoracleRequestRefusedException() + { + } + + public PoracleRequestRefusedException(string message, Exception innerException) + : base(message, innerException) + { + } + + /// The status the API should answer. 400 unless PoracleNG named something absent. + public int StatusCode + { + get; init; + } = 400; +} diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs index b760d57a..9ee24c95 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs @@ -9,6 +9,9 @@ namespace Pgan.PoracleWebNet.Core.Services; public class PoracleHumanProxy(HttpClient httpClient, IConfiguration configuration) : IPoracleHumanProxy { + /// What to say when PoracleNG refused and explained nothing usable. + private const string Unexplained = "Poracle rejected the request."; + private readonly HttpClient _httpClient = httpClient; private readonly string _apiAddress = configuration["Poracle:ApiAddress"] ?? string.Empty; private readonly string _apiSecret = configuration["Poracle:ApiSecret"] ?? string.Empty; @@ -17,23 +20,75 @@ public class PoracleHumanProxy(HttpClient httpClient, IConfiguration configurati /// URL-encodes a userId for safe path construction. Webhook IDs are full URLs /// containing slashes that would break routing without encoding. /// + private static string Encode(string userId) => Uri.EscapeDataString(userId); + /// - /// Turns PoracleNG's "user not found" into something the API can answer 401 to. + /// Turns a refusal from PoracleNG into the answer it deserves, and lets everything else throw. /// /// - /// A JWT outlives the account it names. Without this, every lookup for a deleted user threw an - /// HttpRequestException that the global handler flattened into a 500, so the SPA -- which signs out - /// only on 401 -- left the user in an app where every page failed. See #584. + /// + /// Only 404 was read here, and one 409 on the delete-place path. Everything else fell through to + /// EnsureSuccessStatusCode(), whose the global handler + /// flattens into 500 "An unexpected error occurred" -- so "state is required (true/false)", + /// "profile_no must be specified" and "invalid latitude" all reached the user as a server fault and + /// were logged as one. #539 fixed exactly this on the tracking proxy and never reached here. + /// + /// + /// 422 is the same refusal wearing a different number. Verified live: the v1 human and profile routes + /// answer 400 on 5.1.0 and 5.2.1 alike, but 5.2.1's /api/v2/humans surface answers RFC 9457 + /// problem+json at 422 for the same mistakes. Matching only 400 would re-open this the day a call + /// site moves to v2. + /// + /// + /// A 404 is account-gone only when PoracleNG says so. It also answers 404 "Profile not found" when a + /// profile number does not exist, and gin answers a plaintext "404 page not found" for a route this + /// build does not have -- treating either as a dead account signed the user out of a working session, + /// and in ProfileOverviewService's restore-the-profile finally it did so while + /// swallowing the real failure. Both verified against 5.1.0 and 5.2.1. + /// /// - private static void EnsureAccountStillExists(HttpResponseMessage response) + private static async Task EnsureAcceptedAsync(HttpResponseMessage response) { - if (response.StatusCode == HttpStatusCode.NotFound) + if (response.IsSuccessStatusCode) + { + return; + } + + var payload = await response.Content.ReadAsStringAsync(); + + switch (response.StatusCode) { - throw new AccountGoneException(); + case HttpStatusCode.NotFound when NamesAMissingAccount(payload): + // A JWT outlives the account it names. Without this, every lookup for a deleted user threw + // an HttpRequestException that the global handler flattened into a 500, so the SPA -- which + // signs out only on 401 -- left the user in an app where every page failed. See #584. + throw new AccountGoneException(); + + case HttpStatusCode.NotFound: + throw new PoracleRequestRefusedException( + PoracleProblemDetails.Describe(payload, Unexplained), (int)HttpStatusCode.NotFound); + + case HttpStatusCode.BadRequest: + case HttpStatusCode.UnprocessableEntity: + throw new PoracleRequestRefusedException(PoracleProblemDetails.Describe(payload, Unexplained)); + + default: + response.EnsureSuccessStatusCode(); + return; } } - private static string Encode(string userId) => Uri.EscapeDataString(userId); + /// + /// True when a 404 body is PoracleNG saying the account itself is gone rather than something in it. + /// + /// + /// v1 answers {"message":"User not found"}; the v2 surface and the tracking routes say + /// "human not found". Anything else at 404 names a profile, a place or a route. + /// + private static bool NamesAMissingAccount(string? payload) => + payload is not null + && (payload.Contains("user not found", StringComparison.OrdinalIgnoreCase) + || payload.Contains("human not found", StringComparison.OrdinalIgnoreCase)); public async Task GetHumanAsync(string userId) { @@ -58,22 +113,19 @@ private static void EnsureAccountStillExists(HttpResponseMessage response) public async Task CreateHumanAsync(JsonElement body) { var response = await this.SendAsync(HttpMethod.Post, "/api/humans", body.GetRawText()); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task StartAsync(string userId) { var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/start"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task StopAsync(string userId) { var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/stop"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task AdminDisabledAsync(string userId, bool disabled) @@ -86,23 +138,20 @@ public async Task AdminDisabledAsync(string userId, bool disabled) state = disabled }); var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/adminDisabled", body); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task SetLocationAsync(string userId, double lat, double lon) { var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/setLocation/{lat}/{lon}"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task SetAreasAsync(string userId, string[] areas) { var body = JsonSerializer.Serialize(areas); var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/setAreas", body); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task GetAreasAsync(string userId) => @@ -113,15 +162,13 @@ public async Task SetAreasAsync(string userId, string[] areas) public async Task SwitchProfileAsync(string userId, int profileNo) { var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/switchProfile/{profileNo}"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task GetProfilesAsync(string userId) { var response = await this.SendAsync(HttpMethod.Get, $"/api/profiles/{Encode(userId)}"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); var json = await response.Content.ReadAsStringAsync(); using var doc = JsonDocument.Parse(json); @@ -131,29 +178,25 @@ public async Task GetProfilesAsync(string userId) public async Task AddProfileAsync(string userId, JsonElement body) { var response = await this.SendAsync(HttpMethod.Post, $"/api/profiles/{Encode(userId)}/add", body.GetRawText()); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task UpdateProfileAsync(string userId, JsonElement body) { var response = await this.SendAsync(HttpMethod.Post, $"/api/profiles/{Encode(userId)}/update", body.GetRawText()); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task DeleteProfileAsync(string userId, int profileNo) { var response = await this.SendAsync(HttpMethod.Delete, $"/api/profiles/{Encode(userId)}/byProfileNo/{profileNo}"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task CopyProfileAsync(string userId, int fromProfileNo, int toProfileNo) { var response = await this.SendAsync(HttpMethod.Post, $"/api/profiles/{Encode(userId)}/copy/{fromProfileNo}/{toProfileNo}"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } public async Task CheckLocationAsync(string userId, double lat, double lon) @@ -173,8 +216,7 @@ public async Task CopyProfileAsync(string userId, int fromProfileNo, int toProfi public async Task GetPlacesAsync(string userId) { var response = await this.SendAsync(HttpMethod.Get, $"/api/humans/{Encode(userId)}/locations"); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); var json = await response.Content.ReadAsStringAsync(); using var doc = JsonDocument.Parse(json); @@ -225,8 +267,7 @@ public async Task GetPlacesAsync(string userId) var response = await this.SendAsync( HttpMethod.Post, $"/api/humans/{Encode(userId)}/locations/add", body); - EnsureAccountStillExists(response); - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); // A rejected label is reported inside a 200: PoracleNG answers per row so a batch can partly // succeed. Treating the 200 as success stored nothing and told the user it worked. @@ -255,8 +296,9 @@ public async Task DeletePlaceAsync(string userId, string label) { var response = await this.SendAsync( HttpMethod.Post, $"/api/humans/{Encode(userId)}/locations/{Encode(label)}/delete"); - EnsureAccountStillExists(response); + // Read before the general refusal path: a 409 here names the alarms still pointing at the place, + // which is the difference between "could not delete" and knowing what to repoint first. if (response.StatusCode == HttpStatusCode.Conflict) { var conflict = await response.Content.ReadAsStringAsync(); @@ -269,7 +311,7 @@ public async Task DeletePlaceAsync(string userId, string label) throw new PlaceInUseException(rules); } - response.EnsureSuccessStatusCode(); + await EnsureAcceptedAsync(response); } private async Task SendAsync(HttpMethod method, string path, string? body = null) diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs index b206d285..34afe119 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs @@ -39,11 +39,17 @@ internal static class PoracleProblemDetails /// Reads an explanation out of a response body. Never throws: an unreadable body still has to produce /// something to show, and the alternative is a 500 for a request PoracleNG already described. /// - public static string Describe(string? body) + public static string Describe(string? body) => Describe(body, Unexplained); + + /// + /// As , but says something other than "alarm" when the body explains + /// nothing. The human, profile, area and location routes refuse requests that are not alarms. + /// + public static string Describe(string? body, string fallback) { if (string.IsNullOrWhiteSpace(body)) { - return Unexplained; + return fallback; } JsonElement root; @@ -56,12 +62,12 @@ public static string Describe(string? body) { // Not JSON. gin's plaintext "404 page not found" lands here, as does an HTML error page from // whatever proxy sits in front. Short bodies are still better than nothing. - return body.Length > 300 ? Unexplained : body.Trim(); + return body.Length > 300 ? fallback : body.Trim(); } if (root.ValueKind != JsonValueKind.Object) { - return Unexplained; + return fallback; } var fieldErrors = FieldErrors(root); @@ -81,7 +87,7 @@ public static string Describe(string? body) } } - return Unexplained; + return fallback; } /// True when this body is PoracleNG's problem+json rather than the v1 shape. diff --git a/Tests/Pgan.PoracleWebNet.Tests/Filters/PoracleRequestRefusedExceptionFilterTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Filters/PoracleRequestRefusedExceptionFilterTests.cs new file mode 100644 index 00000000..f317e1fb --- /dev/null +++ b/Tests/Pgan.PoracleWebNet.Tests/Filters/PoracleRequestRefusedExceptionFilterTests.cs @@ -0,0 +1,84 @@ +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.AspNetCore.Routing; +using Pgan.PoracleWebNet.Api.Filters; +using Pgan.PoracleWebNet.Core.Models; + +namespace Pgan.PoracleWebNet.Tests.Filters; + +/// +/// The global filter that answers a PoracleNG refusal on the human, profile, area and location routes. +/// +public class PoracleRequestRefusedExceptionFilterTests +{ + private static ActionExecutedContext BuildContext(Exception ex) + { + var actionContext = new ActionContext(new DefaultHttpContext(), new RouteData(), new ActionDescriptor()); + return new ActionExecutedContext(actionContext, [], controller: null!) { Exception = ex }; + } + + private static object? Read(object value, string property) => + value.GetType().GetProperty(property)?.GetValue(value); + + [Fact] + public void AnswersFourHundredCarryingPoraclesOwnWording() + { + var context = BuildContext(new PoracleRequestRefusedException("state is required (true/false)")); + + new PoracleRequestRefusedExceptionFilter().OnActionExecuted(context); + + var result = Assert.IsType(context.Result); + Assert.Equal(StatusCodes.Status400BadRequest, result.StatusCode); + Assert.True(context.ExceptionHandled); + Assert.Equal("state is required (true/false)", Read(result.Value!, "error")); + } + + [Fact] + public void KeepsTheStatusTheRefusalCarries() + { + // "Profile not found" is a 404 upstream and stays one here. Flattening it to 400 would say the + // request was malformed when it named something that is simply not there. + var context = BuildContext(new PoracleRequestRefusedException("Profile not found", 404)); + + new PoracleRequestRefusedExceptionFilter().OnActionExecuted(context); + + Assert.Equal(StatusCodes.Status404NotFound, Assert.IsType(context.Result).StatusCode); + } + + [Fact] + public void UsesTheSameBodyShapeAsEveryOtherRefusal() + { + // The SPA's interceptor reads `error`. A different key here would show a blank snackbar. + var context = BuildContext(new PoracleRequestRefusedException("invalid latitude")); + + new PoracleRequestRefusedExceptionFilter().OnActionExecuted(context); + + Assert.NotNull(Read(Assert.IsType(context.Result).Value!, "error")); + } + + [Fact] + public void LeavesTheAccountGoneFilterAlone() + { + // Every filter runs on every request. Answering 400 for a dead account would cost the SPA the 401 + // it signs out on -- the defect #584 closed. + var context = BuildContext(new AccountGoneException()); + + new PoracleRequestRefusedExceptionFilter().OnActionExecuted(context); + + Assert.Null(context.Result); + Assert.False(context.ExceptionHandled); + } + + [Fact] + public void IgnoresOtherExceptions() + { + var context = BuildContext(new InvalidOperationException("unrelated")); + + new PoracleRequestRefusedExceptionFilter().OnActionExecuted(context); + + Assert.Null(context.Result); + Assert.False(context.ExceptionHandled); + } +} diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs new file mode 100644 index 00000000..e53f97a9 --- /dev/null +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs @@ -0,0 +1,365 @@ +using System.Net; +using System.Text; +using System.Text.Json; +using Microsoft.Extensions.Configuration; +using Pgan.PoracleWebNet.Core.Models; +using Pgan.PoracleWebNet.Core.Services; + +namespace Pgan.PoracleWebNet.Tests.Services; + +/// +/// What the human, profile, area and location proxy does when PoracleNG refuses. +/// +/// +/// +/// Every status and body quoted here was taken from a live PoracleNG: 5.1.0 on :3040 and 5.2.1 on :3042, +/// probed with deliberately invalid requests. The v1 routes this proxy calls answer 400 identically on +/// both; the 422 bodies come from 5.2.1's /api/v2/humans surface, which the same mistakes reach +/// once a call site moves over. +/// +/// +/// Half of these assert what must keep working rather than what must now fail. A refusal filter that also +/// swallows the account-gone 404 costs the SPA its sign-out; one that swallows the delete-place 409 costs +/// the user the list of alarms blocking the delete. Both are cheaper to catch here than in production. +/// +/// +public class PoracleHumanProxyRefusalTests +{ + private static PoracleHumanProxy CreateSut(MockHandler handler) => + new(new HttpClient(handler), new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Poracle:ApiAddress"] = "http://localhost:3030", + ["Poracle:ApiSecret"] = "test-secret", + }) + .Build()); + + private static PoracleHumanProxy Refusing(HttpStatusCode status, string body) => + CreateSut(new MockHandler(status, body)); + + private static JsonElement Body(string json) => JsonDocument.Parse(json).RootElement; + + // ────────────────────────────────────────────────────────────── + // 400 on the v1 routes -- the caller's mistake, reported as one + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task AdminDisabledPassesPoracleWordingThroughOn400() + { + // Live: POST /api/humans/{id}/adminDisabled {"nope":1} on 5.1.0 and 5.2.1. + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"state is required (true/false)","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.AdminDisabledAsync("user1", true)); + + Assert.Equal("state is required (true/false)", ex.Message); + Assert.Equal(400, ex.StatusCode); + } + + [Fact] + public async Task SetLocationPassesPoracleWordingThroughOn400() + { + // Live: POST /api/humans/{id}/setLocation/abc/def. + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid latitude","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.SetLocationAsync("user1", 0, 0)); + + Assert.Equal("invalid latitude", ex.Message); + } + + [Fact] + public async Task SetAreasPassesPoracleWordingThroughOn400() + { + // Live: POST /api/humans/{id}/setAreas with an object instead of an array. + var sut = Refusing( + HttpStatusCode.BadRequest, + """{"message":"json: cannot unmarshal object into Go value of type []string","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.SetAreasAsync("user1", ["area1"])); + + Assert.Contains("cannot unmarshal object", ex.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task CreateHumanPassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"id and name are required","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.CreateHumanAsync(Body("""{"garbage":true}"""))); + + Assert.Equal("id and name are required", ex.Message); + } + + [Fact] + public async Task UpdateProfilePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"profile_no must be specified","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.UpdateProfileAsync("user1", Body("""{"bad":1}"""))); + + Assert.Equal("profile_no must be specified", ex.Message); + } + + [Fact] + public async Task AddProfilePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid request body","status":"error"}"""); + + await Assert.ThrowsAsync( + () => sut.AddProfileAsync("user1", Body("""{"bad":1}"""))); + } + + [Fact] + public async Task DeleteProfilePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid profile_no","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.DeleteProfileAsync("user1", 1)); + + Assert.Equal("invalid profile_no", ex.Message); + } + + [Fact] + public async Task CopyProfilePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid from profile number","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.CopyProfileAsync("user1", 1, 2)); + + Assert.Equal("invalid from profile number", ex.Message); + } + + [Fact] + public async Task SwitchProfilePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid profile number","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.SwitchProfileAsync("user1", 1)); + + Assert.Equal("invalid profile number", ex.Message); + } + + [Fact] + public async Task AddPlacePassesPoracleWordingThroughOn400() + { + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid request body","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.AddPlaceAsync("user1", new SavedPlace { Label = "home", Latitude = 1, Longitude = 2 })); + + Assert.Equal("invalid request body", ex.Message); + } + + // ────────────────────────────────────────────────────────────── + // 422 -- the same refusal wearing v2's number + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task ReadsProblemJsonFieldErrorsOn422() + { + // Live: POST /api/v2/humans/{id}/admin-disable {"nope":1} on 5.2.1. + var sut = Refusing(HttpStatusCode.UnprocessableEntity, """ + {"title":"Unprocessable Entity","status":422,"detail":"validation failed", + "errors":[{"message":"expected required property disabled to be present","location":"body"}, + {"message":"unexpected property","location":"body.nope"}]} + """); + + var ex = await Assert.ThrowsAsync( + () => sut.AdminDisabledAsync("user1", true)); + + Assert.Contains("expected required property disabled to be present", ex.Message, StringComparison.Ordinal); + Assert.Contains("nope: unexpected property", ex.Message, StringComparison.Ordinal); + Assert.Equal(400, ex.StatusCode); + } + + [Fact] + public async Task ReadsProblemJsonDetailWhenThereAreNoFieldErrors() + { + var sut = Refusing( + HttpStatusCode.UnprocessableEntity, + """{"title":"Unprocessable Entity","status":422,"detail":"latitude must be between -90 and 90"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.SetLocationAsync("user1", 999, 0)); + + Assert.Equal("latitude must be between -90 and 90", ex.Message); + } + + [Fact] + public async Task SaysSomethingOtherThanAlarmWhenTheRefusalExplainsNothing() + { + // PoracleProblemDetails' own fallback names an alarm, which is wrong for a profile or a place. + var sut = Refusing(HttpStatusCode.BadRequest, "{}"); + + var ex = await Assert.ThrowsAsync( + () => sut.UpdateProfileAsync("user1", Body("{}"))); + + Assert.DoesNotContain("alarm", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.NotEmpty(ex.Message); + } + + // ────────────────────────────────────────────────────────────── + // 404 -- account gone, and the two 404s that are not + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task AMissingAccountIsStillAccountGone() + { + // The SPA signs out only on 401, and AccountGoneExceptionFilter is what produces it. Live wording + // from both 5.1.0 and 5.2.1. + var sut = Refusing(HttpStatusCode.NotFound, """{"message":"User not found","status":"error"}"""); + + await Assert.ThrowsAsync(() => sut.StartAsync("deleted-user")); + } + + [Theory] + [InlineData("/stop")] + [InlineData("/setAreas")] + [InlineData("/profiles")] + public async Task EveryWritePathStillReportsAMissingAccountAsGone(string path) + { + var sut = Refusing(HttpStatusCode.NotFound, """{"message":"User not found","status":"error"}"""); + + Task Call() => path switch + { + "/stop" => sut.StopAsync("deleted-user"), + "/setAreas" => sut.SetAreasAsync("deleted-user", ["area1"]), + _ => sut.GetProfilesAsync("deleted-user"), + }; + + await Assert.ThrowsAsync(Call); + } + + [Fact] + public async Task TheV2WordingForAMissingAccountCountsToo() + { + var sut = Refusing(HttpStatusCode.NotFound, """{"title":"Not Found","status":404,"detail":"human not found"}"""); + + await Assert.ThrowsAsync(() => sut.StopAsync("deleted-user")); + } + + [Fact] + public async Task AMissingProfileIsNotAMissingAccount() + { + // Live: POST /api/humans/{id}/switchProfile/99 answers 404 "Profile not found" on both servers. + // Calling that account-gone signed the user out of a working session -- and inside + // ProfileOverviewService's restore-the-profile finally, it did so while hiding the real failure. + var sut = Refusing(HttpStatusCode.NotFound, """{"message":"Profile not found","status":"error"}"""); + + var ex = await Assert.ThrowsAsync( + () => sut.SwitchProfileAsync("user1", 99)); + + Assert.Equal("Profile not found", ex.Message); + Assert.Equal(404, ex.StatusCode); + } + + [Fact] + public async Task ARouteThisBuildDoesNotHaveIsNotAMissingAccount() + { + // gin answers a plaintext body for an absent route. Signing the user out over it told them their + // account was deleted when the truth was an older PoracleNG. + var sut = Refusing(HttpStatusCode.NotFound, "404 page not found"); + + var ex = await Assert.ThrowsAsync( + () => sut.CopyProfileAsync("user1", 1, 2)); + + Assert.Equal(404, ex.StatusCode); + Assert.Contains("404 page not found", ex.Message, StringComparison.Ordinal); + } + + // ────────────────────────────────────────────────────────────── + // What must keep working + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task APlaceStillInUseIsStillAConflictNamingTheAlarms() + { + var handler = new MockHandler( + HttpStatusCode.Conflict, + """{"message":"location in use","referencing_rules":["monster 25","raid 5"]}"""); + var sut = CreateSut(handler); + + var ex = await Assert.ThrowsAsync(() => sut.DeletePlaceAsync("user1", "home")); + + Assert.Equal(["monster 25", "raid 5"], ex.ReferencingRules); + } + + [Fact] + public async Task AServerFaultIsStillAServerFault() + { + // 500, 502 and the rest are not the caller's problem and must not be dressed up as a 400. + var sut = Refusing(HttpStatusCode.InternalServerError, "{}"); + + await Assert.ThrowsAsync(() => sut.SetAreasAsync("user1", ["area1"])); + } + + [Fact] + public async Task AConflictOutsideDeletePlaceIsStillAServerFault() + { + // Only the delete-place route gives 409 a meaning. Nothing else should start reading one. + var sut = Refusing(HttpStatusCode.Conflict, "{}"); + + await Assert.ThrowsAsync(() => sut.CreateHumanAsync(Body("{}"))); + } + + [Fact] + public async Task A200StillSucceedsOnEveryPathThisTouches() + { + var handler = new MockHandler(HttpStatusCode.OK, """{"status":"ok"}"""); + var sut = CreateSut(handler); + + await sut.CreateHumanAsync(Body("""{"id":"u1","name":"n"}""")); + await sut.StartAsync("user1"); + await sut.StopAsync("user1"); + await sut.AdminDisabledAsync("user1", true); + await sut.SetLocationAsync("user1", 1, 2); + await sut.SetAreasAsync("user1", ["area1"]); + await sut.SwitchProfileAsync("user1", 1); + await sut.AddProfileAsync("user1", Body("""{"name":"p"}""")); + await sut.UpdateProfileAsync("user1", Body("""{"profile_no":1}""")); + await sut.DeleteProfileAsync("user1", 1); + await sut.CopyProfileAsync("user1", 1, 2); + await sut.DeletePlaceAsync("user1", "home"); + + Assert.Null(await sut.AddPlaceAsync("user1", new SavedPlace { Label = "home" })); + } + + [Fact] + public async Task AddPlaceStillReportsTheRefusalHiddenInsideA200() + { + // PoracleNG answers per row so a batch can partly succeed. This is not a status-code refusal and + // must not turn into one. + var handler = new MockHandler(HttpStatusCode.OK, """{"results":[{"error":"label already used"}]}"""); + var sut = CreateSut(handler); + + Assert.Equal("label already used", await sut.AddPlaceAsync("user1", new SavedPlace { Label = "home" })); + } + + [Fact] + public async Task ReadsThatAnswerNullOnFailureStillAnswerNull() + { + // GetHumanAsync and CheckLocationAsync are read paths whose callers branch on null. Turning their + // failures into throws would break HumanService, ProfileService and TestAlertService at once. + var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid latitude","status":"error"}"""); + + Assert.Null(await sut.GetHumanAsync("user1")); + Assert.Null(await sut.CheckLocationAsync("user1", 999, 999)); + Assert.Null(await sut.GetAreasAsync("user1")); + } + + private sealed class MockHandler(HttpStatusCode statusCode, string responseBody) : HttpMessageHandler + { + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) => + Task.FromResult(new HttpResponseMessage(statusCode) + { + Content = new StringContent(responseBody, Encoding.UTF8, "application/json"), + }); + } +}