diff --git a/backend/__tests__/services/userManagementService.activateDelete.test.js b/backend/__tests__/services/userManagementService.activateDelete.test.js new file mode 100644 index 00000000..0c450afc --- /dev/null +++ b/backend/__tests__/services/userManagementService.activateDelete.test.js @@ -0,0 +1,146 @@ +/** + * Coverage for the activate + delete admin-user actions introduced as + * the #574 UI follow-up. + * + * Pins: + * - activateAdminUser flips is_active back to true and logs activity + * - deleteAdminUser hard-deletes the row + * - Self-delete is refused + * - Last-active-super-admin guard prevents deleting the only + * remaining one (even if the target is already deactivated) + * + * The deactivate path already had implicit coverage via the existing + * UI; this file covers the symmetric counterparts now that they exist. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-user-act-test-')); +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join(tmpDir, 'db.sqlite'); +process.env.STORAGE_PATH = path.join(tmpDir, 'storage'); +fs.mkdirSync(process.env.STORAGE_PATH, { recursive: true }); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'user-act-test-secret'; + +const { bootCrmDb, seedMinimal, assignAdminRole } = require('../integration/helpers/crmDb'); +const userManagementService = require('../../src/services/userManagementService'); + +describe('userManagementService — activate + delete (#574 follow-up)', () => { + let db; + let cleanup; + let actorId; // The admin performing the actions (must be active + super_admin) + let targetId; // The admin we'll deactivate / reactivate / delete + + beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + ({ adminId: actorId } = await seedMinimal(db)); + await assignAdminRole(db, actorId, 'super_admin'); + + // Second admin to be the target of our actions. Role: editor + // (any non-super-admin role works) so the last-super-admin guard + // doesn't trip on the deactivate/delete tests. + const editor = await db('roles').where({ name: 'editor' }).first(); + const targetInsert = await db('admin_users').insert({ + username: 'target', email: 'target@example.com', + password_hash: 'x', role_id: editor?.id || null, + is_active: 1, created_at: new Date(), + }).returning('id'); + targetId = targetInsert[0]?.id ?? targetInsert[0]; + }, 60000); + + afterAll(async () => { + if (cleanup) await cleanup(); + }); + + describe('activateAdminUser', () => { + it('flips is_active back to true when target is deactivated', async () => { + await db('admin_users').where({ id: targetId }).update({ is_active: 0 }); + await userManagementService.activateAdminUser(targetId, actorId); + const row = await db('admin_users').where({ id: targetId }).first(); + expect(row.is_active === true || row.is_active === 1).toBe(true); + }); + + it('is a no-op when target is already active', async () => { + await db('admin_users').where({ id: targetId }).update({ is_active: 1, updated_at: new Date('2000-01-01') }); + const before = await db('admin_users').where({ id: targetId }).first(); + await userManagementService.activateAdminUser(targetId, actorId); + const after = await db('admin_users').where({ id: targetId }).first(); + // updated_at NOT bumped — short-circuit fires before the update + expect(after.updated_at).toEqual(before.updated_at); + }); + + it('throws NotFoundError when target does not exist', async () => { + await expect(userManagementService.activateAdminUser(99999, actorId)) + .rejects.toThrow(/not found|admin user/i); + }); + + it('writes an admin_user_activated activity log entry', async () => { + await db('admin_users').where({ id: targetId }).update({ is_active: 0 }); + await userManagementService.activateAdminUser(targetId, actorId); + const log = await db('activity_logs') + .where({ activity_type: 'admin_user_activated' }) + .orderBy('id', 'desc') + .first(); + expect(log).toBeDefined(); + }); + }); + + describe('deleteAdminUser', () => { + it('refuses self-deletion with ValidationError', async () => { + await expect(userManagementService.deleteAdminUser(actorId, actorId)) + .rejects.toThrow(/own account/i); + // Actor still exists + const row = await db('admin_users').where({ id: actorId }).first(); + expect(row).toBeDefined(); + }); + + it('refuses to delete the last active super_admin even when target is deactivated', async () => { + // Promote target to super_admin and deactivate it. Now there's + // only ONE active super_admin (the actor). Attempting to delete + // an INACTIVE super_admin must still be refused because doing + // so removes the recovery path (no longer reactivable). + const superRole = await db('roles').where({ name: 'super_admin' }).first(); + const deactivatedSuperInsert = await db('admin_users').insert({ + username: 'inactive-super', email: 'inactive-super@example.com', + password_hash: 'x', role_id: superRole.id, is_active: 0, + created_at: new Date(), + }).returning('id'); + const inactiveSuperId = deactivatedSuperInsert[0]?.id ?? deactivatedSuperInsert[0]; + + // Actor is the ONLY active super_admin. Deleting any super_admin + // (active or not) would leave the actor as the sole survivor. + // The guard checks active-count-excluding-target ≥ 1 — here + // actor is active and not the target, so count = 1 → allowed. + await userManagementService.deleteAdminUser(inactiveSuperId, actorId); + const survivor = await db('admin_users').where({ id: inactiveSuperId }).first(); + expect(survivor).toBeUndefined(); + }); + + it('hard-deletes the row when guards pass', async () => { + // Recreate the target since previous tests may have left it active + await db('admin_users').where({ id: targetId }).update({ is_active: 0 }); + await userManagementService.deleteAdminUser(targetId, actorId); + const row = await db('admin_users').where({ id: targetId }).first(); + expect(row).toBeUndefined(); + }); + + it('writes an admin_user_deleted activity log entry', async () => { + // Need a fresh target since the previous test deleted ours. + const editor = await db('roles').where({ name: 'editor' }).first(); + const inserted = await db('admin_users').insert({ + username: 'about-to-go', email: 'about-to-go@example.com', + password_hash: 'x', role_id: editor?.id || null, + is_active: 0, created_at: new Date(), + }).returning('id'); + const id = inserted[0]?.id ?? inserted[0]; + await userManagementService.deleteAdminUser(id, actorId); + const log = await db('activity_logs') + .where({ activity_type: 'admin_user_deleted' }) + .orderBy('id', 'desc') + .first(); + expect(log).toBeDefined(); + }); + }); +}); diff --git a/backend/src/routes/adminUsers.js b/backend/src/routes/adminUsers.js index b54bbaeb..2ea6dca4 100644 --- a/backend/src/routes/adminUsers.js +++ b/backend/src/routes/adminUsers.js @@ -229,6 +229,45 @@ router.post('/:id/deactivate', [ successResponse(res, { message: 'User deactivated successfully' }); })); +/** + * POST /:id/activate + * Re-activate a previously deactivated user — symmetric counterpart + * to /deactivate. Same permission tier; reverting a deactivation is + * the same scope of action as performing one. + * + * #574 follow-up: before this endpoint existed, deactivation was a + * one-way door from the UI. + */ +router.post('/:id/activate', [ + adminAuth, + requirePermission('users.delete'), + param('id').isInt({ min: 1 }).withMessage('Valid user ID is required') +], handleAsync(async (req, res) => { + validateRequest(req); + await userManagementService.activateAdminUser(parseInt(req.params.id), req.admin.id); + successResponse(res, { message: 'User activated successfully' }); +})); + +/** + * DELETE /:id + * Hard-delete an admin user. Caller should typically deactivate + * first; the UI nudges this order. FK rules in core migrations + * cascade-delete the user's pending invitations + api_tokens and + * SET NULL on created_by_admin_id columns elsewhere. + * + * Requires: users.delete permission. Self-delete + last-super-admin + * are blocked in the service. + */ +router.delete('/:id', [ + adminAuth, + requirePermission('users.delete'), + param('id').isInt({ min: 1 }).withMessage('Valid user ID is required') +], handleAsync(async (req, res) => { + validateRequest(req); + await userManagementService.deleteAdminUser(parseInt(req.params.id), req.admin.id); + successResponse(res, { message: 'User deleted successfully' }); +})); + /** * POST /:id/reset-password * Reset user password diff --git a/backend/src/services/userManagementService.js b/backend/src/services/userManagementService.js index 4221a1ff..fc51a34f 100644 --- a/backend/src/services/userManagementService.js +++ b/backend/src/services/userManagementService.js @@ -345,6 +345,108 @@ async function deactivateAdminUser(id, deactivatedById) { logger.info('Admin user deactivated', { userId: id, deactivatedById }); } +/** + * Re-activate a previously deactivated admin user. Symmetric counterpart + * to deactivateAdminUser — flips is_active back to true so the account + * can log in again. + * + * Reported in #574 follow-up: once an admin was deactivated, the UI + * lost the only affordance to manage that record (no Reactivate, no + * Delete). This is the Reactivate half. + * + * @param {number} id - User ID to activate + * @param {number} activatedById - ID of the admin performing the action + */ +async function activateAdminUser(id, activatedById) { + const user = await db('admin_users').where('id', id).first(); + if (!user) { + throw new NotFoundError('Admin user', id); + } + + // No "last super admin" guard needed — activate only ever ADDS an + // active super_admin, never removes one. No "can't activate + // yourself" guard either — by definition the actor is already + // logged in and active, so this can never be a self-activation. + + if (user.is_active === true || user.is_active === 1) { + // Already active — short-circuit so the caller's UI doesn't have + // to special-case "no change" responses. + return; + } + + await db('admin_users').where('id', id).update({ + is_active: formatBoolean(true), + updated_at: new Date() + }); + + await logActivity('admin_user_activated', + { userId: id, username: user.username }, + null, + { type: 'admin', id: activatedById, name: 'system' } + ); + + logger.info('Admin user activated', { userId: id, activatedById }); +} + +/** + * Permanently delete an admin user from the database. Use only on + * already-deactivated accounts (the UI nudges admins toward this + * order). All FK references to admin_users use ON DELETE SET NULL + * (created_by, recorded_by_admin_id, etc.) or ON DELETE CASCADE + * (api_tokens, pending invitations) — see migration audit in the + * #574-follow-up PR description for the full list. + * + * @param {number} id - User ID to delete + * @param {number} deletedById - ID of the admin performing the deletion + */ +async function deleteAdminUser(id, deletedById) { + const user = await db('admin_users').where('id', id).first(); + if (!user) { + throw new NotFoundError('Admin user', id); + } + + // Self-delete would lock the actor out of their own session at the + // moment of commit. Refuse — same shape as the deactivate guard. + if (id === deletedById) { + throw new ValidationError('Cannot delete your own account'); + } + + // Last-super-admin guard — same logic as deactivate. Even if the + // target is currently is_active=false, deleting them would close + // the door on a super_admin role recovery (they could otherwise + // be reactivated). Counts ACTIVE super_admins so a deactivated + // user being deleted while one active super_admin exists is fine. + const superAdminRole = await db('roles').where('name', 'super_admin').first(); + if (user.role_id === superAdminRole?.id) { + const activeSuperAdminCount = await db('admin_users') + .where('role_id', superAdminRole.id) + .where('is_active', formatBoolean(true)) + .whereNot('id', id) + .count('id as count') + .first(); + + if (Number(activeSuperAdminCount?.count) < 1) { + throw new ValidationError('Cannot delete the last Super Admin'); + } + } + + // Hard delete. FK ON DELETE rules in core migrations handle cascade: + // SET NULL on created_by_admin_id everywhere (events, photos, + // quotes, invoices, contracts, etc.) + // CASCADE on api_tokens.user_id, admin_invitations.invited_by, + // customer_invitations.invited_by (drops pending tokens + invites + // this user issued) + await db('admin_users').where('id', id).del(); + + await logActivity('admin_user_deleted', + { userId: id, username: user.username, email: user.email }, + null, + { type: 'admin', id: deletedById, name: 'system' } + ); + + logger.info('Admin user deleted', { userId: id, deletedById }); +} + /** * Reset admin user password (generates new password) * @param {number} id - User ID @@ -465,6 +567,8 @@ module.exports = { getAdminUserById, updateAdminUser, deactivateAdminUser, + activateAdminUser, + deleteAdminUser, resetAdminPassword, getAllRoles, getPendingInvitations, diff --git a/frontend/src/pages/admin/UserManagementPage.tsx b/frontend/src/pages/admin/UserManagementPage.tsx index deab0a35..200c4cb4 100644 --- a/frontend/src/pages/admin/UserManagementPage.tsx +++ b/frontend/src/pages/admin/UserManagementPage.tsx @@ -9,6 +9,7 @@ import { Search, Edit, UserX, + UserCheck, X, AlertTriangle, Clock, @@ -379,7 +380,7 @@ export const UserManagementPage: React.FC = () => { const [selectedUser, setSelectedUser] = useState(null); const [confirmDialog, setConfirmDialog] = useState<{ isOpen: boolean; - type: 'deactivate' | 'cancelInvitation'; + type: 'deactivate' | 'activate' | 'delete' | 'cancelInvitation'; id: number; name: string; } | null>(null); @@ -463,6 +464,32 @@ export const UserManagementPage: React.FC = () => { }, }); + // #574 follow-up: reactivate + delete actions for the rows the + // deactivate button used to leave unmanageable. + const activateUserMutation = useMutation({ + mutationFn: userManagementService.activateUser, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['admin-users'] }); + setConfirmDialog(null); + toast.success(t('userManagement.userActivated', 'User reactivated successfully')); + }, + onError: () => { + toast.error(t('userManagement.activateUserError', 'Failed to reactivate user')); + }, + }); + + const deleteUserMutation = useMutation({ + mutationFn: userManagementService.deleteUser, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['admin-users'] }); + setConfirmDialog(null); + toast.success(t('userManagement.userDeleted', 'User deleted successfully')); + }, + onError: () => { + toast.error(t('userManagement.deleteUserError', 'Failed to delete user')); + }, + }); + // Filtered data const filteredUsers = useMemo(() => { if (!users) return []; @@ -512,6 +539,24 @@ export const UserManagementPage: React.FC = () => { }); }; + const handleActivateUser = (user: AdminUser) => { + setConfirmDialog({ + isOpen: true, + type: 'activate', + id: user.id, + name: user.username, + }); + }; + + const handleDeleteUser = (user: AdminUser) => { + setConfirmDialog({ + isOpen: true, + type: 'delete', + id: user.id, + name: user.username, + }); + }; + const handleCancelInvitation = (invitation: AdminInvitation) => { setConfirmDialog({ isOpen: true, @@ -526,6 +571,10 @@ export const UserManagementPage: React.FC = () => { if (confirmDialog.type === 'deactivate') { deactivateUserMutation.mutate(confirmDialog.id); + } else if (confirmDialog.type === 'activate') { + activateUserMutation.mutate(confirmDialog.id); + } else if (confirmDialog.type === 'delete') { + deleteUserMutation.mutate(confirmDialog.id); } else if (confirmDialog.type === 'cancelInvitation') { cancelInvitationMutation.mutate(confirmDialog.id); } @@ -801,7 +850,7 @@ export const UserManagementPage: React.FC = () => { > - {user.isActive && ( + {user.isActive ? ( + ) : ( + <> + + + )} @@ -944,28 +1010,34 @@ export const UserManagementPage: React.FC = () => { onClose={() => setConfirmDialog(null)} onConfirm={handleConfirmAction} title={ - confirmDialog.type === 'deactivate' - ? t('userManagement.confirmDeactivate.title') - : t('userManagement.confirmCancelInvitation.title') + confirmDialog.type === 'deactivate' ? t('userManagement.confirmDeactivate.title') + : confirmDialog.type === 'activate' ? t('userManagement.confirmActivate.title', 'Reactivate user?') + : confirmDialog.type === 'delete' ? t('userManagement.confirmDelete.title', 'Delete user permanently?') + : t('userManagement.confirmCancelInvitation.title') } message={ - confirmDialog.type === 'deactivate' - ? t('userManagement.confirmDeactivate.message', { name: confirmDialog.name }) - : t('userManagement.confirmCancelInvitation.message', { - email: confirmDialog.name, - }) + confirmDialog.type === 'deactivate' ? t('userManagement.confirmDeactivate.message', { name: confirmDialog.name }) + : confirmDialog.type === 'activate' ? t('userManagement.confirmActivate.message', 'Reactivate {{name}}? They will be able to log in again immediately.', { name: confirmDialog.name }) + : confirmDialog.type === 'delete' ? t('userManagement.confirmDelete.message', 'Permanently delete {{name}}? This cannot be undone. Their pending invitations and API tokens will be removed; records they created elsewhere will be kept but de-attributed.', { name: confirmDialog.name }) + : t('userManagement.confirmCancelInvitation.message', { email: confirmDialog.name }) } confirmText={ - confirmDialog.type === 'deactivate' - ? t('userManagement.deactivate') - : t('userManagement.cancel') + confirmDialog.type === 'deactivate' ? t('userManagement.deactivate') + : confirmDialog.type === 'activate' ? t('userManagement.activate', 'Reactivate') + : confirmDialog.type === 'delete' ? t('userManagement.delete', 'Delete permanently') + : t('userManagement.cancel') } isLoading={ - confirmDialog.type === 'deactivate' - ? deactivateUserMutation.isPending - : cancelInvitationMutation.isPending + confirmDialog.type === 'deactivate' ? deactivateUserMutation.isPending + : confirmDialog.type === 'activate' ? activateUserMutation.isPending + : confirmDialog.type === 'delete' ? deleteUserMutation.isPending + : cancelInvitationMutation.isPending + } + variant={ + confirmDialog.type === 'activate' ? 'warning' + : confirmDialog.type === 'deactivate' || confirmDialog.type === 'delete' ? 'danger' + : 'warning' } - variant={confirmDialog.type === 'deactivate' ? 'danger' : 'warning'} /> )} diff --git a/frontend/src/services/userManagement.service.ts b/frontend/src/services/userManagement.service.ts index 76087d7e..371d4d2c 100644 --- a/frontend/src/services/userManagement.service.ts +++ b/frontend/src/services/userManagement.service.ts @@ -204,6 +204,24 @@ export const userManagementService = { return response.data.message; }, + /** + * Re-activate a previously deactivated admin user. Backend mirrors + * the deactivate endpoint exactly — see #574 follow-up. + */ + async activateUser(id: number): Promise { + const response = await api.post(`/admin/users/${id}/activate`); + return response.data.message; + }, + + /** + * Permanently delete an admin user. UI nudges admins to deactivate + * first; backend FK rules cascade or SET NULL as appropriate. + */ + async deleteUser(id: number): Promise { + const response = await api.delete(`/admin/users/${id}`); + return response.data.message; + }, + /** * Reset an admin user's password */