From fafe27f87a4de6e9579b1ea7bbff7e6bfc959f8a Mon Sep 17 00:00:00 2001 From: simeng-li Date: Fri, 5 May 2023 16:18:27 +0800 Subject: [PATCH] test(core): add exception cases for role api and scope api (#3802) * feat(core): add roles api guard add roles api guard * feat(core): add scope api response guard add scope api response guard * test(core): add exception cases for role api integration tests add exception cases for role api integration tests * fix(console): fix lint error fix lint error * fix(core): remove guard status code remove guard status code * fix(core): resolve comments resolve comments * fix(core): remove useless 401,403 code guard remove useless 401,403 code guard * fix(core): fix swagger 422 guard error fix swagger 422 guard error --- .../components/LinkAccountSection/index.tsx | 2 +- packages/core/src/routes/dashboard.ts | 6 +- packages/core/src/routes/role.scope.ts | 8 +- packages/core/src/routes/role.ts | 121 +++++++++++------- packages/core/src/utils/http.ts | 1 + .../src/tests/api/role.scope.test.ts | 69 ++++++++++ .../src/tests/api/role.test.ts | 29 +++++ .../src/tests/api/role.user.test.ts | 51 ++++++++ packages/schemas/src/types/user.ts | 23 +++- 9 files changed, 258 insertions(+), 52 deletions(-) diff --git a/packages/console/src/pages/Profile/components/LinkAccountSection/index.tsx b/packages/console/src/pages/Profile/components/LinkAccountSection/index.tsx index 4ff19b40b..6fa9fd795 100644 --- a/packages/console/src/pages/Profile/components/LinkAccountSection/index.tsx +++ b/packages/console/src/pages/Profile/components/LinkAccountSection/index.tsx @@ -62,7 +62,7 @@ function LinkAccountSection({ user, connectors, onUpdate }: Props) { return connectors.map(({ id, name, logo, logoDark, target }) => { const logoSrc = theme === Theme.Dark && logoDark ? logoDark : logo; - const relatedUserDetails = user.identities?.[target]?.details; + const relatedUserDetails = user.identities[target]?.details; const socialUserInfo = socialUserInfoGuard.safeParse(relatedUserDetails); const hasLinked = socialUserInfo.success; diff --git a/packages/core/src/routes/dashboard.ts b/packages/core/src/routes/dashboard.ts index b85ea21b9..6d42f6362 100644 --- a/packages/core/src/routes/dashboard.ts +++ b/packages/core/src/routes/dashboard.ts @@ -27,7 +27,7 @@ export default function dashboardRoutes( response: object({ totalUserCount: number(), }), - status: [200, 401, 403], + status: [200], }), async (ctx, next) => { const { count: totalUserCount } = await countUsers(); @@ -41,7 +41,7 @@ export default function dashboardRoutes( '/dashboard/users/new', koaGuard({ response: getNewUsersResponseGuard, - status: [200, 401, 403], + status: [200], }), async (ctx, next) => { const today = Date.now(); @@ -88,7 +88,7 @@ export default function dashboardRoutes( koaGuard({ query: object({ date: string().regex(dateRegex).optional() }), response: getActiveUsersResponseGuard, - status: [200, 401, 403], + status: [200], }), async (ctx, next) => { const { diff --git a/packages/core/src/routes/role.scope.ts b/packages/core/src/routes/role.scope.ts index c7da9f815..4dc37967d 100644 --- a/packages/core/src/routes/role.scope.ts +++ b/packages/core/src/routes/role.scope.ts @@ -1,5 +1,5 @@ import type { Scope, ScopeResponse } from '@logto/schemas'; -import { scopeResponseGuard } from '@logto/schemas'; +import { scopeResponseGuard, Scopes } from '@logto/schemas'; import { generateStandardId } from '@logto/shared'; import { tryThat } from '@silverhand/essentials'; import { object, string } from 'zod'; @@ -42,6 +42,7 @@ export default function roleScopeRoutes( koaGuard({ params: object({ id: string().min(1) }), response: scopeResponseGuard.array(), + status: [200, 404], }), async (ctx, next) => { const { @@ -94,7 +95,9 @@ export default function roleScopeRoutes( '/roles/:id/scopes', koaGuard({ params: object({ id: string().min(1) }), - body: object({ scopeIds: string().min(1).array() }), + body: object({ scopeIds: string().min(1).array().nonempty() }), + response: Scopes.guard.array(), + status: [200, 404, 422], }), async (ctx, next) => { const { @@ -133,6 +136,7 @@ export default function roleScopeRoutes( '/roles/:id/scopes/:scopeId', koaGuard({ params: object({ id: string().min(1), scopeId: string().min(1) }), + status: [204, 404], }), async (ctx, next) => { const { diff --git a/packages/core/src/routes/role.ts b/packages/core/src/routes/role.ts index 70ee96ca1..7d7202001 100644 --- a/packages/core/src/routes/role.ts +++ b/packages/core/src/routes/role.ts @@ -1,8 +1,8 @@ import type { RoleResponse } from '@logto/schemas'; -import { userInfoSelectFields, Roles } from '@logto/schemas'; +import { userInfoSelectFields, userInfoResponseGuard, Roles, Users } from '@logto/schemas'; import { generateStandardId } from '@logto/shared'; import { pick, tryThat } from '@silverhand/essentials'; -import { object, string, z } from 'zod'; +import { object, string, z, number } from 'zod'; import RequestError from '#src/errors/RequestError/index.js'; import koaGuard from '#src/middleware/koa-guard.js'; @@ -41,50 +41,74 @@ export default function roleRoutes( router.use('/roles(/.*)?', koaRoleRlsErrorHandler()); - router.get('/roles', koaPagination(), async (ctx, next) => { - const { limit, offset } = ctx.pagination; - const { searchParams } = ctx.request.URL; - - return tryThat( - async () => { - const search = parseSearchParamsForSearch(searchParams); - const excludeUserId = searchParams.get('excludeUserId'); - const usersRoles = excludeUserId ? await findUsersRolesByUserId(excludeUserId) : []; - const excludeRoleIds = usersRoles.map(({ roleId }) => roleId); - - const [{ count }, roles] = await Promise.all([ - countRoles(search, { excludeRoleIds }), - findRoles(search, limit, offset, { excludeRoleIds }), - ]); - - const rolesResponse: RoleResponse[] = await Promise.all( - roles.map(async (role) => { - const { count } = await countUsersRolesByRoleId(role.id); - const usersRoles = await findUsersRolesByRoleId(role.id, 3); - const users = await findUsersByIds(usersRoles.map(({ userId }) => userId)); - - return { - ...role, - usersCount: count, - featuredUsers: users.map(({ id, avatar, name }) => ({ id, avatar, name })), - }; + router.get( + '/roles', + koaPagination(), + koaGuard({ + response: Roles.guard + .merge( + object({ + usersCount: number(), + featuredUsers: Users.guard + .pick({ + avatar: true, + id: true, + name: true, + }) + .array(), }) - ); + ) + .array(), + status: [200, 400], + }), + async (ctx, next) => { + const { limit, offset } = ctx.pagination; + const { searchParams } = ctx.request.URL; - // Return totalCount to pagination middleware - ctx.pagination.totalCount = count; - ctx.body = rolesResponse; + return tryThat( + async () => { + const search = parseSearchParamsForSearch(searchParams); + const excludeUserId = searchParams.get('excludeUserId'); + const usersRoles = excludeUserId ? await findUsersRolesByUserId(excludeUserId) : []; + const excludeRoleIds = usersRoles.map(({ roleId }) => roleId); - return next(); - }, - (error) => { - if (error instanceof TypeError) { - throw new RequestError({ code: 'request.invalid_input', details: error.message }, error); + const [{ count }, roles] = await Promise.all([ + countRoles(search, { excludeRoleIds }), + findRoles(search, limit, offset, { excludeRoleIds }), + ]); + + const rolesResponse: RoleResponse[] = await Promise.all( + roles.map(async (role) => { + const { count } = await countUsersRolesByRoleId(role.id); + const usersRoles = await findUsersRolesByRoleId(role.id, 3); + const users = await findUsersByIds(usersRoles.map(({ userId }) => userId)); + + return { + ...role, + usersCount: count, + featuredUsers: users.map(({ id, avatar, name }) => ({ id, avatar, name })), + }; + }) + ); + + // Return totalCount to pagination middleware + ctx.pagination.totalCount = count; + ctx.body = rolesResponse; + + return next(); + }, + (error) => { + if (error instanceof TypeError) { + throw new RequestError( + { code: 'request.invalid_input', details: error.message }, + error + ); + } + throw error; } - throw error; - } - ); - }); + ); + } + ); router.post( '/roles', @@ -92,6 +116,8 @@ export default function roleRoutes( body: Roles.createGuard .omit({ id: true }) .extend({ scopeIds: z.string().min(1).array().optional() }), + status: [200, 422], + response: Roles.guard, }), async (ctx, next) => { const { body } = ctx.guard; @@ -128,6 +154,8 @@ export default function roleRoutes( '/roles/:id', koaGuard({ params: object({ id: string().min(1) }), + response: Roles.guard, + status: [200, 404], }), async (ctx, next) => { const { @@ -145,6 +173,8 @@ export default function roleRoutes( koaGuard({ body: Roles.createGuard.pick({ name: true, description: true }).partial(), params: object({ id: string().min(1) }), + response: Roles.guard, + status: [200, 404, 422], }), async (ctx, next) => { const { @@ -172,6 +202,7 @@ export default function roleRoutes( '/roles/:id', koaGuard({ params: object({ id: string().min(1) }), + status: [204, 404], }), async (ctx, next) => { const { @@ -189,6 +220,8 @@ export default function roleRoutes( koaPagination(), koaGuard({ params: object({ id: string().min(1) }), + response: userInfoResponseGuard.array(), + status: [200, 400, 404], }), async (ctx, next) => { const { @@ -232,7 +265,8 @@ export default function roleRoutes( '/roles/:id/users', koaGuard({ params: object({ id: string().min(1) }), - body: object({ userIds: string().min(1).array() }), + body: object({ userIds: string().min(1).array().nonempty() }), + status: [201, 404, 422], }), async (ctx, next) => { const { @@ -265,6 +299,7 @@ export default function roleRoutes( '/roles/:id/users/:userId', koaGuard({ params: object({ id: string().min(1), userId: string().min(1) }), + status: [204, 404], }), async (ctx, next) => { const { diff --git a/packages/core/src/utils/http.ts b/packages/core/src/utils/http.ts index ac6caa9a7..654987089 100644 --- a/packages/core/src/utils/http.ts +++ b/packages/core/src/utils/http.ts @@ -33,6 +33,7 @@ export const codeToMessage: Record = Object.freeze({ 416: 'Requested Range Not Satisfiable', 417: 'Expectation Failed', 418: "I'm a teapot", + 422: 'Unprocessable Content', 429: 'Too Many Requests', 500: 'Internal Server Error', 501: 'Not Implemented', diff --git a/packages/integration-tests/src/tests/api/role.scope.test.ts b/packages/integration-tests/src/tests/api/role.scope.test.ts index 029309e47..21d1e6ef7 100644 --- a/packages/integration-tests/src/tests/api/role.scope.test.ts +++ b/packages/integration-tests/src/tests/api/role.scope.test.ts @@ -22,6 +22,17 @@ describe('roles scopes', () => { expect(scopes[0]).toHaveProperty('id', scope.id); }); + it('should return 404 if role not found', async () => { + const response = await getRoleScopes('not-found').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should return empty if role has no scopes', async () => { + const role = await createRole(); + const scopes = await getRoleScopes(role.id); + expect(scopes.length).toBe(0); + }); + it('should assign scopes to role successfully', async () => { const role = await createRole(); const resource = await createResource(); @@ -32,6 +43,47 @@ describe('roles scopes', () => { expect(scopes.length).toBe(2); }); + it('should fail when try to assign empty scopes', async () => { + const role = await createRole(); + const response = await assignScopesToRole([], role.id).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(400); + }); + + it('should fail with invalid scope input', async () => { + const role = await createRole(); + const response = await assignScopesToRole([''], role.id).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(400); + }); + + it('should fail if role not found', async () => { + const resource = await createResource(); + const scope = await createScope(resource.id); + const response = await assignScopesToRole([scope.id], 'not-found').catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail if scope not found', async () => { + const role = await createRole(); + const response = await assignScopesToRole(['not-found'], role.id).catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail if scope already assigned to role', async () => { + const role = await createRole(); + const resource = await createResource(); + const scope1 = await createScope(resource.id); + const scope2 = await createScope(resource.id); + await assignScopesToRole([scope1.id], role.id); + const response = await assignScopesToRole([scope1.id, scope2.id], role.id).catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(422); + }); + it('should remove scope from role successfully', async () => { const role = await createRole(); const resource = await createResource(); @@ -46,6 +98,23 @@ describe('roles scopes', () => { expect(newScopes.length).toBe(0); }); + it('should fail when try to remove scope from role that is not assigned', async () => { + const role = await createRole(); + const resource = await createResource(); + const scope = await createScope(resource.id); + const response = await deleteScopeFromRole(scope.id, role.id).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail when try to remove scope from role that is not found', async () => { + const resource = await createResource(); + const scope = await createScope(resource.id); + const response = await deleteScopeFromRole(scope.id, 'not-found').catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + it('should fail when try to assign a scope to an internal role', async () => { const resource = await createResource(); const scope = await createScope(resource.id); diff --git a/packages/integration-tests/src/tests/api/role.test.ts b/packages/integration-tests/src/tests/api/role.test.ts index 43c2f5571..ece67d05c 100644 --- a/packages/integration-tests/src/tests/api/role.test.ts +++ b/packages/integration-tests/src/tests/api/role.test.ts @@ -65,6 +65,12 @@ describe('roles', () => { expect(role.description).toBe(createdRole.description); }); + it('should return 404 if role does not exist', async () => { + const response = await getRole('non_existent_role').catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + it('should update role details successfully', async () => { const role = await createRole(); @@ -91,6 +97,23 @@ describe('roles', () => { expect(response instanceof HTTPError && response.response.statusCode).toBe(422); }); + it('should fail when update a non-existent role', async () => { + const response = await updateRole('non_existent_role', { + name: 'new_name', + }).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail when try to update an internal role', async () => { + const role = await createRole(); + + const response = await updateRole(role.id, { + name: '#internal:foo', + }).catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode).toBe(403); + }); + it('should delete role successfully', async () => { const role = await createRole(); @@ -99,4 +122,10 @@ describe('roles', () => { const response = await getRole(role.id).catch((error: unknown) => error); expect(response instanceof HTTPError && response.response.statusCode).toBe(404); }); + + it('should return 404 if role does not exist', async () => { + const response = await deleteRole('non_existent_role').catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); }); diff --git a/packages/integration-tests/src/tests/api/role.user.test.ts b/packages/integration-tests/src/tests/api/role.user.test.ts index d54630437..9422e84b9 100644 --- a/packages/integration-tests/src/tests/api/role.user.test.ts +++ b/packages/integration-tests/src/tests/api/role.user.test.ts @@ -1,3 +1,5 @@ +import { HTTPError } from 'got'; + import { createUser } from '#src/api/index.js'; import { assignUsersToRole, createRole, deleteUserFromRole, getRoleUsers } from '#src/api/role.js'; import { generateNewUserProfile } from '#src/helpers/user.js'; @@ -13,6 +15,11 @@ describe('roles users', () => { expect(users[0]).toHaveProperty('id', user.id); }); + it('should return 404 if role not found', async () => { + const response = await getRoleUsers('not-found').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + it('should assign users to role successfully', async () => { const role = await createRole(); const user1 = await createUser(generateNewUserProfile({})); @@ -23,6 +30,34 @@ describe('roles users', () => { expect(users.length).toBe(2); }); + it('should fail when try to assign empty users', async () => { + const role = await createRole(); + const response = await assignUsersToRole([], role.id).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(400); + }); + + it('should fail with invalid user input', async () => { + const role = await createRole(); + const response = await assignUsersToRole([''], role.id).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode).toBe(400); + }); + + it('should fail if role not found', async () => { + const user = await createUser(generateNewUserProfile({})); + const response = await assignUsersToRole([user.id], 'not-found').catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail if user not found', async () => { + const role = await createRole(); + const response = await assignUsersToRole(['not-found'], role.id).catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + it('should remove user from role successfully', async () => { const role = await createRole(); const user = await createUser(generateNewUserProfile({})); @@ -35,4 +70,20 @@ describe('roles users', () => { const newUsers = await getRoleUsers(role.id); expect(newUsers.length).toBe(0); }); + + it('should fail if role not found when trying to remove user from role', async () => { + const user = await createUser(generateNewUserProfile({})); + const response = await deleteUserFromRole(user.id, 'not-found').catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); + + it('should fail if user not found when trying to remove user from role', async () => { + const role = await createRole(); + const response = await deleteUserFromRole('not-found', role.id).catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); + }); }); diff --git a/packages/schemas/src/types/user.ts b/packages/schemas/src/types/user.ts index f44c45397..0f6e49477 100644 --- a/packages/schemas/src/types/user.ts +++ b/packages/schemas/src/types/user.ts @@ -1,4 +1,6 @@ -import type { CreateUser } from '../db-entries/index.js'; +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', @@ -15,11 +17,26 @@ export const userInfoSelectFields = Object.freeze([ 'isSuspended', ] as const); -export type UserInfo = Pick< - CreateUser, +export type UserInfo = Pick< + User, Keys >; +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 UserProfileResponse = UserInfo & { hasPassword?: boolean }; /** Internal read-only roles for user tenants. */