From 5660c54cb563e2a9dae9e314976924aa60b5f69a Mon Sep 17 00:00:00 2001 From: wangsijie Date: Tue, 14 May 2024 16:10:31 +0800 Subject: [PATCH] fix(core): should sign out user after deletion or suspension (#5857) fixed #5572 --- .changeset/green-cougars-behave.md | 7 +++++++ packages/core/src/libraries/user.ts | 10 ++++++++++ .../core/src/routes/admin-user/basics.test.ts | 7 ++++--- packages/core/src/routes/admin-user/basics.ts | 15 +++++++++++---- .../src/tests/api/admin-user.test.ts | 14 ++++++++++++-- 5 files changed, 44 insertions(+), 9 deletions(-) create mode 100644 .changeset/green-cougars-behave.md diff --git a/.changeset/green-cougars-behave.md b/.changeset/green-cougars-behave.md new file mode 100644 index 000000000..88646a695 --- /dev/null +++ b/.changeset/green-cougars-behave.md @@ -0,0 +1,7 @@ +--- +"@logto/core": patch +--- + +Sign out user after deletion or suspension + +When a user is deleted or suspended through Management API, they should be signed out immediately, including sessions and refresh tokens. diff --git a/packages/core/src/libraries/user.ts b/packages/core/src/libraries/user.ts index 99070a8df..66a4560c6 100644 --- a/packages/core/src/libraries/user.ts +++ b/packages/core/src/libraries/user.ts @@ -89,6 +89,7 @@ export const createUserLibrary = (queries: Queries) => { rolesScopes: { findRolesScopesByRoleIds }, scopes: { findScopesByIdsAndResourceIndicator }, organizations, + oidcModelInstances: { revokeInstanceByUserId }, } = queries; const generateUserId = async (retries = 500) => @@ -270,6 +271,14 @@ export const createUserLibrary = (queries: Queries) => { return user; }; + const signOutUser = async (userId: string) => { + await Promise.all([ + revokeInstanceByUserId('AccessToken', userId), + revokeInstanceByUserId('RefreshToken', userId), + revokeInstanceByUserId('Session', userId), + ]); + }; + return { generateUserId, insertUser, @@ -279,5 +288,6 @@ export const createUserLibrary = (queries: Queries) => { findUserRoles, addUserMfaVerification, verifyUserPassword, + signOutUser, }; }; diff --git a/packages/core/src/routes/admin-user/basics.test.ts b/packages/core/src/routes/admin-user/basics.test.ts index 280502541..68f1d29e3 100644 --- a/packages/core/src/routes/admin-user/basics.test.ts +++ b/packages/core/src/routes/admin-user/basics.test.ts @@ -14,7 +14,6 @@ const { jest } = import.meta; const { mockEsmWithActual } = createMockUtils(jest); const mockedQueries = { - oidcModelInstances: { revokeInstanceByUserId: jest.fn() }, signInExperiences: { findDefaultSignInExperience: jest.fn( async () => @@ -65,7 +64,6 @@ const mockHasUser = jest.fn(async () => false); const mockHasUserWithEmail = jest.fn(async () => false); const mockHasUserWithPhone = jest.fn(async () => false); -const { revokeInstanceByUserId } = mockedQueries.oidcModelInstances; const { hasUser, findUserById, updateUserById, deleteUserIdentity, deleteUserById } = mockedQueries.users; @@ -77,6 +75,7 @@ const { encryptUserPassword } = await mockEsmWithActual('#src/libraries/user.js' })); const verifyUserPassword = jest.fn(); +const signOutUser = jest.fn(); const usersLibraries = { generateUserId: jest.fn(async () => 'fooId'), insertUser: jest.fn( @@ -86,6 +85,7 @@ const usersLibraries = { }) ), verifyUserPassword, + signOutUser, } satisfies Partial; const adminUserRoutes = await pickDefault(import('./basics.js')); @@ -377,7 +377,7 @@ describe('adminUserRoutes', () => { .patch(`/users/${mockedUserId}/is-suspended`) .send({ isSuspended: true }); expect(updateUserById).toHaveBeenCalledWith(mockedUserId, { isSuspended: true }); - expect(revokeInstanceByUserId).toHaveBeenCalledWith('refreshToken', mockedUserId); + expect(signOutUser).toHaveBeenCalledWith(mockedUserId); expect(response.status).toEqual(200); expect(response.body).toEqual({ ...mockUserResponse, @@ -389,6 +389,7 @@ describe('adminUserRoutes', () => { const userId = 'fooUser'; const response = await userRequest.delete(`/users/${userId}`); expect(response.status).toEqual(204); + expect(signOutUser).toHaveBeenCalledWith(userId); }); it('DELETE /users/:userId should throw if user is deleting self', async () => { diff --git a/packages/core/src/routes/admin-user/basics.ts b/packages/core/src/routes/admin-user/basics.ts index 710bbfe14..338648c35 100644 --- a/packages/core/src/routes/admin-user/basics.ts +++ b/packages/core/src/routes/admin-user/basics.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ import { emailRegEx, phoneRegEx, usernameRegEx } from '@logto/core-kit'; import { UsersPasswordEncryptionMethod, @@ -21,7 +22,6 @@ export default function adminUserBasicsRoutes( ) { const [router, { queries, libraries }] = args; const { - oidcModelInstances: { revokeInstanceByUserId }, users: { deleteUserById, findUserById, @@ -33,7 +33,13 @@ export default function adminUserBasicsRoutes( userSsoIdentities, } = queries; const { - users: { checkIdentifierCollision, generateUserId, insertUser, verifyUserPassword }, + users: { + checkIdentifierCollision, + generateUserId, + insertUser, + verifyUserPassword, + signOutUser, + }, } = libraries; router.get( @@ -343,12 +349,11 @@ export default function adminUserBasicsRoutes( }); if (isSuspended) { - await revokeInstanceByUserId('refreshToken', user.id); + await signOutUser(user.id); } ctx.body = pick(user, ...userInfoSelectFields); - // eslint-disable-next-line max-lines return next(); } ); @@ -368,6 +373,7 @@ export default function adminUserBasicsRoutes( throw new RequestError('user.cannot_delete_self'); } + await signOutUser(userId); await deleteUserById(userId); ctx.status = 204; @@ -376,3 +382,4 @@ export default function adminUserBasicsRoutes( } ); } +/* eslint-enable max-lines */ diff --git a/packages/integration-tests/src/tests/api/admin-user.test.ts b/packages/integration-tests/src/tests/api/admin-user.test.ts index 9662010b8..5c4a60c4a 100644 --- a/packages/integration-tests/src/tests/api/admin-user.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.test.ts @@ -22,7 +22,11 @@ import { } from '#src/api/index.js'; import { clearConnectorsByTypes } from '#src/helpers/connector.js'; import { createUserByAdmin, expectRejects } from '#src/helpers/index.js'; -import { createNewSocialUserWithUsernameAndPassword } from '#src/helpers/interactions.js'; +import { + createNewSocialUserWithUsernameAndPassword, + signInWithPassword, +} from '#src/helpers/interactions.js'; +import { enableAllPasswordSignInMethods } from '#src/helpers/sign-in-experience.js'; import { generateUsername, generateEmail, @@ -177,7 +181,9 @@ describe('admin console user management', () => { }); it('should delete user successfully', async () => { - const user = await createUserByAdmin(); + const username = generateUsername(); + const password = 'password'; + const user = await createUserByAdmin({ username, password }); const userEntity = await getUser(user.id); expect(userEntity).toMatchObject(user); @@ -186,6 +192,10 @@ describe('admin console user management', () => { const response = await getUser(user.id).catch((error: unknown) => error); expect(response instanceof HTTPError && response.response.status === 404).toBe(true); + + await enableAllPasswordSignInMethods(); + // Sign in with deleted user should throw error + await expect(signInWithPassword({ username, password })).rejects.toThrowError(); }); it('should update user password successfully', async () => {