From 4db8c08958ce44d986a496664293829143937821 Mon Sep 17 00:00:00 2001 From: cesnimda Date: Mon, 20 Jul 2026 00:20:09 +0200 Subject: [PATCH] fix(security): evaluate tenant CurrentUserId live, not at construction Production POST /api/cv/variants returned 200 but GET /api/cv/variants/{id} returned 404, with the query logged as `... FROM CvVariants WHERE FALSE` (no parameters). The created row had a correct OwnerUserId; the read was excluded by the global query filter because CurrentUserId was null at query time. Reproduced locally: it affected EVERY tenant-filtered read (CV list returned 0 after creating 5, JobApplications returned total 0), not just CV -- writes worked, reads came back empty. Root cause: the "local" JwtBearer OnTokenValidated resolves the request-scoped JobTrackerContext (to run LocalSessionValidator) BEFORE the authentication middleware assigns HttpContext.User. JobTrackerContext captured CurrentUserId in its constructor from ICurrentUserService.UserId, which reads HttpContext.User -- still unauthenticated at that point -- so CurrentUserId froze to null. That same scoped instance is reused by the controller, so `CurrentUserId != null && OwnerUserId == CurrentUserId` compiled to WHERE FALSE for the whole request. POST worked because CreateAsync sets OwnerUserId from the controller-resolved user, and inserts are not filtered. Fix: make CurrentUserId a computed property that reads ICurrentUserService.UserId live, so the query filters see the authenticated user at query-execution time. Deny-on-null is preserved (still null for an unauthenticated principal). LocalSessionValidator is unaffected -- it already uses IgnoreQueryFilters and queries by explicit sid. Verified on a real MariaDB 11 container end to end: create then read a variant returns 200, the variant list returns all rows, and GET /api/jobapplications reads normally. Added CurrentUserIdLiveEvaluationTests pinning the live-evaluation behaviour (both fail against a constructor snapshot). 422 tests pass. Co-Authored-By: Claude Opus 4.8 --- Data/JobTrackerContext.cs | 15 +++- .../CurrentUserIdLiveEvaluationTests.cs | 69 +++++++++++++++++++ 2 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 JobTrackerApi.Tests/CurrentUserIdLiveEvaluationTests.cs diff --git a/Data/JobTrackerContext.cs b/Data/JobTrackerContext.cs index 7883200..ef2833e 100644 --- a/Data/JobTrackerContext.cs +++ b/Data/JobTrackerContext.cs @@ -6,11 +6,22 @@ namespace JobTrackerApi.Data { public class JobTrackerContext : IdentityDbContext { - public string? CurrentUserId { get; } + private readonly JobTrackerApi.Services.ICurrentUserService _currentUser; + + // Evaluated live on each access, NOT captured in the constructor. The "local" JwtBearer + // OnTokenValidated resolves this request-scoped DbContext to run LocalSessionValidator BEFORE + // the authentication middleware assigns HttpContext.User. A constructor snapshot therefore froze + // CurrentUserId to null for the whole request, and the same scoped instance is reused by the + // controller — so every tenant-filtered read compiled to `WHERE FALSE` and returned nothing + // (created rows 404'd on read, lists came back empty) even though writes set OwnerUserId + // correctly from the controller-resolved user. Reading it live means the query filters see the + // authenticated user at query-execution time. Deny-on-null is preserved: it is still null for an + // unauthenticated principal. + public string? CurrentUserId => _currentUser.UserId; public JobTrackerContext(DbContextOptions options, JobTrackerApi.Services.ICurrentUserService currentUser) : base(options) { - CurrentUserId = currentUser.UserId; + _currentUser = currentUser; } public DbSet Companies => Set(); diff --git a/JobTrackerApi.Tests/CurrentUserIdLiveEvaluationTests.cs b/JobTrackerApi.Tests/CurrentUserIdLiveEvaluationTests.cs new file mode 100644 index 0000000..acc5fa7 --- /dev/null +++ b/JobTrackerApi.Tests/CurrentUserIdLiveEvaluationTests.cs @@ -0,0 +1,69 @@ +using JobTrackerApi.Data; +using JobTrackerApi.Models; +using JobTrackerApi.Services; +using Microsoft.EntityFrameworkCore; +using Moq; +using Xunit; + +namespace JobTrackerApi.Tests; + +// Regression guard for the production tenant-read failure: the "local" JwtBearer OnTokenValidated +// resolves the request-scoped JobTrackerContext to run session validation BEFORE the auth middleware +// sets HttpContext.User. When CurrentUserId was captured in the DbContext constructor, that froze it +// to null for the whole request, and every global-query-filtered read returned WHERE FALSE — created +// rows 404'd on read and lists came back empty, while writes (which set OwnerUserId from the +// controller-resolved user) still succeeded. +// +// The fix makes CurrentUserId read ICurrentUserService.UserId live on each access. These tests fail +// if it reverts to a constructor snapshot. +public sealed class CurrentUserIdLiveEvaluationTests +{ + private static (JobTrackerContext db, Mock user) New(string? initialUserId) + { + var options = new DbContextOptionsBuilder() + .UseInMemoryDatabase(Guid.NewGuid().ToString()).Options; + var user = new Mock(); + user.SetupGet(s => s.UserId).Returns(initialUserId); + return (new JobTrackerContext(options, user.Object), user); + } + + [Fact] + public void CurrentUserId_reflects_the_user_resolved_after_construction() + { + // Constructed while unauthenticated (UserId null), exactly as OnTokenValidated does. + var (db, user) = New(initialUserId: null); + Assert.Null(db.CurrentUserId); + + // The auth middleware then assigns the principal; UserId becomes non-null on the SAME instance. + user.SetupGet(s => s.UserId).Returns("user-1"); + + Assert.Equal("user-1", db.CurrentUserId); + } + + [Fact] + public async Task A_row_created_while_unauthenticated_context_is_readable_once_the_user_resolves() + { + var (db, user) = New(initialUserId: null); + + // Write happens with an explicit owner (mirrors CreateAsync taking the controller-resolved id), + // while the context was built before the user resolved. + db.CvVariants.Add(new CvVariant + { + OwnerUserId = "user-1", + Name = "Test", + PublicSlug = "slug-1", + SettingsJson = "{}", + Version = 1, + }); + await db.SaveChangesAsync(); + + // Before the user resolves, the tenant filter denies (deny-on-null preserved). + Assert.Empty(await db.CvVariants.ToListAsync()); + + // Once the middleware sets the user, the same context reads the row — not WHERE FALSE. + user.SetupGet(s => s.UserId).Returns("user-1"); + var visible = await db.CvVariants.ToListAsync(); + Assert.Single(visible); + Assert.Equal("user-1", visible[0].OwnerUserId); + } +}