Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions clients/dashboard/src/api/identity.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { apiFetch } from "@/lib/api-client";
import { apiFetch, ApiRequestError } from "@/lib/api-client";
import type { PagedResponse } from "@/api/catalog";

// -----------------------------
Expand Down Expand Up @@ -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<UserDto>("/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<void> {
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<void> {
const { profile, etag } = await readProfileWithETag();
await apiFetch<unknown>(`/api/v1/identity/profile`, {
method: "PUT",
headers: etag ? { "If-Match": etag } : undefined,
body: JSON.stringify({
id: profile.id,
firstName: input.firstName ?? profile.firstName ?? null,
Expand Down
11 changes: 10 additions & 1 deletion clients/dashboard/src/lib/api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -156,7 +163,7 @@ export async function apiFetch<T = unknown>(
path: string,
init: RequestInitEx = {},
): Promise<T> {
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") {
Expand Down Expand Up @@ -218,6 +225,8 @@ export async function apiFetch<T = unknown>(
}
}

onResponse?.(response);

if (!response.ok) {
const problem = await parseError(response);
throw new ApiRequestError(
Expand Down
125 changes: 114 additions & 11 deletions clients/dashboard/tests/settings/profile.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down Expand Up @@ -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");
Expand Down
7 changes: 7 additions & 0 deletions src/BuildingBlocks/Web/Cors/Extensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
});
});
});
Expand Down
7 changes: 7 additions & 0 deletions src/Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -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. -->
<PackageVersion Include="MessagePack" Version="2.5.301" />
<!-- Pulled transitively by the Testcontainers packages; versions up to 2025.1.0 fail
NuGet audit (NU1903, GHSA-q939-rpr3-3284 / CVE-2026-48798: ScpClient recursive
download writes outside the target directory), which breaks restore for the whole
solution under TreatWarningsAsErrors. Testcontainers 4.11.0 and 4.13.0 both depend
on 2025.1.0, so bumping Testcontainers does not help; 2026.0.0 is the first patched
release. Remove once Testcontainers depends on a patched version itself. -->
<PackageVersion Include="SSH.NET" Version="2026.0.0" />
</ItemGroup>
</Project>
2 changes: 1 addition & 1 deletion src/Host/FSH.Starter.Api/appsettings.Production.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"CorsOptions": {
"AllowAll": false,
"AllowedOrigins": [],
"AllowedHeaders": [ "content-type", "authorization" ],
"AllowedHeaders": [ "content-type", "authorization", "if-match" ],
"AllowedMethods": [ "GET", "POST", "PUT", "DELETE" ]
},
"JwtOptions": {
Expand Down
2 changes: 1 addition & 1 deletion src/Host/FSH.Starter.Api/appsettings.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
12 changes: 11 additions & 1 deletion src/Modules/Identity/Modules.Identity.Contracts/DTOs/UserDto.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
namespace FSH.Modules.Identity.Contracts.DTOs;
using System.Text.Json.Serialization;

namespace FSH.Modules.Identity.Contracts.DTOs;

public class UserDto
{
Expand All @@ -22,4 +24,12 @@ public class UserDto

/// <summary>Whether the user has enrolled in TOTP-based two-factor authentication.</summary>
public bool TwoFactorEnabled { get; set; }

/// <summary>
/// The stored optimistic-concurrency token for this user, populated only by the self-profile
/// read. It never reaches the response body — <c>GET /identity/profile</c> turns it into the
/// response's <c>ETag</c>, and that header is the token clients echo back in <c>If-Match</c>.
/// </summary>
[JsonIgnore]
public string? ConcurrencyStamp { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,11 @@ public interface IUserProfileService
Task<int> GetCountAsync(CancellationToken cancellationToken);

/// <summary>
/// Updates a user's profile.
/// Updates a user's profile. When <paramref name="expectedConcurrencyStamps"/> is non-null the
/// update is rejected with <see cref="System.Net.HttpStatusCode.PreconditionFailed"/> unless the
/// stored concurrency token matches one of the entries — the caller edited a stale copy.
/// </summary>
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<string>? expectedConcurrencyStamps, CancellationToken cancellationToken = default);

/// <summary>
/// Sets the profile image URL directly (no upload). Used by the presigned-upload flow:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public interface IUserService
Task ToggleStatusAsync(bool activateUser, string userId, CancellationToken cancellationToken);
Task<string> GetOrCreateFromPrincipalAsync(ClaimsPrincipal principal, CancellationToken cancellationToken = default);
Task<string> 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<string>? expectedConcurrencyStamps, CancellationToken cancellationToken = default);
Task DeleteAsync(string userId, CancellationToken cancellationToken = default);
Task<string> ConfirmEmailAsync(string userId, string code, string tenant, CancellationToken cancellationToken);
Task AdminConfirmEmailAsync(string userId, CancellationToken cancellationToken = default);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using FSH.Framework.Shared.Storage;
using Mediator;
using System.Text.Json.Serialization;

namespace FSH.Modules.Identity.Contracts.v1.Users.UpdateUser;

Expand All @@ -12,4 +13,16 @@ public class UpdateUserCommand : ICommand<Unit>
public string? Email { get; set; }
public FileUploadRequest? Image { get; set; }
public bool DeleteCurrentImage { get; set; }

/// <summary>
/// Concurrency tokens the caller is willing to overwrite, taken from the request's
/// <c>If-Match</c> header by the endpoint. <see langword="null"/> 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.
/// </summary>
/// <remarks>
/// Header-derived, never read from the request body — the endpoint always overwrites it.
/// </remarks>
[JsonIgnore]
public IReadOnlyList<string>? ExpectedConcurrencyStamps { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<UserDto>(StatusCodes.Status200OK)
.Produces(StatusCodes.Status401Unauthorized);
Expand Down
Loading
Loading