diff --git a/docs/plan/userdto-refactor.md b/docs/plan/userdto-refactor.md index 17fe98aa..fdf8de8e 100644 --- a/docs/plan/userdto-refactor.md +++ b/docs/plan/userdto-refactor.md @@ -1,5 +1,16 @@ # UserDto Refactor — Three-Layer Split (Public / Full / Self) +> **Status — SHIPPED 2026-08-21.** All eight phases landed and all gates +> pass: `cargo clippy --all-targets --all-features -D warnings` clean, +> `cargo fmt --check` clean, `cargo test three_layer_quarantine` (2/2 +> structural-quarantine tests pass), `npm run check` (593 files, 0 +> errors, 0 warnings), `npm run test:unit` (414 pass / 1 skipped / 0 +> failed), OpenAPI regenerated at `resources/gen/openapi.json`. See the +> [Phasing](#phasing) section below for the per-step outcome. The doc +> is retained as the reference for anyone extending the three-layer +> shape (new field → decide by audience per the rule in the opening +> section). + Establish three DTO shapes for representing a user on the wire, each with a single unambiguous audience, composed hierarchically so the overlap between audiences is defined ONCE: diff --git a/frontend/src/lib/api/endpoints/auth.ts b/frontend/src/lib/api/endpoints/auth.ts index 1e8fff43..b6685dd6 100644 --- a/frontend/src/lib/api/endpoints/auth.ts +++ b/frontend/src/lib/api/endpoints/auth.ts @@ -5,7 +5,7 @@ */ import { ApiError, apiFetch } from '$lib/api/client'; import { getCsrfHeaders } from '$lib/api/csrf'; -import type { AuthResponse, User } from '$lib/api/types'; +import type { AuthResponse, SelfUser } from '$lib/api/types'; /** * Best-effort parse of the backend `ErrorResponse` shape @@ -45,7 +45,7 @@ const JSON_HEADERS = { 'Content-Type': 'application/json' }; * Failure to build a proof (no keypair, missing WebCrypto) falls back to a * headerless request — the server still accepts it for unbound sessions. */ -export async function fetchMe(): Promise { +export async function fetchMe(): Promise { // Build + sign a DPoP proof, send with the header, harvest any // `DPoP-Nonce` off the response into the shared client cache // (so the NEXT apiFetch call reuses it — no wasted round trip). @@ -80,7 +80,7 @@ export async function fetchMe(): Promise { if (dpopMod && dpopMod.isDpopNonceChallenge(res)) res = await send(); if (res.status === 401) return null; if (!res.ok) throw new Error(`/api/auth/me failed: ${res.status}`); - return (await res.json()) as User; + return (await res.json()) as SelfUser; } /** @@ -408,7 +408,7 @@ export async function setupAdmin(email: string, password: string): Promise * failure, not an expired access token. Returns the user on success, null on * any failure so the caller can fall through to the normal login UI. */ -export async function exchangeOidcCode(code: string): Promise { +export async function exchangeOidcCode(code: string): Promise { try { const res = await fetch('/api/auth/oidc/exchange', { method: 'POST', @@ -417,7 +417,7 @@ export async function exchangeOidcCode(code: string): Promise { body: JSON.stringify({ code }) }); if (!res.ok) return null; - const data = (await res.json()) as { user?: User }; + const data = (await res.json()) as { user?: SelfUser }; return data.user ?? null; } catch { return null; @@ -463,7 +463,7 @@ export async function register(email: string, password?: string, username?: stri * authenticated; a 401 here IS a genuine "session expired" and the * refresh interceptor is the right response. */ -export async function upgradeToInternal(password?: string): Promise { +export async function upgradeToInternal(password?: string): Promise { const body: Record = {}; if (password) body.password = password; const res = await apiFetch('/api/auth/upgrade-to-internal', { @@ -482,7 +482,7 @@ export async function upgradeToInternal(password?: string): Promise { message ); } - return (await res.json()) as User; + return (await res.json()) as SelfUser; } export type MagicLinkResult = 'sent' | 'unavailable'; diff --git a/frontend/src/lib/api/endpoints/profile.ts b/frontend/src/lib/api/endpoints/profile.ts index a172cc3f..9b1e003d 100644 --- a/frontend/src/lib/api/endpoints/profile.ts +++ b/frontend/src/lib/api/endpoints/profile.ts @@ -1,7 +1,7 @@ /** Profile / account endpoints — ported from views/profile/profile.js. */ import { apiFetch } from '$lib/api/client'; import { getCsrfHeaders } from '$lib/api/csrf'; -import type { User } from '$lib/api/types'; +import type { PublicUser } from '$lib/api/types'; import { t } from '$lib/i18n/index.svelte'; const JSON_HEADERS = { 'Content-Type': 'application/json' }; @@ -24,7 +24,7 @@ export interface ProfilePatch { ui_preferences?: Record; } -export async function updateProfile(patch: ProfilePatch): Promise { +export async function updateProfile(patch: ProfilePatch): Promise { const res = await apiFetch('/api/auth/me/profile', { method: 'PATCH', credentials: 'same-origin', @@ -57,7 +57,7 @@ export async function updateProfile(patch: ProfilePatch): Promise { } throw new Error(err.message || err.error || `profile update failed: ${res.status}`); } - return (await res.json()) as User; + return (await res.json()) as PublicUser; } export async function changePassword(currentPw: string, newPw: string): Promise { diff --git a/frontend/src/lib/api/endpoints/users.ts b/frontend/src/lib/api/endpoints/users.ts index ff9b9be7..b82727e0 100644 --- a/frontend/src/lib/api/endpoints/users.ts +++ b/frontend/src/lib/api/endpoints/users.ts @@ -57,3 +57,35 @@ export function resolveUser(id: string): Promise { cache.set(id, pending); return pending; } + +/** + * Prime the resolver cache from data the caller already has in hand. + * When a list endpoint (e.g. `/api/admin/users`) ships full + * `PublicUser` rows, the admin page seeds this cache in its load path + * so every subsequent `resolveUser(id)` call (from `UserVignette` + * mounted per-row) hits the cache synchronously — no per-row + * `/api/users/{id}` follow-up fetch. Kills the N+1 that motivated + * widening `/api/admin/users` to include the avatar (see + * `docs/plan/userdto-refactor.md` § N+1). + * + * No-op when the id is already cached (in-flight or resolved). This + * makes seeding safe to call unconditionally — never clobbers an + * authoritative in-flight lookup with a stale seed. + */ +export function seedUser(u: { + id: string; + username?: string | null; + email: string; + image?: string | null; + is_external: boolean; +}): void { + if (cache.has(u.id)) return; + const resolved: ResolvedUser = { + id: u.id, + name: u.username?.trim() || u.email || u.id, + email: u.email, + image: u.image ?? null, + isExternal: u.is_external + }; + cache.set(u.id, Promise.resolve(resolved)); +} diff --git a/frontend/src/lib/api/types.ts b/frontend/src/lib/api/types.ts index c99f2909..9c967495 100644 --- a/frontend/src/lib/api/types.ts +++ b/frontend/src/lib/api/types.ts @@ -179,148 +179,116 @@ export interface TrashResourcesResponse { export type Role = 'user' | 'admin'; -/** Wire shape of `UserDto` (backend: src/application/dtos/user_dto.rs). */ -export interface User { +// ───────────────────────────────────────────────────────────────────────── +// Three-layer user family — mirrors src/application/dtos/user_dto.rs. +// See docs/plan/userdto-refactor.md. +// +// `PublicUser` — public identity. Every authenticated caller may see it. +// Returned by /api/users/{id}, share responses, group +// members, magic-link invitees, recipient enrichment. +// `FullUser` — `{ user: PublicUser, ...admin+self extras }`. Returned +// as Vec by /api/admin/users; embedded in `SelfUser`. +// `SelfUser` — `{ full: FullUser, ...self-only extras }`. Returned by +// /api/auth/me and by every auth response. +// +// Adding a field? Decide by audience: +// * Any authenticated caller may see it about another user → PublicUser. +// * Only admin (about another user) AND self (about self) → FullUser. +// * Only self about themselves → SelfUser. +// ───────────────────────────────────────────────────────────────────────── + +/** Public identity — 9 fields visible to any authenticated caller. */ +export interface PublicUser { id: string; username?: string; email: string; role: string; - storage_quota_bytes: number; - storage_used_bytes: number; + image?: string | null; + is_external: boolean; + given_name?: string; + family_name?: string; + /** Presence — TRUE when the server observed a request on any of this + * user's non-revoked sessions within the last 5 min. Populated on + * list endpoints; single-user public paths default to `false`. + * Backwards-compat: missing on older backend builds → `false`. */ + is_online?: boolean; +} + +/** Full user record — public identity + all fields BOTH an admin (viewing + * another user) AND the subject themselves may see. Returned as `Vec` by + * `/api/admin/users`; embedded in `SelfUser` for `/api/auth/me`. */ +export interface FullUser { + user: PublicUser; + /** IdP linkage. Load-bearing "is federated?" predicate: + * `full.federation_kind === 'oidc'`. */ + federation_kind?: 'oidc' | 'ocm' | 'magic_link'; + /** Authority that minted the OIDC/OCM identity — issuer URL for OIDC, + * peer domain for OCM. FE that wants a friendly label maps this + * against `OidcProviders.issuer → provider_name`. */ + federation_issuer?: string; + preferred_locale?: string; + email_verified_at?: string; created_at: string; updated_at: string; last_login_at?: string | null; active: boolean; - /** - * Which trust chain minted this user's federation identity. `null` - * (omitted from wire) for local users (password / OPAQUE only). - * `"oidc" | "ocm" | "magic_link"` for federated users. Predicate: - * `!user.federation_kind` = local; `user.federation_kind === 'oidc'` - * = OIDC user. Mirrors `auth.users.federation_kind` verbatim. - */ - federation_kind?: 'oidc' | 'ocm' | 'magic_link'; - /** - * Authority that minted this user's OIDC/OCM identity — issuer URL - * for OIDC (id_token `iss`), peer domain for OCM. `null` (omitted) - * for local users. FE that wants a friendly display label maps this - * against `OidcProviders.issuer → provider_name` when they match; - * shows the raw value otherwise. Renamed from the historical - * `auth_provider` (which held a display label pre-Phase-B and a - * `"local"` sentinel for non-federated users — both are gone). - */ - federation_issuer?: string; - image?: string | null; - can_edit_image: boolean; - is_external: boolean; - given_name?: string; - family_name?: string; - email_verified_at?: string; - preferred_locale?: string; - notify_on_share: boolean; - /** - * Opaque UI preferences bag. Server-side JSONB column that persists - * pure UI toggles (hide-dotfiles, view mode, sidebar collapse, …) - * across devices. The server never inspects the contents — the SPA - * defines the keys (see `lib/stores/preferences.svelte.ts` for the - * typed view). Always an object on the wire (empty bag is `{}`, - * never `null` or missing). - * - * When PATCHing back to the server via - * `PATCH /api/auth/me/profile { ui_preferences: {...} }`, the - * server SHALLOW-merges — only the keys present in the patch are - * touched, so partial writes from one device don't clobber - * preferences set on another. Set a key to `null` in the patch to - * delete it from the bag. - */ - ui_preferences: Record; - /** - * Mirrors `auth.users.force_password_change_at_next_login`. Only - * populated by `GET /api/auth/me` (see the backend UserDto doc for - * why other UserDto call-sites default to false). When true, the - * SPA MUST lock navigation to the password-change surface — the - * root layout's guard + the backend's `require_no_password_change_pending` - * middleware together enforce this. Optional on the wire because - * older backend builds omit it and `#[serde(default)]` maps - * missing → `false`. - */ - force_password_change?: boolean; - /** - * TRUE when the account has a local Argon2id `password_hash` on - * file. Distinct from `federation_kind`: an OIDC-linked account - * (`federation_kind === 'oidc'`) can ALSO carry a local password - * (hybrid posture — SSO for daily login, local password as - * fallback). The profile page's change-password card gates on this - * flag rather than on the federation shape so hybrid users can - * rotate their local credential. Optional on the wire for older- - * backend compatibility; missing → `false` (safe default: hide the - * card). - */ - has_password?: boolean; - /** - * TRUE when the caller's current session is DPoP-bound (row's - * `dpop_jkt IS NOT NULL`). Populated only by `/api/auth/me`; other - * User-emitting endpoints leave it unset. - * - * The session store reads this to skip a redundant - * `POST /api/auth/dpop/bind` call — the endpoint returns 409 - * `already_bound` on repeated attempts (anti-downgrade invariant) - * and each rejection logs at audit INFO, so a naive "bind on - * every load" pattern was cluttering the audit stream. We only - * fire bind now when there's actual work to do (fresh OIDC / - * magic-link session that landed unbound). - */ - is_dpop_bound?: boolean; + storage_quota_bytes: number; + storage_used_bytes: number; + /** TRUE when the account has a local Argon2id `password_hash` on file. + * Distinct from `federation_kind`: an OIDC-linked account can ALSO + * carry a local password (hybrid). */ + has_password: boolean; + /** TRUE when the user has an OPAQUE envelope on file. Admin-visible + * rollout signal — kept off `PublicUser` so directory endpoints don't + * leak OPAQUE adoption. */ + opaque_registered: boolean; + /** TRUE when the user has completed ≥1 OPAQUE login. Distinct from + * `opaque_registered` — envelope-on-file vs successful-login. */ + opaque_migrated: boolean; } -/** Fields rendered by the paginated admin table. Full account details remain - * available from the detail endpoint; this shape keeps avatars and preference - * documents off every listing page. - * - * The two OPAQUE flags below are ADMIN-ONLY signals: they surface per-user - * OPAQUE rollout progress in the admin table. The backend deliberately keeps - * them off `UserDto` (`/api/auth/me`, share-recipient DTOs, group members) - * so a non-admin can't enumerate the adoption set through third-party - * endpoints. Both optional on the wire — older backend builds omit them and - * `#[serde(default)]` maps missing → `false`. */ -export type AdminUserSummary = Pick< - User, - | 'id' - | 'username' - | 'email' - | 'role' - | 'storage_quota_bytes' - | 'storage_used_bytes' - | 'last_login_at' - | 'active' - | 'federation_kind' - | 'federation_issuer' - | 'is_external' -> & { - /** TRUE = user has a server-verifiable password on file (legacy or - * admin-set). Combined with `opaque_registered` and `federation_kind`, - * the admin table derives the full auth capability set — a user with - * `has_password=false`, `opaque_registered=false` AND - * `federation_kind === undefined` (no federation) is passwordless - * (magic-link only, which is the default for externals). */ - has_password?: boolean; - /** TRUE = user has an OPAQUE envelope on file (Phase 2 silent migration - * succeeded, or the user completed a manual re-registration). */ - opaque_registered?: boolean; - /** TRUE = user has completed at least one successful OPAQUE login. - * Distinct from `opaque_registered` — the envelope may have been - * cleared by an admin reset while a stale migrated=true remains as - * historical signal (backend clears both atomically today, but the - * two-flag shape keeps the option open for a future policy split). */ - opaque_migrated?: boolean; -}; +/** Self view — everything the caller may see about themselves. + * Returned by `/api/auth/me` and every `AuthResponse` (login / refresh / + * OIDC callback / magic-link redemption ships this so the SPA's post-auth + * state matches its post-`/me` state with no UI race). */ +export interface SelfUser { + full: FullUser; + /** Opaque UI-preferences bag. Cross-device store for pure UI toggles + * (view mode, sidebar collapse, hide-dotfiles, …). Server never + * inspects contents; the SPA defines the keys (see + * `lib/stores/preferences.svelte.ts`). Always an object on the wire + * — empty bag is `{}`, never `null`. PATCH via `/api/auth/me/profile` + * shallow-merges; setting a key to `null` removes it. */ + ui_preferences: Record; + /** Whether the user wants share-notification emails. */ + notify_on_share: boolean; + /** Session-scoped: my current session is DPoP-bound. SPA reads this + * on `session.load()` to skip a redundant `/api/auth/dpop/bind` call + * (409 `already_bound` otherwise, noisy in the audit stream). */ + is_dpop_bound: boolean; + /** Admin-set temp-password gate — SPA nav guard blocks everything + * but /change-password until this flips back. Cleared by a successful + * `POST /api/auth/change-password`. */ + force_password_change: boolean; + /** Caller-scoped: can I edit my own avatar? `false` for OIDC users + * whose avatar comes from the IdP. Only meaningful when caller == + * subject; nonsense on any other DTO. */ + can_edit_image: boolean; +} + +/** Backwards-compat alias while migrating call-sites. Prefer `PublicUser` + * for public-identity contexts (sharee, group member, invitee) or + * `SelfUser` when reading `/api/auth/me`. Delete once no consumers reference + * the bare `User` name. */ +export type User = PublicUser; export interface AdminUsersPage { total: number; - users: AdminUserSummary[]; + users: FullUser[]; } export interface AuthResponse { - user: User; + user: SelfUser; access_token: string; refresh_token: string; token_type: string; diff --git a/frontend/src/lib/components/AppShell.svelte b/frontend/src/lib/components/AppShell.svelte index 595b5637..74f21ad1 100644 --- a/frontend/src/lib/components/AppShell.svelte +++ b/frontend/src/lib/components/AppShell.svelte @@ -437,11 +437,11 @@ { mode: 'dark', icon: 'moon', label: t('user_menu.theme.dark', 'Dark') } ]; - const storagePct = $derived( - session.user && session.user.storage_quota_bytes > 0 - ? Math.min(100, (session.user.storage_used_bytes / session.user.storage_quota_bytes) * 100) - : 0 - ); + const storagePct = $derived.by(() => { + const full = session.me?.full; + if (!full || full.storage_quota_bytes <= 0) return 0; + return Math.min(100, (full.storage_used_bytes / full.storage_quota_bytes) * 100); + }); const initials = $derived(userInitials(session.user?.username || session.user?.email)); @@ -655,12 +655,12 @@
- {#if session.user.storage_quota_bytes > 0} - {Math.round(storagePct)}% · {formatBytes(session.user.storage_used_bytes)} / {formatBytes( - session.user.storage_quota_bytes + {#if (session.me?.full.storage_quota_bytes ?? 0) > 0} + {Math.round(storagePct)}% · {formatBytes(session.me?.full.storage_used_bytes ?? 0)} / {formatBytes( + session.me?.full.storage_quota_bytes ?? 0 )} {:else} - {formatBytes(session.user.storage_used_bytes)} + {formatBytes(session.me?.full.storage_used_bytes ?? 0)} {/if}
@@ -903,18 +903,18 @@
- {#if session.user.storage_quota_bytes > 0} + {#if (session.me?.full.storage_quota_bytes ?? 0) > 0} {t( 'storage.used', { percentage: Math.round(storagePct), - used: formatBytes(session.user.storage_used_bytes), - total: formatBytes(session.user.storage_quota_bytes) + used: formatBytes(session.me?.full.storage_used_bytes ?? 0), + total: formatBytes(session.me?.full.storage_quota_bytes ?? 0) }, '{{percentage}}% used ({{used}} / {{total}})' )} {:else} - {formatBytes(session.user.storage_used_bytes)} + {formatBytes(session.me?.full.storage_used_bytes ?? 0)} {/if}
diff --git a/frontend/src/lib/components/AppShell.test.ts b/frontend/src/lib/components/AppShell.test.ts index 1439ecff..04089694 100644 --- a/frontend/src/lib/components/AppShell.test.ts +++ b/frontend/src/lib/components/AppShell.test.ts @@ -26,16 +26,27 @@ const children = createRawSnippet(() => ({ beforeEach(() => { vi.clearAllMocks(); pageState.url = new URL('http://localhost/files'); - session.user = { - id: '1', - username: 'admin', - email: 'a@x.test', - given_name: 'A', - family_name: 'B', - role: 'admin', - storage_used_bytes: 10, - storage_quota_bytes: 100, - is_external: false + // Post the three-layer UserDto refactor, `session.user` is a + // derived accessor over `session.me.full.user`; only `session.me` + // is settable. Fixture composes the nested shape — public identity + // (username/email/name) on `.full.user`, admin+self extras + // (storage_*, has_password) on `.full`, self-only bag (ui_prefs, + // dpop_bound, force_password_change, can_edit_image) at the top. + // See docs/plan/userdto-refactor.md. + session.me = { + full: { + user: { + id: '1', + username: 'admin', + email: 'a@x.test', + given_name: 'A', + family_name: 'B', + role: 'admin', + is_external: false + }, + storage_used_bytes: 10, + storage_quota_bytes: 100 + } } as never; }); diff --git a/frontend/src/lib/stores/preferences.svelte.ts b/frontend/src/lib/stores/preferences.svelte.ts index e9e141d0..610b0979 100644 --- a/frontend/src/lib/stores/preferences.svelte.ts +++ b/frontend/src/lib/stores/preferences.svelte.ts @@ -69,12 +69,15 @@ const PATCH_DEBOUNCE_MS = 500; class PreferencesStore { /** - * The typed view of the bag. Derived from `session.user?.ui_preferences` - * so signing in / out / refresh flips it in lockstep with the session. + * The typed view of the bag. Derived from `session.me?.ui_preferences` + * (moved from public `User.ui_preferences` to `SelfUser.ui_preferences` + * as part of the three-layer UserDto refactor — the bag is self-only + * state, not something other authenticated callers should see). + * Signing in / out / refresh flips it in lockstep with the session. * Reads pass through DEFAULTS for any missing key. */ private bag = $derived>( - (session.user?.ui_preferences as Record | undefined) ?? {} + (session.me?.ui_preferences as Record | undefined) ?? {} ); // ── Typed accessors ────────────────────────────────────────── @@ -100,11 +103,14 @@ class PreferencesStore { * `jsonb_strip_nulls` after the merge). */ set(patch: Partial>): void { - if (!session.user) return; + if (!session.me) return; - // Optimistic local write — mutate the reactive user shallowly. + // Optimistic local write — mutate the reactive me shallowly. + // `ui_preferences` lives on `SelfUser` (self-only), not on the + // public `User` slice, so the mutation stays at the SelfUser + // level. The nested `full` / `full.user` blocks are untouched. const nextBag = { - ...((session.user.ui_preferences as Record | undefined) ?? {}), + ...((session.me.ui_preferences as Record | undefined) ?? {}), ...patch }; // Strip any explicit-null locally so the derived getters see the @@ -114,7 +120,7 @@ class PreferencesStore { for (const [k, v] of Object.entries(patch)) { if (v === null) delete (nextBag as Record)[k]; } - session.user = { ...session.user, ui_preferences: nextBag }; + session.me = { ...session.me, ui_preferences: nextBag }; // Accumulate keys so successive `set` calls before the debounce // fires collapse into a single PATCH body — matters for diff --git a/frontend/src/lib/stores/session.svelte.test.ts b/frontend/src/lib/stores/session.svelte.test.ts index 20325726..85450069 100644 --- a/frontend/src/lib/stores/session.svelte.test.ts +++ b/frontend/src/lib/stores/session.svelte.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import type { User } from '$lib/api/types'; +import type { SelfUser } from '$lib/api/types'; // `vi.mock` is hoisted above imports, so the spy it references must be created // with `vi.hoisted` (a plain top-level const isn't initialised yet when the @@ -14,7 +14,14 @@ vi.mock('$lib/api/endpoints/auth', () => ({ import { session } from './session.svelte'; -const userWithUsage = (used: number) => ({ storage_used_bytes: used }) as unknown as User; +// `storage_used_bytes` moved to `FullUser` (embedded inside `SelfUser`) +// as part of the three-layer UserDto refactor +// (`docs/plan/userdto-refactor.md`). Build a minimal SelfUser shape that +// satisfies the type checker without hand-populating every field the +// production shape carries — the test only cares about the usage read +// path (`session.me.full.storage_used_bytes`). +const userWithUsage = (used: number) => + ({ full: { storage_used_bytes: used } }) as unknown as SelfUser; describe('session.refresh', () => { beforeEach(() => { @@ -25,7 +32,7 @@ describe('session.refresh', () => { it('pulls the fresh storage usage into the reactive user (upload/delete sync)', async () => { fetchMeMock.mockResolvedValue(userWithUsage(2048)); await session.refresh(); - expect(session.user?.storage_used_bytes).toBe(2048); + expect(session.me?.full.storage_used_bytes).toBe(2048); }); it('leaves the current user intact when the probe returns null', async () => { @@ -33,7 +40,7 @@ describe('session.refresh', () => { await session.refresh(); fetchMeMock.mockResolvedValue(null); await session.refresh(); - expect(session.user?.storage_used_bytes).toBe(2048); + expect(session.me?.full.storage_used_bytes).toBe(2048); }); it('leaves the current user intact when the probe throws', async () => { @@ -41,6 +48,6 @@ describe('session.refresh', () => { await session.refresh(); fetchMeMock.mockRejectedValue(new Error('network')); await session.refresh(); - expect(session.user?.storage_used_bytes).toBe(2048); + expect(session.me?.full.storage_used_bytes).toBe(2048); }); }); diff --git a/frontend/src/lib/stores/session.svelte.ts b/frontend/src/lib/stores/session.svelte.ts index bc2022c1..a46e1e4d 100644 --- a/frontend/src/lib/stores/session.svelte.ts +++ b/frontend/src/lib/stores/session.svelte.ts @@ -11,17 +11,39 @@ import { setLogoutInProgress } from '$lib/api/client'; import { hasSessionHint } from '$lib/api/csrf'; import { seedNonceFromCookie } from '$lib/auth/dpop-proof'; import { drives } from '$lib/stores/drives.svelte'; -import type { User } from '$lib/api/types'; +import type { PublicUser, SelfUser } from '$lib/api/types'; import { ensureActiveUser } from '$lib/utils/localStoragePrefs'; +/** + * Session store — the authenticated user and derived flags. + * + * Post the three-layer UserDto refactor (`docs/plan/userdto-refactor.md`), + * `/api/auth/me` returns `SelfUser` (composed: + * `SelfUser.full.user: PublicUser`). Two shorthand accessors keep every + * existing consumer readable: + * + * - `session.user` → `PublicUser` (via `me.full.user`). Every callsite + * that read `session.user.username / email / id / role / image / + * is_external / given_name / family_name / is_online` keeps working. + * - `session.me` → full `SelfUser`. New code that needs self-only or + * admin-visible fields (`has_password`, `is_dpop_bound`, `active`, + * `ui_preferences`, `federation_kind`, `last_login_at`, quotas, …) + * reads through `session.me.full.foo` or `session.me.foo`. + */ class SessionStore { - user = $state(null); + /** Full `/api/auth/me` payload. Null when unauthenticated. */ + me = $state(null); loaded = $state(false); homeFolderId = $state(null); homeFolderName = $state(null); - isExternalUser = $derived(this.user?.is_external ?? false); - isAuthenticated = $derived(this.user !== null); + /** Public-identity shorthand — same fields any authenticated caller + * can see. Every legacy `session.user.foo` read (username, email, id, + * role, image, is_external, given_name, family_name, is_online) still + * works via this derived accessor. */ + user = $derived(this.me?.full.user ?? null); + isExternalUser = $derived(this.me?.full.user.is_external ?? false); + isAuthenticated = $derived(this.me !== null); /** * TRUE when the backend has set `force_password_change_at_next_login` * on this account — an admin picked a temporary password and the @@ -33,7 +55,7 @@ class SessionStore { * flag (or a malformed `/me` response) doesn't accidentally * quarantine every user. */ - mustChangePassword = $derived(this.user?.force_password_change === true); + mustChangePassword = $derived(this.me?.force_password_change === true); /** * Resolve the session once. Probes /api/auth/me; on 401 it makes a single @@ -41,15 +63,15 @@ class SessionStore { * what to do with an unauthenticated result. Idempotent: subsequent calls * return the cached result (so client-side navigation doesn't re-probe). */ - async load(): Promise { - if (this.loaded) return this.user; + async load(): Promise { + if (this.loaded) return this.me; // No JS-visible session hint ⇒ nothing to probe. The server sets // `oxicloud_csrf` alongside the HttpOnly session cookies and clears // it on logout, so a missing hint means no session. Skips the // doomed 2× /me + /refresh burst that would otherwise fire on // every first landing / post-logout re-mount with no cookies. if (!hasSessionHint()) { - this.user = null; + this.me = null; this.loaded = true; return null; } @@ -71,25 +93,25 @@ class SessionStore { // otherwise clutter the audit stream. Fire-and-forget // so a slow IndexedDB open doesn't stall app boot. if (me.is_dpop_bound === false) void bindDpopIfPossible(); - } else this.user = null; + } else this.me = null; } catch { - this.user = null; + this.me = null; } this.loaded = true; - return this.user; + return this.me; } /** * Set the authenticated user AND run per-user localStorage cleanup * (see `$lib/utils/localStoragePrefs::ensureActiveUser`). Direct - * `session.user = …` assignments skip the cleanup — always call + * `session.me = …` assignments skip the cleanup — always call * `setUser` on login-flow entry points (form login, OIDC exchange, * existing-session probe) so a switch-account flow inside the same * tab observes the wipe. */ - setUser(user: User): void { - this.user = user; - ensureActiveUser(user.id); + setUser(me: SelfUser): void { + this.me = me; + ensureActiveUser(me.full.user.id); // Any successful login clears the session-teardown gate. Without // this, a logout → login within the same SPA session leaves the // gate stuck at `true` — the login POST is exempted via @@ -115,7 +137,7 @@ class SessionStore { async refresh(): Promise { try { const me = await fetchMe(); - if (me) this.user = me; + if (me) this.me = me; } catch { /* keep the existing user on a transient /api/auth/me failure */ } @@ -142,7 +164,7 @@ class SessionStore { } reset(): void { - this.user = null; + this.me = null; this.homeFolderId = null; this.homeFolderName = null; // Mark the store as `loaded` so any subsequent `session.load()` — diff --git a/frontend/src/routes/admin/[[tab]]/+page.svelte b/frontend/src/routes/admin/[[tab]]/+page.svelte index 63243dfe..715bfd2c 100644 --- a/frontend/src/routes/admin/[[tab]]/+page.svelte +++ b/frontend/src/routes/admin/[[tab]]/+page.svelte @@ -63,6 +63,7 @@ type StorageTestResult } from '$lib/api/endpoints/admin'; import { createDrive, updateDrivePolicies } from '$lib/api/endpoints/drives'; + import { seedUser } from '$lib/api/endpoints/users'; import { ensureResolvers, resolveRecipient, @@ -70,7 +71,7 @@ type Recipient } from '$lib/api/endpoints/recipients'; import type { - AdminUserSummary, + FullUser, Drive, DriveMember, DrivePolicies, @@ -156,11 +157,11 @@ deleteUserModal !== null && deleteUserEmailInput.trim().toLowerCase() === deleteUserModal.email.toLowerCase() ); - function openDeleteUser(u: AdminUserSummary) { + function openDeleteUser(u: FullUser) { deleteUserModal = { - userId: u.id, - username: u.username || u.email, - email: u.email + userId: u.user.id, + username: u.user.username || u.user.email, + email: u.user.email }; deleteUserEmailInput = ''; } @@ -817,7 +818,7 @@ } // Users - let users = $state([]); + let users = $state([]); let total = $state(0); let pageIndex = $state(0); let usersError = $state(null); @@ -923,6 +924,12 @@ const page = await listUsers(PAGE_SIZE, pageIndex * PAGE_SIZE); users = page.users; total = page.total; + // Seed the per-user resolver cache with the row's `PublicUser` + // slice so every `UserVignette` mounted per row hits the cache + // synchronously — no per-row `/api/users/{id}` follow-up. + // Kills the N+1 that motivated widening `/api/admin/users` to + // carry the avatar (docs/plan/userdto-refactor.md § N+1). + for (const row of page.users) seedUser(row.user); } catch (e) { usersError = errorMessage(e); } @@ -1003,49 +1010,49 @@ } /** True for the signed-in admin's own row — guards self-destructive actions. */ - function isSelf(u: AdminUserSummary): boolean { - return u.id === currentAdminId; + function isSelf(u: FullUser): boolean { + return u.user.id === currentAdminId; } /** OIDC/SSO-provisioned account (no local password to reset). */ - function isOidcUser(u: AdminUserSummary): boolean { + function isOidcUser(u: FullUser): boolean { return u.federation_kind === 'oidc'; } /** Used-quota percentage (0 when unlimited) for the per-user progress bar. */ - function quotaPct(u: AdminUserSummary): number { + function quotaPct(u: FullUser): number { return u.storage_quota_bytes > 0 ? (u.storage_used_bytes / u.storage_quota_bytes) * 100 : 0; } - async function toggleRole(u: AdminUserSummary) { + async function toggleRole(u: FullUser) { if (isSelf(u)) return; - const role = u.role === 'admin' ? 'user' : 'admin'; + const role = u.user.role === 'admin' ? 'user' : 'admin'; if (!(await showConfirm(t('admin.confirm_role', { role }, 'Change role to {{role}}?')))) return; try { - await setUserRole(u.id, role); + await setUserRole(u.user.id, role); await loadUsers(); } catch (e) { reportError(e); } } - async function toggleActive(u: AdminUserSummary) { + async function toggleActive(u: FullUser) { if (isSelf(u) && u.active) return; const msg = u.active ? t('admin.confirm_deactivate', 'Deactivate this user?') : t('admin.confirm_activate', 'Activate this user?'); if (!(await showConfirm(msg))) return; try { - await setUserActive(u.id, !u.active); + await setUserActive(u.user.id, !u.active); await loadUsers(); } catch (e) { reportError(e); } } - function openQuota(u: AdminUserSummary) { + function openQuota(u: FullUser) { quotaModalError = null; quotaModal = { - userId: u.id, - username: u.username || u.email, + userId: u.user.id, + username: u.user.username || u.user.email, initialBytes: u.storage_quota_bytes }; } @@ -1069,8 +1076,8 @@ } } - function openReset(u: AdminUserSummary) { - resetModal = { userId: u.id, username: u.username || u.email }; + function openReset(u: FullUser) { + resetModal = { userId: u.user.id, username: u.user.username || u.user.email }; resetPassword = ''; resetError = null; } @@ -1094,7 +1101,7 @@ } } - function removeUser(u: AdminUserSummary) { + function removeUser(u: FullUser) { if (isSelf(u)) return; openDeleteUser(u); } @@ -1103,20 +1110,20 @@ // provisions a home drive + flips the is_external flag; irreversible // via the admin UI (there's no demote endpoint on purpose). Backend // refuses when magic-link login is disabled — surfaced as a toast. - async function promoteExternal(u: AdminUserSummary) { - if (!u.is_external) return; + async function promoteExternal(u: FullUser) { + if (!u.user.is_external) return; if ( !(await showConfirm( t( 'admin.confirm_promote_user', - { name: u.username || u.email }, + { name: u.user.username || u.user.email }, 'Promote {{name}} to an internal user? This provisions a home drive and gives the account a normal storage envelope. The account keeps its identity; magic-link login stays the way in unless a password is set later.' ) )) ) return; try { - await promoteUserToInternal(u.id); + await promoteUserToInternal(u.user.id); await loadUsers(); } catch (e) { reportError(e); @@ -2680,15 +2687,15 @@ - {#each users as u (u.id)} + {#each users as u (u.user.id)} {@const pct = quotaPct(u)}
{#if isSelf(u)} {t('admin.you_badge', 'you')} @@ -2703,11 +2710,11 @@ badge is `white-space: nowrap` so the badge label itself never wraps mid-word either. -->
- - {#if u.role === 'admin'}{/if} - {u.role} + + {#if u.user.role === 'admin'}{/if} + {u.user.role} - {#if u.is_external} + {#if u.user.is_external}
- {#if u.is_external} + {#if u.user.is_external} {:else} @@ -2926,7 +2933,7 @@
-
{timeAgo(session.user.last_login_at)}
+
{timeAgo(session.me?.full.last_login_at)}
@@ -752,20 +762,20 @@

{t('profile.storage', 'Storage')}

-
{formatBytes(session.user.storage_used_bytes)}
+
{formatBytes(session.me?.full.storage_used_bytes ?? 0)}
{t('profile.used', 'Used')}
- {session.user.storage_quota_bytes > 0 - ? formatBytes(session.user.storage_quota_bytes) + {(session.me?.full.storage_quota_bytes ?? 0) > 0 + ? formatBytes(session.me?.full.storage_quota_bytes ?? 0) : '∞'}
{t('profile.quota', 'Quota')}
- {session.user.storage_quota_bytes > 0 ? `${storagePct}%` : '—'} + {(session.me?.full.storage_quota_bytes ?? 0) > 0 ? `${storagePct}%` : '—'}
{t('profile.usage', 'Usage')}
diff --git a/frontend/src/routes/profile/page.test.ts b/frontend/src/routes/profile/page.test.ts index 204a7744..10a2cf9b 100644 --- a/frontend/src/routes/profile/page.test.ts +++ b/frontend/src/routes/profile/page.test.ts @@ -1,10 +1,15 @@ import { it, expect, vi, beforeEach } from 'vitest'; import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; -const { session, ui } = vi.hoisted(() => ({ - session: { - loaded: true, - load: vi.fn(), +// Test-double session store. Post the three-layer UserDto refactor +// (docs/plan/userdto-refactor.md), production `session.user` is a +// derived accessor over `session.me.full.user`. The stub here mirrors +// that shape: `me` carries the whole SelfUser tree, and `user` mirrors +// `me.full.user` so any legacy `session.user.foo` read on the tested +// page keeps working through the mock without reproducing the derived +// mechanism. +const buildSelfMe = () => ({ + full: { user: { id: '1', username: 'admin', @@ -12,14 +17,41 @@ const { session, ui } = vi.hoisted(() => ({ given_name: 'A', family_name: 'B', role: 'admin', + is_external: false + }, + storage_used_bytes: 100, + storage_quota_bytes: 1000, + has_password: true + } +}); + +const { session, ui } = vi.hoisted(() => { + const me = { + full: { + user: { + id: '1', + username: 'admin', + email: 'a@x.test', + given_name: 'A', + family_name: 'B', + role: 'admin', + is_external: false + }, storage_used_bytes: 100, storage_quota_bytes: 1000, - is_external: false, has_password: true } - }, - ui: { notify: vi.fn() } -})); + }; + return { + session: { + loaded: true, + load: vi.fn(), + me, + user: me.full.user + }, + ui: { notify: vi.fn() } + }; +}); vi.mock('$lib/stores/session.svelte', () => ({ session })); vi.mock('$lib/stores/ui.svelte', () => ({ ui })); vi.mock('$lib/stores/dialogs.svelte', () => ({ confirmDialog: vi.fn() })); @@ -44,20 +76,13 @@ const m = (fn: unknown) => fn as ReturnType; beforeEach(() => { vi.clearAllMocks(); - // Reset the shared session each test (handlers may mutate session.user). + // Reset the shared session each test (handlers may mutate session.me + // on save / refresh). `me` is the SelfUser tree; `user` mirrors + // `me.full.user` for legacy `session.user.foo` reads. session.loaded = true; - session.user = { - id: '1', - username: 'admin', - email: 'a@x.test', - given_name: 'A', - family_name: 'B', - role: 'admin', - storage_used_bytes: 100, - storage_quota_bytes: 1000, - is_external: false, - has_password: true - }; + const me = buildSelfMe(); + session.me = me; + session.user = me.full.user; m(profile.listAppPasswords).mockResolvedValue([]); m(profile.updateProfile).mockResolvedValue(undefined); m(getOidcProviders).mockResolvedValue({ password_login_enabled: true }); diff --git a/src/application/dtos/user_dto.rs b/src/application/dtos/user_dto.rs index b3c225c1..8742b1de 100644 --- a/src/application/dtos/user_dto.rs +++ b/src/application/dtos/user_dto.rs @@ -586,3 +586,142 @@ pub struct OidcUserInfoDto { pub name: Option, pub groups: Vec, } + +#[cfg(test)] +mod three_layer_quarantine { + use super::*; + use serde_json::Value; + + /// Structural-quarantine guard for `SelfUserDto`. The self-only + /// bag (`ui_preferences`, `notify_on_share`, `is_dpop_bound`, + /// `force_password_change`, `can_edit_image`) MUST live at the + /// top level, NOT nested inside `.full` or `.full.user`. If a + /// future refactor accidentally moves one of them down, the + /// wire shape leaks it through every `PublicUserDto` / + /// `FullUserDto` emitter (share responses, group members, + /// `/api/admin/users`, magic-link invitees) — exactly what the + /// three-layer split exists to prevent. Fails loudly here. + #[test] + fn self_only_fields_stay_at_top_level_of_self_user_dto() { + let self_dto = SelfUserDto { + full: FullUserDto { + user: PublicUserDto { + id: "00000000-0000-0000-0000-000000000001".into(), + username: None, + email: "self@example.invalid".into(), + role: "user".into(), + image: None, + is_external: false, + given_name: None, + family_name: None, + is_online: false, + }, + federation_kind: None, + federation_issuer: None, + preferred_locale: None, + email_verified_at: None, + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + last_login_at: None, + active: true, + storage_quota_bytes: 0, + storage_used_bytes: 0, + has_password: true, + opaque_registered: false, + opaque_migrated: false, + }, + ui_preferences: serde_json::json!({}), + notify_on_share: true, + is_dpop_bound: false, + force_password_change: false, + can_edit_image: true, + }; + let json: Value = serde_json::to_value(&self_dto).expect("SelfUserDto serialises"); + assert!( + json.get("ui_preferences").is_some(), + "top-level ui_preferences" + ); + assert!( + json.get("full") + .expect("full block") + .get("ui_preferences") + .is_none(), + "ui_preferences must NOT appear inside `.full`" + ); + assert!( + json.pointer("/full/user/ui_preferences").is_none(), + "ui_preferences must NOT appear inside `.full.user`" + ); + // Same guard for the other self-only fields. + for k in [ + "notify_on_share", + "is_dpop_bound", + "force_password_change", + "can_edit_image", + ] { + assert!(json.get(k).is_some(), "{k} at top level"); + assert!( + json.pointer(&format!("/full/{k}")).is_none(), + "{k} must NOT nest in .full" + ); + assert!( + json.pointer(&format!("/full/user/{k}")).is_none(), + "{k} must NOT nest in .full.user" + ); + } + } + + /// Structural-quarantine guard for `FullUserDto`. Admin-visible + /// extras (`has_password`, OPAQUE flags, `federation_*`, + /// `last_login_at`, `active`, quotas, `preferred_locale`, + /// `email_verified_at`) MUST live at the top level of + /// `FullUserDto`, NOT inside `.user`. If a future refactor + /// accidentally lifts one of them onto `PublicUserDto` (the + /// embedded `user` field), it leaks through `/api/users/{id}` + /// and every other public directory endpoint. + #[test] + fn admin_only_fields_stay_at_top_level_of_full_user_dto() { + let full = FullUserDto { + user: PublicUserDto { + id: "00000000-0000-0000-0000-000000000002".into(), + username: Some("bob".into()), + email: "bob@example.invalid".into(), + role: "user".into(), + image: None, + is_external: false, + given_name: None, + family_name: None, + is_online: false, + }, + federation_kind: None, + federation_issuer: None, + preferred_locale: None, + email_verified_at: None, + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + last_login_at: None, + active: true, + storage_quota_bytes: 10_737_418_240, + storage_used_bytes: 0, + has_password: true, + opaque_registered: false, + opaque_migrated: false, + }; + let json: Value = serde_json::to_value(&full).expect("FullUserDto serialises"); + for k in [ + "has_password", + "opaque_registered", + "opaque_migrated", + "last_login_at", + "active", + "storage_quota_bytes", + "storage_used_bytes", + ] { + assert!(json.get(k).is_some(), "{k} at top level of FullUserDto"); + assert!( + json.pointer(&format!("/user/{k}")).is_none(), + "{k} must NOT nest in .user" + ); + } + } +}