From bf9fdd4f8deb26ec33208628daa5e53d3ac07981 Mon Sep 17 00:00:00 2001 From: Simon Hornby Date: Mon, 10 Feb 2025 14:17:46 +0200 Subject: [PATCH] feat: allow SCIM user deletion (#9190) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gastón Fournier --- .../ScimSettings/ScimDeleteUsersDialog.tsx | 36 +++++++ .../admin/auth/ScimSettings/ScimSettings.tsx | 100 ++++++++++++++++++ .../component/admin/groups/Group/Group.tsx | 5 +- .../GroupsList/GroupCard/GroupCardActions.tsx | 1 - .../users/UsersList/DeleteUser/DeleteUser.tsx | 3 + .../UsersActionsCell/UsersActionsCell.tsx | 3 +- .../useAdminUsersApi/useAdminUsersApi.ts | 14 +++ .../api/actions/useGroupApi/useGroupApi.ts | 10 ++ src/lib/db/group-store.ts | 4 + src/lib/db/user-store.ts | 4 + src/lib/routes/admin-api/user-admin.ts | 28 ++++- src/lib/services/group-service.ts | 11 ++ src/lib/services/user-service.ts | 12 +++ src/lib/types/events.ts | 28 +++++ src/lib/types/stores/group-store.ts | 2 + src/lib/types/stores/user-store.ts | 1 + src/test/e2e/api/admin/user-admin.e2e.test.ts | 29 +++++ .../e2e/api/admin/user-admin.scim.e2e.test.ts | 24 +++-- src/test/fixtures/fake-group-store.ts | 4 + src/test/fixtures/fake-user-store.ts | 4 + 20 files changed, 308 insertions(+), 15 deletions(-) create mode 100644 frontend/src/component/admin/auth/ScimSettings/ScimDeleteUsersDialog.tsx diff --git a/frontend/src/component/admin/auth/ScimSettings/ScimDeleteUsersDialog.tsx b/frontend/src/component/admin/auth/ScimSettings/ScimDeleteUsersDialog.tsx new file mode 100644 index 0000000000..38537913c0 --- /dev/null +++ b/frontend/src/component/admin/auth/ScimSettings/ScimDeleteUsersDialog.tsx @@ -0,0 +1,36 @@ +import { Alert, styled, Typography } from '@mui/material'; +import { Dialogue } from 'component/common/Dialogue/Dialogue'; + +const StyledAlert = styled(Alert)(({ theme }) => ({ + marginBottom: theme.spacing(3), +})); + +export type EntityType = 'Users' | 'Groups'; + +interface IScimDeleteUsersProps { + open: boolean; + entityType: EntityType; + closeDialog: () => void; + deleteEntities: () => void; +} + +export const ScimDeleteEntityDialog = ({ + open, + closeDialog, + deleteEntities: removeUser, + entityType, +}: IScimDeleteUsersProps) => ( + + + This will delete all {entityType.toLocaleLowerCase()} created or + managed by SCIM. + + +); diff --git a/frontend/src/component/admin/auth/ScimSettings/ScimSettings.tsx b/frontend/src/component/admin/auth/ScimSettings/ScimSettings.tsx index 5efcee8330..b159441f44 100644 --- a/frontend/src/component/admin/auth/ScimSettings/ScimSettings.tsx +++ b/frontend/src/component/admin/auth/ScimSettings/ScimSettings.tsx @@ -9,6 +9,9 @@ import { formatUnknownError } from 'utils/formatUnknownError'; import useToast from 'hooks/useToast'; import { useScimSettingsApi } from 'hooks/api/actions/useScimSettingsApi/useScimSettingsApi'; import { useScimSettings } from 'hooks/api/getters/useScimSettings/useScimSettings'; +import { ScimDeleteEntityDialog } from './ScimDeleteUsersDialog'; +import useAdminUsersApi from 'hooks/api/actions/useAdminUsersApi/useAdminUsersApi'; +import { useGroupApi } from 'hooks/api/actions/useGroupApi/useGroupApi'; const StyledContainer = styled('div')(({ theme }) => ({ padding: theme.spacing(3), @@ -25,8 +28,12 @@ export const ScimSettings = () => { const { setToastData, setToastApiError } = useToast(); const [newToken, setNewToken] = useState(''); const [tokenGenerationDialog, setTokenGenerationDialog] = useState(false); + const [deleteGroupsDialog, setDeleteGroupsDialog] = useState(false); + const [deleteUsersDialog, setDeleteUsersDialog] = useState(false); const [tokenDialog, setTokenDialog] = useState(false); const { settings, refetch } = useScimSettings(); + const { deleteScimUsers } = useAdminUsersApi(); + const { deleteScimGroups } = useGroupApi(); const [enabled, setEnabled] = useState(settings.enabled ?? true); useEffect(() => { @@ -40,6 +47,34 @@ export const ScimSettings = () => { setTokenGenerationDialog(true); }; + const onDeleteScimGroups = async () => { + try { + await deleteScimGroups(); + setToastData({ + text: 'Scim Groups have been deleted', + type: 'success', + }); + setDeleteGroupsDialog(false); + refetch(); + } catch (error: unknown) { + setToastApiError(formatUnknownError(error)); + } + }; + + const onDeleteScimUsers = async () => { + try { + await deleteScimUsers(); + setToastData({ + text: 'Scim Users have been deleted', + type: 'success', + }); + setDeleteUsersDialog(false); + refetch(); + } catch (error: unknown) { + setToastApiError(formatUnknownError(error)); + } + }; + const onGenerateNewTokenConfirm = async () => { setTokenGenerationDialog(false); const token = await generateNewToken(); @@ -138,6 +173,57 @@ export const ScimSettings = () => { /> + + + + + Delete SCIM Users + +

+ This will remove all SCIM users from the Unleash + database. This action cannot be undone through + Unleash but the upstream SCIM provider may re sync + these users. +

+ + + + + + + Delete SCIM Groups + +

+ This will remove all SCIM groups from the Unleash + database. This action cannot be undone through + Unleash but the upstream SCIM provider may re sync + these groups. Note that this may affect the + permissions of users present in those groups. +

+ + + + + + { setOpen={setTokenDialog} token={newToken} /> + + setDeleteUsersDialog(false)} + deleteEntities={onDeleteScimUsers} + entityType='Users' + /> + + setDeleteGroupsDialog(false)} + deleteEntities={onDeleteScimGroups} + entityType='Groups' + /> ); diff --git a/frontend/src/component/admin/groups/Group/Group.tsx b/frontend/src/component/admin/groups/Group/Group.tsx index 721a5c6ca9..2d61a505e4 100644 --- a/frontend/src/component/admin/groups/Group/Group.tsx +++ b/frontend/src/component/admin/groups/Group/Group.tsx @@ -270,11 +270,8 @@ export const Group: VFC = () => { onClick={() => setRemoveOpen(true)} permission={ADMIN} tooltipProps={{ - title: isScimGroup - ? scimGroupTooltip - : 'Delete group', + title: 'Delete group', }} - disabled={isScimGroup} > diff --git a/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions.tsx b/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions.tsx index a38f563e27..3a45f67194 100644 --- a/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions.tsx +++ b/frontend/src/component/admin/groups/GroupsList/GroupCard/GroupCardActions.tsx @@ -125,7 +125,6 @@ export const GroupCardActions: FC = ({ onRemove(); handleClose(); }} - disabled={isScimGroup} > diff --git a/frontend/src/component/admin/users/UsersList/DeleteUser/DeleteUser.tsx b/frontend/src/component/admin/users/UsersList/DeleteUser/DeleteUser.tsx index af157cd4c1..cda7c4b8e5 100644 --- a/frontend/src/component/admin/users/UsersList/DeleteUser/DeleteUser.tsx +++ b/frontend/src/component/admin/users/UsersList/DeleteUser/DeleteUser.tsx @@ -77,6 +77,9 @@ const DeleteUser = ({ })` : ''} ? + {user.scimId + ? ' This user is currently managed by SCIM and may be re-added by your SCIM provider.' + : ''} diff --git a/frontend/src/component/admin/users/UsersList/UsersActionsCell/UsersActionsCell.tsx b/frontend/src/component/admin/users/UsersList/UsersActionsCell/UsersActionsCell.tsx index 07f3715d3f..715c365d05 100644 --- a/frontend/src/component/admin/users/UsersList/UsersActionsCell/UsersActionsCell.tsx +++ b/frontend/src/component/admin/users/UsersList/UsersActionsCell/UsersActionsCell.tsx @@ -91,9 +91,8 @@ export const UsersActionsCell: VFC = ({ onClick={onDelete} permission={ADMIN} tooltipProps={{ - title: isScimUser ? scimTooltip : 'Remove user', + title: 'Remove user', }} - disabled={isScimUser} > diff --git a/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts b/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts index ec71c06ded..72ce0e341a 100644 --- a/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts +++ b/frontend/src/hooks/api/actions/useAdminUsersApi/useAdminUsersApi.ts @@ -94,6 +94,19 @@ const useAdminUsersApi = () => { return makeRequest(req.caller, req.id); }; + const deleteScimUsers = async () => { + const requestId = 'deleteScimUsers'; + const req = createRequest( + 'api/admin/user-admin/scim-users', + { + method: 'DELETE', + }, + requestId, + ); + + return makeRequest(req.caller, req.id); + }; + return { addUser, updateUser, @@ -101,6 +114,7 @@ const useAdminUsersApi = () => { changePassword, validatePassword, resetPassword, + deleteScimUsers, userApiErrors: errors, userLoading: loading, }; diff --git a/frontend/src/hooks/api/actions/useGroupApi/useGroupApi.ts b/frontend/src/hooks/api/actions/useGroupApi/useGroupApi.ts index 7a96530e87..4f5811a1e6 100644 --- a/frontend/src/hooks/api/actions/useGroupApi/useGroupApi.ts +++ b/frontend/src/hooks/api/actions/useGroupApi/useGroupApi.ts @@ -46,10 +46,20 @@ export const useGroupApi = () => { await makeRequest(req.caller, req.id); }; + const deleteScimGroups = async () => { + const path = `api/admin/groups/scim-groups`; + const req = createRequest(path, { + method: 'DELETE', + }); + + await makeRequest(req.caller, req.id); + }; + return { createGroup, updateGroup, removeGroup, + deleteScimGroups, errors, loading, }; diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts index f353be131a..dd50512836 100644 --- a/src/lib/db/group-store.ts +++ b/src/lib/db/group-store.ts @@ -354,4 +354,8 @@ export default class GroupStore implements IGroupStore { const { present } = result.rows[0]; return present; } + + async deleteScimGroups(): Promise { + await this.db(T.GROUPS).whereNotNull('scim_id').del(); + } } diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 4edaaacb07..67ac271501 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -274,6 +274,10 @@ class UserStore implements IUserStore { await this.activeUsers().del(); } + async deleteScimUsers(): Promise { + await this.db(TABLE).whereNotNull('scim_id').del(); + } + async count(): Promise { return this.activeUsers() .count('*') diff --git a/src/lib/routes/admin-api/user-admin.ts b/src/lib/routes/admin-api/user-admin.ts index 344dc09e69..986f759c5f 100644 --- a/src/lib/routes/admin-api/user-admin.ts +++ b/src/lib/routes/admin-api/user-admin.ts @@ -411,6 +411,26 @@ export default class UserAdminController extends Controller { ], }); + this.route({ + method: 'delete', + path: '/scim-users', + acceptAnyContentType: true, + handler: this.deleteScimUsers, + permission: ADMIN, + middleware: [ + openApiService.validPath({ + tags: ['Users'], + operationId: 'deleteScimUsers', + summary: 'Delete all SCIM users', + description: 'Deletes all users managed by SCIM', + responses: { + 200: emptyResponse, + ...getStandardResponses(401, 403), + }, + }), + ], + }); + this.route({ method: 'delete', path: '/:id', @@ -654,8 +674,6 @@ export default class UserAdminController extends Controller { const { user, params } = req; const { id } = params; - await this.throwIfScimUser({ id }); - await this.userService.deleteUser(+id, req.audit); res.status(200).send(); } @@ -766,4 +784,10 @@ export default class UserAdminController extends Controller { Boolean((await this.userService.getUser(id)).scimId) ); } + + async deleteScimUsers(req: IAuthRequest, res: Response): Promise { + const { audit } = req; + await this.userService.deleteScimUsers(audit); + res.status(200).send(); + } } diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index d629211ba8..c8da53965f 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -21,6 +21,7 @@ import { GROUP_CREATED, GroupUserAdded, GroupUserRemoved, + ScimGroupsDeleted, type IBaseEvent, } from '../types/events'; import NameExistsError from '../error/name-exists-error'; @@ -310,6 +311,16 @@ export class GroupService { } } + async deleteScimGroups(auditUser: IAuditUser): Promise { + await this.groupStore.deleteScimGroups(); + await this.eventService.storeEvent( + new ScimGroupsDeleted({ + data: null, + auditUser, + }), + ); + } + private mapGroupWithUsers( group: IGroup, allGroupUsers: IGroupUser[], diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 5ed8bd7030..c94cec6e16 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -24,6 +24,7 @@ import type SessionService from './session-service'; import type { IUnleashStores } from '../types/stores'; import PasswordUndefinedError from '../error/password-undefined'; import { + ScimUsersDeleted, UserCreatedEvent, UserDeletedEvent, UserUpdatedEvent, @@ -401,6 +402,17 @@ class UserService { ); } + async deleteScimUsers(auditUser: IAuditUser): Promise { + await this.store.deleteScimUsers(); + + await this.eventService.storeEvent( + new ScimUsersDeleted({ + data: null, + auditUser, + }), + ); + } + async loginUser( usernameOrEmail: string, password: string, diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index 956f204d32..3fffaf3509 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -217,6 +217,8 @@ export const RELEASE_PLAN_MILESTONE_STARTED = 'release-plan-milestone-started' as const; export const USER_PREFERENCE_UPDATED = 'user-preference-updated' as const; +export const SCIM_USERS_DELETED = 'scim-users-deleted' as const; +export const SCIM_GROUPS_DELETED = 'scim-groups-deleted' as const; export const IEventTypes = [ APPLICATION_CREATED, @@ -372,6 +374,8 @@ export const IEventTypes = [ RELEASE_PLAN_REMOVED, RELEASE_PLAN_MILESTONE_STARTED, USER_PREFERENCE_UPDATED, + SCIM_USERS_DELETED, + SCIM_GROUPS_DELETED, ] as const; export type IEventType = (typeof IEventTypes)[number]; @@ -1608,6 +1612,30 @@ export class UserDeletedEvent extends BaseEvent { } } +export class ScimUsersDeleted extends BaseEvent { + readonly data: any; + + constructor(eventData: { + data: any; + auditUser: IAuditUser; + }) { + super(SCIM_USERS_DELETED, eventData.auditUser); + this.data = eventData.data; + } +} + +export class ScimGroupsDeleted extends BaseEvent { + readonly data: any; + + constructor(eventData: { + data: any; + auditUser: IAuditUser; + }) { + super(SCIM_GROUPS_DELETED, eventData.auditUser); + this.data = eventData.data; + } +} + export class TagTypeCreatedEvent extends BaseEvent { readonly data: any; diff --git a/src/lib/types/stores/group-store.ts b/src/lib/types/stores/group-store.ts index 5e3cc758a9..94fcdac399 100644 --- a/src/lib/types/stores/group-store.ts +++ b/src/lib/types/stores/group-store.ts @@ -66,4 +66,6 @@ export interface IGroupStore extends Store { create(group: IStoreGroup): Promise; count(): Promise; + + deleteScimGroups(): Promise; } diff --git a/src/lib/types/stores/user-store.ts b/src/lib/types/stores/user-store.ts index e3fe1aea44..17332935e8 100644 --- a/src/lib/types/stores/user-store.ts +++ b/src/lib/types/stores/user-store.ts @@ -40,4 +40,5 @@ export interface IUserStore extends Store { count(): Promise; countRecentlyDeleted(): Promise; countServiceAccounts(): Promise; + deleteScimUsers(): Promise; } diff --git a/src/test/e2e/api/admin/user-admin.e2e.test.ts b/src/test/e2e/api/admin/user-admin.e2e.test.ts index 51a8e0affd..56a0d0bb98 100644 --- a/src/test/e2e/api/admin/user-admin.e2e.test.ts +++ b/src/test/e2e/api/admin/user-admin.e2e.test.ts @@ -462,3 +462,32 @@ test('should return number of sessions per user', async () => { ]), }); }); + +test('should only delete scim users', async () => { + userStore.insert({ + email: 'boring@example.com', + }); + + await userStore.insert({ + email: 'really-boring@example.com', + }); + + const scimUser = ( + await db + .rawDatabase('users') + .insert({ + email: 'made-by-scim@example.com', + scim_id: 'some-random-scim-id', + }) + .returning('id') + )[0].id; + + await app.request.delete('/api/admin/user-admin/scim-users').expect(200); + const response = await app.request.get(`/api/admin/user-admin`).expect(200); + const users = response.body.users; + + expect(users.length).toBe(2); + expect(users.every((u) => u.email !== 'made-by-scim@example.com')).toBe( + true, + ); +}); diff --git a/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts index 4a78191c2d..b93b33f60e 100644 --- a/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts +++ b/src/test/e2e/api/admin/user-admin.scim.e2e.test.ts @@ -12,6 +12,7 @@ let app: IUnleashTest; let scimUserId: number; let regularUserId: number; +let scimDeletableUser: number; const scimUser = { email: 'scim-user@test.com', @@ -24,6 +25,12 @@ const regularUser = { name: 'Regular User', }; +const scimUserToBeDeleted = { + email: 'scim-victim@test.com', + name: 'SCIM Victim', + scim_id: 'some-other-random-scim-id', +}; + const scimGuardErrorMessage = 'This user is managed by your SCIM provider and cannot be changed manually'; @@ -50,6 +57,13 @@ beforeAll(async () => { regularUserId = ( await db.rawDatabase('users').insert(regularUser).returning('id') )[0].id; + + scimDeletableUser = ( + await db + .rawDatabase('users') + .insert(scimUserToBeDeleted) + .returning('id') + )[0].id; }); afterAll(async () => { @@ -86,12 +100,10 @@ test('should prevent editing a SCIM user', async () => { expect(body.details[0].message).toBe(scimGuardErrorMessage); }); -test('should prevent deleting a SCIM user', async () => { - const { body } = await app.request - .delete(`/api/admin/user-admin/${scimUserId}`) - .expect(403); - - expect(body.details[0].message).toBe(scimGuardErrorMessage); +test('should not prevent deleting a SCIM user', async () => { + await app.request + .delete(`/api/admin/user-admin/${scimDeletableUser}`) + .expect(200); }); test('should prevent changing password for a SCIM user', async () => { diff --git a/src/test/fixtures/fake-group-store.ts b/src/test/fixtures/fake-group-store.ts index 22bc06cab2..8b64d8c696 100644 --- a/src/test/fixtures/fake-group-store.ts +++ b/src/test/fixtures/fake-group-store.ts @@ -125,4 +125,8 @@ export default class FakeGroupStore implements IGroupStore { hasProjectRole(groupId: number): Promise { throw new Error('Method not implemented.'); } + + deleteScimGroups(): Promise { + throw new Error('Method not implemented.'); + } } diff --git a/src/test/fixtures/fake-user-store.ts b/src/test/fixtures/fake-user-store.ts index f37b6c67d3..4802ad0810 100644 --- a/src/test/fixtures/fake-user-store.ts +++ b/src/test/fixtures/fake-user-store.ts @@ -159,6 +159,10 @@ class UserStoreMock implements IUserStore { return Promise.resolve(undefined); } + deleteScimUsers(): Promise { + throw new Error('Method not implemented.'); + } + upsert(user: ICreateUser): Promise { this.data.splice(this.data.findIndex((u) => u.email === user.email)); const userToReturn = {