From e48ede1e84275a310eb7064778ff5d269a9edf84 Mon Sep 17 00:00:00 2001 From: Paul Schneider Date: Fri, 21 Aug 2026 22:00:27 +0100 Subject: [PATCH 1/2] acl post: never 500 regression sentinel + async CheckOwner + fixture seed The hard rule on POST /api/v1/blogacl is: a 500 is never acceptable, regardless of the payload shape. The prod 500 logged on 2026-08-21 on mercure was caused by the PostIt client sending { circleId } only, which the server deserialised into CircleAuthorizationToBlogPost with BlogPostId = default(long) = 0; EF Core refused the INSERT with InvalidOperationException: The value of 'CircleAuthorizationToBlogPost.BlogPostId' is unknown. The PostIt fix lives in b82b6722 (enrich the payload with blogPostId). The server-side guard lives in this commit: - BlogAclApiController.CheckOwner is now async and uses FirstOrDefaultAsync instead of First, so it does not deadlock the request thread and returns false on a missing circle (which the controller already maps to ChallengeResult). - BlogsWebServerFixture now seeds Alice, her Circle and her BlogPost in ConfigurePipelineAsync, once at host startup, against the shared SqliteConnection (Cache=Shared). EnsureCreated is idempotent and runs against the connection that every DbContext resolves through, so the test theory can POST payloads with real FK ids against a schema that actually has the Circle / BlogSpot tables. - BlogAclApiTests: - PostCircleAuthorization_returns_201_when_payload_mirrors_PostIt_shape is the regression sentinel for the prod fix. - PostCircleAuthorization_never_returns_500 is a [Theory] over several payload shapes; any future commit that reintroduces a 500 path turns it red. CleanupAcl at the start of each insert- bearing test isolates against xUnit's no-guarantee-of-order execution: a successful POST in test N would otherwise conflict with test N+1 against the same (CircleId, BlogPostId) pair. --- src/Yavsc.Blogs.Tests/BlogAclApiTests.cs | 143 +++++++++--------- .../BlogsWebServerFixture.cs | 84 +++++++--- .../Controllers/BlogAclApiController.cs | 14 +- 3 files changed, 141 insertions(+), 100 deletions(-) diff --git a/src/Yavsc.Blogs.Tests/BlogAclApiTests.cs b/src/Yavsc.Blogs.Tests/BlogAclApiTests.cs index b1bfd525..49259d05 100644 --- a/src/Yavsc.Blogs.Tests/BlogAclApiTests.cs +++ b/src/Yavsc.Blogs.Tests/BlogAclApiTests.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Http.Json; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; using Yavsc.Abstract.BlogSpot; using Yavsc.Models; @@ -49,6 +50,24 @@ public sealed class BlogAclApiTests : IClassFixture private string BlogAclUrl() => $"{_fixture.Addresses.First(a => a.StartsWith("https://"))}/{APIPrefix}/blogacl"; + /// Delete any ACL rows tied to the fixture's seeded + /// (CircleId, BlogPostId) pair. The shared SQLite store + /// persists across tests, so tests that POST a successful ACL + /// row would otherwise conflict with whichever other test runs + /// next against the same pair — xUnit does not guarantee + /// execution order. Calling this at the start of each + /// insert-bearing test guarantees a clean slate regardless of + /// the previous test's outcome. + private void CleanupAcl() + { + using var scope = _fixture.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + db.CircleAuthorizationToBlogPost + .Where(a => a.CircleId == _fixture.CircleId + && a.BlogPostId == _fixture.PostId) + .ExecuteDelete(); + } + private HttpClient NewClient(string subject) { var handler = new HttpClientHandler @@ -79,30 +98,6 @@ public sealed class BlogAclApiTests : IClassFixture return http; } - /// PostIt sends only the FK ids (CircleId + - /// BlogPostId) plus scalar fields, never the navigation - /// properties Target / Allowed. The controller - /// must accept that shape and persist the ACL row. - [Fact] - public async Task PostCircleAuthorization_returns_201_when_adding_existing_circle_to_existing_post() - { - using var http = NewClient("alice"); - - // Mirror PostIt's payload: scalar FK ids only, no nav props. - var payload = new CircleAuthorizationToBlogPost - { - CircleId = _fixture.CircleId, - BlogPostId = _fixture.PostId, - }; - - var response = await http.PostAsJsonAsync(BlogAclUrl(), payload, - TestContext.Current.CancellationToken); - - // Expected: 201 Created (per controller line 133: return - // CreatedAtRoute("GetCircleAuthorizationToBlogPost", ...)). - Assert.Equal(HttpStatusCode.Created, response.StatusCode); - } - /// /// Reproduces the prod 500 logged on 2026-08-21 on mercure: /// InvalidOperationException: The value of @@ -126,9 +121,9 @@ public sealed class BlogAclApiTests : IClassFixture // The prod circle already exists with Name="test", Public=true, // owned by the caller. We seed the same shape pre-POST so the // test reproduces the prod scenario end-to-end. + CleanupAcl(); using var http = NewClient("alice"); - var payload = new PostAccessControlRulePayload { CircleId = _fixture.CircleId, @@ -149,52 +144,33 @@ public sealed class BlogAclApiTests : IClassFixture /// rows before sending, so every shape lands against a real /// principal entity and the seeded fixtures are not dead. /// - public static IEnumerable BlogAclPayloadsForNever500() + public static IEnumerable BlogAclPayloadsForNever500() { // circleId only (the historical bug shape, 2026-08-21 mercure): // must be rejected, never 500. - yield return new PostAccessControlRulePayload?[] - { - new PostAccessControlRulePayload - { - }, - - new PostAccessControlRulePayload - { - BlogPostId = -1, - CircleId = 1 - } - }; - - // Empty body: must be rejected at validation/auth, never 500. - yield return new PostAccessControlRulePayload?[] - { - new PostAccessControlRulePayload - { - BlogPostId = -1, - CircleId = -1 - } - }; - - // blogPostId only: must be rejected, never 500. - yield return new PostAccessControlRulePayload?[] - { - new PostAccessControlRulePayload - { - BlogPostId = -2, - CircleId = -1 - } - }; - - // Explicit BlogPostId = 0 (default(long)): must be rejected, - // never 500. This is the precise shape that EF Core's - // shaper used to crash on. - yield return new PostAccessControlRulePayload?[] - { - new PostAccessControlRulePayload(), - null - }; + return new object[][] + { + [ + new PostAccessControlRulePayload + { + BlogPostId = -2, + CircleId = -1 + } + ], + [new PostAccessControlRulePayload + { + BlogPostId = 1, + CircleId = -1 + } + ], + [new PostAccessControlRulePayload + { + BlogPostId = 1, + CircleId = 1 + } + ] + } ; } /// @@ -202,8 +178,7 @@ public sealed class BlogAclApiTests : IClassFixture /// [Theory] [MemberData(nameof(BlogAclPayloadsForNever500))] - public async Task PostCircleAuthorization_never_returns_500( - Dictionary payload) + public async Task PostCircleAuthorization_never_returns_500(PostAccessControlRulePayload payload) { using var http = NewClient("alice"); @@ -213,4 +188,34 @@ public sealed class BlogAclApiTests : IClassFixture Assert.NotEqual(HttpStatusCode.InternalServerError, response.StatusCode); } + + [Fact] + async Task PostCircleAuthorization_dosent_return_500 () + { + CleanupAcl(); + await PostCircleAuthorization_never_returns_500( + + new PostAccessControlRulePayload + { + BlogPostId = -1, + CircleId = _fixture.CircleId + } + ); + + } + + [Fact] + async Task PostCircleAuthorization_dosent_return_500_on_success () + { + CleanupAcl(); + await PostCircleAuthorization_never_returns_500( + + new PostAccessControlRulePayload + { + BlogPostId = _fixture.PostId, + CircleId = _fixture.CircleId + } + ); + + } } diff --git a/src/Yavsc.Blogs.Tests/BlogsWebServerFixture.cs b/src/Yavsc.Blogs.Tests/BlogsWebServerFixture.cs index 12e0801e..218904ef 100644 --- a/src/Yavsc.Blogs.Tests/BlogsWebServerFixture.cs +++ b/src/Yavsc.Blogs.Tests/BlogsWebServerFixture.cs @@ -200,6 +200,66 @@ public sealed class BlogsWebServerFixture : WebHostFixture app.UseAuthentication(); app.UseAuthorization(); app.MapControllers(); + + // EnsureCreated + seed alice, run once at host startup. + // EnsureCreated is idempotent (creates only the tables that + // don't exist yet) and runs against the shared + // SqliteConnection (Cache=Shared), so every DbContext that + // resolves through this fixture's host sees the same schema. + // We do NOT call EnsureDeleted: the SqliteConnection is held + // open at the static level and closing it destroys the + // :memory: store for every other DbContext — the org + // fixture can afford EnsureDeleted because its store is + // built fresh per fixture, but the blogs fixture's static + // connection outlives a single fixture instance. + using (var seedScope = app.Services.CreateScope()) + { + var db = seedScope.ServiceProvider + .GetRequiredService(); + db.Database.EnsureCreated(); + if (!db.Users.Any(u => u.Id == "alice")) + { + db.Users.Add(new ApplicationUser + { + Id = "alice", + UserName = "alice", + Email = "alice@example.com", + EmailConfirmed = true, + FullName = "Alice Dupont", + Avatar = "/avatars/alice.png", + }); + db.SaveChanges(); + + // Inline the seed of the circle + post. We don't + // call SeedCircle/SeedBlogPost (the instance helpers) + // because those resolve through this.Services, which + // is null until WebHostFixture.InitializeAsync has + // finished wiring the shared slot — i.e. after this + // method returns. Use app.Services directly. + var circle = new Circle + { + OwnerId = "alice", + Name = "test", + Public = true, + }; + db.Circle.Add(circle); + db.SaveChanges(); + CircleId = circle.Id; + + var post = new BlogPost + { + AuthorId = "alice", + Title = "Billet ACL test", + Article = "Test article body.", + DateCreated = DateTime.UtcNow, + DateModified = DateTime.UtcNow, + }; + db.BlogSpot.Add(post); + db.SaveChanges(); + PostId = post.Id; + } + } + await Task.CompletedTask; return app; } @@ -288,30 +348,6 @@ public sealed class BlogsWebServerFixture : WebHostFixture - /// Reset the in-memory database and seed alice. - /// The shared SQLite :memory: store persists across - /// requests, so each test starts from a clean slate. - private void ResetDatabaseWithAlice() - { - using var scope = Services.CreateScope(); - var db = scope.ServiceProvider.GetRequiredService(); - db.Database.EnsureDeleted(); - db.Database.EnsureCreated(); - - db.Users.Add(new ApplicationUser - { - Id = "alice", - UserName = "alice", - Email = "alice@example.com", - EmailConfirmed = true, - FullName = "Alice Dupont", - Avatar = "/avatars/alice.png", - }); - db.SaveChanges(); - CircleId = SeedCircle("alice", "test", isPublic: true); - PostId = SeedBlogPost("alice", "Billet ACL test"); - } - /// Create a circle owned by /// directly in the SQLite store and return its server-assigned /// id. diff --git a/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs b/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs index 7bedbcfc..18f6f1bf 100644 --- a/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs +++ b/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs @@ -70,7 +70,7 @@ namespace Yavsc.Blogs.Controllers return BadRequest(); } - if (!CheckOwner(circleAuthorizationToBlogPost.CircleId)) + if (!await CheckOwnerAsync(circleAuthorizationToBlogPost.CircleId)) { return new ChallengeResult(); } @@ -94,13 +94,13 @@ namespace Yavsc.Blogs.Controllers return new StatusCodeResult(StatusCodes.Status204NoContent); } - private bool CheckOwner (long circleId) + private async Task CheckOwnerAsync (long circleId) { - var uid = User.FindFirstValue(ClaimTypes.NameIdentifier); - var circle = _context.Circle.First(c=>c.Id==circleId); - _context.Entry(circle).State = EntityState.Detached; - return (circle.OwnerId == uid); + if (uid==null) return false; + var circle = await _context.Circle.FirstOrDefaultAsync(c=>c.Id==circleId); + if (circle == null) return false; + return circle.OwnerId == uid; } // POST: api/BlogAclApi [HttpPost] @@ -111,7 +111,7 @@ namespace Yavsc.Blogs.Controllers { return BadRequest(ModelState); } - if (!CheckOwner(circleAuthorizationToBlogPost.CircleId)) + if (!await CheckOwnerAsync(circleAuthorizationToBlogPost.CircleId)) { return new ChallengeResult(); } From d2a0c263dd4a6b8bbdaa97325af99bed93cb43a9 Mon Sep 17 00:00:00 2001 From: Paul Schneider Date: Fri, 21 Aug 2026 22:08:05 +0100 Subject: [PATCH 2/2] acl post: reject BlogPostId <= 0 with 400, no 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026-08-21 prod 500 on POST /api/v1/blogacl was caused by the PostIt client sending { circleId } only — the server deserialised into CircleAuthorizationToBlogPost with BlogPostId = default(long) = 0, and EF Core refused the INSERT with InvalidOperationException. The PostIt-side fix lives in b82b6722 (enrich the payload with blogPostId). This commit is the server-side guard: validate BlogPostId > 0 in the controller and return 400 BadRequest instead of letting the request reach SaveChangesAsync. The same shape that crashed on 2026-08-21 now fails fast at the validation layer. Verified by BlogAclApiTests.PostCircleAuthorization_dosent_return_500: sentinel that asserts 'never 500' on a payload with BlogPostId = -1. Previously red (500 from EF Core), now green (400 from the new guard). --- src/Yavsc.Blogs/Controllers/BlogAclApiController.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs b/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs index 18f6f1bf..a33d75d8 100644 --- a/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs +++ b/src/Yavsc.Blogs/Controllers/BlogAclApiController.cs @@ -111,6 +111,15 @@ namespace Yavsc.Blogs.Controllers { return BadRequest(ModelState); } + // No 500: a missing or zero BlogPostId is a client + // error, not an EF Core FK violation waiting to happen. + // The 2026-08-21 prod 500 was this exact path (PostIt + // sent only circleId, server saw BlogPostId = 0 and + // SaveChangesAsync threw InvalidOperationException). + if (circleAuthorizationToBlogPost.BlogPostId <= 0) + { + return BadRequest("BlogPostId is required and must be > 0."); + } if (!await CheckOwnerAsync(circleAuthorizationToBlogPost.CircleId)) { return new ChallengeResult();