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..c48b25439f 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,107 @@ 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 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", + } 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"); 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/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 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/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/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/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..937ff727e2 100644 --- a/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs +++ b/src/Tests/Integration.Tests/Tests/Users/UserProfileTests.cs @@ -113,6 +113,254 @@ 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 — 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); + + 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); + dto.FirstName.ShouldBe("Concurrent"); + } + + [Fact] + 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"); + 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]