From 537e7f15efbe7c7ee248f24e4624297db2fc8ff3 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Fri, 21 Aug 2026 23:59:40 +0200 Subject: [PATCH] fix(users): /api/admin/users always returns a FullUserDto[] --- frontend/src/lib/api/endpoints/admin.test.ts | 5 +- frontend/src/lib/api/endpoints/admin.ts | 8 ++- frontend/src/lib/stores/preferences.svelte.ts | 10 ++- .../src/routes/admin/[[tab]]/+page.svelte | 9 ++- src/application/dtos/settings_dto.rs | 9 +-- .../services/auth_application_service.rs | 28 ++------ src/interfaces/api/handlers/admin_handler.rs | 67 +++++++------------ tests/api/storage_cleanup_check.sh | 12 ++-- 8 files changed, 62 insertions(+), 86 deletions(-) diff --git a/frontend/src/lib/api/endpoints/admin.test.ts b/frontend/src/lib/api/endpoints/admin.test.ts index 5c4a6977..f234f36e 100644 --- a/frontend/src/lib/api/endpoints/admin.test.ts +++ b/frontend/src/lib/api/endpoints/admin.test.ts @@ -64,10 +64,7 @@ describe('admin mutate-based endpoints', () => { describe('admin read endpoints', () => { it('call apiJson for the listing/settings reads', async () => { await admin.listUsers(25, 0); - expect(jsonMock).toHaveBeenCalledWith( - '/api/admin/users?limit=25&offset=0&summary=true', - expect.anything() - ); + expect(jsonMock).toHaveBeenCalledWith('/api/admin/users?limit=25&offset=0', expect.anything()); await admin.getDashboard(); await admin.getSmtpInfo(); await admin.getOidcSettings(); diff --git a/frontend/src/lib/api/endpoints/admin.ts b/frontend/src/lib/api/endpoints/admin.ts index 586b0b4b..9d1eae3d 100644 --- a/frontend/src/lib/api/endpoints/admin.ts +++ b/frontend/src/lib/api/endpoints/admin.ts @@ -277,10 +277,12 @@ export function revokeAdminSession(sessionId: string): Promise { // ── Users ─────────────────────────────────────────────────────────────── -/** List the compact rows rendered by the management table; full account - * details remain available through {@link getUserAdmin}. */ +/** List admin users — always returns `FullUser` rows. The former + * `?summary` toggle is retired; a single canonical shape carries + * the vignette + admin-visible extras the table needs. Single-user + * details still available via {@link getUserAdmin}. */ export function listUsers(limit: number, offset: number): Promise { - return apiJson(`/api/admin/users?limit=${limit}&offset=${offset}&summary=true`, { + return apiJson(`/api/admin/users?limit=${limit}&offset=${offset}`, { credentials: 'same-origin' }); } diff --git a/frontend/src/lib/stores/preferences.svelte.ts b/frontend/src/lib/stores/preferences.svelte.ts index 610b0979..a905da65 100644 --- a/frontend/src/lib/stores/preferences.svelte.ts +++ b/frontend/src/lib/stores/preferences.svelte.ts @@ -137,16 +137,20 @@ class PreferencesStore { this.pendingPatch = {}; if (Object.keys(patch).length === 0) return; - const previousUser = session.user; + // `session.user` is a derived read-through on `session.me.full.user` + // — the source of truth is `session.me: SelfUser`. Snapshot + assign + // there so the optimistic update / rollback matches the store shape + // (see `docs/plan/userdto-refactor.md` for the layering). + const previousMe = session.me; try { const updated = await updateProfile({ ui_preferences: patch }); - session.user = updated; + session.me = updated; } catch { // Roll back to whatever the server last confirmed. The // optimistic local mutation is discarded and the derived // `hideDotfiles` / other getters snap back on the next // reactivity tick. - session.user = previousUser; + session.me = previousMe; ui.notify( t('preferences.save_failed', "Couldn't save your preference. Please try again."), 'error' diff --git a/frontend/src/routes/admin/[[tab]]/+page.svelte b/frontend/src/routes/admin/[[tab]]/+page.svelte index 9d66fbde..3a80c6e0 100644 --- a/frontend/src/routes/admin/[[tab]]/+page.svelte +++ b/frontend/src/routes/admin/[[tab]]/+page.svelte @@ -1247,8 +1247,13 @@ .map(async (d) => { const ownerMember = nextMembers[d.id]?.find((m) => m.subject.type === 'user'); if (!ownerMember) return; - const user = await getUserAdmin(ownerMember.subject.id); - if (user) nextOwners[d.id] = user; + // `getUserAdmin` returns `FullUser` (admin-visible extras + // + nested `.user: PublicUser`). The drive row only reads + // public-identity fields (username, email, image) so keep + // the map typed as `PublicUser` and unwrap the embedded + // public block on insert. See docs/plan/userdto-refactor.md. + const full = await getUserAdmin(ownerMember.subject.id); + if (full) nextOwners[d.id] = full.user; }) ); personalDriveOwners = nextOwners; diff --git a/src/application/dtos/settings_dto.rs b/src/application/dtos/settings_dto.rs index 835f4719..7917d519 100644 --- a/src/application/dtos/settings_dto.rs +++ b/src/application/dtos/settings_dto.rs @@ -108,14 +108,15 @@ pub struct AdminResetPasswordDto { pub new_password: String, } -/// Query parameters for listing users +/// Query parameters for listing users. `/api/admin/users` used to +/// bifurcate on `?summary=` (flat `PublicUserDto` vs nested +/// `FullUserDto`); that split was retired — the endpoint now always +/// returns `FullUserDto`. Unknown query params are ignored, so +/// existing callers still passing `?summary=true` keep working. #[derive(Debug, Serialize, Deserialize)] pub struct ListUsersQueryDto { pub limit: Option, pub offset: Option, - /// Return only the fields rendered by the paginated management table. - /// Defaults to `false` so existing API clients keep the full user shape. - pub summary: Option, } /// Query parameters for the admin sessions listing. diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index d34ff312..618fdd22 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -3247,7 +3247,7 @@ impl AuthApplicationService { /// out so that internal-user surfaces — system address book, OCS /// sharee search, etc. — never expose external identities. Admin /// surfaces that need the full list should call - /// [`list_users_including_external_with_perms`] instead. + /// [`list_user_summaries_including_external_with_perms`] instead. pub async fn list_users( &self, limit: i64, @@ -3260,23 +3260,6 @@ impl AuthApplicationService { .collect()) } - /// Admin-only: lists users including external (grant-only) recipients. - /// Used by the admin user-management UI. - pub async fn list_users_including_external_with_perms( - &self, - authorization: &A, - caller_id: Uuid, - limit: i64, - offset: i64, - ) -> Result, DomainError> { - self.require_admin_caller(authorization, caller_id).await?; - let users = self.user_storage.list_users(limit, offset, true).await?; - Ok(users - .into_iter() - .map(|u| PublicUserDto::new(u, false)) - .collect()) - } - /// Admin-only user listing. Returns `Vec` — same /// `FullUserDto` shape [`SelfUserDto`] embeds, so the FE reads /// admin table rows and `/me` responses through identical field @@ -3284,7 +3267,10 @@ impl AuthApplicationService { /// (`user.is_online`) so the admin table renders the vignette + /// green dot without per-row follow-up fetches to /// `/api/users/{id}` (the N+1 that motivated the widening — see - /// `docs/plan/userdto-refactor.md` § N+1). + /// `docs/plan/userdto-refactor.md` § N+1). This is the sole + /// admin-visible listing path; the former flat + /// `list_users_including_external_with_perms` variant was + /// retired when `?summary` was dropped. pub async fn list_user_summaries_including_external_with_perms( &self, authorization: &A, @@ -3362,8 +3348,8 @@ impl AuthApplicationService { // `interfaces/api/routes.rs::admin_router`) — but every admin // method here still calls `require_admin_caller` as a // defense-in-depth check, matching the pattern - // `list_users_including_external_with_perms` established. If a - // handler is ever wired outside the /admin subtree, the AuthZ + // `list_user_summaries_including_external_with_perms` established. + // If a handler is ever wired outside the /admin subtree, the AuthZ // still holds. /// List sessions for the admin panel. `user_id_filter = Some(uuid)` diff --git a/src/interfaces/api/handlers/admin_handler.rs b/src/interfaces/api/handlers/admin_handler.rs index 4856f0c6..e1985f77 100644 --- a/src/interfaces/api/handlers/admin_handler.rs +++ b/src/interfaces/api/handlers/admin_handler.rs @@ -39,25 +39,14 @@ use crate::interfaces::middleware::auth::AuthUser; use std::sync::Arc; use uuid::Uuid; -#[derive(serde::Serialize)] -#[serde(untagged)] -enum AdminUsersPayload { - /// Fat-`PublicUserDto` per row. Emitted when `?summary=false` — legacy - /// path retained until the FE drops the `summary=false` query - /// (rare; the SPA uses `summary=true` for the paginated table). - Full(Vec), - /// `FullUserDto` per row — same shape one row of the /me - /// response's embedded `full` carries. Emitted when - /// `?summary=true`. The FE seeds `resolveUser` cache from - /// `row.user` here (kills the per-row `/api/users/{id}` fetch). - /// The old `AdminUserSummaryDto` returned here has been replaced - /// by `FullUserDto`; see `docs/plan/userdto-refactor.md`. - Summary(Vec), -} - +/// Response envelope for `GET /api/admin/users`. `users` is always +/// `Vec` — same shape one row of `/me`'s embedded +/// `full` block carries; the FE seeds `resolveUser` cache from +/// `row.user` (kills the per-row `/api/users/{id}` fetch). See +/// `docs/plan/userdto-refactor.md`. #[derive(serde::Serialize)] struct AdminUsersPageResponse { - users: AdminUsersPayload, + users: Vec, total: i64, limit: i64, offset: i64, @@ -1094,13 +1083,20 @@ pub async fn get_dashboard_stats( // ============================================================================ /// GET /api/admin/users?limit=50&offset=0 — list all users +/// +/// Always returns `Vec` — the shape one row of the +/// `/me` response's embedded `full` block carries. The former +/// `?summary` toggle (flat `PublicUserDto` vs nested `FullUserDto`) +/// has been retired: admin listing is low-volume and the FE always +/// asked for the nested shape anyway, so the two-shape split served +/// no caller and only invited jq-path bugs. See +/// `docs/plan/userdto-refactor.md`. #[utoipa::path( get, path = "/api/admin/users", params( ("limit" = Option, Query, description = "Max users to return (default 100, max 500)"), - ("offset" = Option, Query, description = "Pagination offset"), - ("summary" = Option, Query, description = "Return the compact management-table projection") + ("offset" = Option, Query, description = "Pagination offset") ), responses( (status = 200, description = "List of users"), @@ -1128,31 +1124,16 @@ pub async fn list_users( // internal-only variant is used by system address book / sharee // search, where surfacing externals would leak identities. See // `auth_application_service::list_users` doc for the split. - let users = if query.summary.unwrap_or(false) { - AdminUsersPayload::Summary( - auth.auth_application_service - .list_user_summaries_including_external_with_perms( - state.authorization.as_ref(), - auth_user.id, - limit, - offset, - ) - .await - .map_err(AppError::from)?, + let users = auth + .auth_application_service + .list_user_summaries_including_external_with_perms( + state.authorization.as_ref(), + auth_user.id, + limit, + offset, ) - } else { - AdminUsersPayload::Full( - auth.auth_application_service - .list_users_including_external_with_perms( - state.authorization.as_ref(), - auth_user.id, - limit, - offset, - ) - .await - .map_err(AppError::from)?, - ) - }; + .await + .map_err(AppError::from)?; let total = auth .auth_application_service diff --git a/tests/api/storage_cleanup_check.sh b/tests/api/storage_cleanup_check.sh index 378fdf60..ec416797 100755 --- a/tests/api/storage_cleanup_check.sh +++ b/tests/api/storage_cleanup_check.sh @@ -75,18 +75,18 @@ log "Probe blob and thumbnail confirmed present on disk." # subsequent trash-empty triggers garbage_collect() to remove the # now-orphaned blob files from disk. -# /api/admin/users returns { users: [PublicUserDto…], total, limit, offset } -# under the default `?summary=false` path — flat public-identity rows. The -# `?summary=true` path emits nested FullUserDto rows instead (used by the -# admin table); see `docs/plan/userdto-refactor.md`. +# /api/admin/users returns { users: [FullUserDto…], total, limit, offset } +# — public identity nests under `.user`; admin-visible extras +# (`storage_used_bytes`, `last_login_at`, …) sit at the top level of +# each row. See `docs/plan/userdto-refactor.md`. USERS_JSON=$(curl -sf -H "$AUTH" "$base_url/api/admin/users?limit=500") ADMIN_USER_ID=$(echo "$USERS_JSON" \ - | jq -r --arg u "$username" '.users[] | select(.username == $u) | .id') + | jq -r --arg u "$username" '.users[] | select(.user.username == $u) | .user.id') [[ -z "$ADMIN_USER_ID" || "$ADMIN_USER_ID" == "null" ]] && fail "could not resolve admin user id" OTHER_USER_IDS=$(echo "$USERS_JSON" \ - | jq -r --arg admin_id "$ADMIN_USER_ID" '.users[] | select(.id != $admin_id) | .id') + | jq -r --arg admin_id "$ADMIN_USER_ID" '.users[] | select(.user.id != $admin_id) | .user.id') OTHER_USER_COUNT=0 while IFS= read -r uid; do