fix(security): evaluate tenant CurrentUserId live, not at construction
CI and Deploy / test (push) Successful in 2m38s
CI and Deploy / deploy (push) Failing after 7s

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 <noreply@anthropic.com>
This commit is contained in:
cesnimda
2026-07-20 00:20:09 +02:00
parent 474654b1c1
commit 4db8c08958
2 changed files with 82 additions and 2 deletions
+13 -2
View File
@@ -6,11 +6,22 @@ namespace JobTrackerApi.Data
{ {
public class JobTrackerContext : IdentityDbContext<ApplicationUser> public class JobTrackerContext : IdentityDbContext<ApplicationUser>
{ {
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<JobTrackerContext> options, JobTrackerApi.Services.ICurrentUserService currentUser) : base(options) public JobTrackerContext(DbContextOptions<JobTrackerContext> options, JobTrackerApi.Services.ICurrentUserService currentUser) : base(options)
{ {
CurrentUserId = currentUser.UserId; _currentUser = currentUser;
} }
public DbSet<Company> Companies => Set<Company>(); public DbSet<Company> Companies => Set<Company>();
@@ -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<ICurrentUserService> user) New(string? initialUserId)
{
var options = new DbContextOptionsBuilder<JobTrackerContext>()
.UseInMemoryDatabase(Guid.NewGuid().ToString()).Options;
var user = new Mock<ICurrentUserService>();
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);
}
}