fix(expense): resolve current user id from 'sub' JWT claim
Live verification revealed the JWT carries the user id in the 'sub' claim (NameClaimType=sub, MapInboundClaims=false), so ClaimTypes.NameIdentifier is null at runtime. This caused ExpensesController.GetMine/GetById to throw NullReferenceException (500) on the '!.Value', and made the services fall back to 'system' — silently defeating the self-ownership guard. Resolve via NameIdentifier (unit tests) then 'sub' (real tokens). Adds a regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -55,6 +55,16 @@ public class ExpenseServiceTests
|
|||||||
return new ExpenseService(db, http.Object, fs);
|
return new ExpenseService(db, http.Object, fs);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Builds a service whose principal carries ONLY the "sub" claim (no NameIdentifier),
|
||||||
|
// mirroring the real JWT (NameClaimType="sub", MapInboundClaims=false).
|
||||||
|
private static ExpenseService SvcWithSubClaim(AppDbContext db, FakeStorage fs, string userId)
|
||||||
|
{
|
||||||
|
var http = new Mock<IHttpContextAccessor>();
|
||||||
|
var ctx = new DefaultHttpContext { User = new(new ClaimsIdentity(new[] { new Claim("sub", userId) })) };
|
||||||
|
http.Setup(x => x.HttpContext).Returns(ctx);
|
||||||
|
return new ExpenseService(db, http.Object, fs);
|
||||||
|
}
|
||||||
|
|
||||||
private static CreateExpenseRequest Reimb() => new()
|
private static CreateExpenseRequest Reimb() => new()
|
||||||
{
|
{
|
||||||
Type = "StaffReimbursement", MinistryId = 1, CategoryGroupId = 1, SubCategoryId = 1,
|
Type = "StaffReimbursement", MinistryId = 1, CategoryGroupId = 1, SubCategoryId = 1,
|
||||||
@@ -69,6 +79,24 @@ public class ExpenseServiceTests
|
|||||||
ExpenseDate = r.ExpenseDate, Notes = r.Notes,
|
ExpenseDate = r.ExpenseDate, Notes = r.Notes,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Create_Reimbursement_ResolvesUserId_FromSubClaim()
|
||||||
|
{
|
||||||
|
// Regression: the real JWT exposes the user id as "sub", not ClaimTypes.NameIdentifier.
|
||||||
|
// SubmittedBy must be the sub value (not "system"), or the self-ownership guard breaks.
|
||||||
|
var db = BuildDb("ignored");
|
||||||
|
db.Ministries.Add(new Ministry { Id = 1, Name_en = "Worship" });
|
||||||
|
db.ExpenseCategoryGroups.Add(new ExpenseCategoryGroup { Id = 1, Name_en = "Equipment" });
|
||||||
|
db.ExpenseSubCategories.Add(new ExpenseSubCategory { Id = 1, GroupId = 1, Name_en = "Purchase" });
|
||||||
|
await db.SaveChangesAsync();
|
||||||
|
var svc = SvcWithSubClaim(db, new FakeStorage(), "user-guid-123");
|
||||||
|
|
||||||
|
var id = await svc.CreateAsync(Reimb(), isFinance: false);
|
||||||
|
|
||||||
|
var e = await db.Expenses.FindAsync(id);
|
||||||
|
Assert.Equal("user-guid-123", e!.SubmittedBy);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task Create_Vendor_AsFinance_IsImmediatelyPaid()
|
public async Task Create_Vendor_AsFinance_IsImmediatelyPaid()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
using System.Security.Claims;
|
||||||
using Microsoft.AspNetCore.Authorization;
|
using Microsoft.AspNetCore.Authorization;
|
||||||
using Microsoft.AspNetCore.Mvc;
|
using Microsoft.AspNetCore.Mvc;
|
||||||
using ROLAC.API.DTOs.Expense;
|
using ROLAC.API.DTOs.Expense;
|
||||||
@@ -16,6 +17,10 @@ public class ExpensesController : ControllerBase
|
|||||||
private bool IsFinance() => User.IsInRole("finance") || User.IsInRole("super_admin");
|
private bool IsFinance() => User.IsInRole("finance") || User.IsInRole("super_admin");
|
||||||
private bool CanViewAll() => IsFinance() || User.IsInRole("pastor");
|
private bool CanViewAll() => IsFinance() || User.IsInRole("pastor");
|
||||||
|
|
||||||
|
// User id lives in the "sub" claim (NameClaimType="sub"); NameIdentifier is absent at runtime.
|
||||||
|
private string CurrentUserId() =>
|
||||||
|
User.FindFirstValue(ClaimTypes.NameIdentifier) ?? User.FindFirstValue("sub") ?? "";
|
||||||
|
|
||||||
[HttpGet]
|
[HttpGet]
|
||||||
public async Task<IActionResult> GetPaged(
|
public async Task<IActionResult> GetPaged(
|
||||||
[FromQuery] int page = 1, [FromQuery] int pageSize = 20, [FromQuery] string? search = null,
|
[FromQuery] int page = 1, [FromQuery] int pageSize = 20, [FromQuery] string? search = null,
|
||||||
@@ -29,8 +34,7 @@ public class ExpensesController : ControllerBase
|
|||||||
[HttpGet("mine")]
|
[HttpGet("mine")]
|
||||||
public async Task<IActionResult> GetMine([FromQuery] string? status = null, [FromQuery] int page = 1, [FromQuery] int pageSize = 20)
|
public async Task<IActionResult> GetMine([FromQuery] string? status = null, [FromQuery] int page = 1, [FromQuery] int pageSize = 20)
|
||||||
{
|
{
|
||||||
var uid = User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)!.Value;
|
return Ok(await _svc.GetMineAsync(CurrentUserId(), status, page, pageSize));
|
||||||
return Ok(await _svc.GetMineAsync(uid, status, page, pageSize));
|
|
||||||
}
|
}
|
||||||
|
|
||||||
[HttpGet("{id:int}")]
|
[HttpGet("{id:int}")]
|
||||||
@@ -38,8 +42,7 @@ public class ExpensesController : ControllerBase
|
|||||||
{
|
{
|
||||||
var dto = await _svc.GetByIdAsync(id);
|
var dto = await _svc.GetByIdAsync(id);
|
||||||
if (dto is null) return NotFound();
|
if (dto is null) return NotFound();
|
||||||
var uid = User.FindFirst(System.Security.Claims.ClaimTypes.NameIdentifier)!.Value;
|
if (!CanViewAll() && dto.SubmittedBy != CurrentUserId()) return Forbid();
|
||||||
if (!CanViewAll() && dto.SubmittedBy != uid) return Forbid();
|
|
||||||
return Ok(dto);
|
return Ok(dto);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -17,8 +17,13 @@ public class ExpenseService : IExpenseService
|
|||||||
public ExpenseService(AppDbContext db, IHttpContextAccessor http, IFileStorage storage)
|
public ExpenseService(AppDbContext db, IHttpContextAccessor http, IFileStorage storage)
|
||||||
{ _db = db; _http = http; _storage = storage; }
|
{ _db = db; _http = http; _storage = storage; }
|
||||||
|
|
||||||
|
// The JWT carries the user id in the "sub" claim (NameClaimType="sub", MapInboundClaims=false),
|
||||||
|
// so ClaimTypes.NameIdentifier is absent at runtime. Check NameIdentifier first (unit tests set it),
|
||||||
|
// then fall back to "sub" (real tokens). Required for the self-ownership guard to work in production.
|
||||||
private string CurrentUserId =>
|
private string CurrentUserId =>
|
||||||
_http.HttpContext?.User.FindFirstValue(ClaimTypes.NameIdentifier) ?? "system";
|
_http.HttpContext?.User.FindFirstValue(ClaimTypes.NameIdentifier)
|
||||||
|
?? _http.HttpContext?.User.FindFirstValue("sub")
|
||||||
|
?? "system";
|
||||||
|
|
||||||
public async Task<PagedResult<ExpenseListItemDto>> GetPagedAsync(
|
public async Task<PagedResult<ExpenseListItemDto>> GetPagedAsync(
|
||||||
int page, int pageSize, string? search, int? ministryId,
|
int page, int pageSize, string? search, int? ministryId,
|
||||||
|
|||||||
@@ -12,8 +12,11 @@ public class MonthlyStatementService : IMonthlyStatementService
|
|||||||
private readonly IHttpContextAccessor _http;
|
private readonly IHttpContextAccessor _http;
|
||||||
public MonthlyStatementService(AppDbContext db, IHttpContextAccessor http) { _db = db; _http = http; }
|
public MonthlyStatementService(AppDbContext db, IHttpContextAccessor http) { _db = db; _http = http; }
|
||||||
|
|
||||||
|
// See ExpenseService: the user id lives in the "sub" claim at runtime; NameIdentifier is for tests.
|
||||||
private string CurrentUserId =>
|
private string CurrentUserId =>
|
||||||
_http.HttpContext?.User.FindFirstValue(ClaimTypes.NameIdentifier) ?? "system";
|
_http.HttpContext?.User.FindFirstValue(ClaimTypes.NameIdentifier)
|
||||||
|
?? _http.HttpContext?.User.FindFirstValue("sub")
|
||||||
|
?? "system";
|
||||||
|
|
||||||
public async Task<List<MonthlyStatementDto>> GetAllAsync(int? year)
|
public async Task<List<MonthlyStatementDto>> GetAllAsync(int? year)
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user