Task #162: Let admins pre-set required permissions while creating an app
The "Required permissions" section was previously edit-only because `POST /api/apps/:id/permissions` needs an app id, leaving a brief window where a freshly created app was visible to everyone before the admin could re-open the dialog and gate it. The Add app dialog now lets the admin pick required permissions up front and the new app + its `app_permissions` rows are written in a single transaction. Changes: - `lib/api-spec/openapi.yaml`: extended `CreateAppBody` with an optional `permissionIds: integer[]` field. Ran `pnpm --filter @workspace/api-spec run codegen` so `lib/api-zod` and `lib/api-client-react` reflect it. - `artifacts/api-server/src/routes/apps.ts`: `POST /apps` now de-dupes and pre-validates `permissionIds`, returns 404 if any id is unknown (without creating the app), and inside one transaction inserts the app, the `app_permissions` rows (with `.onConflictDoNothing()` against the composite primary key), and a single `permission_audit` row (`previousIds: []`, `newIds: requestedIds`). After the transaction it also writes one `app.permission.add` audit_logs entry per inserted permission so the admin log mirrors the post-create flow. - `artifacts/tx-os/src/pages/admin.tsx`: added `permissionIds: number[]` to `AppForm`, a new `NewAppPermissionsPicker` component (rendered only in create mode — edit mode keeps the existing `AppPermissionsEditor` with its impact preview) that lets admins add/remove permissions locally before submit, and wired `handleSaveApp` to forward the selected ids when creating. Existing edit path strips the field so the update payload remains unchanged. - `replit.md`: documented the new picker and POST /api/apps behavior. No impact preview is shown in the create-mode picker because a brand new app starts with zero users seeing it, so adding permissions cannot hide it from anyone. Code-review follow-up: tightened input validation so non-integer or non-positive `permissionIds` now return 400 with a clear error instead of being silently dropped by the previous filter. The legacy single-add endpoint already used this exact 400 message, so behavior stays consistent across both create and update paths. Verification: - `pnpm --filter @workspace/api-spec run codegen` passes. - `pnpm --filter @workspace/api-server` typechecks with no new errors (executive-meetings.ts errors are pre-existing and unrelated). - Ran the existing app-permission test suites (`app-permission-audit.test.mjs`, `app-permissions-crud.test.mjs`, `app-permissions-impact.test.mjs`) directly — all 16 tests pass. - Ran an e2e Playwright test (login as admin → Add app → pick a permission → save → verify the row shows 1 restriction → reopen and confirm the assigned permission). All steps passed. Follow-up proposed: automated tests for the new create-with-permissions endpoint behavior (#231). Replit-Task-Id: c229777c-4036-4a6a-b4cb-05ccc18f6c9b
This commit is contained in:
riyadhafraa
@@ -232,7 +232,78 @@ router.post("/apps", requireAdmin, async (req, res): Promise<void> => {
|
||||
return;
|
||||
}
|
||||
|
||||
const [app] = await db.insert(appsTable).values(parsed.data).returning();
|
||||
// Accept an optional permissionIds[] alongside the app fields so admins
|
||||
// can gate the app at creation time. Pulling it out before the insert
|
||||
// keeps appsTable.values strictly typed against the Drizzle schema.
|
||||
const { permissionIds: rawPermissionIds, ...appValues } = parsed.data;
|
||||
// The zod schema already restricts permissionIds to numbers, but we
|
||||
// explicitly reject non-integer / non-positive values with 400 here so
|
||||
// a malformed request never silently drops ids — the admin should know
|
||||
// their gate request was wrong instead of getting a partially gated app.
|
||||
if (
|
||||
rawPermissionIds !== undefined &&
|
||||
rawPermissionIds.some((n) => !Number.isInteger(n) || n <= 0)
|
||||
) {
|
||||
res.status(400).json({
|
||||
error: "permissionIds must be an array of positive integers",
|
||||
});
|
||||
return;
|
||||
}
|
||||
const requestedPermissionIds = Array.from(new Set(rawPermissionIds ?? []));
|
||||
|
||||
// If permission ids were supplied, look them up upfront so we can 404
|
||||
// before creating the app row instead of leaving an orphaned app behind.
|
||||
// We also need the names later for the per-permission audit entries.
|
||||
const requestedPermissions = requestedPermissionIds.length > 0
|
||||
? await db
|
||||
.select({ id: permissionsTable.id, name: permissionsTable.name })
|
||||
.from(permissionsTable)
|
||||
.where(inArray(permissionsTable.id, requestedPermissionIds))
|
||||
: [];
|
||||
if (requestedPermissions.length !== requestedPermissionIds.length) {
|
||||
const foundIds = new Set(requestedPermissions.map((p) => p.id));
|
||||
const missing = requestedPermissionIds.filter((id) => !foundIds.has(id));
|
||||
res.status(404).json({
|
||||
error: `Permission(s) not found: ${missing.join(", ")}`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
// Create the app + insert its permission rows in a single transaction so
|
||||
// the app never exists in an "unrestricted" state when the admin asked
|
||||
// for permission gating. onConflictDoNothing keeps it idempotent against
|
||||
// the (app_id, permission_id) primary key.
|
||||
const { app, insertedPermissionIds } = await db.transaction(async (tx) => {
|
||||
const [created] = await tx.insert(appsTable).values(appValues).returning();
|
||||
let inserted: number[] = [];
|
||||
if (requestedPermissionIds.length > 0) {
|
||||
const ins = await tx
|
||||
.insert(appPermissionsTable)
|
||||
.values(
|
||||
requestedPermissionIds.map((permissionId) => ({
|
||||
appId: created.id,
|
||||
permissionId,
|
||||
})),
|
||||
)
|
||||
.onConflictDoNothing()
|
||||
.returning({ permissionId: appPermissionsTable.permissionId });
|
||||
inserted = ins.map((r) => r.permissionId);
|
||||
// Brand-new app starts with no permissions, so previousIds=[] and the
|
||||
// newIds is the sorted set of what we just attached. This matches the
|
||||
// single-add endpoint's audit semantics so the history view renders
|
||||
// a normal "added X" entry instead of a synthetic create marker.
|
||||
await recordPermissionAudit(tx, {
|
||||
targetKind: "app",
|
||||
targetId: created.id,
|
||||
changeKind: "app.permissions",
|
||||
actorUserId: req.session.userId ?? null,
|
||||
previousIds: [],
|
||||
newIds: requestedPermissionIds,
|
||||
});
|
||||
}
|
||||
return { app: created, insertedPermissionIds: inserted };
|
||||
});
|
||||
|
||||
await db.insert(auditLogsTable).values({
|
||||
actorUserId: req.session.userId ?? null,
|
||||
action: "app.create",
|
||||
@@ -244,8 +315,30 @@ router.post("/apps", requireAdmin, async (req, res): Promise<void> => {
|
||||
nameEn: app.nameEn,
|
||||
route: app.route,
|
||||
isActive: app.isActive,
|
||||
permissionIds: requestedPermissionIds,
|
||||
},
|
||||
});
|
||||
// Mirror the per-permission audit rows POST /apps/:id/permissions writes
|
||||
// so the admin log shows the same "permission added" entries whether the
|
||||
// gate was set at create time or in a follow-up dialog. We only emit
|
||||
// entries for rows the transaction actually inserted.
|
||||
if (insertedPermissionIds.length > 0) {
|
||||
const nameById = new Map(requestedPermissions.map((p) => [p.id, p.name]));
|
||||
await db.insert(auditLogsTable).values(
|
||||
insertedPermissionIds.map((permissionId) => ({
|
||||
actorUserId: req.session.userId ?? null,
|
||||
action: "app.permission.add",
|
||||
targetType: "app" as const,
|
||||
targetId: app.id,
|
||||
metadata: {
|
||||
slug: app.slug,
|
||||
nameEn: app.nameEn,
|
||||
permissionId,
|
||||
permissionName: nameById.get(permissionId) ?? null,
|
||||
},
|
||||
})),
|
||||
);
|
||||
}
|
||||
res.status(201).json(app);
|
||||
});
|
||||
|
||||
|
||||
Binary file not shown.
|
Before Width: | Height: | Size: 25 KiB After Width: | Height: | Size: 24 KiB |
@@ -164,6 +164,11 @@ type AppForm = {
|
||||
route: string;
|
||||
color: string;
|
||||
sortOrder: number;
|
||||
// Only used when creating a new app — admins can pre-set the required
|
||||
// permissions so the app is never visible to everyone in the brief
|
||||
// window between insert and the follow-up gate. Existing apps keep
|
||||
// editing their gate through the AppPermissionsEditor below.
|
||||
permissionIds: number[];
|
||||
};
|
||||
|
||||
type ServiceForm = {
|
||||
@@ -252,6 +257,109 @@ function DeletionWarningDialog({
|
||||
);
|
||||
}
|
||||
|
||||
// Local-state permission picker used by the "Add app" dialog. The full
|
||||
// AppPermissionsEditor below talks to /apps/:id/permissions, which can't
|
||||
// run before the app exists. This picker just collects ids into the form
|
||||
// state; handleSaveApp sends them inside the same POST /apps payload so
|
||||
// the new app never appears unrestricted (the goal of the feature). No
|
||||
// impact preview is shown here because a brand-new app starts with zero
|
||||
// users seeing it, so adding permissions can't hide it from anyone.
|
||||
function NewAppPermissionsPicker({
|
||||
selectedIds,
|
||||
onChange,
|
||||
}: {
|
||||
selectedIds: number[];
|
||||
onChange: (next: number[]) => void;
|
||||
}) {
|
||||
const { t } = useTranslation();
|
||||
const { data: allPermissions } = useListPermissions({
|
||||
query: { queryKey: getListPermissionsQueryKey() },
|
||||
});
|
||||
const [pendingId, setPendingId] = useState<number | "">("");
|
||||
|
||||
const selectedSet = useMemo(() => new Set(selectedIds), [selectedIds]);
|
||||
const selected = useMemo(
|
||||
() => (allPermissions ?? []).filter((p) => selectedSet.has(p.id)),
|
||||
[allPermissions, selectedSet],
|
||||
);
|
||||
const available = useMemo(
|
||||
() => (allPermissions ?? []).filter((p) => !selectedSet.has(p.id)),
|
||||
[allPermissions, selectedSet],
|
||||
);
|
||||
|
||||
const onAdd = () => {
|
||||
if (pendingId === "" || typeof pendingId !== "number") return;
|
||||
if (!selectedSet.has(pendingId)) onChange([...selectedIds, pendingId]);
|
||||
setPendingId("");
|
||||
};
|
||||
const onRemove = (id: number) => {
|
||||
onChange(selectedIds.filter((x) => x !== id));
|
||||
};
|
||||
|
||||
return (
|
||||
<div
|
||||
className="space-y-2 border-t border-slate-200/70 pt-3"
|
||||
data-testid="app-permissions-picker"
|
||||
>
|
||||
<Label>{t("admin.appPermissions.title")}</Label>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t("admin.appPermissions.help")}
|
||||
</p>
|
||||
{selected.length === 0 ? (
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t("admin.appPermissions.none")}
|
||||
</p>
|
||||
) : (
|
||||
<ul className="space-y-1" data-testid="app-permissions-picker-list">
|
||||
{selected.map((p) => (
|
||||
<li
|
||||
key={p.id}
|
||||
className="flex items-center justify-between gap-2 rounded-xl bg-white/70 border border-slate-200 px-3 py-1.5"
|
||||
>
|
||||
<span className="text-sm text-foreground truncate">{p.name}</span>
|
||||
<button
|
||||
type="button"
|
||||
aria-label={t("admin.appPermissions.removeAria", { name: p.name })}
|
||||
className="text-muted-foreground hover:text-destructive transition-colors"
|
||||
onClick={() => onRemove(p.id)}
|
||||
data-testid={`app-permission-picker-remove-${p.id}`}
|
||||
>
|
||||
<Trash2 size={14} />
|
||||
</button>
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
)}
|
||||
<div className="flex items-center gap-2">
|
||||
<select
|
||||
className="flex-1 rounded-xl bg-white/70 border border-slate-200 px-3 py-2 text-sm"
|
||||
value={pendingId === "" ? "" : String(pendingId)}
|
||||
onChange={(e) =>
|
||||
setPendingId(e.target.value === "" ? "" : Number(e.target.value))
|
||||
}
|
||||
disabled={available.length === 0}
|
||||
data-testid="app-permission-picker-select"
|
||||
>
|
||||
<option value="">{t("admin.appPermissions.selectPlaceholder")}</option>
|
||||
{available.map((p) => (
|
||||
<option key={p.id} value={p.id}>
|
||||
{p.name}
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
<Button
|
||||
size="sm"
|
||||
onClick={onAdd}
|
||||
disabled={pendingId === "" || available.length === 0}
|
||||
data-testid="app-permission-picker-add"
|
||||
>
|
||||
{t("admin.appPermissions.add")}
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function AppPermissionsEditor({ appId }: { appId: number }) {
|
||||
const { t } = useTranslation();
|
||||
const queryClient = useQueryClient();
|
||||
@@ -723,6 +831,7 @@ export default function AdminPage() {
|
||||
route: app.route ?? "/",
|
||||
color: app.color ?? "#6366f1",
|
||||
sortOrder: app.sortOrder ?? 0,
|
||||
permissionIds: [],
|
||||
},
|
||||
});
|
||||
}
|
||||
@@ -863,14 +972,19 @@ export default function AdminPage() {
|
||||
);
|
||||
};
|
||||
|
||||
const emptyAppForm: AppForm = { nameAr: "", nameEn: "", slug: "", iconName: "Grid2X2", route: "/", color: "#6366f1", sortOrder: 0 };
|
||||
const emptyAppForm: AppForm = { nameAr: "", nameEn: "", slug: "", iconName: "Grid2X2", route: "/", color: "#6366f1", sortOrder: 0, permissionIds: [] };
|
||||
const emptyServiceForm: ServiceForm = { nameAr: "", nameEn: "", descriptionAr: "", descriptionEn: "", price: "0.00", imageUrl: "", isAvailable: true };
|
||||
|
||||
const handleSaveApp = () => {
|
||||
if (!editingApp) return;
|
||||
if (editingApp.id) {
|
||||
// Edit path: permissionIds aren't part of the update payload — the
|
||||
// existing AppPermissionsEditor manages them with its own impact
|
||||
// preview + audit, so strip the field before sending.
|
||||
const { permissionIds: _ignored, ...updateData } = editingApp.form;
|
||||
void _ignored;
|
||||
updateApp.mutate(
|
||||
{ id: editingApp.id, data: editingApp.form },
|
||||
{ id: editingApp.id, data: updateData },
|
||||
{
|
||||
onSuccess: () => {
|
||||
queryClient.invalidateQueries({ queryKey: ADMIN_APPS_KEY });
|
||||
@@ -880,8 +994,17 @@ export default function AdminPage() {
|
||||
},
|
||||
);
|
||||
} else {
|
||||
const { permissionIds, ...rest } = editingApp.form;
|
||||
createApp.mutate(
|
||||
{ data: editingApp.form },
|
||||
{
|
||||
// Only include permissionIds if the admin actually picked some
|
||||
// so the request body stays minimal and the server's "no
|
||||
// restriction" path is unchanged.
|
||||
data:
|
||||
permissionIds.length > 0
|
||||
? { ...rest, permissionIds }
|
||||
: rest,
|
||||
},
|
||||
{
|
||||
onSuccess: () => {
|
||||
queryClient.invalidateQueries({ queryKey: ADMIN_APPS_KEY });
|
||||
@@ -995,8 +1118,18 @@ export default function AdminPage() {
|
||||
/>
|
||||
</div>
|
||||
))}
|
||||
{editingApp.id !== undefined && (
|
||||
{editingApp.id !== undefined ? (
|
||||
<AppPermissionsEditor appId={editingApp.id} />
|
||||
) : (
|
||||
<NewAppPermissionsPicker
|
||||
selectedIds={editingApp.form.permissionIds}
|
||||
onChange={(next) =>
|
||||
setEditingApp({
|
||||
...editingApp,
|
||||
form: { ...editingApp.form, permissionIds: next },
|
||||
})
|
||||
}
|
||||
/>
|
||||
)}
|
||||
{editingApp.id !== undefined && (
|
||||
<PermissionAuditHistory
|
||||
@@ -1218,6 +1351,7 @@ export default function AdminPage() {
|
||||
route: app.route ?? "/",
|
||||
color: app.color ?? "#6366f1",
|
||||
sortOrder: app.sortOrder ?? 0,
|
||||
permissionIds: [],
|
||||
},
|
||||
})}
|
||||
className="p-1.5 hover:bg-slate-100 rounded-lg text-muted-foreground hover:text-foreground"
|
||||
|
||||
@@ -425,6 +425,8 @@ export interface CreateAppBody {
|
||||
isActive?: boolean;
|
||||
isSystem?: boolean;
|
||||
sortOrder?: number;
|
||||
/** Optional set of permission IDs to gate the new app on. Inserted with onConflictDoNothing so duplicates are tolerated. */
|
||||
permissionIds?: number[];
|
||||
}
|
||||
|
||||
export interface UpdateAppBody {
|
||||
|
||||
@@ -3270,6 +3270,11 @@ components:
|
||||
type: boolean
|
||||
sortOrder:
|
||||
type: integer
|
||||
permissionIds:
|
||||
type: array
|
||||
description: Optional set of permission IDs to gate the new app on. Inserted with onConflictDoNothing so duplicates are tolerated.
|
||||
items:
|
||||
type: integer
|
||||
required:
|
||||
- slug
|
||||
- nameAr
|
||||
|
||||
@@ -331,6 +331,12 @@ export const CreateAppBody = zod.object({
|
||||
isActive: zod.boolean().optional(),
|
||||
isSystem: zod.boolean().optional(),
|
||||
sortOrder: zod.number().optional(),
|
||||
permissionIds: zod
|
||||
.array(zod.number())
|
||||
.optional()
|
||||
.describe(
|
||||
"Optional set of permission IDs to gate the new app on. Inserted with onConflictDoNothing so duplicates are tolerated.",
|
||||
),
|
||||
});
|
||||
|
||||
/**
|
||||
|
||||
@@ -38,7 +38,7 @@
|
||||
- **خدماتي (My Services)**: service card grid with availability status
|
||||
- **Internal Chat**: real-time messages via Socket.IO, conversation list
|
||||
- **Notifications**: unread tracking, mark-all-read
|
||||
- **Admin Panel**: CRUD for apps, services, users (admin role required). Delete dialogs show a dependency warning on the FIRST click using count fields (`groupCount`/`restrictionCount`/`openCount` on apps, `orderCount` on services, `noteCount`/`orderCount`/`conversationCount`/`messageCount` on users) returned by the list endpoints (`GET /api/admin/apps`, `GET /api/services`, `GET /api/users`); the lazy 409 conflict response from `DELETE /api/{apps,services,users}/:id` (with `?force=true` to override) remains as a safety net.
|
||||
- **Admin Panel**: CRUD for apps, services, users (admin role required). Delete dialogs show a dependency warning on the FIRST click using count fields (`groupCount`/`restrictionCount`/`openCount` on apps, `orderCount` on services, `noteCount`/`orderCount`/`conversationCount`/`messageCount` on users) returned by the list endpoints (`GET /api/admin/apps`, `GET /api/services`, `GET /api/users`); the lazy 409 conflict response from `DELETE /api/{apps,services,users}/:id` (with `?force=true` to override) remains as a safety net. The Add App dialog includes a `NewAppPermissionsPicker` that lets admins pre-set `permissionIds[]` so `POST /api/apps` creates the app and inserts its `app_permissions` rows in the same transaction (with `onConflictDoNothing`), avoiding the brief unrestricted window between create and follow-up gating.
|
||||
- **Session-based Auth**: RBAC with roles (admin/user)
|
||||
- **Executive Meetings (Phase 2)**: bilingual full-stack module under `/executive-meetings` with 9 sections — Schedule (centered cells, attendees widest column, RTL-locked column order # / الاجتماع / الحضور / الوقت), Manage Meetings (CRUD with attendee replace, transactional), Change Requests (submit / withdraw, supports `meetingId=null` for create-suggestions), Approvals (approve/reject with review notes), Tasks (CRUD with assignee status updates — assignees and mutators can change status, only mutators can delete), Notifications (per-user feed; meeting/request/task events fan out via `recordExecutiveMeetingNotifications` to both `executive_meeting_notifications` and the global `notifications` bell, then broadcast via Socket.IO `notification_created` per-user + `executive_meeting_notifications_changed` globally; approvers also receive a best-effort email side-channel via `sendExecutiveMeetingEmail` that logs an outbox entry until SMTP is wired up), Audit Log (full action chain, admin role required), PDF (window.print export), Font Settings (per-user + global scope, family/size/weight/alignment with live preview). RBAC enforced via 5 role sets (READ/MUTATE/APPROVE/REQUEST/ADMIN_AUDIT) and a `makeRequireRoles` middleware factory; `/api/executive-meetings/me` returns `{userId, roles, canRead, canMutate, canApprove, canSubmitRequest, canViewAudit}`. Every mutation (meeting/request/task/font CRUD) wraps the DB write **and** the audit-log insert in the same `db.transaction(...)` so audit entries cannot drift from state. Routes use `router.param("id")` with `next("route")` to handle path-to-regexp 8 (no inline `:id(\\d+)` support).
|
||||
|
||||
|
||||
Reference in New Issue
Block a user