Infrastructure/logto
0
Fork 0
mirror of https://github.com/logto-io/logto.git synced 2025-01-20 21:32:31 -05:00
Code Issues Activity

refactor(core): guard status from request errors (#5343)

Browse source 
* refactor(core): guard status from request errors

* refactor(core): update routes

* refactor: update tests and routes

* refactor(core): update protected app response code
This commit is contained in:
Gao Sun 2024-01-31 17:45:27 +08:00 • committed by GitHub
parent a3db1f7177
commit c7353f68e9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
22 changed files with 127 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

15
packages/core/src/libraries/protected-app.ts
View file

@ -31,14 +31,14 @@ export type ProtectedAppLibrary = ReturnType<typeof createProtectedAppLibrary>;
const getProviderConfig = async () => {
const { protectedAppConfigProviderConfig } = SystemContext.shared;
assertThat(protectedAppConfigProviderConfig, 'application.protected_app_not_configured');
assertThat(protectedAppConfigProviderConfig, 'application.protected_app_not_configured', 501);
return protectedAppConfigProviderConfig;
};
const getHostnameProviderConfig = async () => {
const { protectedAppHostnameProviderConfig } = SystemContext.shared;
assertThat(protectedAppHostnameProviderConfig, 'application.protected_app_not_configured');
assertThat(protectedAppHostnameProviderConfig, 'application.protected_app_not_configured', 501);
return protectedAppHostnameProviderConfig;
};
@ -114,7 +114,8 @@ const addDomainToRemote = async (
!(blockedDomains ?? []).some(
(domain) => hostname === domain || isSubdomainOf(hostname, domain)
),
'domain.domain_is_not_allowed'
'domain.domain_is_not_allowed',
422
);
const [fallbackOrigin, cloudflareData] = await Promise.all([
@ -205,11 +206,11 @@ export const createProtectedAppLibrary = (queries: Queries) => {
}
> => {
const { protectedAppHostnameProviderConfig } = SystemContext.shared;
assertThat(protectedAppHostnameProviderConfig, 'domain.not_configured');
assertThat(protectedAppHostnameProviderConfig, 'domain.not_configured', 501);
const application = await findApplicationById(applicationId);
const { protectedAppMetadata } = application;
assertThat(protectedAppMetadata, 'application.protected_app_not_configured');
assertThat(protectedAppMetadata, 'application.protected_app_not_configured', 501);
if (!protectedAppMetadata.customDomains || protectedAppMetadata.customDomains.length === 0) {
return {
@ -220,7 +221,7 @@ export const createProtectedAppLibrary = (queries: Queries) => {
const customDomains: CustomDomain[] = await Promise.all(
protectedAppMetadata.customDomains.map(async (domain) => {
assertThat(domain.cloudflareData, 'domain.cloudflare_data_missing');
assertThat(domain.cloudflareData, 'domain.cloudflare_data_missing', 501);
const cloudflareData = await getCustomHostname(
protectedAppHostnameProviderConfig,
@ -259,7 +260,7 @@ export const createProtectedAppLibrary = (queries: Queries) => {
}
);
// Not expected to happen, just to make TS happy
assertThat(updatedProtectedAppMetadata, 'application.protected_app_not_configured');
assertThat(updatedProtectedAppMetadata, 'application.protected_app_not_configured', 501);
return {
...application,

31
packages/core/src/middleware/koa-guard.test.ts
View file

@ -1,6 +1,7 @@
import { createMockUtils } from '@logto/shared/esm';
import { z } from 'zod';
import RequestError from '#src/errors/RequestError/index.js';
import ServerError from '#src/errors/ServerError/index.js';
import { emptyMiddleware, createContextWithRouteParameters } from '#src/utils/test-utils.js';
@ -134,6 +135,36 @@ describe('koaGuardMiddleware', () => {
await expect(koaGuard({ status: [200, 204] })(ctx, next)).rejects.toThrow(ServerError);
});
it('should throw when inner middleware throws invalid status', async () => {
const ctx = {
...baseCtx,
params: {},
body: {},
guard: {},
response: {},
};
next.mockRejectedValueOnce(new RequestError({ code: 'request.general', status: 400 }));
// @ts-expect-error
await expect(koaGuard({ status: 200 })(ctx, next)).rejects.toThrow(ServerError);
});
it('should pass when inner middleware throws valid status', async () => {
const ctx = {
...baseCtx,
params: {},
body: {},
guard: {},
response: {},
};
next.mockRejectedValueOnce(new RequestError({ code: 'request.general', status: 400 }));
// @ts-expect-error
await expect(koaGuard({ status: [200, 400] })(ctx, next)).rejects.toThrow(RequestError);
});
it('should throw when params type is invalid', async () => {
const ctx = {
...baseCtx,

47
packages/core/src/middleware/koa-guard.ts
View file

@ -49,8 +49,11 @@ export type GuardConfig<QueryT, BodyT, ParametersT, ResponseT, FilesT> = {
*/
response?: ZodType<ResponseT, ZodTypeDef, unknown>;
/**
* Guard response status code. It produces a `ServerError` (500)
* if the response does not satisfy any of the given value(s).
* Guard response status code. It produces a `ServerError` (500) if the response does not satisfy
* any of the given value(s).
*
* Note: It will also guard the status code from `RequestError` that is thrown by inner
* middleware.
*/
status?: number | number[];
files?: ZodType<FilesT, ZodTypeDef, unknown>;
@ -150,19 +153,43 @@ export default function koaGuard<
GuardResponseT
>
> = async function (ctx, next) {
await (body ?? files
? koaBody<StateT, ContextT>({ multipart: Boolean(files) })(ctx, async () => guard(ctx, next))
: guard(ctx, next));
/**
* Assert the status code matches the value(s) in the config. If the config does not
* specify a status code, it will not assert anything.
*
* @param value The status code to assert.
* @throws {StatusCodeError} If the status code does not match the value(s) in the config.
*/
const assertStatusCode = (value: number) => {
if (status === undefined) {
return;
}
if (status !== undefined) {
assertThat(
Array.isArray(status)
? status.includes(ctx.response.status)
: status === ctx.response.status,
new StatusCodeError(status, ctx.response.status)
Array.isArray(status) ? status.includes(value) : status === value,
new StatusCodeError(status, value)
);
};
try {
await (body ?? files
? koaBody<StateT, ContextT>({ multipart: Boolean(files) })(ctx, async () =>
guard(ctx, next)
)
: guard(ctx, next));
} catch (error: unknown) {
// Assert the status code from `RequestError` that is thrown by inner middleware.
// Ignore guard errors since they will be always 400 and can be automatically documented
// in the OpenAPI route.
if (error instanceof RequestError && !error.code.startsWith('guard.')) {
assertStatusCode(error.status);
}
throw error;
}
assertStatusCode(ctx.response.status);
if (response !== undefined) {
const result = response.safeParse(ctx.body);

2
packages/core/src/routes/applications/application-protected-app-metadata.test.ts
View file

@ -126,7 +126,7 @@ describe('application protected app metadata routes', () => {
.send({
domain: mockDomain,
});
expect(response.status).toEqual(400);
expect(response.status).toEqual(422);
});
});

11
packages/core/src/routes/applications/application-protected-app-metadata.ts
View file

@ -45,7 +45,7 @@ export default function applicationProtectedAppMetadataRoutes<T extends AuthedRo
customDomainsPathname,
koaGuard({
params: z.object(params),
status: [200, 400, 404],
status: [200, 400, 404, 501],
response: customDomainsGuard,
}),
async (ctx, next) => {
@ -65,20 +65,21 @@ export default function applicationProtectedAppMetadataRoutes<T extends AuthedRo
koaGuard({
params: z.object(params),
body: z.object({ domain: z.string() }),
status: [201, 404, 422],
status: [201, 400, 404, 422, 501],
}),
async (ctx, next) => {
const { id } = ctx.guard.params;
const { domain } = ctx.guard.body;
const { protectedAppMetadata, oidcClientMetadata } = await findApplicationById(id);
assertThat(protectedAppMetadata, 'application.protected_app_not_configured');
assertThat(protectedAppMetadata, 'application.protected_app_not_configured', 501);
// Only allow one domain, be careful when changing this, the unique index on the database
// is based on this assumption
assertThat(
!protectedAppMetadata.customDomains || protectedAppMetadata.customDomains.length === 0,
'domain.limit_to_one_domain'
'domain.limit_to_one_domain',
422
);
const customDomain = await addDomainToRemote(domain);
@ -116,7 +117,7 @@ export default function applicationProtectedAppMetadataRoutes<T extends AuthedRo
...params,
domain: z.string(),
}),
status: [204, 404],
status: [204, 404, 501],
}),
async (ctx, next) => {
const { id, domain } = ctx.guard.params;

2
packages/core/src/routes/applications/application-sign-in-experience.ts
View file

@ -43,7 +43,7 @@ function applicationSignInExperienceRoutes<T extends AuthedRouter>(
}),
body: applicationSignInExperienceCreateGuard,
response: ApplicationSignInExperiences.guard,
status: [200, 201, 404],
status: [200, 201, 404, 422],
}),
async (ctx, next) => {
const {

2
packages/core/src/routes/applications/application.test.ts
View file

@ -354,7 +354,7 @@ describe('application route', () => {
});
await expect(applicationRequest.delete('/applications/foo')).resolves.toHaveProperty(
'status',
400
422
);
});
});

5
packages/core/src/routes/applications/application.ts
View file

@ -314,7 +314,7 @@ export default function applicationRoutes<T extends AuthedRouter>(
koaGuard({
params: object({ id: string().min(1) }),
response: z.undefined(),
status: [204, 404],
status: [204, 404, 422],
}),
async (ctx, next) => {
const { id } = ctx.guard.params;
@ -322,7 +322,8 @@ export default function applicationRoutes<T extends AuthedRouter>(
if (type === ApplicationType.Protected && protectedAppMetadata) {
assertThat(
!protectedAppMetadata.customDomains || protectedAppMetadata.customDomains.length === 0,
'application.should_delete_custom_domains_first'
'application.should_delete_custom_domains_first',
422
);
await protectedApps.deleteRemoteAppConfigs(protectedAppMetadata.host);
}

2
packages/core/src/routes/authn.ts
View file

@ -110,7 +110,7 @@ export default function authnRoutes<T extends AnonymousRouter>(
koaGuard({
body: jsonObjectGuard,
params: z.object({ connectorId: z.string().min(1) }),
status: 302,
status: [302, 404],
}),
async (ctx, next) => {
const {

2
packages/core/src/routes/connector/index.ts
View file

@ -68,7 +68,7 @@ export default function connectorRoutes<T extends AuthedRouter>(
*/
.merge(Connectors.createGuard.pick({ id: true }).partial()),
response: connectorResponseGuard,
status: [200, 422],
status: [200, 400, 422],
}),
async (ctx, next) => {
const {

4
packages/core/src/routes/interaction/additional.ts
View file

@ -91,7 +91,7 @@ export default function additionalRoutes<T extends IRouterParamContext>(
`${interactionPrefix}/${verificationPath}/verification-code`,
koaGuard({
body: requestVerificationCodePayloadGuard,
status: [204, 400, 404],
status: [204, 400, 404, 501],
}),
async (ctx, next) => {
const { interactionDetails, guard, createLog } = ctx;
@ -263,7 +263,7 @@ export default function additionalRoutes<T extends IRouterParamContext>(
router.post(
`${interactionPrefix}/${verificationPath}/webauthn-authentication`,
koaGuard({
status: [200],
status: [200, 400],
response: webAuthnAuthenticationOptionsGuard,
}),
async (ctx, next) => {

2
packages/core/src/routes/interaction/index.ts
View file

@ -324,7 +324,7 @@ export default function interactionRoutes<T extends AnonymousRouter>(
router.post(
`${interactionPrefix}/submit`,
koaGuard({
status: [200, 204, 400, 401, 404, 422],
status: [200, 204, 400, 401, 403, 404, 422],
response: z
.object({
redirectTo: z.string(),

6
packages/core/src/routes/organization/invitations.openapi.json
View file

@ -54,6 +54,9 @@
"201": {
"description": "The organization invitation was created successfully."
},
"400": {
"description": "The organization invitation could not be created. This can happen if the input is invalid or if the expiration date is in the past."
},
"501": {
"description": "No email connector is configured for the tenant."
}
@ -105,6 +108,9 @@
"responses": {
"200": {
"description": "The organization invitation status was updated successfully."
},
"422": {
"description": "The organization invitation status could not be updated. This can happen if the current status is not \"Pending\" or if the status is \"Accepted\" and the accepted user ID is not provided."
}
}
}

4
packages/core/src/routes/organization/invitations.ts
View file

@ -51,7 +51,7 @@ export default function organizationInvitationRoutes<T extends AuthedRouter>(
messagePayload: sendMessagePayloadGuard.or(z.literal(false)).default(false),
}),
response: organizationInvitationEntityGuard,
status: [201],
status: [201, 400, 501],
}),
async (ctx, next) => {
const {
@ -89,7 +89,7 @@ export default function organizationInvitationRoutes<T extends AuthedRouter>(
]),
}),
response: organizationInvitationEntityGuard,
status: [200],
status: [200, 422],
}),
async (ctx, next) => {
const { id } = ctx.guard.params;

2
packages/core/src/routes/resource.ts
View file

@ -291,7 +291,7 @@ export default function resourceRoutes<T extends AuthedRouter>(
params: object({ resourceId: string().min(1), scopeId: string().min(1) }),
body: Scopes.createGuard.pick({ name: true, description: true }).partial(),
response: Scopes.guard,
status: [200, 404, 422],
status: [200, 400, 404, 422],
}),
async (ctx, next) => {
const {

9
packages/core/src/routes/sso-connector/index.ts
View file

@ -72,7 +72,7 @@ export default function singleSignOnConnectorsRoutes<T extends AuthedRouter>(
koaGuard({
body: ssoConnectorCreateGuard,
response: SsoConnectors.guard,
status: [200, 422],
status: [200, 409, 422],
}),
async (ctx, next) => {
const { body } = ctx.guard;
@ -98,7 +98,7 @@ export default function singleSignOnConnectorsRoutes<T extends AuthedRouter>(
// Validate the connector name is unique
if (connectorName) {
const duplicateConnector = await ssoConnectors.findByConnectorName(connectorName);
assertThat(!duplicateConnector, 'single_sign_on.duplicate_connector_name');
assertThat(!duplicateConnector, 'single_sign_on.duplicate_connector_name', 409);
}
const connectorId = generateStandardShortId();
@ -212,7 +212,7 @@ export default function singleSignOnConnectorsRoutes<T extends AuthedRouter>(
params: z.object({ id: z.string().min(1) }),
body: ssoConnectorPatchGuard,
response: ssoConnectorWithProviderConfigGuard,
status: [200, 404, 422],
status: [200, 404, 409, 422],
}),
async (ctx, next) => {
const {
@ -238,7 +238,8 @@ export default function singleSignOnConnectorsRoutes<T extends AuthedRouter>(
// Should not block the update of the current connector.
assertThat(
!duplicateConnector || duplicateConnector.id === id,
'single_sign_on.duplicate_connector_name'
'single_sign_on.duplicate_connector_name',
409
);
}

2
packages/core/src/routes/system.ts
View file

@ -18,7 +18,7 @@ export default function systemRoutes<T extends AuthedRouter>(
'/systems/application',
koaGuard({
response: object({ protectedApps: object({ defaultDomain: string() }) }),
status: [200],
status: [200, 501],
}),
async (ctx, next) => {
const defaultDomain = await protectedApps.getDefaultDomain();

2
packages/core/src/routes/verification-code.ts
View file

@ -21,7 +21,7 @@ export default function verificationCodeRoutes<T extends AuthedRouter>(
'/verification-codes',
koaGuard({
body: requestVerificationCodePayloadGuard,
status: [204, 400],
status: [204, 400, 501],
}),
async (ctx, next) => {
const code = await createPasscode(undefined, codeType, ctx.guard.body);

16
packages/core/src/utils/assert-that.ts
View file

@ -3,13 +3,17 @@ import { assert } from '@silverhand/essentials';
import RequestError from '#src/errors/RequestError/index.js';
type AssertThatFunction = <E extends Error>(
value: unknown,
error: E | LogtoErrorCode
) => asserts value;
type AssertThatFunction = {
<E extends Error>(value: unknown, error: E): asserts value;
(value: unknown, error: LogtoErrorCode, status?: number): asserts value;
};
const assertThat: AssertThatFunction = (value, error): asserts value => {
assert(value, error instanceof Error ? error : new RequestError({ code: error }));
const assertThat: AssertThatFunction = <E extends Error>(
value: unknown,
error: E | LogtoErrorCode,
status?: number
): asserts value => {
assert(value, error instanceof Error ? error : new RequestError({ code: error, status }));
};
export default assertThat;

2
packages/integration-tests/src/tests/api/application/application-sign-in-experience.test.ts
View file

@ -56,7 +56,7 @@ describe('application sign in experience', () => {
);
});
it('should throw 400 if application is not third-party', async () => {
it('should throw if application is not third-party', async () => {
await expectRejects(
setApplicationSignInExperience(
applications.get('firstPartyApp')!.id,

6
packages/integration-tests/src/tests/api/resource.scope.test.ts
View file

@ -1,3 +1,5 @@
import assert from 'node:assert';
import { defaultManagementApi } from '@logto/schemas';
import { HTTPError } from 'got';
@ -89,7 +91,9 @@ describe('scopes', () => {
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);
assert(response instanceof HTTPError);
expect(response.response.statusCode).toBe(400);
});
it('should return 404 when update scope with invalid resource id', async () => {

4
packages/integration-tests/src/tests/api/sso-connectors.test.ts
View file

@ -64,7 +64,7 @@ describe('post sso-connectors', () => {
providerName: 'OIDC',
connectorName: 'test connector name',
}),
{ code: 'single_sign_on.duplicate_connector_name', statusCode: 400 }
{ code: 'single_sign_on.duplicate_connector_name', statusCode: 409 }
);
await deleteSsoConnectorById(id);
@ -202,7 +202,7 @@ describe('patch sso-connector by id', () => {
patchSsoConnectorById(id2, {
connectorName: 'test connector name',
}),
{ code: 'single_sign_on.duplicate_connector_name', statusCode: 400 }
{ code: 'single_sign_on.duplicate_connector_name', statusCode: 409 }
);
await deleteSsoConnectorById(id);