From 3f817b14b0b7ee626d3f4e6922955392c00f8be2 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 11 May 2022 16:07:13 +0200 Subject: [PATCH] feat: Extend OIDC error descriptions --- RELEASE_NOTES.md | 6 ++ .../handler/provider-factory/identity.json | 1 + .../configuration/IdentityProviderFactory.ts | 57 ++++++++++++++----- .../IdentityProviderFactory.test.ts | 16 ++++-- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index eb26fea76..c540766f7 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -36,6 +36,10 @@ The following changes are relevant for v4 custom configs that replaced certain f - RegexPathRouting has changed from a map datastructure to an array datastructure, allowing for fallthrough regex parsing. The change is reflected in the following default configs: - `/storage/backend/regex.json` - `/sparql-file-storage.json` +- The `IdentityProviderFactory` inputs have been extended. + - `/identity/handler/provider-factory/identity.json` +- LDP components have slightly changed so the preference parser is in a separate config file. + - `/config/ldp/handler/*` ### Interface changes These changes are relevant if you wrote custom modules for the server that depend on existing interfaces. @@ -43,6 +47,8 @@ These changes are relevant if you wrote custom modules for the server that depen - `RedirectAllHttpHandler` was removed and fully replaced by `RedirectingHttpHandler`. - `SingleThreadedResourceLocker` has been renamed to `MemoryResourceLocker`. - Both `TemplateEngine` implementations now take a `baseUrl` parameter as input. +- The `IdentityProviderFactory` and `ConvertingErrorHandler` now additionally take a `PreferenceParser` as input. +- Error handlers now take the incoming HttpRequest as input instead of just the preferences. A new interface `SingleThreaded` has been added. This empty interface can be implemented to mark a component as not-threadsafe. When the CSS starts in multithreaded mode, it will error and halt if any SingleThreaded components are instantiated. diff --git a/config/identity/handler/provider-factory/identity.json b/config/identity/handler/provider-factory/identity.json index c03e04af9..c0fd507c1 100644 --- a/config/identity/handler/provider-factory/identity.json +++ b/config/identity/handler/provider-factory/identity.json @@ -18,6 +18,7 @@ "relativePath": "/idp/keys/", "source": { "@id": "urn:solid-server:default:KeyValueStorage" } }, + "args_showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }, "args_errorHandler": { "@id": "urn:solid-server:default:ErrorHandler" }, "args_responseWriter": { "@id": "urn:solid-server:default:ResponseWriter" }, "config": { diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index 1fee6c6fb..a3fc51489 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -10,7 +10,8 @@ import type { Account, ErrorOut, KoaContextWithOIDC, ResourceServer, - UnknownObject } from 'oidc-provider'; + UnknownObject, + errors } from 'oidc-provider'; import { Provider } from 'oidc-provider'; import type { Operation } from '../../http/Operation'; import type { ErrorHandler } from '../../http/output/error/ErrorHandler'; @@ -51,6 +52,10 @@ export interface IdentityProviderFactoryArgs { * Storage used to store cookie and JWT keys so they can be re-used in case of multithreading. */ storage: KeyValueStorage; + /** + * Extra information will be added to the error output if this is true. + */ + showStackTrace: boolean; /** * Used to convert errors thrown by the OIDC library. */ @@ -73,14 +78,15 @@ const COOKIES_KEY = 'cookie-secret'; */ export class IdentityProviderFactory implements ProviderFactory { private readonly config: Configuration; - private readonly adapterFactory!: AdapterFactory; - private readonly baseUrl!: string; - private readonly oidcPath!: string; - private readonly interactionHandler!: InteractionHandler; - private readonly credentialStorage!: KeyValueStorage; - private readonly storage!: KeyValueStorage; - private readonly errorHandler!: ErrorHandler; - private readonly responseWriter!: ResponseWriter; + private readonly adapterFactory: AdapterFactory; + private readonly baseUrl: string; + private readonly oidcPath: string; + private readonly interactionHandler: InteractionHandler; + private readonly credentialStorage: KeyValueStorage; + private readonly storage: KeyValueStorage; + private readonly showStackTrace: boolean; + private readonly errorHandler: ErrorHandler; + private readonly responseWriter: ResponseWriter; private readonly jwtAlg = 'ES256'; private provider?: Provider; @@ -91,7 +97,16 @@ export class IdentityProviderFactory implements ProviderFactory { */ public constructor(config: Configuration, args: IdentityProviderFactoryArgs) { this.config = config; - Object.assign(this, args); + + this.adapterFactory = args.adapterFactory; + this.baseUrl = args.baseUrl; + this.oidcPath = args.oidcPath; + this.interactionHandler = args.interactionHandler; + this.credentialStorage = args.credentialStorage; + this.storage = args.storage; + this.showStackTrace = args.showStackTrace; + this.errorHandler = args.errorHandler; + this.responseWriter = args.responseWriter; } public async getProvider(): Promise { @@ -317,13 +332,27 @@ export class IdentityProviderFactory implements ProviderFactory { * Pipes library errors to the provided ErrorHandler and ResponseWriter. */ private configureErrors(config: Configuration): void { - config.renderError = async(ctx: KoaContextWithOIDC, out: ErrorOut, error: Error): Promise => { + config.renderError = async(ctx: KoaContextWithOIDC, out: ErrorOut, error: errors.OIDCProviderError | Error): + Promise => { // This allows us to stream directly to the response object, see https://github.com/koajs/koa/issues/944 ctx.respond = false; - // OIDC library hides extra details in this field - if (out.error_description) { - error.message += ` - ${out.error_description}`; + // Doesn't really matter which type it is since all relevant fields are optional + const oidcError = error as errors.OIDCProviderError; + + // OIDC library hides extra details in these fields + if (this.showStackTrace) { + if (oidcError.error_description) { + error.message += ` - ${oidcError.error_description}`; + } + if (oidcError.error_detail) { + oidcError.message += ` - ${oidcError.error_detail}`; + } + + // Also change the error message in the stack trace + if (error.stack) { + error.stack = error.stack.replace(/.*/u, `${error.name}: ${error.message}`); + } } const result = await this.errorHandler.handleSafe({ error, request: guardStream(ctx.req) }); diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index 92ef028f1..45bb09b4b 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -1,5 +1,5 @@ import { Readable } from 'stream'; -import type { Configuration, KoaContextWithOIDC } from 'oidc-provider'; +import type { errors, Configuration, KoaContextWithOIDC } from 'oidc-provider'; import type { ErrorHandler } from '../../../../src/http/output/error/ErrorHandler'; import type { ResponseWriter } from '../../../../src/http/output/ResponseWriter'; import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; @@ -92,6 +92,7 @@ describe('An IdentityProviderFactory', (): void => { interactionHandler, storage, credentialStorage, + showStackTrace: true, errorHandler, responseWriter, }); @@ -175,6 +176,7 @@ describe('An IdentityProviderFactory', (): void => { interactionHandler, storage, credentialStorage, + showStackTrace: true, errorHandler, responseWriter, }); @@ -198,6 +200,7 @@ describe('An IdentityProviderFactory', (): void => { interactionHandler, storage, credentialStorage, + showStackTrace: true, errorHandler, responseWriter, }); @@ -214,16 +217,17 @@ describe('An IdentityProviderFactory', (): void => { const provider = await factory.getProvider() as any; const { config } = provider as { config: Configuration }; - const error = new Error('bad data'); - const out = { error_description: 'more info' }; + const error = new Error('bad data') as errors.OIDCProviderError; + error.error_description = 'more info'; + error.error_detail = 'more details'; - await expect((config.renderError as any)(ctx, out, error)).resolves.toBeUndefined(); + await expect((config.renderError as any)(ctx, {}, error)).resolves.toBeUndefined(); expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1); expect(errorHandler.handleSafe) .toHaveBeenLastCalledWith({ error, request: ctx.req }); expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1); expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }}); - expect(error.message).toBe('bad data - more info'); - expect(error.stack).toContain('Error: bad data - more info'); + expect(error.message).toBe('bad data - more info - more details'); + expect(error.stack).toContain('Error: bad data - more info - more details'); }); });