From beb6ebad501731ef48ac467f803af2da02c5ae2e Mon Sep 17 00:00:00 2001 From: simeng-li Date: Mon, 15 May 2023 15:12:15 +0800 Subject: [PATCH] test(core): update the resource response guard and integration tests (#3824) * test(core): update the resouce response guard and integration tests update the resource response guard and integration tests * fix(test): fix test fix resource test * fix(schemas): add non-empty string type guard (#3826) * fix(schemas): add non-empty zod guard to the string typed schema field add non-empty zod guard to the string typed schema field * fix(schemas): comment out non-empty string guard comment our non-empty string guard for now * chore: add change set add change set * fix(schemas): remove the guard if has default value remove the guard if has default value * refactor(core): replace zod merge using zod extend replace zod merge using zod extend --- .changeset/smooth-icons-fetch.md | 7 +++ packages/core/src/routes/resource.test.ts | 4 +- packages/core/src/routes/resource.ts | 57 ++++++++++++++++--- .../integration-tests/src/api/resource.ts | 2 + .../src/tests/api/resource.scope.test.ts | 49 +++++++++++++++- .../src/tests/api/resource.test.ts | 43 +++++++++++++- packages/schemas/src/gen/schema.ts | 45 ++++++++++----- 7 files changed, 181 insertions(+), 26 deletions(-) create mode 100644 .changeset/smooth-icons-fetch.md diff --git a/.changeset/smooth-icons-fetch.md b/.changeset/smooth-icons-fetch.md new file mode 100644 index 000000000..d53aafea5 --- /dev/null +++ b/.changeset/smooth-icons-fetch.md @@ -0,0 +1,7 @@ +--- +"@logto/schemas": patch +--- + +## Add min length 1 type guard for all string typed db schema fields + +Update the `@logto/schemas` zod guard generation method to include a min length of 1 for all the required string typed db fields. diff --git a/packages/core/src/routes/resource.test.ts b/packages/core/src/routes/resource.test.ts index e1d4a8793..e3ae4dc64 100644 --- a/packages/core/src/routes/resource.test.ts +++ b/packages/core/src/routes/resource.test.ts @@ -95,7 +95,7 @@ describe('resource routes', () => { .post('/resources') .send({ name, indicator, accessTokenTtl }); - expect(response.status).toEqual(200); + expect(response.status).toEqual(201); expect(response.body).toEqual({ tenantId: 'fake_tenant', id: 'randomId', @@ -188,7 +188,7 @@ describe('resource routes', () => { .post('/resources/foo/scopes') .send({ name, description }); - expect(response.status).toEqual(200); + expect(response.status).toEqual(201); expect(findResourceById).toHaveBeenCalledWith('foo'); expect(insertScope).toHaveBeenCalledWith({ id: 'randomId', diff --git a/packages/core/src/routes/resource.ts b/packages/core/src/routes/resource.ts index 35e336ba9..6db684e61 100644 --- a/packages/core/src/routes/resource.ts +++ b/packages/core/src/routes/resource.ts @@ -45,6 +45,8 @@ export default function resourceRoutes( query: object({ includeScopes: string().optional(), }), + response: Resources.guard.extend({ scopes: Scopes.guard.array().optional() }).array(), + status: [200], }), async (ctx, next) => { const { limit, offset, disabled } = ctx.pagination; @@ -75,6 +77,8 @@ export default function resourceRoutes( '/resources', koaGuard({ body: Resources.createGuard.omit({ id: true }), + response: Resources.guard.extend({ scopes: Scopes.guard.array().optional() }), + status: [201, 422], }), async (ctx, next) => { const { body } = ctx.guard; @@ -94,6 +98,7 @@ export default function resourceRoutes( ...body, }); + ctx.status = 201; ctx.body = { ...resource, scopes: [] }; return next(); @@ -102,7 +107,11 @@ export default function resourceRoutes( router.get( '/resources/:id', - koaGuard({ params: object({ id: string().min(1) }) }), + koaGuard({ + params: object({ id: string().min(1) }), + response: Resources.guard, + status: [200, 404], + }), async (ctx, next) => { const { params: { id }, @@ -120,6 +129,8 @@ export default function resourceRoutes( koaGuard({ params: object({ id: string().min(1) }), body: Resources.createGuard.omit({ id: true, indicator: true }).partial(), + response: Resources.guard, + status: [200, 404], }), async (ctx, next) => { const { @@ -136,7 +147,7 @@ export default function resourceRoutes( router.delete( '/resources/:id', - koaGuard({ params: object({ id: string().min(1) }) }), + koaGuard({ params: object({ id: string().min(1) }), status: [204, 404] }), async (ctx, next) => { const { id } = ctx.guard.params; await deleteResourceById(id); @@ -149,7 +160,11 @@ export default function resourceRoutes( router.get( '/resources/:resourceId/scopes', koaPagination(), - koaGuard({ params: object({ resourceId: string().min(1) }) }), + koaGuard({ + params: object({ resourceId: string().min(1) }), + status: [200, 400], + response: Scopes.guard.array(), + }), async (ctx, next) => { const { params: { resourceId }, @@ -161,14 +176,14 @@ export default function resourceRoutes( async () => { const search = parseSearchParamsForSearch(searchParams); - const [{ count }, roles] = await Promise.all([ + const [{ count }, scopes] = await Promise.all([ countScopesByResourceId(resourceId, search), searchScopesByResourceId(resourceId, search, limit, offset), ]); // Return totalCount to pagination middleware ctx.pagination.totalCount = count; - ctx.body = roles; + ctx.body = scopes; return next(); }, @@ -190,6 +205,8 @@ export default function resourceRoutes( koaGuard({ params: object({ resourceId: string().min(1) }), body: Scopes.createGuard.pick({ name: true, description: true }), + response: Scopes.guard, + status: [201, 422, 400, 404], }), async (ctx, next) => { const { @@ -199,6 +216,17 @@ export default function resourceRoutes( assertThat(!/\s/.test(body.name), 'scope.name_with_space'); + assertThat( + await findResourceById(resourceId), + new RequestError({ + code: 'entity.not_exists_with_id', + name: 'resource', + id: resourceId, + resourceId, + status: 404, + }) + ); + assertThat( !(await findScopeByNameAndResourceId(body.name, resourceId)), new RequestError({ @@ -208,6 +236,7 @@ export default function resourceRoutes( }) ); + ctx.status = 201; ctx.body = await insertScope({ ...body, id: scopeId(), @@ -222,7 +251,9 @@ export default function resourceRoutes( '/resources/:resourceId/scopes/:scopeId', koaGuard({ params: object({ resourceId: string().min(1), scopeId: string().min(1) }), - body: Scopes.createGuard.pick({ name: true, description: true }), + body: Scopes.createGuard.pick({ name: true, description: true }).partial(), + response: Scopes.guard, + status: [200, 404, 422], }), async (ctx, next) => { const { @@ -230,6 +261,17 @@ export default function resourceRoutes( body, } = ctx.guard; + assertThat( + await findResourceById(resourceId), + new RequestError({ + code: 'entity.not_exists_with_id', + name: 'resource', + id: resourceId, + resourceId, + status: 404, + }) + ); + if (body.name) { assertThat(!/\s/.test(body.name), 'scope.name_with_space'); assertThat( @@ -252,10 +294,11 @@ export default function resourceRoutes( '/resources/:resourceId/scopes/:scopeId', koaGuard({ params: object({ resourceId: string().min(1), scopeId: string().min(1) }), + status: [204, 404], }), async (ctx, next) => { const { - params: { resourceId, scopeId }, + params: { scopeId }, } = ctx.guard; await deleteScopeById(scopeId); diff --git a/packages/integration-tests/src/api/resource.ts b/packages/integration-tests/src/api/resource.ts index 3b81ddffa..4aaa44b25 100644 --- a/packages/integration-tests/src/api/resource.ts +++ b/packages/integration-tests/src/api/resource.ts @@ -15,6 +15,8 @@ export const createResource = async (name?: string, indicator?: string) => }) .json(); +export const getResources = async () => authedAdminApi.get('resources').json(); + export const getResource = async (resourceId: string, options?: OptionsOfTextResponseBody) => authedAdminApi.get(`resources/${resourceId}`, options).json(); diff --git a/packages/integration-tests/src/tests/api/resource.scope.test.ts b/packages/integration-tests/src/tests/api/resource.scope.test.ts index 8c8aebbf7..e17581c38 100644 --- a/packages/integration-tests/src/tests/api/resource.scope.test.ts +++ b/packages/integration-tests/src/tests/api/resource.scope.test.ts @@ -33,6 +33,27 @@ describe('scopes', () => { expect(response instanceof HTTPError && response.response.statusCode === 422).toBe(true); }); + it('should return 404 when create scope with invalid resource id', async () => { + const response = await createScope('invalid_resource_id', 'invalid_scope_name').catch( + (error: unknown) => error + ); + expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + }); + + it('should return 400 if scope name is empty', async () => { + const resource = await createResource(); + + const response = await createScope(resource.id, '').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode === 400).toBe(true); + }); + + it('should return 400 if scope name has empty space', async () => { + const resource = await createResource(); + + const response = await createScope(resource.id, 'scope id').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode === 400).toBe(true); + }); + it('should update scope successfully', async () => { const resource = await createResource(); const scope = await createScope(resource.id); @@ -58,11 +79,37 @@ describe('scopes', () => { const createdScope2 = await createScope(resource.id); const response = await updateScope(resource.id, createdScope2.id, { name: createdScope.name, - description: '', }).catch((error: unknown) => error); expect(response instanceof HTTPError && response.response.statusCode === 422).toBe(true); }); + it('should return 400 if update scope name that has empty space', async () => { + const resource = await createResource(); + const scope = await createScope(resource.id); + const response = await updateScope(resource.id, scope.id, { + name: 'scope name', + }).catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode === 400).toBe(true); + }); + + it('should return 404 when update scope with invalid resource id', async () => { + const response = await updateScope('invalid_resource_id', 'invalid_scope_id', { + name: 'scope', + }).catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + }); + + it('should return 404 when update scope with invalid scope id', async () => { + const resource = await createResource(); + + const response = await updateScope(resource.id, 'invalid_scope_id', { + name: 'scope', + }).catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + }); + it('should delete scope successfully', async () => { const resource = await createResource(); const scope = await createScope(resource.id); diff --git a/packages/integration-tests/src/tests/api/resource.test.ts b/packages/integration-tests/src/tests/api/resource.test.ts index c6f4b2b74..3fc4f57e8 100644 --- a/packages/integration-tests/src/tests/api/resource.test.ts +++ b/packages/integration-tests/src/tests/api/resource.test.ts @@ -1,7 +1,13 @@ import { defaultManagementApi } from '@logto/schemas'; import { HTTPError } from 'got'; -import { createResource, getResource, updateResource, deleteResource } from '#src/api/index.js'; +import { + createResource, + getResource, + getResources, + updateResource, + deleteResource, +} from '#src/api/index.js'; import { createResponseWithCode } from '#src/helpers/admin-tenant.js'; import { generateResourceIndicator, generateResourceName } from '#src/utils.js'; @@ -12,6 +18,11 @@ describe('admin console api resources', () => { expect(fetchedManagementApiResource).toMatchObject(defaultManagementApi.resource); }); + it('should return 404 if resource not found', async () => { + const response = await getResource('not_found').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + }); + it('should create api resource successfully', async () => { const resourceName = generateResourceName(); const resourceIndicator = generateResourceIndicator(); @@ -40,6 +51,20 @@ describe('admin console api resources', () => { ); }); + it('should get resource list successfully', async () => { + const resourceName = generateResourceName(); + const resourceIndicator = generateResourceIndicator(); + + // Create first resource + await createResource(resourceName, resourceIndicator); + + // Get all resources + const resources = await getResources(); + + expect(resources.length).toBeGreaterThan(0); + expect(resources.findIndex(({ name }) => name === resourceName)).not.toBe(-1); + }); + it('should update api resource details successfully', async () => { const resource = await createResource(); @@ -58,6 +83,17 @@ describe('admin console api resources', () => { expect(updatedResource.accessTokenTtl).toBe(newAccessTokenTtl); }); + it('should throw error when update api resource with invalid payload', async () => { + const resource = await createResource(); + + const response = await updateResource(resource.id, { + // @ts-expect-error for testing + name: 123, + }).catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode === 400).toBe(true); + }); + it('should not update api resource indicator', async () => { const resource = await createResource(); const newResourceName = `new_${resource.name}`; @@ -83,4 +119,9 @@ describe('admin console api resources', () => { const response = await getResource(createdResource.id).catch((error: unknown) => error); expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); }); + + it('should throw 404 when delete api resource not found', async () => { + const response = await deleteResource('dummy_id').catch((error: unknown) => error); + expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + }); }); diff --git a/packages/schemas/src/gen/schema.ts b/packages/schemas/src/gen/schema.ts index fb733c533..d879e2727 100644 --- a/packages/schemas/src/gen/schema.ts +++ b/packages/schemas/src/gen/schema.ts @@ -45,11 +45,17 @@ export const generateSchema = ({ name, fields }: TableWithType) => { )}${conditionalString((nullable || hasDefaultValue) && '.optional()')},`; } - return ` ${camelcase(name)}: z.${ - isEnum ? `nativeEnum(${type})` : `${type}()` - }${conditionalString(isString && maxLength && `.max(${maxLength})`)}${conditionalString( - isArray && '.array()' - )}${conditionalString(nullable && '.nullable()')}${conditionalString( + return ` ${camelcase(name)}: z.${isEnum ? `nativeEnum(${type})` : `${type}()`}${ + // Non-nullable strings should have a min length of 1 + conditionalString( + isString && !(nullable || hasDefaultValue || name === tenantId) && `.min(1)` + ) + }${ + // String types value in DB should have a max length + conditionalString(isString && maxLength && `.max(${maxLength})`) + }${conditionalString(isArray && '.array()')}${conditionalString( + nullable && '.nullable()' + )}${conditionalString( (nullable || hasDefaultValue || name === tenantId) && '.optional()' )},`; } @@ -58,19 +64,28 @@ export const generateSchema = ({ name, fields }: TableWithType) => { '', `const guard: Guard<${modelName}> = z.object({`, - ...fields.map(({ name, type, isArray, isEnum, nullable, tsType, isString, maxLength }) => { - if (tsType) { - return ` ${camelcase(name)}: ${camelcase(tsType)}Guard${conditionalString( + ...fields.map( + // eslint-disable-next-line complexity + ({ name, type, isArray, isEnum, nullable, tsType, isString, maxLength, hasDefaultValue }) => { + if (tsType) { + return ` ${camelcase(name)}: ${camelcase(tsType)}Guard${conditionalString( + nullable && '.nullable()' + )},`; + } + + return ` ${camelcase(name)}: z.${isEnum ? `nativeEnum(${type})` : `${type}()`}${ + // Non-nullable strings should have a min length of 1 + conditionalString( + isString && !(nullable || hasDefaultValue || name === tenantId) && `.min(1)` + ) + }${ + // String types value in DB should have a max length + conditionalString(isString && maxLength && `.max(${maxLength})`) + }${conditionalString(isArray && '.array()')}${conditionalString( nullable && '.nullable()' )},`; } - - return ` ${camelcase(name)}: z.${ - isEnum ? `nativeEnum(${type})` : `${type}()` - }${conditionalString(isString && maxLength && `.max(${maxLength})`)}${conditionalString( - isArray && '.array()' - )}${conditionalString(nullable && '.nullable()')},`; - }), + ), ' });', '', `export const ${camelcase(name, {