From 14ce113e70132c7ae4ee5d5a14d7e497089ea401 Mon Sep 17 00:00:00 2001 From: Riyadh Date: Wed, 22 Apr 2026 11:05:34 +0000 Subject: [PATCH] Warn admins before deleting non-empty groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Original task: Task #79 — Make DELETE /api/groups/:id refuse to wipe a non-empty group unless explicitly forced, surface the impacted counts in the admin UI, and log forced deletions to an audit trail. Changes: - API: DELETE /api/groups/:id now computes member/app/role counts. If any are non-zero and `?force=true` is not passed, it returns 409 with { error, memberCount, appCount, roleCount }. With `force=true` it deletes and writes an entry to the new `audit_logs` table (action='group.force_delete', target_type='group', target_id, metadata). System group guard and 404 behaviour preserved. - DB: Added `audit_logs` table (lib/db/src/schema/audit-logs.ts) with actor, action, target type/id, jsonb metadata, timestamp. Pushed via drizzle-kit. - OpenAPI: Added `force` query param, 409 response, and `GroupDeletionConflict` schema. Re-ran orval codegen for api-client-react and api-zod. - api-client-react: Re-exported `ApiError` and `ErrorType` so the UI can inspect 409 responses. - Admin UI (artifacts/teaboy-os/src/pages/admin.tsx GroupsPanel): Replaced the bare `confirm()` with a confirmation dialog that shows the group name and impacted member/app/role counts. Empty groups get a simple "Delete" confirmation; non-empty groups get a warning + "Delete anyway" button which sends `force=true`. If the server returns 409 (race), the dialog updates to show the server-reported counts. - Locales: Added en/ar strings for the new dialog (deleteTitle, deleteWarning, deleteForceHint, deleteEmptyBody, deleteConfirm, deleteAnyway). Verified end-to-end: 409 on first DELETE, dialog warning visible, force deletion succeeds, single audit_logs row recorded with correct metadata. --- artifacts/api-server/src/routes/groups.ts | 37 ++++++ artifacts/tx-os/src/locales/ar.json | 6 + artifacts/tx-os/src/locales/en.json | 6 + artifacts/tx-os/src/pages/admin.tsx | 110 ++++++++++++++++-- .../src/generated/api.schemas.ts | 14 +++ lib/api-client-react/src/generated/api.ts | 43 ++++--- lib/api-client-react/src/index.ts | 4 +- lib/api-spec/openapi.yaml | 30 +++++ lib/api-zod/src/generated/api.ts | 11 ++ lib/db/src/schema/audit-logs.ts | 14 +++ lib/db/src/schema/index.ts | 1 + 11 files changed, 252 insertions(+), 24 deletions(-) create mode 100644 lib/db/src/schema/audit-logs.ts diff --git a/artifacts/api-server/src/routes/groups.ts b/artifacts/api-server/src/routes/groups.ts index 7eed2b1a..eec0c13a 100644 --- a/artifacts/api-server/src/routes/groups.ts +++ b/artifacts/api-server/src/routes/groups.ts @@ -9,6 +9,7 @@ import { usersTable, appsTable, rolesTable, + auditLogsTable, } from "@workspace/db"; import { requireAdmin } from "../middlewares/auth"; import { emitAppsChangedToUsers } from "../lib/realtime"; @@ -448,8 +449,44 @@ router.delete("/groups/:id", requireAdmin, async (req, res): Promise => { res.status(400).json({ error: "Cannot delete a system group" }); return; } + const force = req.query.force === "true" || req.query.force === "1"; const memberIds = await getGroupMemberIds(id); + const [appCountRow] = await db + .select({ count: sql`count(*)::int` }) + .from(groupAppsTable) + .where(eq(groupAppsTable.groupId, id)); + const [roleCountRow] = await db + .select({ count: sql`count(*)::int` }) + .from(groupRolesTable) + .where(eq(groupRolesTable.groupId, id)); + const memberCount = memberIds.length; + const appCount = appCountRow?.count ?? 0; + const roleCount = roleCountRow?.count ?? 0; + const isNonEmpty = memberCount > 0 || appCount > 0 || roleCount > 0; + if (isNonEmpty && !force) { + res.status(409).json({ + error: "Group is not empty", + memberCount, + appCount, + roleCount, + }); + return; + } await db.delete(groupsTable).where(eq(groupsTable.id, id)); + if (isNonEmpty && force) { + await db.insert(auditLogsTable).values({ + actorUserId: req.session.userId ?? null, + action: "group.force_delete", + targetType: "group", + targetId: id, + metadata: { + name: existing.name, + memberCount, + appCount, + roleCount, + }, + }); + } await emitAppsChangedToUsers(memberIds); res.sendStatus(204); }); diff --git a/artifacts/tx-os/src/locales/ar.json b/artifacts/tx-os/src/locales/ar.json index d071bac3..02c228a5 100644 --- a/artifacts/tx-os/src/locales/ar.json +++ b/artifacts/tx-os/src/locales/ar.json @@ -395,6 +395,12 @@ "usersHint": "أعضاء هذه المجموعة.", "rolesHint": "يحصل أعضاء هذه المجموعة تلقائياً على هذه الأدوار.", "rolesEmpty": "لا توجد أدوار متاحة.", + "deleteTitle": "حذف المجموعة \"{{name}}\"؟", + "deleteWarning": "هذه المجموعة لا تزال قيد الاستخدام. حذفها سيلغي الوصول لما يلي:", + "deleteForceHint": "سيؤدي الحذف إلى فصل الأعضاء وإلغاء التطبيقات والأدوار الممنوحة عبر المجموعة. لا يمكن التراجع عن هذا الإجراء.", + "deleteEmptyBody": "لا تحتوي هذه المجموعة على أعضاء أو تطبيقات أو أدوار. هل تريد المتابعة؟", + "deleteConfirm": "حذف", + "deleteAnyway": "حذف رغم ذلك", "tab": { "info": "المعلومات", "apps": "التطبيقات", diff --git a/artifacts/tx-os/src/locales/en.json b/artifacts/tx-os/src/locales/en.json index 57789296..6fc245ed 100644 --- a/artifacts/tx-os/src/locales/en.json +++ b/artifacts/tx-os/src/locales/en.json @@ -392,6 +392,12 @@ "usersHint": "Members of this group.", "rolesHint": "Members of this group are automatically granted these roles.", "rolesEmpty": "No roles available.", + "deleteTitle": "Delete group \"{{name}}\"?", + "deleteWarning": "This group is still in use. Deleting it will revoke access for the following:", + "deleteForceHint": "Deleting will detach these members and revoke their group-granted apps and roles. This cannot be undone.", + "deleteEmptyBody": "This group has no members, apps, or roles. Continue?", + "deleteConfirm": "Delete", + "deleteAnyway": "Delete anyway", "tab": { "info": "Info", "apps": "Apps", diff --git a/artifacts/tx-os/src/pages/admin.tsx b/artifacts/tx-os/src/pages/admin.tsx index cbec2cc0..eee2fc3b 100644 --- a/artifacts/tx-os/src/pages/admin.tsx +++ b/artifacts/tx-os/src/pages/admin.tsx @@ -40,8 +40,10 @@ import { useDeleteGroup, useListRoles, getListRolesQueryKey, + ApiError, } from "@workspace/api-client-react"; import type { + GroupDeletionConflict, App, Service, UserProfile, @@ -2417,9 +2419,45 @@ function GroupsPanel() { const [createOpen, setCreateOpen] = useState(false); const [createForm, setCreateForm] = useState({ name: "", descriptionAr: "", descriptionEn: "" }); const [groupSearch, setGroupSearch] = useState(""); + const [deleteTarget, setDeleteTarget] = useState(null); + const [conflictCounts, setConflictCounts] = useState<{ + memberCount: number; + appCount: number; + roleCount: number; + } | null>(null); const invalidate = () => queryClient.invalidateQueries({ queryKey: getListGroupsQueryKey() }); + const closeDeleteDialog = () => { + setDeleteTarget(null); + setConflictCounts(null); + }; + + const performDelete = (group: Group, force: boolean) => { + deleteGroup.mutate( + { id: group.id, params: force ? { force: true } : undefined }, + { + onSuccess: () => { + invalidate(); + toast({ title: t("common.success") }); + closeDeleteDialog(); + }, + onError: (err: unknown) => { + if (err instanceof ApiError && err.status === 409 && err.data) { + const data = err.data as GroupDeletionConflict; + setConflictCounts({ + memberCount: data.memberCount, + appCount: data.appCount, + roleCount: data.roleCount, + }); + return; + } + toast({ title: t("common.error"), variant: "destructive" }); + }, + }, + ); + }; + const visibleGroups = (groups ?? []).filter((g) => !groupSearch.trim() || g.name.toLowerCase().includes(groupSearch.trim().toLowerCase()) || @@ -2474,17 +2512,11 @@ function GroupsPanel() { {!g.isSystem && ( @@ -2557,6 +2589,66 @@ function GroupsPanel() { }} /> )} + + {deleteTarget && (() => { + const counts = conflictCounts ?? { + memberCount: deleteTarget.memberCount, + appCount: deleteTarget.appCount, + roleCount: deleteTarget.roleCount, + }; + const isNonEmpty = + counts.memberCount > 0 || counts.appCount > 0 || counts.roleCount > 0; + const showWarning = conflictCounts !== null || isNonEmpty; + return ( +
+
+

+ {t("admin.groups.deleteTitle", { name: deleteTarget.name })} +

+ {showWarning ? ( + <> +

+ {t("admin.groups.deleteWarning")} +

+
    +
  • {t("admin.groups.members", { count: counts.memberCount })}
    • +
  • {t("admin.groups.appsCount", { count: counts.appCount })}
    • +
  • {t("admin.groups.rolesCount", { count: counts.roleCount })}
    • +
+

+ {t("admin.groups.deleteForceHint")} +

+ + ) : ( +

+ {t("admin.groups.deleteEmptyBody")} +

+ )} +
+ + +
+
+
+ ); + })()} ); } diff --git a/lib/api-client-react/src/generated/api.schemas.ts b/lib/api-client-react/src/generated/api.schemas.ts index a524f7fb..976aa89d 100644 --- a/lib/api-client-react/src/generated/api.schemas.ts +++ b/lib/api-client-react/src/generated/api.schemas.ts @@ -193,6 +193,13 @@ export interface Group { createdAt: string; } +export interface GroupDeletionConflict { + error: string; + memberCount: number; + appCount: number; + roleCount: number; +} + export interface GroupDetail { id: number; name: string; @@ -768,6 +775,13 @@ export type ListGroupsParams = { q?: string; }; +export type DeleteGroupParams = { + /** + * Force deletion of a non-empty group (records an audit log entry). + */ + force?: boolean; +}; + export type GetAdminStatsParams = { /** * Time range for trend stats. Use "custom" with from/to. diff --git a/lib/api-client-react/src/generated/api.ts b/lib/api-client-react/src/generated/api.ts index 8e4e2d68..8b5a4d6e 100644 --- a/lib/api-client-react/src/generated/api.ts +++ b/lib/api-client-react/src/generated/api.ts @@ -33,6 +33,7 @@ import type { CreateServiceBody, CreateServiceOrderBody, CreateUserBody, + DeleteGroupParams, ErrorResponse, ForgotPasswordBody, ForgotPasswordResponse, @@ -40,6 +41,7 @@ import type { GetAdminAppOpensByUserParams, GetAdminStatsParams, Group, + GroupDeletionConflict, GroupDetail, HealthStatus, HomeStats, @@ -5094,35 +5096,48 @@ export const useUpdateGroup = < /** * @summary Delete group (admin) */ -export const getDeleteGroupUrl = (id: number) => { - return `/api/groups/${id}`; +export const getDeleteGroupUrl = (id: number, params?: DeleteGroupParams) => { + const normalizedParams = new URLSearchParams(); + + Object.entries(params || {}).forEach(([key, value]) => { + if (value !== undefined) { + normalizedParams.append(key, value === null ? "null" : value.toString()); + } + }); + + const stringifiedParams = normalizedParams.toString(); + + return stringifiedParams.length > 0 + ? `/api/groups/${id}?${stringifiedParams}` + : `/api/groups/${id}`; }; export const deleteGroup = async ( id: number, + params?: DeleteGroupParams, options?: RequestInit, ): Promise => { - return customFetch(getDeleteGroupUrl(id), { + return customFetch(getDeleteGroupUrl(id, params), { ...options, method: "DELETE", }); }; export const getDeleteGroupMutationOptions = < - TError = ErrorType, + TError = ErrorType, TContext = unknown, >(options?: { mutation?: UseMutationOptions< Awaited>, TError, - { id: number }, + { id: number; params?: DeleteGroupParams }, TContext >; request?: SecondParameter; }): UseMutationOptions< Awaited>, TError, - { id: number }, + { id: number; params?: DeleteGroupParams }, TContext > => { const mutationKey = ["deleteGroup"]; @@ -5136,11 +5151,11 @@ export const getDeleteGroupMutationOptions = < const mutationFn: MutationFunction< Awaited>, - { id: number } + { id: number; params?: DeleteGroupParams } > = (props) => { - const { id } = props ?? {}; + const { id, params } = props ?? {}; - return deleteGroup(id, requestOptions); + return deleteGroup(id, params, requestOptions); }; return { mutationFn, ...mutationOptions }; @@ -5150,26 +5165,28 @@ export type DeleteGroupMutationResult = NonNullable< Awaited> >; -export type DeleteGroupMutationError = ErrorType; +export type DeleteGroupMutationError = ErrorType< + ErrorResponse | GroupDeletionConflict +>; /** * @summary Delete group (admin) */ export const useDeleteGroup = < - TError = ErrorType, + TError = ErrorType, TContext = unknown, >(options?: { mutation?: UseMutationOptions< Awaited>, TError, - { id: number }, + { id: number; params?: DeleteGroupParams }, TContext >; request?: SecondParameter; }): UseMutationResult< Awaited>, TError, - { id: number }, + { id: number; params?: DeleteGroupParams }, TContext > => { return useMutation(getDeleteGroupMutationOptions(options)); diff --git a/lib/api-client-react/src/index.ts b/lib/api-client-react/src/index.ts index 980b8e28..e570700d 100644 --- a/lib/api-client-react/src/index.ts +++ b/lib/api-client-react/src/index.ts @@ -1,4 +1,4 @@ export * from "./generated/api"; export * from "./generated/api.schemas"; -export { setBaseUrl, setAuthTokenGetter } from "./custom-fetch"; -export type { AuthTokenGetter } from "./custom-fetch"; +export { setBaseUrl, setAuthTokenGetter, ApiError } from "./custom-fetch"; +export type { AuthTokenGetter, ErrorType } from "./custom-fetch"; diff --git a/lib/api-spec/openapi.yaml b/lib/api-spec/openapi.yaml index 9f92faec..11699739 100644 --- a/lib/api-spec/openapi.yaml +++ b/lib/api-spec/openapi.yaml @@ -1357,6 +1357,13 @@ paths: required: true schema: type: integer + - name: force + in: query + required: false + description: Force deletion of a non-empty group (records an audit log entry). + schema: + type: boolean + default: false responses: "204": description: Deleted @@ -1366,6 +1373,12 @@ paths: application/json: schema: $ref: "#/components/schemas/ErrorResponse" + "409": + description: Group is not empty; pass force=true to confirm deletion. + content: + application/json: + schema: + $ref: "#/components/schemas/GroupDeletionConflict" # Stats /stats/home: @@ -1910,6 +1923,23 @@ components: - roleCount - createdAt + GroupDeletionConflict: + type: object + properties: + error: + type: string + memberCount: + type: integer + appCount: + type: integer + roleCount: + type: integer + required: + - error + - memberCount + - appCount + - roleCount + GroupDetail: type: object properties: diff --git a/lib/api-zod/src/generated/api.ts b/lib/api-zod/src/generated/api.ts index 2f23f30d..126ded34 100644 --- a/lib/api-zod/src/generated/api.ts +++ b/lib/api-zod/src/generated/api.ts @@ -1689,6 +1689,17 @@ export const DeleteGroupParams = zod.object({ id: zod.coerce.number(), }); +export const deleteGroupQueryForceDefault = false; + +export const DeleteGroupQueryParams = zod.object({ + force: zod.coerce + .boolean() + .default(deleteGroupQueryForceDefault) + .describe( + "Force deletion of a non-empty group (records an audit log entry).", + ), +}); + /** * @summary Get home screen stats */ diff --git a/lib/db/src/schema/audit-logs.ts b/lib/db/src/schema/audit-logs.ts new file mode 100644 index 00000000..e77d96b6 --- /dev/null +++ b/lib/db/src/schema/audit-logs.ts @@ -0,0 +1,14 @@ +import { pgTable, serial, timestamp, integer, varchar, jsonb } from "drizzle-orm/pg-core"; +import { usersTable } from "./users"; + +export const auditLogsTable = pgTable("audit_logs", { + id: serial("id").primaryKey(), + actorUserId: integer("actor_user_id").references(() => usersTable.id, { onDelete: "set null" }), + action: varchar("action", { length: 100 }).notNull(), + targetType: varchar("target_type", { length: 50 }).notNull(), + targetId: integer("target_id"), + metadata: jsonb("metadata"), + createdAt: timestamp("created_at", { withTimezone: true }).notNull().defaultNow(), +}); + +export type AuditLog = typeof auditLogsTable.$inferSelect; diff --git a/lib/db/src/schema/index.ts b/lib/db/src/schema/index.ts index ead13d16..19f1ac11 100644 --- a/lib/db/src/schema/index.ts +++ b/lib/db/src/schema/index.ts @@ -11,3 +11,4 @@ export * from "./app-opens"; export * from "./password-reset-tokens"; export * from "./notes"; export * from "./groups"; +export * from "./audit-logs";