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.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 4fe544d89b..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 @@ -26,12 +26,9 @@ 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)) - { - response.Headers.ETag = new EntityTagHeaderValue($"\"{profile.ConcurrencyStamp}\"", isWeak: false).ToString(); - } + // profile version as a strong ETag lets that caller echo it back in If-Match and have + // the server reject the stale write. + 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 27860d5ab5..8eb6d4abe0 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserProfileService.cs @@ -10,6 +10,9 @@ using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using System.Net; +using System.Security.Cryptography; +using System.Text; +using System.Text.Json; namespace FSH.Modules.Identity.Services; @@ -44,7 +47,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 +85,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 +123,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 +144,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 +154,25 @@ private static void EnsureConcurrencyStampMatches(FshUser user, IReadOnlyList + 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( "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..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() { @@ -193,6 +220,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 +424,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(