0
Fork 0
mirror of https://github.com/logto-io/logto.git synced 2025-03-24 22:41:28 -05:00

refactor(core): improve oidc error handling (#4573)

* refactor(core): improve oidc error handling

* refactor(core): fix tests
This commit is contained in:
Gao Sun 2023-09-25 16:16:04 +08:00 committed by GitHub
parent 7b43dd5de6
commit b8e592d669
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
25 changed files with 247 additions and 167 deletions

View file

@ -137,7 +137,9 @@ export const parseLocaleFiles = (filePath: string): ParsedTuple => {
// eslint-disable-next-line @silverhand/fp/no-mutation
nestedObject[key] = [value, false];
} else {
consoleLog.fatal('Unsupported comment:', comment);
// eslint-disable-next-line @silverhand/fp/no-mutation
nestedObject[key] = [value, true];
consoleLog.warn('Unsupported comment:', comment);
}
} else {
// eslint-disable-next-line @silverhand/fp/no-mutation

View file

@ -2,7 +2,9 @@
* Setup environment variables for unit test
*/
import en from '@logto/phrases/lib/locales/en/index.js';
import { createMockUtils } from '@logto/shared/esm';
import { init } from 'i18next';
const { jest } = import.meta;
const { mockEsm, mockEsmDefault } = createMockUtils(jest);
@ -32,3 +34,10 @@ mockEsmDefault('#src/env-set/oidc.js', () => () => ({
// Logger is not considered in all test cases
// eslint-disable-next-line unicorn/consistent-function-scoping
mockEsm('koa-logger', () => ({ default: () => (_, next) => next() }));
// Init i18next and load en locale only
await init({
fallbackLng: 'en',
supportedLngs: ['en'],
resources: { en },
});

View file

@ -138,7 +138,7 @@ describe('testHook', () => {
await expect(testHook(hook.id, [HookEvent.PostSignIn], hook.config)).rejects.toThrowError(
new RequestError({
code: 'hook.send_test_payload_failed',
message: 'test error',
message: 'Error: test error',
status: 422,
})
);

View file

@ -65,7 +65,7 @@ describe('validate sign-up', () => {
}).toMatchError(
new RequestError({
code: 'sign_in_experiences.enabled_connector_not_found',
type: ConnectorType.Sms,
type: ConnectorType.Email,
})
);
});

View file

@ -217,6 +217,7 @@ describe('koaConnectorErrorHandler middleware', () => {
{
code: 'connector.general',
status: 400,
errorDescription: JSON.stringify({ message }),
},
{ message }
)
@ -235,7 +236,7 @@ describe('koaConnectorErrorHandler middleware', () => {
{
code: 'connector.general',
status: 400,
errorDescription: '\nMock General connector errors',
errorDescription: 'Mock General connector errors',
},
message
)

View file

@ -16,7 +16,7 @@ export default function koaErrorHandler<StateT, ContextT, BodyT>(): Middleware<
try {
await next();
} catch (error: unknown) {
if (!EnvSet.values.isProduction) {
if (!EnvSet.values.isUnitTest && !EnvSet.values.isProduction) {
consoleLog.error(error);
}

View file

@ -1,118 +1,98 @@
import i18next from 'i18next';
import { type Context } from 'koa';
import { errors } from 'oidc-provider';
import RequestError from '#src/errors/RequestError/index.js';
import { createContextWithRouteParameters } from '#src/utils/test-utils.js';
import koaOIDCErrorHandler from './koa-oidc-error-handler.js';
import koaOidcErrorHandler, { errorOut } from './koa-oidc-error-handler.js';
const { jest } = import.meta;
describe('koaOIDCErrorHandler middleware', () => {
describe('koaOidcErrorHandler middleware', () => {
const next = jest.fn();
const ctx = createContextWithRouteParameters();
const expectErrorResponse = async (
ctx: Context,
error: errors.OIDCProviderError,
extraMatch?: Record<string, unknown>
) => {
next.mockImplementationOnce(() => {
throw error;
});
const out = errorOut(error);
const code = `oidc.${out.error}`;
await koaOidcErrorHandler()(ctx, next);
expect(ctx.status).toBe(error.statusCode);
expect(ctx.body).toMatchObject({
...out,
code,
message: i18next.t('errors:' + code),
...extraMatch,
});
};
it('should throw no errors if no errors are caught', async () => {
await expect(koaOIDCErrorHandler()(ctx, next)).resolves.not.toThrow();
const ctx = createContextWithRouteParameters();
await expect(koaOidcErrorHandler()(ctx, next)).resolves.not.toThrow();
});
it('should throw original error if error type is not OIDCProviderError', async () => {
const ctx = createContextWithRouteParameters();
const error = new Error('err');
next.mockImplementationOnce(() => {
throw error;
});
await expect(koaOIDCErrorHandler()(ctx, next)).rejects.toMatchError(error);
await expect(koaOidcErrorHandler()(ctx, next)).rejects.toMatchError(error);
});
it('Invalid Scope', async () => {
it('should recognize invalid scope error', async () => {
const ctx = createContextWithRouteParameters();
const error_description = 'Mock scope is invalid';
const mockScope = 'read:foo';
const error = new errors.InvalidScope(error_description, mockScope);
next.mockImplementationOnce(() => {
throw error;
});
await expect(koaOIDCErrorHandler()(ctx, next)).rejects.toMatchError(
new RequestError(
{
code: 'oidc.invalid_scope',
status: error.status,
expose: true,
scope: mockScope,
},
{ error_description }
)
);
await expectErrorResponse(ctx, new errors.InvalidScope(error_description, mockScope));
});
it('Session Not Found', async () => {
const error_description = 'session not found';
const error = new errors.SessionNotFound('session not found');
it('should transform session not found error code', async () => {
const ctx = createContextWithRouteParameters();
next.mockImplementationOnce(() => {
throw error;
await expectErrorResponse(ctx, new errors.SessionNotFound('session not found'), {
code: 'session.not_found',
message: i18next.t('errors:session.not_found'),
});
await expect(koaOIDCErrorHandler()(ctx, next)).rejects.toMatchError(
new RequestError(
{
code: 'session.not_found',
status: error.status,
expose: true,
},
{
error_description,
}
)
);
});
it('Insufficient Scope', async () => {
it('should add scope in response when needed', async () => {
const ctx = createContextWithRouteParameters();
const error_description = 'Insufficient scope for access_token';
const scope = 'read:foo';
const error = new errors.InsufficientScope(error_description, scope);
next.mockImplementationOnce(() => {
throw error;
await expectErrorResponse(ctx, new errors.InsufficientScope(error_description, scope), {
scope,
});
await expect(koaOIDCErrorHandler()(ctx, next)).rejects.toMatchError(
new RequestError(
{
code: 'oidc.insufficient_scope',
status: error.status,
expose: true,
scopes: scope,
},
{
error_description,
}
)
);
});
it('Unhandled OIDCProvider Error', async () => {
const error = new errors.AuthorizationPending();
it('should add error uri when available', async () => {
const ctx = createContextWithRouteParameters();
const error = new errors.InvalidGrant('invalid grant');
next.mockImplementationOnce(() => {
throw error;
await expectErrorResponse(ctx, error, {
error_uri: 'https://openid.sh/debug/invalid_grant',
});
});
await expect(koaOIDCErrorHandler()(ctx, next)).rejects.toMatchError(
new RequestError(
{
code: 'oidc.provider_error',
status: error.status,
expose: true,
message: error.message,
},
{
error_description: error.error_description,
error_detail: error.error_detail,
}
)
);
it('should handle unrecognized oidc error', async () => {
const ctx = createContextWithRouteParameters();
const unrecognizedError = { error: 'some_error', error_description: 'some error description' };
ctx.status = 500;
ctx.body = unrecognizedError;
await koaOidcErrorHandler()(ctx, next);
expect(ctx.status).toBe(500);
expect(ctx.body).toMatchObject({ ...unrecognizedError, code: 'oidc.some_error' });
});
});

View file

@ -1,17 +1,83 @@
import type { LogtoErrorCode } from '@logto/phrases';
import decamelize from 'decamelize';
import { condObject, isObject } from '@silverhand/essentials';
import i18next from 'i18next';
import type { Middleware } from 'koa';
import { errors } from 'oidc-provider';
import { z } from 'zod';
import RequestError from '#src/errors/RequestError/index.js';
import { EnvSet } from '#src/env-set/index.js';
import { consoleLog } from '#src/utils/console.js';
/**
* OIDC Provider Error Definition: https://github.com/panva/node-oidc-provider/blob/main/lib/helpers/errors.js
* Supplementary URIs for oidc-provider errors.
*/
const errorUris: Record<string, string> = Object.freeze({
invalid_grant: 'https://openid.sh/debug/invalid_grant',
});
export default function koaOIDCErrorHandler<StateT, ContextT>(): Middleware<StateT, ContextT> {
// Too many error types :-)
// eslint-disable-next-line complexity
/**
* Transform oidc-provider error to a format for the client. This is edited from oidc-provider's
* own implementation.
*
* @see {@link https://github.com/panva/node-oidc-provider/blob/37d0a6cfb3c618141a44cbb904ce45659438f821/lib/helpers/err_out.js | oidc-provider/lib/helpers/err_out.js}
*/
export const errorOut = ({
expose,
message,
error_description: description,
...rest
}: errors.OIDCProviderError) => {
if (expose) {
return {
error: message,
...condObject({
error_description: description,
scope: 'scope' in rest ? rest.scope : undefined,
}),
};
}
return {
error: 'server_error',
error_description: 'oops! something went wrong',
};
};
/**
* Check if the error is a session not found error according to oidc-provider's error description.
* This is a little dumb but we can't do much about it until oidc-provider provides a custom error
* handler for all request types.
*
* @see {@link https://github.com/search?q=repo%3Apanva%2Fnode-oidc-provider%20SessionNotFound&type=code | oidc-provider SessionNotFound}
*/
const isSessionNotFound = (description?: string) =>
Boolean(
['session', 'not found'].every((word) => description?.includes(word)) ||
description?.includes('authorization request has expired')
);
/**
* Build a unified handler for `oidc-provider` errors by checking if the status is >= 400 and
* there's a string property error in the response. If an `oidc-provider` error is caught, the
* handler will add several properties to the response body:
*
* - `code`: The error code in Logto's error system. Usually it just prepends `oidc.` to the error
* name. For example, `invalid_grant` will be `oidc.invalid_grant`.
* - `message`: The error message in the current language. If the error code is not found in the
* current language, it will fallback to `oidc.provider_error_fallback`.
* - `error_uri`: The error URI for the error, if available. (OAuth 2.0 spec)
*
* The original `error` and `error_description` properties will be kept for compliance with the
* OAuth 2.0 spec.
*
* If the error is not an `oidc-provider` error, the handler will throw the error.
*
* @remarks
* Currently, this is the only way we can check if the error is handled by the `oidc-provider`,
* because it doesn't call renderError when the [request prefers JSON response](https://github.com/panva/node-oidc-provider/blob/37d0a6cfb3c618141a44cbb904ce45659438f821/lib/shared/error_handler.js#L48-L55).
*
* @see {@link errorUris} for the list of error URIs.
*/
export default function koaOidcErrorHandler<StateT, ContextT>(): Middleware<StateT, ContextT> {
return async (ctx, next) => {
try {
await next();
@ -20,79 +86,39 @@ export default function koaOIDCErrorHandler<StateT, ContextT>(): Middleware<Stat
throw error;
}
const {
status = 400,
message,
error_description,
error_detail,
name,
expose,
constructor,
...interpolation
} = error;
// Mimic oidc-provider's error handling, thus we can use the unified logic below.
// See https://github.com/panva/node-oidc-provider/blob/37d0a6cfb3c618141a44cbb904ce45659438f821/lib/shared/error_handler.js
ctx.status = error.statusCode || 500;
ctx.body = errorOut(error);
// Original OIDCProvider Error description and details are provided in the data field
const data = { error_description, error_detail };
if (!EnvSet.values.isUnitTest && (!EnvSet.values.isProduction || ctx.status >= 500)) {
consoleLog.error(error);
}
}
switch (constructor) {
case errors.InvalidScope:
case errors.InvalidTarget:
case errors.InvalidToken:
case errors.InvalidClientMetadata:
case errors.InvalidRedirectUri:
case errors.AccessDenied:
case errors.UnsupportedGrantType:
case errors.UnsupportedResponseMode:
case errors.UnsupportedResponseType:
case errors.InvalidGrant: {
throw new RequestError(
{
// Manually mapped all OIDC error name to the LogtoErrorCode
// eslint-disable-next-line no-restricted-syntax
code: `oidc.${decamelize(name)}` as LogtoErrorCode,
status,
expose,
...interpolation,
},
data
);
}
// This is the only way we can check if the error is handled by the oidc-provider, because
// oidc-provider doesn't call `renderError` when the request prefers JSON response.
if (ctx.status >= 400 && isObject(ctx.body)) {
const parsed = z
.object({
error: z.string(),
error_description: z.string().optional(),
})
.safeParse(ctx.body);
case errors.SessionNotFound: {
throw new RequestError(
{
code: 'session.not_found',
status,
expose,
...interpolation,
},
data
);
}
if (parsed.success) {
const { data } = parsed;
const code = isSessionNotFound(data.error_description)
? 'session.not_found'
: `oidc.${data.error}`;
const uri = errorUris[data.error];
case errors.InsufficientScope: {
throw new RequestError(
{
code: 'oidc.insufficient_scope',
status,
expose,
...interpolation,
},
data
);
}
default: {
throw new RequestError(
{
code: 'oidc.provider_error',
status,
expose,
message,
},
data
);
}
ctx.body = {
code,
message: i18next.t(['errors:' + code, 'errors:oidc.provider_error_fallback'], { code }),
error_uri: uri,
...ctx.body,
};
}
}
};

View file

@ -28,7 +28,6 @@ import { isOriginAllowed, validateCustomClientMetadata } from '#src/oidc/utils.j
import { routes } from '#src/routes/consts.js';
import type Libraries from '#src/tenants/Libraries.js';
import type Queries from '#src/tenants/Queries.js';
import { consoleLog } from '#src/utils/console.js';
import defaults from './defaults.js';
import { getUserClaimData, getUserClaims } from './scope.js';
@ -47,7 +46,6 @@ export default function initOidc(
const {
resources: { findResourceByIndicator, findDefaultResource },
users: { findUserById },
dailyActiveUsers: { insertActiveUser },
} = queries;
const { findUserScopesForResourceIndicator } = libraries.users;
const { findApplicationScopesForResourceIndicator } = libraries.applications;
@ -62,10 +60,15 @@ export default function initOidc(
const oidc = new Provider(issuer, {
adapter: postgresAdapter.bind(null, envSet, queries),
renderError: (_ctx, _out, error) => {
consoleLog.error(error);
throw error;
// Align the error response regardless of the request format. It will be `application/json` by default.
// Rendering different error response based on the request format is okay, but it brought more trouble
// since `renderError` will be only called when the request prefers `text/html` format.
//
// See https://github.com/panva/node-oidc-provider/blob/37d0a6cfb3c618141a44cbb904ce45659438f821/lib/shared/error_handler.js#L48-L55
//
// Since we prefer a more consistent error response, we will handle it in `koaOidcErrorHandler` middleware.
renderError: (ctx, out, _error) => {
ctx.body = out;
},
cookies: {
keys: cookieKeys,

View file

@ -17,7 +17,7 @@ import koaConnectorErrorHandler from '#src/middleware/koa-connector-error-handle
import koaConsoleRedirectProxy from '#src/middleware/koa-console-redirect-proxy.js';
import koaErrorHandler from '#src/middleware/koa-error-handler.js';
import koaI18next from '#src/middleware/koa-i18next.js';
import koaOIDCErrorHandler from '#src/middleware/koa-oidc-error-handler.js';
import koaOidcErrorHandler from '#src/middleware/koa-oidc-error-handler.js';
import koaSecurityHeaders from '#src/middleware/koa-security-headers.js';
import koaSlonikErrorHandler from '#src/middleware/koa-slonik-error-handler.js';
import koaSpaProxy from '#src/middleware/koa-spa-proxy.js';
@ -72,7 +72,7 @@ export default class Tenant implements TenantContext {
app.use(koaLogger());
app.use(koaErrorHandler());
app.use(koaOIDCErrorHandler());
app.use(koaOidcErrorHandler());
app.use(koaSlonikErrorHandler());
app.use(koaConnectorErrorHandler());
app.use(koaI18next());

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Nicht unterstützter `response_mode` angefragt.',
unsupported_response_type: 'Nicht unterstützter `response_type` angefragt.',
provider_error: 'OIDC interner Fehler: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,7 +14,10 @@ const oidc = {
unsupported_grant_type: 'Unsupported `grant_type` requested.',
unsupported_response_mode: 'Unsupported `response_mode` requested.',
unsupported_response_type: 'Unsupported `response_type` requested.',
/** @deprecated Use {@link oidc.server_error} or {@link oidc.provider_error_fallback} instead. */
provider_error: 'OIDC Internal Error: {{message}}.',
server_error: 'An unknown OIDC error occurred. Please try again later.',
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Modo de respuesta no compatible solicitado.',
unsupported_response_type: 'Tipo de respuesta no admitido solicitado.',
provider_error: 'Error interno de OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: "Le `response_mode` demandé n'est pas supporté.",
unsupported_response_type: "Le `response_type` demandé n'est pas supporté.",
provider_error: "Erreur interne de l'OIDC : {{message}}.",
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Modalità di risposta `response_mode` richiesta non supportata.',
unsupported_response_type: 'Tipo di risposta `response_type` richiesto non supportato.',
provider_error: 'Errore interno OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: '要求された`response_mode`はサポートされていません。',
unsupported_response_type: '要求された`response_type`はサポートされていません。',
provider_error: 'OIDC内部エラー:{{message}}。',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,6 +14,10 @@ const oidc = {
unsupported_response_mode: '지원하지 않는 `response_mode` 요청이에요.',
unsupported_response_type: '지원하지 않은 `response_type` 요청이에요.',
provider_error: 'OIDC 내부 오류: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Żądany tryb `response_mode` nie jest obsługiwany.',
unsupported_response_type: 'Żądany typ `response_type` nie jest obsługiwany.',
provider_error: 'Wewnętrzny błąd OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: '`response_mode` não suportado.',
unsupported_response_type: '`response_type` não suportado.',
provider_error: 'Erro interno OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,6 +14,10 @@ const oidc = {
unsupported_response_mode: '`response_mode` solicitado não é suportado.',
unsupported_response_type: '`response_type` solicitado não é suportado.',
provider_error: 'Erro interno OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Запрошенный response_mode не поддерживается.',
unsupported_response_type: 'Запрошенный response_type не поддерживается.',
provider_error: 'Внутренняя ошибка OIDC: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -15,6 +15,10 @@ const oidc = {
unsupported_response_mode: 'Desteklenmeye `response_mode` istendi.',
unsupported_response_type: 'Desteklenmeyen `response_type` istendi.',
provider_error: 'Dahili OIDC Hatası: {{message}}.',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,6 +14,10 @@ const oidc = {
unsupported_response_mode: '不支持的 response_mode',
unsupported_response_type: '不支持的 response_type',
provider_error: 'OIDC 内部错误: {{message}}',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,6 +14,10 @@ const oidc = {
unsupported_response_mode: '不支持的 response_mode',
unsupported_response_type: '不支持的 response_type',
provider_error: 'OIDC 內部錯誤: {{message}}',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);

View file

@ -14,6 +14,10 @@ const oidc = {
unsupported_response_mode: '不支援的 response_mode',
unsupported_response_type: '不支援的 response_type',
provider_error: 'OIDC 內部錯誤: {{message}}',
/** UNTRANSLATED */
server_error: 'An unknown OIDC error occurred. Please try again later.',
/** UNTRANSLATED */
provider_error_fallback: 'An OIDC error occurred: {{code}}.',
};
export default Object.freeze(oidc);