From 0241abfb16a7008bde1bd383c66671f00b45c937 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:17:43 -0300 Subject: [PATCH 1/2] fix(identity): keep the profile ETag stable across a failed sign-in GET /identity/profile published ASP.NET Identity's ConcurrencyStamp as the ETag, but the stamp rotates on every write to the user row, including the AccessFailedCount bump of a wrong password and its reset on the next good sign-in. An open profile form then got a false 412 on save. The ETag is now a SHA-256 of the fields PUT /profile writes (first name, last name, phone number, stored avatar URL), and the If-Match check compares against that. The store's own ConcurrencyStamp check at save time is unchanged and still maps to 412. Closes #1398 --- .../DTOs/UserDto.cs | 8 +-- .../GetUserProfile/GetUserProfileEndpoint.cs | 8 +-- .../Services/UserProfileService.cs | 26 +++++--- .../Tests/Users/UserProfileTests.cs | 65 +++++++++++++++++++ 4 files changed, 91 insertions(+), 16 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs index 24c8094ab5..90917d0a83 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs @@ -26,10 +26,10 @@ public class UserDto 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. + /// A hash of the profile fields PUT /identity/profile writes, 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; } + public string? ProfileVersion { 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 4fe544d89b..ff229af8f5 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 @@ -26,11 +26,11 @@ internal static RouteHandlerBuilder MapGetMeEndpoint(this IEndpointRouteBuilder // 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)) + // profile version 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.ProfileVersion)) { - response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ConcurrencyStamp}\"", isWeak: false).ToString(); + response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ProfileVersion}\"", isWeak: false).ToString(); } return TypedResults.Ok(profile); diff --git a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs index 27860d5ab5..850d941424 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -10,6 +10,8 @@ using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using System.Net; +using System.Security.Cryptography; +using System.Text.Json; namespace FSH.Modules.Identity.Services; @@ -44,7 +46,7 @@ public async Task GetAsync(string userId, CancellationToken cancellatio EmailConfirmed = user.EmailConfirmed, PhoneNumber = user.PhoneNumber, TwoFactorEnabled = user.TwoFactorEnabled, - ConcurrencyStamp = user.ConcurrencyStamp, + ProfileVersion = ComputeProfileVersion(user), }; } @@ -82,7 +84,7 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, // 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); + EnsureProfileUnchanged(user, expectedConcurrencyStamps); // The old blob is only deleted once the database write has gone through. UpdateAsync can // still lose a race here — the If-Match check above is not the last word, because another @@ -120,9 +122,11 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, 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. + // so it would otherwise surface as a generic 500. The store compares its own + // ConcurrencyStamp, which every write to the row rotates, so a failed sign-in landing + // between our read and our save also ends here, as a 412 with the profile untouched. + // Accepted: the window is this request's own, and re-reading and saving again recovers + // exactly as it does from a real conflict. if (result.Errors.Any(error => string.Equals(error.Code, errorDescriber.ConcurrencyFailure().Code, StringComparison.Ordinal))) { throw StaleProfileException(); @@ -139,7 +143,7 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, await signInManager.RefreshSignInAsync(user); } - private static void EnsureConcurrencyStampMatches(FshUser user, IReadOnlyList? expectedConcurrencyStamps) + private static void EnsureProfileUnchanged(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 @@ -149,13 +153,19 @@ private static void EnsureConcurrencyStampMatches(FshUser user, IReadOnlyList + Convert.ToHexStringLower(SHA256.HashData(JsonSerializer.SerializeToUtf8Bytes( + new[] { user.FirstName, user.LastName, user.PhoneNumber, user.ImageUrl?.OriginalString }))); + private static CustomException StaleProfileException() => new( "The profile changed since you loaded it. Reload it and apply your changes again.", diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 1b5dad3deb..79853ee51e 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -193,6 +193,62 @@ public async Task UpdateProfile_Should_Return412AndKeepConcurrentChange_When_IfM dto.PhoneNumber.ShouldBe("5550001111"); } + [Fact] + public async Task UpdateProfile_Should_Succeed_When_SignInAttemptsLandAfterRead() + { + // Arrange — a wrong password bumps AccessFailedCount and the next good sign-in resets it. + // Both write the user row, but neither touches the profile, so neither may turn the + // caller's open form into a 412. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-signin"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var etag = await ReadProfileETagAsync(userClient); + + var failedSignIn = await AttemptSignInAsync(user.Email, "Wrong-Password-1!"); + failedSignIn.StatusCode.ShouldBe(HttpStatusCode.Unauthorized); + (await ReadProfileETagAsync(userClient)).ShouldBe(etag); + + var succeededSignIn = await AttemptSignInAsync(user.Email, user.Password); + succeededSignIn.StatusCode.ShouldBe(HttpStatusCode.OK); + (await ReadProfileETagAsync(userClient)).ShouldBe(etag); + + // Act + var response = await PutProfileAsync(userClient, new { firstName = "AfterSignIn" }, etag); + + // Assert + response.StatusCode.ShouldBe(HttpStatusCode.OK); + + var profile = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var dto = await profile.DeserializeAsync(); + dto.FirstName.ShouldBe("AfterSignIn"); + } + + [Fact] + public async Task UpdateProfile_Should_Return412_When_AvatarChangedAfterRead() + { + // Arrange — the avatar has its own endpoint, but PUT /profile writes it too, so a change + // made there must still invalidate a form loaded before it. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var user = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-avatar"); + using var userClient = await _auth.CreateAuthenticatedClientAsync(user.Email, user.Password); + var staleETag = await ReadProfileETagAsync(userClient); + + const string imageUrl = "https://cdn.example.com/avatars/set-elsewhere.png"; + var setImage = await userClient.PutAsJsonAsync( + $"{TestConstants.IdentityBasePath}/profile/image", new { imageUrl }); + setImage.StatusCode.ShouldBe(HttpStatusCode.NoContent); + + // 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); + } + [Fact] public async Task UpdateProfile_Should_Succeed_When_IfMatchIsAny() { @@ -341,6 +397,15 @@ private static async Task ReadProfileETagAsync(HttpClient client) return response.Headers.ETag.ToString(); } + private async Task AttemptSignInAsync(string email, string password) + { + using var client = _factory.CreateClient(); + using var request = new HttpRequestMessage(HttpMethod.Post, $"{TestConstants.IdentityBasePath}/token/issue"); + request.Headers.Add("tenant", TestConstants.RootTenantId); + request.Content = JsonContent.Create(new { email, password }); + return await client.SendAsync(request); + } + private static async Task PutProfileAsync(HttpClient client, object body, string? ifMatch) { using var request = new HttpRequestMessage( From 878cfaeb212f90123c3821a70884b3a91ba8cf0a Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:58:05 -0300 Subject: [PATCH 2/2] fix(identity): key the profile ETag with the security stamp A plain SHA-256 of name and phone in a response header lets anyone holding a logged tag confirm a guessed phone number offline. The tag is now an HMAC keyed with the user's SecurityStamp, which never leaves the server. Also drops the dead empty-tag guard on GET /profile and fixes doc comments that still described a stored concurrency token. --- .../Services/IUserProfileService.cs | 2 +- .../v1/Users/UpdateUser/UpdateUserCommand.cs | 2 +- .../GetUserProfile/GetUserProfileEndpoint.cs | 5 +--- .../v1/Users/UpdateUser/UpdateUserEndpoint.cs | 2 +- .../Services/UserProfileService.cs | 13 ++++++--- .../Tests/Users/UserProfileTests.cs | 27 +++++++++++++++++++ 6 files changed, 41 insertions(+), 10 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs index 4b6ce7c26e..4c6c7272d2 100644 --- a/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity.Contracts/Services/IUserProfileService.cs @@ -26,7 +26,7 @@ public interface IUserProfileService /// /// 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. + /// current profile version 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, IReadOnlyList? expectedConcurrencyStamps, 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 1299b88100..9ced170f2a 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 @@ -18,7 +18,7 @@ public class UpdateUserCommand : ICommand /// 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. + /// only proceeds when the current profile version matches one of the entries. /// /// /// Header-derived, never read from the request body — the endpoint always overwrites it. 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 ff229af8f5..66b5d971d3 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 @@ -28,10 +28,7 @@ internal static RouteHandlerBuilder MapGetMeEndpoint(this IEndpointRouteBuilder // a caller editing a stale copy would blank whatever changed meanwhile. Publishing the // profile version 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.ProfileVersion)) - { - response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ProfileVersion}\"", isWeak: false).ToString(); - } + response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ProfileVersion}\"", isWeak: false).ToString(); return TypedResults.Ok(profile); }) 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 7ebeba09f8..5efce4680c 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 @@ -69,7 +69,7 @@ internal static RouteHandlerBuilder MapUpdateUserEndpoint(this IEndpointRouteBui } // 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 + // Dropping the weak entries leaves a list no profile version matches, which is exactly the // 412 the RFC asks for. return entityTags .Where(entityTag => !entityTag.IsWeak) diff --git a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs index 850d941424..8eb6d4abe0 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -11,6 +11,7 @@ using Microsoft.EntityFrameworkCore; using System.Net; using System.Security.Cryptography; +using System.Text; using System.Text.Json; namespace FSH.Modules.Identity.Services; @@ -161,10 +162,16 @@ private static void EnsureProfileUnchanged(FshUser user, IReadOnlyList? // Covers only the fields PUT /profile writes. Identity's ConcurrencyStamp also rotates on // writes that leave the profile alone, such as the failed-sign-in counter, and would turn a - // wrong password typed anywhere into a 412 on an untouched form. + // wrong password typed anywhere into a 412 on an untouched form. Keyed with the SecurityStamp, + // which never leaves the server, because the tag travels in a header proxies and APM log and a + // plain hash of name plus phone is brute-forceable over the small phone-number space. + // ponytail: the stamp also rotates on a password or 2FA change, so those move the tag too; + // swap for a server-wide HMAC key if that ever needs to stop. private static string ComputeProfileVersion(FshUser user) => - Convert.ToHexStringLower(SHA256.HashData(JsonSerializer.SerializeToUtf8Bytes( - new[] { user.FirstName, user.LastName, user.PhoneNumber, user.ImageUrl?.OriginalString }))); + Convert.ToHexStringLower(HMACSHA256.HashData( + Encoding.UTF8.GetBytes(user.SecurityStamp ?? string.Empty), + JsonSerializer.SerializeToUtf8Bytes( + new[] { user.FirstName, user.LastName, user.PhoneNumber, user.ImageUrl?.OriginalString }))); private static CustomException StaleProfileException() => new( diff --git a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs index 79853ee51e..eaecbfad94 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -134,6 +134,33 @@ public async Task GetProfile_Should_ReturnStrongETag_When_ProfileIsRead() response.Headers.ETag.Tag.ShouldEndWith("\""); } + [Fact] + public async Task GetProfile_Should_ReturnDifferentETags_When_TwoUsersHoldTheSameProfileValues() + { + // Arrange — the tag goes out in a header, so it must not be a bare hash of the profile: + // that would let anyone holding a logged tag confirm a guessed name and phone offline. + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var first = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-key-a"); + var second = await IdentityUserSeeder.CreateLoginableUserAsync(_factory, adminClient, "etag-key-b"); + using var firstClient = await _auth.CreateAuthenticatedClientAsync(first.Email, first.Password); + using var secondClient = await _auth.CreateAuthenticatedClientAsync(second.Email, second.Password); + + var sameValues = new { firstName = "Same", lastName = "Person", phoneNumber = "5550002222" }; + (await PutProfileAsync(firstClient, sameValues, ifMatch: null)).StatusCode.ShouldBe(HttpStatusCode.OK); + (await PutProfileAsync(secondClient, sameValues, ifMatch: null)).StatusCode.ShouldBe(HttpStatusCode.OK); + + // Act + var firstRead = await firstClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + var secondRead = await secondClient.GetAsync($"{TestConstants.IdentityBasePath}/profile"); + + // Assert + var firstDto = await firstRead.DeserializeAsync(); + var secondDto = await secondRead.DeserializeAsync(); + (firstDto.FirstName, firstDto.LastName, firstDto.PhoneNumber, firstDto.ImageUrl) + .ShouldBe((secondDto.FirstName, secondDto.LastName, secondDto.PhoneNumber, secondDto.ImageUrl)); + firstRead.Headers.ETag!.ToString().ShouldNotBe(secondRead.Headers.ETag!.ToString()); + } + [Fact] public async Task UpdateProfile_Should_PersistAndRotateETag_When_IfMatchMatches() {