diff --git a/clients/dashboard/src/api/identity.ts b/clients/dashboard/src/api/identity.ts index 3297c8c18a..baa860057f 100644 --- a/clients/dashboard/src/api/identity.ts +++ b/clients/dashboard/src/api/identity.ts @@ -146,10 +146,6 @@ export async function getMyPermissions(): Promise { return (await apiFetch(`/api/v1/identity/permissions`)) ?? []; } -export async function getMyProfile(): Promise { - return apiFetch("/api/v1/identity/profile"); -} - export async function registerUser(input: RegisterUserInput): Promise { return apiFetch(`/api/v1/identity/register`, { method: "POST", @@ -391,22 +387,61 @@ export async function endImpersonation(): Promise { // ----------------------------- export type UpdateProfileInput = { - firstName?: string | null; - lastName?: string | null; - phoneNumber?: string | null; + /** + * The profile the form was seeded from, and the ETag that read carried. Both come from the + * caller rather than from a read inside the save: the lost update this guards against happens + * between the moment the user saw the values and the moment they press save, so a tag fetched + * inside the save has no chance of being stale and no chance of catching anything. + */ + profile: UserDto; + expectedETag: string | null; + firstName: string | null; + lastName: string | null; + phoneNumber: string | null; }; /** - * 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. + * 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. + * + * Use this as the read that populates an edit form, and hand the tag it returns back to + * {@link updateMyProfile}. A tag read at save time cannot detect anything. + */ +export async function getMyProfileWithETag(): Promise<{ profile: UserDto; etag: string | null }> { + let etag: string | null = null; + const profile = await apiFetch("/api/v1/identity/profile", { + onResponse: (response) => { + // Strip a `W/` prefix rather than pass it through. The endpoint only ever emits a strong + // validator, so a weak one is an artefact of the transport: a compressing edge (Cloudflare + // does this by default once it re-encodes a response) downgrades the tag it forwards. Sending + // it back as-is means the server drops it under the strong comparison `If-Match` mandates and + // answers 412 forever, which is a profile the user can never save. + const header = response.headers.get("ETag"); + etag = header ? header.replace(/^W\//, "") : null; + }, + }); + 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. The unedited fields come off `input.profile`, the copy + * the form was seeded from, so they keep their values instead of being nulled. + * + * The PUT carries `If-Match` with that same copy's ETag, so the server answers 412 when the + * profile moved after the user last saw it, instead of accepting a full representation built + * from a stale snapshot and blanking the concurrent change. A 412 is NOT retried here: the only + * body this function has is the one the user typed against the old values, and resending it + * against a fresh tag performs exactly the overwrite the 412 exists to prevent. The caller + * decides — normally by telling the user the profile changed and asking for a deliberate re-save. */ export async function updateMyProfile(input: UpdateProfileInput): Promise { - const profile = await getMyProfile(); + const { profile, expectedETag } = input; await apiFetch(`/api/v1/identity/profile`, { method: "PUT", + headers: expectedETag ? { "If-Match": expectedETag } : undefined, body: JSON.stringify({ id: profile.id, firstName: input.firstName ?? profile.firstName ?? null, diff --git a/clients/dashboard/src/components/layout/topbar.tsx b/clients/dashboard/src/components/layout/topbar.tsx index 0383d28e30..d78dfdc19a 100644 --- a/clients/dashboard/src/components/layout/topbar.tsx +++ b/clients/dashboard/src/components/layout/topbar.tsx @@ -36,7 +36,7 @@ import { DropdownMenuTrigger, } from "@/components/ui/dropdown-menu"; import { Avatar } from "@/components/ui/avatar"; -import { getMyProfile } from "@/api/identity"; +import { getMyProfileWithETag } from "@/api/identity"; import { useAuth } from "@/auth/use-auth"; import { useSseStatus } from "@/sse/sse-context"; import { useTheme } from "@/components/theme/theme-provider"; @@ -149,13 +149,15 @@ function SimpleMenuItem({ export function Topbar() { const { user, logout } = useAuth(); // Shared with the Profile settings page (same query key), so changing the - // photo there invalidates this and the topbar avatar updates live. + // photo there invalidates this and the topbar avatar updates live. That sharing is also why + // this reads through the ETag-carrying variant: one query key must hold one shape, and the + // settings page needs the tag to save against. const { data: profile } = useQuery({ queryKey: ["identity", "me"], - queryFn: getMyProfile, + queryFn: getMyProfileWithETag, staleTime: 5 * 60 * 1000, }); - const avatarUrl = profile?.imageUrl ?? null; + const avatarUrl = profile?.profile.imageUrl ?? null; const { status: sseStatus, eventCount } = useSseStatus(); const { mode, setMode } = useTheme(); const { setOpen: setPaletteOpen } = useCommandPalette(); 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/src/pages/settings/profile.tsx b/clients/dashboard/src/pages/settings/profile.tsx index 373b3212a0..02776556d8 100644 --- a/clients/dashboard/src/pages/settings/profile.tsx +++ b/clients/dashboard/src/pages/settings/profile.tsx @@ -3,7 +3,8 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { Camera, Fingerprint, UserCircle2 } from "lucide-react"; import { toast } from "sonner"; import { useAuth } from "@/auth/use-auth"; -import { getMyProfile, setProfileImage, updateMyProfile } from "@/api/identity"; +import { getMyProfileWithETag, setProfileImage, updateMyProfile } from "@/api/identity"; +import type { UserDto } from "@/api/identity"; import { ApiRequestError } from "@/lib/api-client"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; @@ -19,11 +20,18 @@ export function ProfileSettings() { const profileQuery = useQuery({ queryKey: PROFILE_KEY, - queryFn: getMyProfile, + queryFn: getMyProfileWithETag, }); - const profile = profileQuery.data; + const profile = profileQuery.data?.profile; const loading = profileQuery.isLoading; + + // The version this form is editing against, captured when the form is seeded. Deliberately a + // ref and not `profileQuery.data`: a background refetch would otherwise move it to a version + // the user never saw, and the save would carry an If-Match that matches whatever someone else + // just wrote — silently overwriting it, which is exactly what the ETag exists to prevent. It + // moves only on a deliberate step: a successful save, or the user being told about a conflict. + const editingVersionRef = useRef<{ profile: UserDto; etag: string | null } | null>(null); const [firstName, setFirstName] = useState(""); const [lastName, setLastName] = useState(""); const [phone, setPhone] = useState(""); @@ -35,29 +43,59 @@ export function ProfileSettings() { const seededRef = useRef(false); useEffect(() => { if (seededRef.current) return; - if (profile) { - setFirstName(profile.firstName ?? ""); - setLastName(profile.lastName ?? ""); - setPhone(profile.phoneNumber ?? ""); + if (profileQuery.data) { + const { profile: seeded } = profileQuery.data; + setFirstName(seeded.firstName ?? ""); + setLastName(seeded.lastName ?? ""); + setPhone(seeded.phoneNumber ?? ""); + editingVersionRef.current = profileQuery.data; seededRef.current = true; } else if (user && loading) { setFirstName(user.name?.split(" ")[0] ?? ""); setLastName(user.name?.split(" ").slice(1).join(" ") ?? ""); } - }, [profile, user, loading]); + }, [profileQuery.data, user, loading]); + + // Re-reads the profile and adopts it as the version the form edits against, so the next save + // carries a tag the server will accept. + const adoptCurrentVersion = async () => { + const fresh = await queryClient.fetchQuery({ + queryKey: PROFILE_KEY, + queryFn: getMyProfileWithETag, + // Must reach the network. The client's default staleTime would hand back the cached copy, + // and the cached copy carries the very tag the server just rejected. + staleTime: 0, + }); + editingVersionRef.current = fresh; + return fresh; + }; const saveMutation = useMutation({ - mutationFn: () => - updateMyProfile({ - firstName: firstName.trim() || null, - lastName: lastName.trim() || null, - phoneNumber: phone.trim() || null, - }), - onSuccess: () => { + mutationFn: updateMyProfile, + onSuccess: async () => { toast.success("Profile saved"); - queryClient.invalidateQueries({ queryKey: PROFILE_KEY }); + // The save moved the profile on, so the tag the form holds is spent: adopt the new one or + // a second save in the same sitting would 412 against the user's own write. Awaited rather + // than fire-and-forget: isPending has to stay true until the new tag is in hand, or the + // button re-enables over a spent one and a quick second click 412s against the user's own + // save. It also keeps a failed refetch from becoming an unhandled rejection that would + // strand the form on a tag the server has already rejected. + await adoptCurrentVersion(); }, - onError: (err: unknown) => { + onError: async (err: unknown) => { + // 412 means someone else wrote the profile after this form was seeded. Do NOT resend: the + // only body available is the one typed against the old values, and pushing it through + // against a fresh tag performs the overwrite the 412 just prevented. Keep the user's + // edits on screen, adopt the current version, and let them decide whether to save again. + if (err instanceof ApiRequestError && err.status === 412) { + await adoptCurrentVersion(); + toast.warning("Profile changed elsewhere", { + description: + "Someone updated this profile while you were editing. Review your changes and save again to apply them.", + }); + return; + } + const message = err instanceof ApiRequestError ? err.problem?.detail ?? err.problem?.title ?? err.message @@ -68,7 +106,17 @@ export function ProfileSettings() { const onSubmit = (e: FormEvent) => { e.preventDefault(); - saveMutation.mutate(); + const editing = editingVersionRef.current; + if (!editing) return; + // Everything the save needs travels through mutate(), never through state the callbacks + // close over: the values that go out must be the ones on screen when the button was pressed. + saveMutation.mutate({ + profile: editing.profile, + expectedETag: editing.etag, + firstName: firstName.trim() || null, + lastName: lastName.trim() || null, + phoneNumber: phone.trim() || null, + }); }; const onReset = () => { @@ -84,12 +132,19 @@ export function ProfileSettings() { (profile?.firstName ?? "") !== firstName || (profile?.lastName ?? "") !== lastName || (profile?.phoneNumber ?? "") !== phone; + // A save carries the unedited fields and the version tag off the profile read, so until that + // read lands there is nothing to save against. Disabled rather than silently doing nothing. + const canSave = profileQuery.isSuccess; const imageMutation = useMutation({ mutationFn: (url: string | null) => setProfileImage(url), - onSuccess: () => { + onSuccess: async () => { toast.success("Profile image updated"); - queryClient.invalidateQueries({ queryKey: PROFILE_KEY }); + // Setting the image is a second write to the same row, so ASP.NET Identity rotates the + // concurrency stamp and the tag this form is holding is spent. Adopting the new version (which + // also refreshes the cache, so the topbar avatar still updates) keeps the next save from + // answering 412 and telling the user someone else edited their profile. + await adoptCurrentVersion(); }, onError: (e: unknown) => { const message = @@ -108,8 +163,8 @@ export function ProfileSettings() { className="flex items-start gap-2 rounded-lg border border-[oklch(from_var(--color-destructive)_l_c_h_/_0.30)] bg-[oklch(from_var(--color-destructive)_l_c_h_/_0.06)] px-3 py-2 text-[13px] text-[var(--color-destructive)]" > - Couldn't load your profile. Showing details from your session; - saved changes may not reflect the latest server state. + Couldn't load your profile. Showing details from your session; saving is disabled + until the profile loads, because a save has to carry the version it was read at. )} @@ -137,12 +192,12 @@ export function ProfileSettings() { type="button" variant="ghost" onClick={onReset} - disabled={saving || !dirty} + disabled={saving || !dirty || !canSave} size="sm" > Reset - diff --git a/clients/dashboard/src/pages/settings/security.tsx b/clients/dashboard/src/pages/settings/security.tsx index b9dc17ddc6..63ecfef218 100644 --- a/clients/dashboard/src/pages/settings/security.tsx +++ b/clients/dashboard/src/pages/settings/security.tsx @@ -41,7 +41,7 @@ import { changePassword, disableTwoFactor, enrollTwoFactor, - getMyProfile, + getMyProfileWithETag, verifyEnrollTwoFactor, type TwoFactorEnrollmentResponse, } from "@/api/identity"; @@ -95,8 +95,9 @@ function apiErrorMessage(err: unknown, fallback: string): string { export function SecuritySettings() { const queryClient = useQueryClient(); - const profileQuery = useQuery({ queryKey: PROFILE_KEY, queryFn: getMyProfile }); - const twoFactorEnabled = profileQuery.data?.twoFactorEnabled ?? false; + // Same key as the topbar and the profile page, so same shape: the read carries the ETag. + const profileQuery = useQuery({ queryKey: PROFILE_KEY, queryFn: getMyProfileWithETag }); + const twoFactorEnabled = profileQuery.data?.profile.twoFactorEnabled ?? false; const sessionsQuery = useQuery({ queryKey: ["identity", "sessions", "me"], diff --git a/clients/dashboard/tests/settings/profile.spec.ts b/clients/dashboard/tests/settings/profile.spec.ts index 91b97df567..348de47f82 100644 --- a/clients/dashboard/tests/settings/profile.spec.ts +++ b/clients/dashboard/tests/settings/profile.spec.ts @@ -1,21 +1,23 @@ import { expect, test } from "@playwright/test"; -import { mockJsonResponse, mockProblemDetails } from "../helpers/api-mocks"; +import { mockJsonResponse } 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", () => { @@ -92,9 +94,23 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { }); test("surfaces a destructive toast on server error", async ({ page }) => { - await mockProblemDetails(page, "**/api/v1/identity/profile", 400, { - title: "Validation failed", - detail: "First name cannot be empty.", + // The 400 belongs to the PUT. The GET has to keep working: the save is built from the + // profile that read returned, so failing it would test "cannot save yet", not "save failed". + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() !== "PUT") { + await route.fallback(); + return; + } + await route.fulfill({ + status: 400, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ + type: "https://httpstatuses.io/400", + title: "Validation failed", + status: 400, + detail: "First name cannot be empty.", + }), + }); }); await page.goto("/settings/profile"); @@ -107,6 +123,160 @@ 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("the If-Match comes from the read that seeded the form, not from a read at save time", async ({ + page, + }) => { + // The lost update happens between the user seeing the values and pressing save. A tag read + // inside the save is always current by construction, so it matches whatever the other writer + // just stored and the overwrite goes through. Here the server moves on after the form is + // seeded: the save must still carry the seeded tag, which is what lets the server say 412. + const sentIfMatch: string[] = []; + let currentStamp = 1; + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + sentIfMatch.push(request.headers()["if-match"] ?? ""); + 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: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + // Someone else writes the profile while the user is typing. + currentStamp = 2; + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect(page.getByText(/profile changed elsewhere/i)).toBeVisible(); + expect(sentIfMatch[0]).toBe('"stamp-1"'); + }); + + test("a 412 warns and keeps the edits instead of resending the stale body", async ({ page }) => { + // Retrying the same body against a fresh tag performs exactly the overwrite the 412 just + // prevented. The save stops, the user's typing stays on screen, and a deliberate second save + // goes out against the version they were just told about. + const sentIfMatch: string[] = []; + let currentStamp = 1; + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + const ifMatch = request.headers()["if-match"] ?? ""; + sentIfMatch.push(ifMatch); + if (ifMatch !== `"stamp-${currentStamp}"`) { + 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; + } + + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, + body: JSON.stringify(PROFILE), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + currentStamp = 2; + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect(page.getByText(/profile changed elsewhere/i)).toBeVisible(); + await expect(page.getByText(/profile saved/i)).toBeHidden(); + // One PUT only: no silent retry behind the user's back. + expect(sentIfMatch).toHaveLength(1); + // The typing survived the rejection — nothing to retype. + await expect(page.getByLabel("First name")).toHaveValue("Alicia"); + + // Saving again now carries the version the warning told the user about. + await page.getByRole("button", { name: /save changes/i }).click(); + await expect(page.getByText(/profile saved/i)).toBeVisible(); + expect(sentIfMatch).toHaveLength(2); + expect(sentIfMatch[1]).toBe('"stamp-2"'); + }); + 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"); @@ -119,4 +289,101 @@ test.describe("settings/profile — wired to PUT /identity/profile", () => { await expect(page.getByLabel("First name")).toHaveValue("Alice"); await expect(page.getByLabel("Phone")).toHaveValue(""); }); + + // A compressing edge re-encodes the response and downgrades the validator it forwards: + // Cloudflare does exactly this by default once Brotli/gzip is on. The endpoint only ever emits a + // strong tag, so a weak one reaching the client is a transport artefact — and echoing it back + // unchanged means the server drops it under the strong comparison If-Match mandates and answers + // 412 to every save, forever, on a profile nobody else is touching. + test("sends a strong If-Match even when the edge downgraded the ETag to a weak one", async ({ + page, + }) => { + 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: 'W/"stamp-1"' }, + 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; + + expect(putReq.headers()["if-match"]).toBe('"stamp-1"'); + }); + + // Setting the image is a second write to the same row, so Identity rotates the concurrency stamp. + // Without adopting the new version the next save carries the pre-image tag, gets a 412, and the + // user is told someone else edited their profile — on a profile only they touched. + test("a save after changing the avatar carries the tag the image write produced", async ({ page }) => { + let currentStamp = 1; + const sentIfMatch: string[] = []; + + await page.route("**/api/v1/identity/profile/image", async (route) => { + currentStamp += 1; + await route.fulfill({ status: 200, headers: { "Content-Type": "application/json" }, body: '""' }); + }); + + await page.route("**/api/v1/identity/profile", async (route) => { + const request = route.request(); + if (request.method() === "PUT") { + sentIfMatch.push(request.headers()["if-match"] ?? ""); + await route.fulfill({ status: 200, headers: { "Content-Type": "application/json" }, body: '""' }); + return; + } + await route.fulfill({ + status: 200, + headers: { ...ETAG_CORS_HEADERS, ETag: `"stamp-${currentStamp}"` }, + body: JSON.stringify({ ...PROFILE, imageUrl: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==" }), + }); + }); + + await page.goto("/settings/profile"); + await expect(page.getByLabel("First name")).toHaveValue("Alice"); + + await page.getByRole("button", { name: /^remove$/i }).click(); + await expect(page.getByText(/profile image updated/i)).toBeVisible(); + + await page.getByLabel("First name").fill("Alicia"); + await page.getByRole("button", { name: /save changes/i }).click(); + + await expect.poll(() => sentIfMatch).toEqual(['"stamp-2"']); + }); + + // Without the profile read there is no ETag and no unedited-field values, so a save would either + // be a silent no-op or blank the fields it cannot see. The button is disabled and says why — + // which nothing exercised, so re-enabling it would not have failed anything. + test("saving is disabled while the profile read is failing", async ({ page }) => { + await page.route("**/api/v1/identity/profile", async (route) => { + if (route.request().method() === "GET") { + await route.fulfill({ + status: 500, + headers: { "Content-Type": "application/problem+json" }, + body: JSON.stringify({ status: 500, title: "Server Error" }), + }); + return; + } + throw new Error("no write may be attempted while the read is failing"); + }); + + await page.goto("/settings/profile"); + + await expect(page.getByText(/saving is disabled/i)).toBeVisible(); + await expect(page.getByRole("button", { name: /save changes/i })).toBeDisabled(); + }); }); diff --git a/deploy/docker/README.md b/deploy/docker/README.md index bcb1304593..0219164b7c 100644 --- a/deploy/docker/README.md +++ b/deploy/docker/README.md @@ -8,9 +8,9 @@ This brings up the full stack on a single host: | `admin` | `fsh/admin:local` | `FSH_ADMIN_PORT` (default 8081) | Operator console (nginx + React) | | `dashboard` | `fsh/dashboard:local` | `FSH_DASHBOARD_PORT` (default 8082) | Tenant dashboard (nginx + React) | | `migrator` | `fsh/dbmigrator:local` | — | One-shot: applies EF migrations + seeds the root tenant + creates the default admin user | -| `postgres` | `postgres:17-alpine` | (internal) | Identity, tenant catalog, module schemas | -| `redis` | `redis:7-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | -| `minio` | `minio/minio:latest` | (internal) | S3-compatible blob store for the Files module | +| `postgres` | `postgres:18-alpine` | (internal) | Identity, tenant catalog, module schemas | +| `redis` | `valkey/valkey:9.1.0-alpine` | (internal) | HybridCache L2, Data Protection keys, idempotency store | +| `minio` | `quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z` | (internal) | S3-compatible blob store for the Files module | The compose file does **not** include a reverse proxy or TLS terminator. You bring your own edge — Cloudflare Tunnel, AWS ALB, Tailscale Funnel, your existing nginx, anything that can route a TLS subdomain to a host:port on this machine. diff --git a/deploy/docker/docker-compose.yml b/deploy/docker/docker-compose.yml index d43c744f5b..dea9a817ff 100644 --- a/deploy/docker/docker-compose.yml +++ b/deploy/docker/docker-compose.yml @@ -54,7 +54,8 @@ services: # - "6379:6379" minio: - image: minio/minio:latest + # quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z container_name: fsh-minio restart: unless-stopped command: ["server", "/data", "--console-address", ":9001"] @@ -79,7 +80,8 @@ services: # policy is set — objects are served via the API / presigned URLs, not a # public bucket. minio-init: - image: minio/mc:latest + # quay.io: minio/mc is gone from Docker Hub too. Tag pinned; quay stopped moving :latest. + image: quay.io/minio/mc:RELEASE.2025-08-13T08-35-41Z container_name: fsh-minio-init restart: "no" depends_on: 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..854deb9530 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -9,7 +9,8 @@ - + + @@ -122,9 +123,10 @@ - - - + + + + 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/Host/FSH.Starter.AppHost/AppHost.cs b/src/Host/FSH.Starter.AppHost/AppHost.cs index e7a70abd05..4fc689599d 100644 --- a/src/Host/FSH.Starter.AppHost/AppHost.cs +++ b/src/Host/FSH.Starter.AppHost/AppHost.cs @@ -52,7 +52,10 @@ var minioUser = builder.AddParameter("minio-user", "minioadmin"); var minioPassword = builder.AddParameter("minio-password", "minioadmin", secret: true); +// quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. var minio = builder.AddContainer("minio", "minio/minio") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-09-07T16-13-09Z") .WithArgs("server", "/data", "--console-address", ":9001") .WithHttpEndpoint(port: 9000, targetPort: 9000, name: "api") .WithHttpEndpoint(port: 9001, targetPort: 9001, name: "console") @@ -73,6 +76,8 @@ """).ReplaceLineEndings("\n"); var minioInit = builder.AddContainer("minio-init", "minio/mc") + .WithImageRegistry("quay.io") + .WithImageTag("RELEASE.2025-08-13T08-35-41Z") .WithEntrypoint("/bin/sh") .WithArgs("-c", minioInitScript) .WithEnvironment("MC_USER", minioUser) 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..7bd605fa90 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,23 @@ 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); + + // 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 + // writer can land between it and the save — and deleting first would leave AspNetUsers + // pointing at a blob that no longer exists, which no retry can repair. + string? replacedBlob = null; 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. @@ -90,12 +104,12 @@ public async Task UpdateAsync(string userId, string firstName, string lastName, user.ImageUrl = new Uri(imageString, UriKind.RelativeOrAbsolute); if (deleteCurrentImage && imageUri != null) { - await storageService.RemoveAsync(imageUri.ToString(), cancellationToken); + replacedBlob = imageUri.ToString(); } } else if (deleteCurrentImage && imageUri != null) { - await storageService.RemoveAsync(imageUri.ToString(), cancellationToken); + replacedBlob = imageUri.ToString(); user.ImageUrl = null; } @@ -108,14 +122,52 @@ 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"); } + + if (replacedBlob is not null) + { + await storageService.RemoveAsync(replacedBlob, cancellationToken); + } + + 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/CorsHeaderConfigurationTests.cs b/src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs new file mode 100644 index 0000000000..4293386d62 --- /dev/null +++ b/src/Tests/Framework.Tests/Web/CorsHeaderConfigurationTests.cs @@ -0,0 +1,49 @@ +using Microsoft.Extensions.Configuration; + +namespace Framework.Tests.Web; + +/// +/// The `If-Match` contract only reaches the endpoint if CORS lets the header through: it is not +/// safelisted, so with CorsOptions:AllowAll = false the browser's preflight decides whether +/// the precondition ever arrives. CorsPolicyTests builds its configuration in memory, so it +/// cannot notice the shipped files dropping the header — and dropping it degrades the feature back +/// to the lost update this PR exists to prevent, silently, with every test still green. +/// +public sealed class CorsHeaderConfigurationTests +{ + private static string HostDirectory() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + while (directory is not null) + { + var candidate = Path.Combine(directory.FullName, "src", "Host", "FSH.Starter.Api"); + if (File.Exists(Path.Combine(candidate, "appsettings.json"))) + { + return candidate; + } + + directory = directory.Parent; + } + + throw new InvalidOperationException("Could not locate src/Host/FSH.Starter.Api from the test output directory."); + } + + [Theory] + [InlineData("Development")] + [InlineData("Production")] + public void AllowedHeaders_Should_CarryIfMatch_When_TheShippedFilesAreLoadedInOrder(string environment) + { + var host = HostDirectory(); + var configuration = new ConfigurationBuilder() + .SetBasePath(host) + .AddJsonFile("appsettings.json", optional: false) + .AddJsonFile($"appsettings.{environment}.json", optional: false) + .Build(); + + var headers = configuration.GetSection("CorsOptions:AllowedHeaders").Get() ?? []; + + headers.ShouldContain( + "if-match", + "the restricted CORS policy strips any header not on this list, so the PUT would never see the precondition"); + } +} 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.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs index 4c2939c454..e8b7898023 100644 --- a/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs +++ b/src/Tests/Integration.Middleware.Tests/Infrastructure/MiddlewareWebApplicationFactory.cs @@ -55,7 +55,8 @@ public sealed class MiddlewareWebApplicationFactory : WebApplicationFactory, I .WithCleanUp(true) .Build(); - private readonly MinioContainer _minio = new MinioBuilder("minio/minio:latest") + // quay.io: minio/minio is gone from Docker Hub. Tag pinned; quay stopped moving :latest. + private readonly MinioContainer _minio = new MinioBuilder("quay.io/minio/minio:RELEASE.2025-09-07T16-13-09Z") .WithUsername(MinioAccessKey) .WithPassword(MinioSecretKey) .WithAutoRemove(true) 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]