From 8baf8e5be6009897d3b4d470bd3406148d49f62e Mon Sep 17 00:00:00 2001 From: wangsijie Date: Fri, 5 May 2023 21:21:09 +0800 Subject: [PATCH] test(core): add api response guard and error case tests to admin user (#3809) test(core): add api response guard and error case tests to admin user api --- packages/core/src/routes-me/user.ts | 3 +- .../core/src/routes/admin-user-search.test.ts | 88 +++++++++++++++++++ packages/core/src/routes/admin-user-search.ts | 61 +++++++++++++ packages/core/src/routes/admin-user.test.ts | 42 +-------- packages/core/src/routes/admin-user.ts | 65 +++++--------- packages/core/src/routes/init.ts | 2 + packages/core/src/routes/role.ts | 4 +- .../src/tests/api/admin-user.roles.test.ts | 11 +++ .../src/tests/api/admin-user.test.ts | 38 ++++++-- packages/schemas/src/types/user.ts | 30 +++---- 10 files changed, 232 insertions(+), 112 deletions(-) create mode 100644 packages/core/src/routes/admin-user-search.test.ts create mode 100644 packages/core/src/routes/admin-user-search.ts diff --git a/packages/core/src/routes-me/user.ts b/packages/core/src/routes-me/user.ts index 104ecf4cd..ac5cc4a8e 100644 --- a/packages/core/src/routes-me/user.ts +++ b/packages/core/src/routes-me/user.ts @@ -1,5 +1,4 @@ import { emailRegEx, passwordRegEx, usernameRegEx } from '@logto/core-kit'; -import type { UserProfileResponse } from '@logto/schemas'; import { userInfoSelectFields, jsonObjectGuard } from '@logto/schemas'; import { conditional, pick } from '@silverhand/essentials'; import { literal, object, string } from 'zod'; @@ -31,7 +30,7 @@ export default function userRoutes( const user = await findUserById(userId); - const responseData: UserProfileResponse = { + const responseData = { ...pick(user, ...userInfoSelectFields), ...conditional(user.passwordEncrypted && { hasPassword: Boolean(user.passwordEncrypted) }), }; diff --git a/packages/core/src/routes/admin-user-search.test.ts b/packages/core/src/routes/admin-user-search.test.ts new file mode 100644 index 000000000..b0cd6128b --- /dev/null +++ b/packages/core/src/routes/admin-user-search.test.ts @@ -0,0 +1,88 @@ +import type { CreateUser, Role, User } from '@logto/schemas'; +import { userInfoSelectFields } from '@logto/schemas'; +import { pickDefault } from '@logto/shared/esm'; +import { pick } from '@silverhand/essentials'; + +import { mockUser, mockUserList, mockUserListResponse } from '#src/__mocks__/index.js'; +import type Libraries from '#src/tenants/Libraries.js'; +import type Queries from '#src/tenants/Queries.js'; +import { MockTenant, type Partial2 } from '#src/test-utils/tenant.js'; +import { createRequester } from '#src/utils/test-utils.js'; + +const { jest } = import.meta; + +const filterUsersWithSearch = (users: User[], search: string) => + users.filter((user) => + [user.username, user.primaryEmail, user.primaryPhone, user.name].some((value) => + value ? !value.includes(search) : false + ) + ); + +const mockedQueries = { + users: { + countUsers: jest.fn(async (search) => ({ + count: search + ? filterUsersWithSearch(mockUserList, String(search)).length + : mockUserList.length, + })), + findUsers: jest.fn( + async (limit, offset, search): Promise => + // For testing, type should be `Search` but we use `string` in `filterUsersWithSearch()` here + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + search ? filterUsersWithSearch(mockUserList, String(search)) : mockUserList + ), + }, + roles: { + findRolesByRoleNames: jest.fn( + async (): Promise => [ + { tenantId: 'fake_tenant', id: 'role_id', name: 'admin', description: 'none' }, + ] + ), + }, + usersRoles: { + deleteUsersRolesByUserIdAndRoleId: jest.fn(), + }, +} satisfies Partial2; + +const usersLibraries = { + generateUserId: jest.fn(async () => 'fooId'), + insertUser: jest.fn( + async (user: CreateUser): Promise => ({ + ...mockUser, + ...user, + }) + ), +} satisfies Partial; + +const adminUserRoutes = await pickDefault(import('./admin-user-search.js')); + +describe('adminUserRoutes', () => { + const tenantContext = new MockTenant(undefined, mockedQueries, undefined, { + users: usersLibraries, + }); + const userRequest = createRequester({ authedRoutes: adminUserRoutes, tenantContext }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + it('GET /users', async () => { + const response = await userRequest.get('/users'); + expect(response.status).toEqual(200); + expect(response.body).toEqual(mockUserListResponse); + expect(response.header).toHaveProperty('total-number', `${mockUserList.length}`); + }); + + it('GET /users should return matched data', async () => { + const search = 'foo'; + const response = await userRequest.get('/users').send({ search }); + expect(response.status).toEqual(200); + expect(response.body).toEqual( + filterUsersWithSearch(mockUserList, search).map((user) => pick(user, ...userInfoSelectFields)) + ); + expect(response.header).toHaveProperty( + 'total-number', + `${filterUsersWithSearch(mockUserList, search).length}` + ); + }); +}); diff --git a/packages/core/src/routes/admin-user-search.ts b/packages/core/src/routes/admin-user-search.ts new file mode 100644 index 000000000..03dffa0cc --- /dev/null +++ b/packages/core/src/routes/admin-user-search.ts @@ -0,0 +1,61 @@ +import { userInfoSelectFields, userProfileResponseGuard } from '@logto/schemas'; +import { pick, tryThat } from '@silverhand/essentials'; + +import RequestError from '#src/errors/RequestError/index.js'; +import koaGuard from '#src/middleware/koa-guard.js'; +import koaPagination from '#src/middleware/koa-pagination.js'; +import { parseSearchParamsForSearch } from '#src/utils/search.js'; + +import type { AuthedRouter, RouterInitArgs } from './types.js'; + +export default function adminUserSearchRoutes( + ...[router, { queries }]: RouterInitArgs +) { + const { + users: { findUsers, countUsers }, + usersRoles: { findUsersRolesByRoleId }, + } = queries; + + router.get( + '/users', + koaPagination(), + koaGuard({ + response: userProfileResponseGuard.array(), + status: [200, 400], + }), + async (ctx, next) => { + const { limit, offset } = ctx.pagination; + const { searchParams } = ctx.request.URL; + + return tryThat( + async () => { + const search = parseSearchParamsForSearch(searchParams); + const excludeRoleId = searchParams.get('excludeRoleId'); + const excludeUsersRoles = excludeRoleId + ? await findUsersRolesByRoleId(excludeRoleId) + : []; + const excludeUserIds = excludeUsersRoles.map(({ userId }) => userId); + + const [{ count }, users] = await Promise.all([ + countUsers(search, excludeUserIds), + findUsers(limit, offset, search, excludeUserIds), + ]); + + ctx.pagination.totalCount = count; + ctx.body = users.map((user) => pick(user, ...userInfoSelectFields)); + + return next(); + }, + (error) => { + if (error instanceof TypeError) { + throw new RequestError( + { code: 'request.invalid_input', details: error.message }, + error + ); + } + throw error; + } + ); + } + ); +} diff --git a/packages/core/src/routes/admin-user.test.ts b/packages/core/src/routes/admin-user.test.ts index 9fedf640b..f89046b92 100644 --- a/packages/core/src/routes/admin-user.test.ts +++ b/packages/core/src/routes/admin-user.test.ts @@ -1,15 +1,7 @@ -/* eslint-disable max-lines */ import type { CreateUser, Role, SignInExperience, User } from '@logto/schemas'; -import { userInfoSelectFields } from '@logto/schemas'; import { createMockUtils, pickDefault } from '@logto/shared/esm'; -import { pick } from '@silverhand/essentials'; -import { - mockUser, - mockUserList, - mockUserListResponse, - mockUserResponse, -} from '#src/__mocks__/index.js'; +import { mockUser, mockUserResponse } from '#src/__mocks__/index.js'; import RequestError from '#src/errors/RequestError/index.js'; import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; @@ -43,17 +35,6 @@ const mockedQueries = { ), }, users: { - countUsers: jest.fn(async (search) => ({ - count: search - ? filterUsersWithSearch(mockUserList, String(search)).length - : mockUserList.length, - })), - findUsers: jest.fn( - async (limit, offset, search): Promise => - // For testing, type should be `Search` but we use `string` in `filterUsersWithSearch()` here - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - search ? filterUsersWithSearch(mockUserList, String(search)) : mockUserList - ), findUserById: jest.fn(async (id: string) => mockUser), hasUser: jest.fn(async () => mockHasUser()), hasUserWithEmail: jest.fn(async () => mockHasUserWithEmail()), @@ -120,26 +101,6 @@ describe('adminUserRoutes', () => { jest.clearAllMocks(); }); - it('GET /users', async () => { - const response = await userRequest.get('/users'); - expect(response.status).toEqual(200); - expect(response.body).toEqual(mockUserListResponse); - expect(response.header).toHaveProperty('total-number', `${mockUserList.length}`); - }); - - it('GET /users should return matched data', async () => { - const search = 'foo'; - const response = await userRequest.get('/users').send({ search }); - expect(response.status).toEqual(200); - expect(response.body).toEqual( - filterUsersWithSearch(mockUserList, search).map((user) => pick(user, ...userInfoSelectFields)) - ); - expect(response.header).toHaveProperty( - 'total-number', - `${filterUsersWithSearch(mockUserList, search).length}` - ); - }); - it('GET /users/:userId', async () => { const response = await userRequest.get('/users/foo'); expect(response.status).toEqual(200); @@ -474,4 +435,3 @@ describe('adminUserRoutes', () => { expect(deleteUserIdentity).toHaveBeenCalledWith(arbitraryUserId, arbitraryTarget); }); }); -/* eslint-enable max-lines */ diff --git a/packages/core/src/routes/admin-user.ts b/packages/core/src/routes/admin-user.ts index 1359b8171..52d84f695 100644 --- a/packages/core/src/routes/admin-user.ts +++ b/packages/core/src/routes/admin-user.ts @@ -1,14 +1,12 @@ import { emailRegEx, passwordRegEx, phoneRegEx, usernameRegEx } from '@logto/core-kit'; -import { jsonObjectGuard, userInfoSelectFields } from '@logto/schemas'; -import { conditional, has, pick, tryThat } from '@silverhand/essentials'; +import { jsonObjectGuard, userInfoSelectFields, userProfileResponseGuard } from '@logto/schemas'; +import { conditional, has, pick } from '@silverhand/essentials'; import { boolean, literal, object, string } from 'zod'; import RequestError from '#src/errors/RequestError/index.js'; import { encryptUserPassword, verifyUserPassword } from '#src/libraries/user.js'; import koaGuard from '#src/middleware/koa-guard.js'; -import koaPagination from '#src/middleware/koa-pagination.js'; import assertThat from '#src/utils/assert-that.js'; -import { parseSearchParamsForSearch } from '#src/utils/search.js'; import type { AuthedRouter, RouterInitArgs } from './types.js'; @@ -20,54 +18,23 @@ export default function adminUserRoutes( users: { deleteUserById, deleteUserIdentity, - findUsers, - countUsers, findUserById, hasUser, updateUserById, hasUserWithEmail, hasUserWithPhone, }, - usersRoles: { findUsersRolesByRoleId }, } = queries; const { - users: { checkIdentifierCollision, generateUserId, insertUser, findUsersByRoleName }, + users: { checkIdentifierCollision, generateUserId, insertUser }, } = libraries; - router.get('/users', koaPagination(), async (ctx, next) => { - const { limit, offset } = ctx.pagination; - const { searchParams } = ctx.request.URL; - - return tryThat( - async () => { - const search = parseSearchParamsForSearch(searchParams); - const excludeRoleId = searchParams.get('excludeRoleId'); - const excludeUsersRoles = excludeRoleId ? await findUsersRolesByRoleId(excludeRoleId) : []; - const excludeUserIds = excludeUsersRoles.map(({ userId }) => userId); - - const [{ count }, users] = await Promise.all([ - countUsers(search, excludeUserIds), - findUsers(limit, offset, search, excludeUserIds), - ]); - - ctx.pagination.totalCount = count; - ctx.body = users.map((user) => pick(user, ...userInfoSelectFields)); - - return next(); - }, - (error) => { - if (error instanceof TypeError) { - throw new RequestError({ code: 'request.invalid_input', details: error.message }, error); - } - throw error; - } - ); - }); - router.get( '/users/:userId', koaGuard({ params: object({ userId: string() }), + response: userProfileResponseGuard, + status: [200, 404], }), async (ctx, next) => { const { @@ -87,6 +54,7 @@ export default function adminUserRoutes( koaGuard({ params: object({ userId: string() }), response: jsonObjectGuard, + status: [200], }), async (ctx, next) => { const { @@ -106,6 +74,7 @@ export default function adminUserRoutes( params: object({ userId: string() }), body: object({ customData: jsonObjectGuard }), response: jsonObjectGuard, + status: [200, 404], }), async (ctx, next) => { const { @@ -135,6 +104,8 @@ export default function adminUserRoutes( password: string().regex(passwordRegEx), name: string(), }).partial(), + response: userProfileResponseGuard, + status: [200, 404, 422], }), async (ctx, next) => { const { primaryEmail, primaryPhone, username, password, name } = ctx.guard.body; @@ -155,7 +126,7 @@ export default function adminUserRoutes( ); assertThat( !primaryPhone || !(await hasUserWithPhone(primaryPhone)), - new RequestError({ code: 'user.phone_already_in_use' }) + new RequestError({ code: 'user.phone_already_in_use', status: 422 }) ); const id = await generateUserId(); @@ -190,6 +161,8 @@ export default function adminUserRoutes( avatar: string().url().or(literal('')).nullable(), customData: jsonObjectGuard, }).partial(), + response: userProfileResponseGuard, + status: [200, 404, 422], }), async (ctx, next) => { const { @@ -212,6 +185,8 @@ export default function adminUserRoutes( koaGuard({ params: object({ userId: string() }), body: object({ password: string().regex(passwordRegEx) }), + response: userProfileResponseGuard, + status: [200, 422], }), async (ctx, next) => { const { @@ -239,6 +214,7 @@ export default function adminUserRoutes( koaGuard({ params: object({ userId: string() }), body: object({ password: string() }), + status: [204], }), async (ctx, next) => { const { @@ -260,7 +236,7 @@ export default function adminUserRoutes( koaGuard({ params: object({ userId: string() }), response: object({ hasPassword: boolean() }), - status: [200], + status: [200, 404], }), async (ctx, next) => { const { userId } = ctx.guard.params; @@ -279,6 +255,8 @@ export default function adminUserRoutes( koaGuard({ params: object({ userId: string() }), body: object({ isSuspended: boolean() }), + response: userProfileResponseGuard, + status: [200, 404], }), async (ctx, next) => { const { @@ -306,6 +284,7 @@ export default function adminUserRoutes( '/users/:userId', koaGuard({ params: object({ userId: string() }), + status: [204, 400, 404], }), async (ctx, next) => { const { @@ -326,7 +305,11 @@ export default function adminUserRoutes( router.delete( '/users/:userId/identities/:target', - koaGuard({ params: object({ userId: string(), target: string() }) }), + koaGuard({ + params: object({ userId: string(), target: string() }), + response: userProfileResponseGuard, + status: [200, 404], + }), async (ctx, next) => { const { params: { userId, target }, diff --git a/packages/core/src/routes/init.ts b/packages/core/src/routes/init.ts index b0de184d3..ddfa7b05a 100644 --- a/packages/core/src/routes/init.ts +++ b/packages/core/src/routes/init.ts @@ -10,6 +10,7 @@ import type TenantContext from '#src/tenants/TenantContext.js'; import koaAuth from '../middleware/koa-auth/index.js'; import adminUserRoleRoutes from './admin-user-role.js'; +import adminUserSearchRoutes from './admin-user-search.js'; import adminUserRoutes from './admin-user.js'; import applicationRoutes from './application.js'; import authnRoutes from './authn.js'; @@ -43,6 +44,7 @@ const createRouters = (tenant: TenantContext) => { resourceRoutes(managementRouter, tenant); signInExperiencesRoutes(managementRouter, tenant); adminUserRoutes(managementRouter, tenant); + adminUserSearchRoutes(managementRouter, tenant); adminUserRoleRoutes(managementRouter, tenant); logRoutes(managementRouter, tenant); roleRoutes(managementRouter, tenant); diff --git a/packages/core/src/routes/role.ts b/packages/core/src/routes/role.ts index 7d7202001..1f9c1c6ed 100644 --- a/packages/core/src/routes/role.ts +++ b/packages/core/src/routes/role.ts @@ -1,5 +1,5 @@ import type { RoleResponse } from '@logto/schemas'; -import { userInfoSelectFields, userInfoResponseGuard, Roles, Users } from '@logto/schemas'; +import { userInfoSelectFields, userProfileResponseGuard, Roles, Users } from '@logto/schemas'; import { generateStandardId } from '@logto/shared'; import { pick, tryThat } from '@silverhand/essentials'; import { object, string, z, number } from 'zod'; @@ -220,7 +220,7 @@ export default function roleRoutes( koaPagination(), koaGuard({ params: object({ id: string().min(1) }), - response: userInfoResponseGuard.array(), + response: userProfileResponseGuard.array(), status: [200, 400, 404], }), async (ctx, next) => { diff --git a/packages/integration-tests/src/tests/api/admin-user.roles.test.ts b/packages/integration-tests/src/tests/api/admin-user.roles.test.ts index 2c55157df..c9c075291 100644 --- a/packages/integration-tests/src/tests/api/admin-user.roles.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.roles.test.ts @@ -3,6 +3,7 @@ import { HTTPError } from 'got'; import { assignRolesToUser, getUserRoles, deleteRoleFromUser } from '#src/api/index.js'; import { createRole } from '#src/api/role.js'; +import { createResponseWithCode } from '#src/helpers/admin-tenant.js'; import { createUserByAdmin } from '#src/helpers/index.js'; describe('admin console user management (roles)', () => { @@ -22,6 +23,16 @@ describe('admin console user management (roles)', () => { expect(roles[0]).toHaveProperty('id', role.id); }); + it('should fail when assign duplicated role to user', async () => { + const user = await createUserByAdmin(); + const role = await createRole(); + + await assignRolesToUser(user.id, [role.id]); + await expect(assignRolesToUser(user.id, [role.id])).rejects.toMatchObject( + createResponseWithCode(422) + ); + }); + it('should delete role from user successfully', async () => { const user = await createUserByAdmin(); 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 64ef17ecd..72360fdcb 100644 --- a/packages/integration-tests/src/tests/api/admin-user.test.ts +++ b/packages/integration-tests/src/tests/api/admin-user.test.ts @@ -7,7 +7,6 @@ import { } from '#src/__mocks__/connectors-mock.js'; import { getUser, - getUsers, updateUser, deleteUser, updateUserPassword, @@ -16,22 +15,40 @@ import { updateConnectorConfig, deleteConnectorById, } from '#src/api/index.js'; +import { createResponseWithCode } from '#src/helpers/admin-tenant.js'; import { createUserByAdmin } from '#src/helpers/index.js'; import { createNewSocialUserWithUsernameAndPassword } from '#src/helpers/interactions.js'; +import { generateUsername, generateEmail, generatePhone, generatePassword } from '#src/utils.js'; describe('admin console user management', () => { - it('should create user successfully', async () => { + it('should create and get user successfully', async () => { const user = await createUserByAdmin(); const userDetails = await getUser(user.id); expect(userDetails.id).toBe(user.id); }); - it('should get user list successfully', async () => { - await createUserByAdmin(); - const users = await getUsers(); + it('should fail when create user with conflict identifiers', async () => { + const [username, password, email, phone] = [ + generateUsername(), + generatePassword(), + generateEmail(), + generatePhone(), + ]; + await createUserByAdmin(username, password, email, phone); + await expect(createUserByAdmin(username, password)).rejects.toMatchObject( + createResponseWithCode(422) + ); + await expect(createUserByAdmin(undefined, undefined, email)).rejects.toMatchObject( + createResponseWithCode(422) + ); + await expect(createUserByAdmin(undefined, undefined, undefined, phone)).rejects.toMatchObject( + createResponseWithCode(422) + ); + }); - expect(users.length).not.toBeLessThan(1); + it('should fail when get user by invalid id', async () => { + await expect(getUser('invalid-user-id')).rejects.toMatchObject(createResponseWithCode(404)); }); it('should update userinfo successfully', async () => { @@ -50,6 +67,15 @@ describe('admin console user management', () => { expect(updatedUser).toMatchObject(newUserData); }); + it('should fail when update userinfo with conflict identifiers', async () => { + const user = await createUserByAdmin(); + const anotherUser = await createUserByAdmin(); + + await expect(updateUser(user.id, { username: anotherUser.username })).rejects.toMatchObject( + createResponseWithCode(422) + ); + }); + it('should delete user successfully', async () => { const user = await createUserByAdmin(); diff --git a/packages/schemas/src/types/user.ts b/packages/schemas/src/types/user.ts index 0f6e49477..01fd8a4c8 100644 --- a/packages/schemas/src/types/user.ts +++ b/packages/schemas/src/types/user.ts @@ -1,6 +1,6 @@ +import { z } from 'zod'; + import { Users } from '../db-entries/index.js'; -import type { User } from '../db-entries/index.js'; -import { type CreateGuard } from '../index.js'; export const userInfoSelectFields = Object.freeze([ 'id', @@ -17,27 +17,17 @@ export const userInfoSelectFields = Object.freeze([ 'isSuspended', ] as const); -export type UserInfo = Pick< - User, - Keys ->; +export const userInfoGuard = Users.guard.pick( + Object.fromEntries(userInfoSelectFields.map((key) => [key, true])) +); -export const userInfoResponseGuard: CreateGuard = Users.guard.pick({ - id: true, - username: true, - primaryEmail: true, - primaryPhone: true, - name: true, - avatar: true, - customData: true, - identities: true, - lastSignInAt: true, - createdAt: true, - applicationId: true, - isSuspended: true, +export type UserInfo = z.infer; + +export const userProfileResponseGuard = userInfoGuard.extend({ + hasPassword: z.boolean().optional(), }); -export type UserProfileResponse = UserInfo & { hasPassword?: boolean }; +export type UserProfileResponse = z.infer; /** Internal read-only roles for user tenants. */ export enum InternalRole {