From 97909222f2bb5ed0fe5a4571851e182e05ecadf5 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 01:43:15 -0300 Subject: [PATCH 1/6] build: pin SSH.NET to 2026.0.0 so restore passes while #1333 is open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NU1903` / `GHSA-q939-rpr3-3284` on `SSH.NET` 2025.1.0, pulled transitively by Testcontainers, fails `restore` for the whole solution under `TreatWarningsAsErrors` — on `main` too. It is not introduced here and the fix belongs to #1333, which is still open. Carried byte-identical to #1333's version of the file, comment included, so both stay mergeable in either order and this copy can simply be dropped once #1333 lands. --- src/Directory.Packages.props | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 0d38b28190..7674befa8f 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -143,5 +143,12 @@ AccessViolation). Transitive pinning is enabled, so this entry alone bumps it. Remove once the SignalR backplane package depends on a patched version itself. --> + + \ No newline at end of file From a2815d2f2f654267e8c9ecfe36424e3a860c6229 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 06:59:04 -0300 Subject: [PATCH 2/6] fix(identity): reject a stale profile update instead of losing the write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `PUT /identity/profile` is a full-representation update: every field is assigned from the request, so a caller working from a stale read blanks whatever changed in between. Nothing on the request said which version the caller had edited, so the server could not tell a deliberate overwrite from a lost update and accepted both. `AspNetUsers.ConcurrencyStamp` is already mapped as an EF concurrency token and Identity's store rotates it on every `UserManager.UpdateAsync`, so the version marker exists — it just was not on the wire. `GET /identity/profile` now publishes it as a strong `ETag`, and `PUT /identity/profile` honours `If-Match`: a token that no longer matches gets `412 Precondition Failed` instead of silently winning. No migration and no schema change. The header stays optional — absent means today's behaviour, so existing clients keep working. A `ponytail:` comment marks the future path where it becomes required and a missing header answers `428`. Details worth calling out: - The precondition is checked immediately after the user is loaded, before the storage calls. Any later and a rejected update would already have uploaded an orphan blob or, on the `deleteCurrentImage` path, deleted the avatar for a request that then fails and changes nothing in the database. - `IdentityResult`'s `ConcurrencyFailure` is mapped to the same 412. Identity's store returns it rather than throwing, so a race lost one layer down used to surface as a generic 500. - `RefreshSignInAsync` now runs after the success guard. It used to refresh the sign-in even when the update had failed. - `*` in `If-Match` asks only that the resource exist. Weak validators can never satisfy the strong comparison the header mandates, so they answer 412. A malformed header answers 400: 412 would send a client into a refetch-and-retry loop it can never win, since the broken header is its own bug. Tests: integration coverage for the ETag shape, matching/stale/list/`*`/weak/ malformed preconditions, token rotation and the avatar-survives-412 case, plus a handler unit test that the tokens reach the service. --- .../DTOs/UserDto.cs | 12 +- .../Services/IUserProfileService.cs | 6 +- .../Services/IUserService.cs | 2 +- .../v1/Users/UpdateUser/UpdateUserCommand.cs | 13 ++ .../GetUserProfile/GetUserProfileEndpoint.cs | 18 +- .../UpdateUser/UpdateUserCommandHandler.cs | 1 + .../v1/Users/UpdateUser/UpdateUserEndpoint.cs | 46 +++- .../Services/UserProfileService.cs | 46 +++- .../Modules.Identity/Services/UserService.cs | 4 +- .../Handlers/UpdateUserCommandHandlerTests.cs | 31 ++- .../Tests/Users/UserProfileTests.cs | 219 ++++++++++++++++++ 11 files changed, 381 insertions(+), 17 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs index 0ccd71384e..24c8094ab5 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs @@ -1,4 +1,6 @@ -namespace FSH.Modules.Identity.Contracts.DTOs; +using System.Text.Json.Serialization; + +namespace FSH.Modules.Identity.Contracts.DTOs; public class UserDto { @@ -22,4 +24,12 @@ public class UserDto /// Whether the user has enrolled in TOTP-based two-factor authentication. public bool TwoFactorEnabled { get; set; } + + /// + /// The stored optimistic-concurrency token for this user, populated only by the self-profile + /// read. It never reaches the response body — GET /identity/profile turns it into the + /// response's ETag, and that header is the token clients echo back in If-Match. + /// + [JsonIgnore] + public string? ConcurrencyStamp { get; set; } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs index f305b4a782..4b6ce7c26e 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs @@ -24,9 +24,11 @@ public interface IUserProfileService Task GetCountAsync(CancellationToken cancellationToken); /// - /// Updates a user's profile. + /// Updates a user's profile. When is non-null the + /// update is rejected with unless the + /// stored concurrency token matches one of the entries — the caller edited a stale copy. /// - Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default); + Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default); /// /// Sets the profile image URL directly (no upload). Used by the presigned-upload flow: diff --git a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs index 91ab3467fa..b365a46e89 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserService.cs @@ -15,7 +15,7 @@ public interface IUserService Task ToggleStatusAsync(bool activateUser, string userId, CancellationToken cancellationToken); Task GetOrCreateFromPrincipalAsync(ClaimsPrincipal principal, CancellationToken cancellationToken = default); Task RegisterAsync(string firstName, string lastName, string email, string userName, string password, string confirmPassword, string phoneNumber, string origin, CancellationToken cancellationToken); - Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default); + Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default); Task DeleteAsync(string userId, CancellationToken cancellationToken = default); Task ConfirmEmailAsync(string userId, string code, string tenant, CancellationToken cancellationToken); Task AdminConfirmEmailAsync(string userId, CancellationToken cancellationToken = default); diff --git a/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs b/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs index 09292a46bc..1299b88100 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/v1/Users/UpdateUser/UpdateUserCommand.cs @@ -1,5 +1,6 @@ using FSH.Framework.Shared.Storage; using Mediator; +using System.Text.Json.Serialization; namespace FSH.Modules.Identity.Contracts.v1.Users.UpdateUser; @@ -12,4 +13,16 @@ public class UpdateUserCommand : ICommand public string? Email { get; set; } public FileUploadRequest? Image { get; set; } public bool DeleteCurrentImage { get; set; } + + /// + /// Concurrency tokens the caller is willing to overwrite, taken from the request's + /// If-Match header by the endpoint. means the caller sent no + /// precondition and accepts whatever version is stored; a non-null list means the update + /// only proceeds when the stored token matches one of the entries. + /// + /// + /// Header-derived, never read from the request body — the endpoint always overwrites it. + /// + [JsonIgnore] + public IReadOnlyList? ExpectedConcurrencyStamps { get; set; } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs index c6038cdbb9..4fe544d89b 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/GetUserProfile/GetUserProfileEndpoint.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; +using Microsoft.Net.Http.Headers; using System.Security.Claims; namespace FSH.Modules.Identity.Features.v1.Users.GetUserProfile; @@ -14,18 +15,29 @@ public static class GetUserProfileEndpoint { internal static RouteHandlerBuilder MapGetMeEndpoint(this IEndpointRouteBuilder endpoints) { - return endpoints.MapGet("/profile", async (ClaimsPrincipal user, IMediator mediator, CancellationToken cancellationToken) => + return endpoints.MapGet("/profile", async (ClaimsPrincipal user, HttpResponse response, IMediator mediator, CancellationToken cancellationToken) => { if (user.GetUserId() is not { } userId || string.IsNullOrEmpty(userId)) { throw new UnauthorizedException(); } - return TypedResults.Ok(await mediator.Send(new GetCurrentUserProfileQuery(userId), cancellationToken)); + var profile = await mediator.Send(new GetCurrentUserProfileQuery(userId), cancellationToken); + + // The profile is a full-representation resource: PUT /profile rewrites every field, so + // a caller editing a stale copy would blank whatever changed meanwhile. Publishing the + // stored concurrency token as a strong ETag lets that caller echo it back in If-Match + // and have the server reject the stale write. + if (!string.IsNullOrEmpty(profile.ConcurrencyStamp)) + { + response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ConcurrencyStamp}\"", isWeak: false).ToString(); + } + + return TypedResults.Ok(profile); }) .WithName("GetCurrentUserProfile") .WithSummary("Get current user profile") - .WithDescription("Retrieve the authenticated user's profile from the access token.") + .WithDescription("Retrieve the authenticated user's profile from the access token. The response carries a strong ETag — echo it in If-Match on PUT /identity/profile to reject a lost update.") .RequireAuthorization() .Produces(StatusCodes.Status200OK) .Produces(StatusCodes.Status401Unauthorized); diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs index 9b6608e03a..61bbf01cf0 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserCommandHandler.cs @@ -24,6 +24,7 @@ await _userService.UpdateAsync( command.PhoneNumber ?? string.Empty, command.Image!, command.DeleteCurrentImage, + command.ExpectedConcurrencyStamps, cancellationToken).ConfigureAwait(false); return Unit.Value; diff --git a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs index 68751c8654..7ebeba09f8 100644 --- a/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs +++ b/src/Modules/Identity/Modules.Identity/Features/v1/Users/UpdateUser/UpdateUserEndpoint.cs @@ -6,6 +6,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Routing; +using Microsoft.Net.Http.Headers; using System.Security.Claims; namespace FSH.Modules.Identity.Features.v1.Users.UpdateUser; @@ -14,7 +15,7 @@ public static class UpdateUserEndpoint { internal static RouteHandlerBuilder MapUpdateUserEndpoint(this IEndpointRouteBuilder endpoints) { - return endpoints.MapPut("/profile", async ([FromBody] UpdateUserCommand request, ClaimsPrincipal user, IMediator mediator, CancellationToken cancellationToken) => + return endpoints.MapPut("/profile", async ([FromBody] UpdateUserCommand request, ClaimsPrincipal user, HttpRequest httpRequest, IMediator mediator, CancellationToken cancellationToken) => { if (user.GetUserId() is not { } userId || string.IsNullOrEmpty(userId)) { @@ -25,15 +26,54 @@ internal static RouteHandlerBuilder MapUpdateUserEndpoint(this IEndpointRouteBui // only, regardless of any id the caller supplied in the body. request.Id = userId; + // Header-derived, so it overwrites whatever the body carried. + request.ExpectedConcurrencyStamps = ReadExpectedConcurrencyStamps(httpRequest); + await mediator.Send(request, cancellationToken); return TypedResults.Ok(); }) .WithName("UpdateUserProfile") .WithSummary("Update user profile") .RequireAuthorization() - .WithDescription("Update profile details for the authenticated user. Any signed-in user may edit their own profile; no admin permission required.") + .WithDescription("Update profile details for the authenticated user. Any signed-in user may edit their own profile; no admin permission required. Echo the ETag from GET /identity/profile in If-Match and a stale full-representation update is rejected with 412 instead of silently overwriting a concurrent change.") .Produces(StatusCodes.Status200OK) .Produces(StatusCodes.Status401Unauthorized) - .Produces(StatusCodes.Status400BadRequest); + .Produces(StatusCodes.Status400BadRequest) + .Produces(StatusCodes.Status412PreconditionFailed); + } + + /// + /// Turns the request's If-Match header into the set of concurrency tokens the caller is + /// willing to overwrite. Returns when there is no precondition to + /// enforce: either the header is absent, or it is *, which asks only that the resource + /// exist — and it does, or the update answers 404 on its own. + /// + private static List? ReadExpectedConcurrencyStamps(HttpRequest request) + { + var ifMatch = request.Headers.IfMatch; + if (ifMatch.Count == 0) + { + return null; + } + + if (!EntityTagHeaderValue.TryParseStrictList(ifMatch, out var entityTags)) + { + // Answering 412 would send a well-behaved client into a refetch-and-retry loop it can + // never win, since the malformed header is its own bug. 400 names the bug instead. + throw new BadHttpRequestException("The If-Match header is not a valid entity-tag list."); + } + + if (entityTags.Contains(EntityTagHeaderValue.Any)) + { + return null; + } + + // If-Match mandates the strong comparison function, so a weak validator can never match. + // Dropping the weak entries leaves a list no stored token matches, which is exactly the + // 412 the RFC asks for. + return entityTags + .Where(entityTag => !entityTag.IsWeak) + .Select(entityTag => entityTag.Tag.ToString().Trim('"')) + .ToList(); } } \ No newline at end of file diff --git a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs index c96c90384b..b984fbd571 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -12,6 +12,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Options; +using System.Net; namespace FSH.Modules.Identity.Services; @@ -21,6 +22,7 @@ internal sealed class UserProfileService( IStorageService storageService, IMultiTenantContextAccessor multiTenantContextAccessor, IOptions originOptions, + IdentityErrorDescriber errorDescriber, IHttpContextAccessor httpContextAccessor) : IUserProfileService { private readonly Uri? _originUrl = originOptions.Value.OriginUrl; @@ -48,6 +50,7 @@ public async Task GetAsync(string userId, CancellationToken cancellatio EmailConfirmed = user.EmailConfirmed, PhoneNumber = user.PhoneNumber, TwoFactorEnabled = user.TwoFactorEnabled, + ConcurrencyStamp = user.ConcurrencyStamp, }; } @@ -75,12 +78,18 @@ public async Task> GetListAsync(CancellationToken cancellationToke return result; } - public async Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default) + public async Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default) { var user = await userManager.FindByIdAsync(userId); _ = user ?? throw new NotFoundException("user not found"); + // This is a full-representation update, so a caller working from a stale read would + // silently blank whatever changed since. The precondition is checked here, before the + // storage calls below: a rejected update must not leave an orphan upload behind, and on + // the deleteCurrentImage path it must not remove the avatar with no database change. + EnsureConcurrencyStampMatches(user, expectedConcurrencyStamps); + Uri imageUri = user.ImageUrl ?? null!; // image is optional: text-only edits forward a null FileUploadRequest, so guard before // dereferencing Data or the common no-image update path NREs. @@ -108,14 +117,47 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, } var result = await userManager.UpdateAsync(user); - await signInManager.RefreshSignInAsync(user); if (!result.Succeeded) { + // Identity's store answers a lost race with ConcurrencyFailure instead of throwing, + // so it would otherwise surface as a generic 500. It is the same condition the + // If-Match check above reports, just detected one layer down: another writer landed + // between our read and our save. + if (result.Errors.Any(error => string.Equals(error.Code, errorDescriber.ConcurrencyFailure().Code, StringComparison.Ordinal))) + { + throw StaleProfileException(); + } + throw new CustomException("Update profile failed"); } + + await signInManager.RefreshSignInAsync(user); + } + + private static void EnsureConcurrencyStampMatches(FshUser user, IReadOnlyList? expectedConcurrencyStamps) + { + // A null list means the caller sent no If-Match and accepts the stored version as-is. + // ponytail: keep the precondition optional for backward compatibility; a future major can + // require it and answer 428 Precondition Required when the header is missing. + if (expectedConcurrencyStamps is null) + { + return; + } + + var storedStamp = user.ConcurrencyStamp; + if (storedStamp is null || !expectedConcurrencyStamps.Contains(storedStamp, StringComparer.Ordinal)) + { + throw StaleProfileException(); + } } + private static CustomException StaleProfileException() => + new( + "The profile changed since you loaded it. Reload it and apply your changes again.", + errors: null, + HttpStatusCode.PreconditionFailed); + public async Task SetImageUrlAsync(string userId, string? imageUrl, CancellationToken cancellationToken) { EnsureValidTenant(); diff --git a/src/Modules/Identity/Modules.Identity/Services/UserService.cs b/src/Modules/Identity/Modules.Identity/Services/UserService.cs index e11963512f..d2797a1cd0 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserService.cs @@ -55,8 +55,8 @@ public Task> GetListAsync(CancellationToken cancellationToken) public Task GetCountAsync(CancellationToken cancellationToken) => profileService.GetCountAsync(cancellationToken); - public Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, CancellationToken cancellationToken = default) - => profileService.UpdateAsync(userId, firstName, lastName, phoneNumber, image, deleteCurrentImage, cancellationToken); + public Task UpdateAsync(string userId, string firstName, string lastName, string phoneNumber, FileUploadRequest image, bool deleteCurrentImage, IReadOnlyList? expectedConcurrencyStamps, CancellationToken cancellationToken = default) + => profileService.UpdateAsync(userId, firstName, lastName, phoneNumber, image, deleteCurrentImage, expectedConcurrencyStamps, cancellationToken); public Task ExistsWithEmailAsync(string email, string? exceptId = null, CancellationToken cancellationToken = default) => profileService.ExistsWithEmailAsync(email, exceptId, cancellationToken); diff --git a/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs b/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs index f89478916a..b7e6980f85 100644 --- a/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs +++ b/src/Tests/Identity.Tests/Handlers/UpdateUserCommandHandlerTests.cs @@ -39,7 +39,31 @@ await _userService.Received(1).UpdateAsync( command.LastName ?? string.Empty, command.PhoneNumber ?? string.Empty, command.Image!, - command.DeleteCurrentImage); + command.DeleteCurrentImage, + command.ExpectedConcurrencyStamps); + } + + [Fact] + public async Task Handle_Should_ForwardExpectedConcurrencyStamps_When_CallerSentIfMatch() + { + // Arrange — the endpoint fills ExpectedConcurrencyStamps from the If-Match header; the + // handler has to carry it through or the precondition is silently dropped. + var command = _fixture.Create(); + var stamps = new List { "stamp-a", "stamp-b" }; + command.ExpectedConcurrencyStamps = stamps; + + // Act + await _sut.Handle(command, CancellationToken.None); + + // Assert + await _userService.Received(1).UpdateAsync( + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Any(), + Arg.Is?>(actual => actual != null && actual.SequenceEqual(stamps))); } [Fact] @@ -66,7 +90,8 @@ await _userService.Received(1).UpdateAsync( string.Empty, string.Empty, null!, - true); + true, + null); } [Fact] @@ -83,7 +108,7 @@ public async Task Handle_Should_ThrowException_When_UserServiceThrows() // Arrange var command = _fixture.Create(); var expectedExceptionMessage = "Update failed"; - _userService.UpdateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + _userService.UpdateAsync(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any?>()) .Returns(x => throw new InvalidOperationException(expectedExceptionMessage)); // Act & Assert diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index f999e85300..272f0fe7dd 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -113,6 +113,225 @@ public async Task UpdateProfile_Should_Return400_When_PhoneNumberExceedsMaxLengt #endregion + #region Optimistic concurrency (ETag / If-Match) + + [Fact] + public async Task GetProfile_Should_ReturnStrongETag_When_ProfileIsRead() + { + // Arrange + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-read"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + + // Assert — If-Match mandates strong comparison, so the tag must not be weak. + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + response.Headers.ETag!.IsWeak.ShouldBeFalse(); + response.Headers.ETag.Tag.ShouldStartWith("\""); + response.Headers.ETag.Tag.ShouldEndWith("\""); + } + + [Fact] + public async Task UpdateProfile_Should_PersistAndRotateETag_When_IfMatchMatches() + { + // Arrange + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-match"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Matched" }, etag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var reread = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await reread.DeserializeAsync(); + dto.FirstName.ShouldBe("Matched"); + + // The token has to move, or a second save built from the same snapshot would be accepted. + reread.Headers.ETag!.ToString().ShouldNotBe(etag); + var replay = await PutProfileAsync(userClient, new { firstName = "Replayed" }, etag); + replay.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + } + + [Fact] + public async Task UpdateProfile_Should_Return412AndKeepConcurrentChange_When_IfMatchIsStale() + { + // Arrange — the lost update itself: a caller reads, someone else writes, and the caller's + // full-representation PUT would otherwise echo every old value back over that write. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-stale"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + var staleETag = await ReadProfileETagAsync(userClient); + + // A concurrent writer lands between that read and the write below. + var concurrent = await PutProfileAsync( + userClient, + new { firstName = "Concurrent", lastName = "Winner", phoneNumber = "5550001111" }, + ifMatch: null); + concurrent.StatusCode.ShouldBe(HttpStatusCode.OK); + + // Act — the first caller saves the snapshot it loaded before that write. + var response = await PutProfileAsync( + userClient, + new { firstName = "Stale", lastName = "Loser", phoneNumber = "5559998888" }, + staleETag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Concurrent"); + dto.LastName.ShouldBe("Winner"); + dto.PhoneNumber.ShouldBe("5550001111"); + } + + [Fact] + public async Task UpdateProfile_Should_Succeed_When_IfMatchIsAny() + { + // Arrange — `*` asks only that the resource exist, so it must not block the update. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-any"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Wildcard" }, "*"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Wildcard"); + } + + [Fact] + public async Task UpdateProfile_Should_Return412_When_IfMatchIsWeak() + { + // Arrange — a weak validator can never satisfy the strong comparison If-Match requires, + // even when the tag it carries is the current one. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-weak"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Weak" }, $"W/{etag}"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldNotBe("Weak"); + } + + [Theory] + [InlineData("not-an-entity-tag")] + [InlineData("\"unterminated")] + public async Task UpdateProfile_Should_Return400_When_IfMatchIsMalformed(string ifMatch) + { + // Arrange — a malformed header is the client's own bug. 412 would send it into a + // refetch-and-retry loop it can never win, so the request is rejected as a bad request. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-bad"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Malformed" }, ifMatch); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.BadRequest); + } + + [Fact] + public async Task UpdateProfile_Should_Succeed_When_IfMatchListContainsCurrentETag() + { + // Arrange — If-Match takes a list; matching any entry is enough. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-list"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "Listed" }, $"\"someone-elses-tag\", {etag}"); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("Listed"); + } + + [Fact] + public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequested() + { + // Arrange — the precondition is checked before the storage calls run. Checking it any later + // would delete the avatar (and orphan uploads) on a request that then answers 412 and + // changes nothing in the database. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-image"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + const string imageUrl = "https://cdn.example.com/avatars/keep-me.png"; + var setImage = await userClient.PutAsJsonAsync( + $"{TestConstants.IdentityBasePath}/profile/image", new { imageUrl }); + setImage.StatusCode.ShouldBe(HttpStatusCode.NoContent); + + var staleETag = await ReadProfileETagAsync(userClient); + var concurrent = await PutProfileAsync(userClient, new { firstName = "Concurrent" }, ifMatch: null); + concurrent.StatusCode.ShouldBe(HttpStatusCode.OK); + + // Act + var response = await PutProfileAsync( + userClient, + new { firstName = "Stale", deleteCurrentImage = true }, + staleETag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.PreconditionFailed); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.ImageUrl.ShouldBe(imageUrl); + } + + private static async Task ReadProfileETagAsync(HttpClient client) + { + var response = await client.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + return response.Headers.ETag!.ToString(); + } + + private static async Task PutProfileAsync(HttpClient client, object body, string? ifMatch) + { + using var request = new HttpRequestMessage( + HttpMethod.Put, + $"{TestConstants.IdentityBasePath}/profile") + { + Content = JsonContent.Create(body) + }; + + if (ifMatch is not null) + { + // Unvalidated on purpose: the malformed-header cases have to reach the server. + request.Headers.TryAddWithoutValidation("If-Match", ifMatch); + } + + return await client.SendAsync(request); + } + + #endregion + #region SetProfileImage (PUT /profile/image) [Fact] From c35d13d8b21e5bb81be7df3b96bbb2da6753f75d Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:35:13 -0300 Subject: [PATCH 3/6] test(identity): assert a rejected profile update leaves no partial write The avatar case only checked the image URL. `SetPhoneNumberAsync` persists on its own, ahead of the final `UserManager.UpdateAsync`, so a precondition checked too late would let a field through on a request that then answers 412. Asserting the name as well pins that down, and the comment now says what the test proves rather than claiming the storage call itself is observed. --- .../Integration.Tests/Tests/Users/UserProfileTests.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 272f0fe7dd..86a5123e02 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -274,9 +274,10 @@ public async Task UpdateProfile_Should_Succeed_When_IfMatchListContainsCurrentET [Fact] public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCurrentImageRequested() { - // Arrange — the precondition is checked before the storage calls run. Checking it any later - // would delete the avatar (and orphan uploads) on a request that then answers 412 and - // changes nothing in the database. + // Arrange — a rejected delete-my-avatar request must leave the profile exactly as it was. + // The precondition runs as the first statement after the user is loaded, ahead of the + // storage calls and of SetPhoneNumberAsync (which persists on its own), so a 412 cannot + // leave a half-applied update behind. using var adminClient = await _auth.CreateRootAdminClientAsync(); var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-image"); using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); @@ -302,6 +303,7 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); var dto = await profile.DeserializeAsync(); dto.ImageUrl.ShouldBe(imageUrl); + dto.FirstName.ShouldBe("Concurrent"); } private static async Task ReadProfileETagAsync(HttpClient client) From e4650e4649f4b25d730fe8a2c6eb3589e8ca3c7b Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:42:20 -0300 Subject: [PATCH 4/6] fix(dashboard): send If-Match when saving the profile, retry once on 412 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updateMyProfile` reads the profile, merges the edited fields and PUTs the whole representation back. Nothing tied that write to the version it was built from, so a concurrent change — another tab, a phone, a slow save racing a fast one — was silently overwritten. The read now also picks up the profile's `ETag` and the PUT echoes it in `If-Match`, so the server can answer 412 instead of accepting a stale representation. A 412 is retried once from a fresh read: the token rotates on writes the user never thinks of as profile edits (a password change, a failed sign-in, a new avatar), and turning those into a failed save would be noise. A second 412 propagates. `apiFetch` grew an `onResponse` hook, because it returns the parsed body and there was no way to reach a response header from a caller. Note for anyone running the API on a separate origin (the dev setup does — the page is on 5174 and the API on 7030): `ETag` is not a CORS-safelisted response header, so the browser hides it from JS unless the API also sends `Access-Control-Expose-Headers: ETag`, and `If-Match` has to be an allowed request header. The framework's CORS policy does neither today, which is a separate change in protected code. Until it lands this path degrades to the old behaviour — the client reads no tag and sends no precondition. Same-origin deployments (the shipped `apiBase: ""` default) are unaffected. --- clients/dashboard/src/api/identity.ts | 40 +++++- clients/dashboard/src/lib/api-client.ts | 11 +- .../dashboard/tests/settings/profile.spec.ts | 123 ++++++++++++++++-- 3 files changed, 160 insertions(+), 14 deletions(-) diff --git a/clients/dashboard/src/api/identity.ts b/clients/dashboard/src/api/identity.ts index 3297c8c18a..d2ee87eb31 100644 --- a/clients/dashboard/src/api/identity.ts +++ b/clients/dashboard/src/api/identity.ts @@ -1,4 +1,4 @@ -import { apiFetch } from "@/lib/api-client"; +import { apiFetch, ApiRequestError } from "@/lib/api-client"; import type { PagedResponse } from "@/api/catalog"; // ----------------------------- @@ -396,17 +396,53 @@ export type UpdateProfileInput = { phoneNumber?: string | null; }; +/** + * Reads the profile along with the ETag the server publishes for it. The tag is the + * profile's version marker: echoing it back in `If-Match` on the PUT below is what lets + * the server reject a save built from a snapshot someone else has since changed. + */ +async function readProfileWithETag(): Promise<{ profile: UserDto; etag: string | null }> { + let etag: string | null = null; + const profile = await apiFetch("/api/v1/identity/profile", { + onResponse: (response) => { + etag = response.headers.get("ETag"); + }, + }); + return { profile, etag }; +} + /** * Updates the authenticated user's profile. Maps to UpdateUserCommand * server-side. Image and email changes go through their own dedicated * endpoints — this is for the editable profile fields surfaced in * settings/profile. Reads the current profile first so unset optional * fields keep their existing values instead of being nulled. + * + * That read-modify-write is why the PUT carries `If-Match`: the server answers 412 when + * the profile moved in between, instead of accepting a full representation built from a + * stale copy and blanking the concurrent change. A 412 is retried once against a fresh + * read, because the token also rotates on writes the user never sees as profile edits (a + * password change, a failed sign-in, a new avatar) and surfacing those as a failed save + * would be noise. A second 412 means the profile is changing faster than this client can + * follow, and the error propagates. */ export async function updateMyProfile(input: UpdateProfileInput): Promise { - const profile = await getMyProfile(); + try { + await putProfileFromFreshRead(input); + } catch (error) { + if (error instanceof ApiRequestError && error.status === 412) { + await putProfileFromFreshRead(input); + return; + } + throw error; + } +} + +async function putProfileFromFreshRead(input: UpdateProfileInput): Promise { + const { profile, etag } = await readProfileWithETag(); await apiFetch(`/api/v1/identity/profile`, { method: "PUT", + headers: etag ? { "If-Match": etag } : undefined, body: JSON.stringify({ id: profile.id, firstName: input.firstName ?? profile.firstName ?? null, diff --git a/clients/dashboard/src/lib/api-client.ts b/clients/dashboard/src/lib/api-client.ts index 417eab0ca8..d83991b80f 100644 --- a/clients/dashboard/src/lib/api-client.ts +++ b/clients/dashboard/src/lib/api-client.ts @@ -73,6 +73,13 @@ type RequestInitEx = RequestInit & { * uploads) should override this explicitly. */ timeoutMs?: number; + /** + * Called with the final response before its body is read, so a caller can pick up a + * response header `apiFetch` does not model — the `ETag` on `GET /identity/profile`, + * which a later `PUT` echoes back in `If-Match`. Runs for error responses too, and + * must not throw. + */ + onResponse?: (response: Response) => void; }; const DEFAULT_TIMEOUT_MS = 30_000; @@ -156,7 +163,7 @@ export async function apiFetch( path: string, init: RequestInitEx = {}, ): Promise { - const { skipAuth, headers, timeoutMs = DEFAULT_TIMEOUT_MS, signal, ...rest } = init; + const { skipAuth, headers, timeoutMs = DEFAULT_TIMEOUT_MS, signal, onResponse, ...rest } = init; const mergedHeaders = new Headers(headers); if (!mergedHeaders.has("Content-Type") && rest.body && typeof rest.body === "string") { @@ -218,6 +225,8 @@ export async function apiFetch( } } + onResponse?.(response); + if (!response.ok) { const problem = await parseError(response); throw new ApiRequestError( diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index 91b97df567..01ef2fff3a 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -2,20 +2,22 @@ import { expect, test } from "@playwright/test"; import { mockJsonResponse, mockProblemDetails } from "../helpers/api-mocks"; import { seedAuthedSession, TEST_USER } from "../helpers/auth-seed"; +const PROFILE = { + id: TEST_USER.sub, + userName: "alice", + email: TEST_USER.email, + firstName: TEST_USER.firstName, + lastName: TEST_USER.lastName, + phoneNumber: "", + isActive: true, + emailConfirmed: true, + twoFactorEnabled: false, +}; + // All settings tests need an authed session and a mocked profile fetch. test.beforeEach(async ({ page }) => { await seedAuthedSession(page, TEST_USER); - await mockJsonResponse(page, "**/api/v1/identity/profile", { - id: TEST_USER.sub, - userName: "alice", - email: TEST_USER.email, - firstName: TEST_USER.firstName, - lastName: TEST_USER.lastName, - phoneNumber: "", - isActive: true, - emailConfirmed: true, - twoFactorEnabled: false, - }); + await mockJsonResponse(page, "**/api/v1/identity/profile", PROFILE); }); test.describe("settings/profile — wired to PUT /identity/profile", () => { @@ -107,6 +109,105 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await expect(page.getByText(/first name cannot be empty/i)).toBeVisible(); }); + // The dashboard talks to the API cross-origin in dev, and `ETag` is not a CORS-safelisted + // response header — the browser hides it from JS unless the server also sends + // `Access-Control-Expose-Headers: ETag`. These mocks send it for the same reason the API + // has to: without it the client reads `null` and silently stops sending `If-Match`. + const ETAG_CORS_HEADERS = { + "Content-Type": "application/json", + "Access-Control-Expose-Headers": "ETag", + } as const; + + test("echoes the profile ETag back as If-Match on save", async ({ page }) => { + const etag = '"stamp-1"'; + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() === "PUT") { + await route.fulfill({ + status: 200, + headers: { "Content-Type": "application/json" }, + body: '""', + }); + return; + } + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: etag }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByLabel("First name").fill("Alicia"); + + const putReqPromise = page.waitForRequest( + (req) => + req.url().includes("/api/v1/identity/profile") && + req.method() === "PUT" && + !req.url().includes("/image"), + { timeout: 5_000 }, + ); + await page.getByRole("button", { name: /save changes/i }).click(); + const putReq = await putReqPromise; + + // Without this the server cannot tell a deliberate overwrite from a lost update. + expect(putReq.headers()["if-match"]).toBe(etag); + }); + + test("refetches and retries once when the save is rejected with 412", async ({ page }) => { + // The token also rotates on writes the user never sees as profile edits (a password + // change, a failed sign-in, a new avatar), so a single 412 has to resolve itself + // against a fresh read instead of surfacing as a failed save. + const sentIfMatch: string[] = []; + let getCount = 0; + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + sentIfMatch.push(request.headers()["if-match"] ?? ""); + if (sentIfMatch.length === 1) { + await route.fulfill({ + status: 412, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ + status: 412, + title: "CustomException", + detail: "The profile changed since you loaded it.", + }), + }); + return; + } + await route.fulfill({ + status: 200, + headers: { "Content-Type": "application/json" }, + body: '""', + }); + return; + } + + // Every read hands out a fresh token, so the retry provably carries a re-read one. + getCount += 1; + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${getCount}"` }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect(page.getByText(/profile saved/i)).toBeVisible(); + await expect(page.getByText(/save failed/i)).toBeHidden(); + expect(sentIfMatch).toHaveLength(2); + expect(sentIfMatch[0]).not.toBe(""); + expect(sentIfMatch[1]).not.toBe(sentIfMatch[0]); + }); + test("Reset button reverts edits to the original profile values", async ({ page }) => { await page.goto("/settings/profile"); await expect(page.getByLabel("First name")).toHaveValue("Alice"); From acd3d5d521423c0a2ae8ae74dd07e2ef68b6a08b Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 07:56:55 -0300 Subject: [PATCH 5/6] test(identity): gate the ETag CORS exposure the front-end depends on The dashboard specs mock `Access-Control-Expose-Headers: ETag`, which the API does not send: `FSH.Framework.Web.Cors` never calls `WithExposedHeaders`. A browser therefore hides the tag from JS on any cross-origin call, the client stops sending `If-Match`, and the endpoint silently falls back to the lost-update behaviour this branch set out to fix -- with every test still green. Assert it instead of describing it in a comment. The test is skipped so the suite stays green until the framework change lands (protected code, needs approval); the skip reason names exactly what has to change to un-skip it. Verified: un-skipped it fails on the missing header; with `WithExposedHeaders("ETag")` added locally to the AllowAll branch it passes. That temporary edit was reverted -- `src/BuildingBlocks` is untouched by this branch. Refs #1359 --- .../Tests/Users/UserProfileTests.cs | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 86a5123e02..b9354e6c10 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -306,6 +306,33 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu dto.FirstName.ShouldBe("Concurrent"); } + [Fact(Skip = "Blocked on CORS: FSH.Framework.Web.Cors never calls WithExposedHeaders, so a browser hides the ETag from JS on a cross-origin call and the precondition silently degrades to the old lost-update behaviour. Drop the Skip once ETag is exposed.")] + public async Task GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead() + { + // Arrange — ETag is not a CORS-safelisted response header, so the contract only reaches a + // front-end if the server also lists it in Access-Control-Expose-Headers. Asserted here + // rather than left as a comment: the front-end specs mock the header, so nothing else in + // the suite notices when the server stops sending it. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-cors"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + + using var request = new HttpRequestMessage(HttpMethod.Get, $"{TestConstants.IdentityBasePath}/profile"); + request.Headers.TryAddWithoutValidation("Origin", "http://localhost:5174"); + + // Act + var response = await userClient.SendAsync(request); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + response.Headers.ETag.ShouldNotBeNull(); + response.Headers.TryGetValues("Access-Control-Expose-Headers", out var exposedHeaders).ShouldBeTrue(); + exposedHeaders! + .SelectMany(value => value.Split(',')) + .Select(value => value.Trim()) + .ShouldContain(value => string.Equals(value, "ETag", StringComparison.OrdinalIgnoreCase)); + } + private static async Task ReadProfileETagAsync(HttpClient client) { var response = await client.GetAsync($"{TestConstants.IdentityBasePath}/profile"); From 7daadf535701d5919975fbb2388e8842072362bd Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:02:30 -0300 Subject: [PATCH 6/6] feat(cors): expose ETag and allow If-Match so clients can use preconditions `ETag` is not a CORS-safelisted response header, so a browser hid it from JS on every cross-origin call -- which is every dev run, since both React apps point `apiBase` at the API's own origin. A front-end that cannot read the validator cannot send `If-Match`, so the optimistic-concurrency precondition on `PUT /identity/profile` degraded straight back to the lost update it exists to prevent, with the whole suite still green. Exposed for both policy branches: neither `AllowAnyHeader` nor `WithHeaders` implies exposure, and the header carries no data of its own, only a validator. `if-match` joins `AllowedHeaders` in both shipped appsettings for the mirror-image reason: with `AllowAll: false` the request header is stripped before it reaches the endpoint. Gates: `CorsPolicyTests` covers both branches at the policy level and `GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead` covers it end to end, so the front-end mocks can no longer hide a server that stops sending the header. Verified by mutation -- dropping the argument turns all three red; restored and re-run green. Refs #1359 --- .../dashboard/tests/settings/profile.spec.ts | 6 ++- src/BuildingBlocks/Web/Cors/Extensions.cs | 7 +++ .../appsettings.Production.json | 2 +- src/Host/FSH.Starter.Api/appsettings.json | 2 +- .../Framework.Tests/Web/CorsPolicyTests.cs | 46 +++++++++++++++++++ .../Tests/Users/UserProfileTests.cs | 2 +- 6 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 src/Tests/Framework.Tests/Web/CorsPolicyTests.cs diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index 01ef2fff3a..c48b25439f 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -111,8 +111,10 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { // The dashboard talks to the API cross-origin in dev, and `ETag` is not a CORS-safelisted // response header — the browser hides it from JS unless the server also sends - // `Access-Control-Expose-Headers: ETag`. These mocks send it for the same reason the API - // has to: without it the client reads `null` and silently stops sending `If-Match`. + // `Access-Control-Expose-Headers: ETag`. These mocks mirror what the CORS policy now sends; + // without it the client reads `null` and silently stops sending `If-Match`. The server side of + // that contract is asserted by `GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead`, + // since a mock alone would keep passing if the policy stopped exposing the header. const ETAG_CORS_HEADERS = { "Content-Type": "application/json", "Access-Control-Expose-Headers": "ETag", diff --git a/src/BuildingBlocks/Web/Cors/Extensions.cs b/src/BuildingBlocks/Web/Cors/Extensions.cs index 6475dc844e..49e1e30177 100644 --- a/src/BuildingBlocks/Web/Cors/Extensions.cs +++ b/src/BuildingBlocks/Web/Cors/Extensions.cs @@ -2,6 +2,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; +using Microsoft.Net.Http.Headers; using System; using AspNetCorsOptions = Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions; @@ -53,6 +54,12 @@ public static IServiceCollection AddHeroCors( .WithMethods(settings.AllowedMethods) .AllowCredentials(); } + + // `ETag` is not a CORS-safelisted response header, so a browser hides it from JS on any + // cross-origin call — and a front-end that cannot read the validator cannot send + // `If-Match`, which degrades an optimistic-concurrency endpoint back to a lost update. + // Exposed for both policies: the header carries no data of its own, only a validator. + builder.WithExposedHeaders(HeaderNames.ETag); }); }); }); diff --git a/src/Host/FSH.Starter.Api/appsettings.Production.json b/src/Host/FSH.Starter.Api/appsettings.Production.json index 332724534b..869b1c5ffc 100644 --- a/src/Host/FSH.Starter.Api/appsettings.Production.json +++ b/src/Host/FSH.Starter.Api/appsettings.Production.json @@ -59,7 +59,7 @@ "CorsOptions": { "AllowAll": false, "AllowedOrigins": [], - "AllowedHeaders": [ "content-type", "authorization" ], + "AllowedHeaders": [ "content-type", "authorization", "if-match" ], "AllowedMethods": [ "GET", "POST", "PUT", "DELETE" ] }, "JwtOptions": { diff --git a/src/Host/FSH.Starter.Api/appsettings.json b/src/Host/FSH.Starter.Api/appsettings.json index 293fdfebb6..527ed10c93 100644 --- a/src/Host/FSH.Starter.Api/appsettings.json +++ b/src/Host/FSH.Starter.Api/appsettings.json @@ -100,7 +100,7 @@ "http://localhost:5173", "http://localhost:5174" ], - "AllowedHeaders": [ "content-type", "authorization" ], + "AllowedHeaders": [ "content-type", "authorization", "if-match" ], "AllowedMethods": [ "GET", "POST", "PUT", "DELETE" ] }, "JwtOptions": { diff --git a/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs b/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs new file mode 100644 index 0000000000..f17ece3e0f --- /dev/null +++ b/src/Tests/Framework.Tests/Web/CorsPolicyTests.cs @@ -0,0 +1,46 @@ +using FSH.Framework.Web.Cors; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using AspNetCorsOptions = Microsoft.AspNetCore.Cors.Infrastructure.CorsOptions; + +namespace Framework.Tests.Web; + +public sealed class CorsPolicyTests +{ + private const string PolicyName = "FSHCorsPolicy"; + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Policy_Should_ExposeETag_When_Built(bool allowAll) + { + // Arrange — ETag is not a CORS-safelisted response header, so a front-end can only read the + // concurrency validator (and answer with If-Match) if the policy exposes it explicitly. + // Both branches are covered: the restricted one builds from configured lists, and neither + // AllowAnyHeader nor WithHeaders implies exposure. + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["CorsOptions:AllowAll"] = allowAll ? "true" : "false", + ["CorsOptions:AllowedOrigins:0"] = "https://app.example.com", + ["CorsOptions:AllowedHeaders:0"] = "content-type", + ["CorsOptions:AllowedMethods:0"] = "GET" + }) + .Build(); + + var services = new ServiceCollection(); + services.AddHeroCors(configuration); + + // Act + var policy = services + .BuildServiceProvider() + .GetRequiredService>() + .Value + .GetPolicy(PolicyName); + + // Assert + policy.ShouldNotBeNull(); + policy!.ExposedHeaders.ShouldContain("ETag"); + } +} diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index b9354e6c10..937ff727e2 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -306,7 +306,7 @@ public async Task UpdateProfile_Should_KeepAvatar_When_IfMatchIsStaleAndDeleteCu dto.FirstName.ShouldBe("Concurrent"); } - [Fact(Skip = "Blocked on CORS: FSH.Framework.Web.Cors never calls WithExposedHeaders, so a browser hides the ETag from JS on a cross-origin call and the precondition silently degrades to the old lost-update behaviour. Drop the Skip once ETag is exposed.")] + [Fact] public async Task GetProfile_Should_ExposeETagToCrossOriginCallers_When_ProfileIsRead() { // Arrange — ETag is not a CORS-safelisted response header, so the contract only reaches a