diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 1656f83eb..70bb6acde 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -15,6 +15,9 @@ The following changes are relevant for v2 custom configs that replaced certain f - `/util/representation-conversion/default.json` - The IDP settings have changed to support the latest Solid-OIDC draft. - `/identity/handler/provider-factory/identity.json` +- Requests targeting the OIDC library now use a separate handler. + - `/http/handler/default.json` + - `/identity/handler/default.json` ### Interface changes These changes are relevant if you wrote custom modules for the server that depend on existing interfaces. diff --git a/config/http/handler/default.json b/config/http/handler/default.json index 01e0b34b1..acbcfb97d 100644 --- a/config/http/handler/default.json +++ b/config/http/handler/default.json @@ -1,7 +1,7 @@ { "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^2.0.0/components/context.jsonld", "import": [ - "files-scs:config/app/init/initializers/root.json" + "files-scs:config/http/handler/handlers/oidc.json" ], "@graph": [ { @@ -15,6 +15,7 @@ "handlers": [ { "@id": "urn:solid-server:default:StaticAssetHandler" }, { "@id": "urn:solid-server:default:SetupHandler" }, + { "@id": "urn:solid-server:default:OidcHandler" }, { "@id": "urn:solid-server:default:AuthResourceHttpHandler" }, { "@id": "urn:solid-server:default:IdentityProviderHandler" }, { "@id": "urn:solid-server:default:LdpHandler" } diff --git a/config/http/handler/handlers/oidc.json b/config/http/handler/handlers/oidc.json new file mode 100644 index 000000000..e9cf0fa0f --- /dev/null +++ b/config/http/handler/handlers/oidc.json @@ -0,0 +1,18 @@ +{ + "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^2.0.0/components/context.jsonld", + "@graph": [ + { + "comment": "Routes all OIDC related requests to the OIDC library.", + "@id": "urn:solid-server:default:OidcHandler", + "@type": "RouterHandler", + "args_baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "args_targetExtractor": { "@id": "urn:solid-server:default:TargetExtractor" }, + "args_allowedMethods": [ "*" ], + "args_allowedPathNames": [ "^/.oidc/.*", "^/\\.well-known/openid-configuration" ], + "args_handler": { + "@type": "OidcHttpHandler", + "providerFactory": { "@id": "urn:solid-server:default:IdentityProviderFactory" } + } + } + ] +} diff --git a/config/identity/handler/default.json b/config/identity/handler/default.json index f6104b5d3..c9b13dc4c 100644 --- a/config/identity/handler/default.json +++ b/config/identity/handler/default.json @@ -14,7 +14,7 @@ "args_baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "args_targetExtractor": { "@id": "urn:solid-server:default:TargetExtractor" }, "args_allowedMethods": [ "*" ], - "args_allowedPathNames": [ "^/idp/.*", "^/\\.well-known/openid-configuration" ], + "args_allowedPathNames": [ "^/idp/.*" ], "args_handler": { "@id": "urn:solid-server:default:IdentityProviderParsingHandler" } }, { diff --git a/config/identity/handler/provider-factory/identity.json b/config/identity/handler/provider-factory/identity.json index 5c8686ee9..cde397c61 100644 --- a/config/identity/handler/provider-factory/identity.json +++ b/config/identity/handler/provider-factory/identity.json @@ -10,6 +10,7 @@ "@type": "IdentityProviderFactory", "args_adapterFactory": { "@id": "urn:solid-server:default:IdpAdapterFactory" }, "args_baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "args_oidcPath": "/.oidc", "args_idpPath": "/idp", "args_storage": { "@id": "urn:solid-server:default:IdpKeyStorage" }, "args_errorHandler": { "@id": "urn:solid-server:default:ErrorHandler" }, diff --git a/src/identity/IdentityProviderHttpHandler.ts b/src/identity/IdentityProviderHttpHandler.ts index 5d9f416b2..5be0a772a 100644 --- a/src/identity/IdentityProviderHttpHandler.ts +++ b/src/identity/IdentityProviderHttpHandler.ts @@ -10,6 +10,7 @@ import { OperationHttpHandler } from '../server/OperationHttpHandler'; import type { RepresentationConverter } from '../storage/conversion/RepresentationConverter'; import { APPLICATION_JSON } from '../util/ContentTypes'; import { BadRequestHttpError } from '../util/errors/BadRequestHttpError'; +import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { joinUrl, trimTrailingSlashes } from '../util/PathUtil'; import { addTemplateMetadata, cloneRepresentation } from '../util/ResourceUtil'; import { readJsonStream } from '../util/StreamUtil'; @@ -77,8 +78,6 @@ export class IdentityProviderHttpHandler extends OperationHttpHandler { private readonly controls: Record; public constructor(args: IdentityProviderHttpHandlerArgs) { - // It is important that the RequestParser does not read out the Request body stream. - // Otherwise we can't pass it anymore to the OIDC library when needed. super(); // Trimming trailing slashes so the relative URL starts with a slash after slicing this off this.baseUrl = trimTrailingSlashes(joinUrl(args.baseUrl, args.idpPath)); @@ -97,29 +96,19 @@ export class IdentityProviderHttpHandler extends OperationHttpHandler { /** * Finds the matching route and resolves the operation. */ - public async handle({ operation, request, response }: OperationHttpHandlerInput): - Promise { + public async handle({ operation, request, response }: OperationHttpHandlerInput): Promise { // This being defined means we're in an OIDC session let oidcInteraction: Interaction | undefined; try { const provider = await this.providerFactory.getProvider(); - // This being defined means we're in an OIDC session oidcInteraction = await provider.interactionDetails(request, response); } catch { // Just a regular request } - // If our own interaction handler does not support the input, it is either invalid or a request for the OIDC library const route = await this.findRoute(operation, oidcInteraction); - if (!route) { - const provider = await this.providerFactory.getProvider(); - this.logger.debug(`Sending request to oidc-provider: ${request.url}`); - // Even though the typings do not indicate this, this is a Promise that needs to be awaited. - // Otherwise the `BaseHttpServerFactory` will write a 404 before the OIDC library could handle the response. - // eslint-disable-next-line @typescript-eslint/await-thenable - await provider.callback(request, response); - return; + throw new NotFoundHttpError(); } // Cloning input data so it can be sent back in case of errors @@ -149,7 +138,7 @@ export class IdentityProviderHttpHandler extends OperationHttpHandler { */ private async findRoute(operation: Operation, oidcInteraction?: Interaction): Promise { if (!operation.target.path.startsWith(this.baseUrl)) { - // This is either an invalid request or a call to the .well-known configuration + // This is an invalid request return; } const pathName = operation.target.path.slice(this.baseUrl.length); diff --git a/src/identity/OidcHttpHandler.ts b/src/identity/OidcHttpHandler.ts new file mode 100644 index 000000000..a8a3fe924 --- /dev/null +++ b/src/identity/OidcHttpHandler.ts @@ -0,0 +1,27 @@ +import { getLoggerFor } from '../logging/LogUtil'; +import type { HttpHandlerInput } from '../server/HttpHandler'; +import { HttpHandler } from '../server/HttpHandler'; +import type { ProviderFactory } from './configuration/ProviderFactory'; + +/** + * HTTP handler that redirects all requests to the OIDC library. + */ +export class OidcHttpHandler extends HttpHandler { + protected readonly logger = getLoggerFor(this); + + private readonly providerFactory: ProviderFactory; + + public constructor(providerFactory: ProviderFactory) { + super(); + this.providerFactory = providerFactory; + } + + public async handle({ request, response }: HttpHandlerInput): Promise { + const provider = await this.providerFactory.getProvider(); + this.logger.debug(`Sending request to oidc-provider: ${request.url}`); + // Even though the typings do not indicate this, this is a Promise that needs to be awaited. + // Otherwise the `BaseHttpServerFactory` will write a 404 before the OIDC library could handle the response. + // eslint-disable-next-line @typescript-eslint/await-thenable + await provider.callback(request, response); + } +} diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index 5ef2ff3e1..cec78ee1e 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -30,7 +30,11 @@ export interface IdentityProviderFactoryArgs { */ baseUrl: string; /** - * Path of the IDP component in the server. + * Path for all requests targeting the OIDC library. + */ + oidcPath: string; + /** + * The entry point for the custom IDP handlers of the server. * Should start with a slash. */ idpPath: string; @@ -62,6 +66,7 @@ export class IdentityProviderFactory implements ProviderFactory { private readonly config: Configuration; private readonly adapterFactory!: AdapterFactory; private readonly baseUrl!: string; + private readonly oidcPath!: string; private readonly idpPath!: string; private readonly storage!: KeyValueStorage; private readonly errorHandler!: ErrorHandler; @@ -107,6 +112,7 @@ export class IdentityProviderFactory implements ProviderFactory { // Allow provider to interpret reverse proxy headers const provider = new Provider(this.baseUrl, config); provider.proxy = true; + return provider; } @@ -210,11 +216,11 @@ export class IdentityProviderFactory implements ProviderFactory { /** * Creates the route string as required by the `oidc-provider` library. - * In case base URL is `http://test.com/foo/`, `idpPath` is `/idp` and `relative` is `device/auth`, + * In case base URL is `http://test.com/foo/`, `oidcPath` is `/idp` and `relative` is `device/auth`, * this would result in `/foo/idp/device/auth`. */ private createRoute(relative: string): string { - return new URL(joinUrl(this.baseUrl, this.idpPath, relative)).pathname; + return new URL(joinUrl(this.baseUrl, this.oidcPath, relative)).pathname; } /** diff --git a/src/index.ts b/src/index.ts index fd5342005..fc5667b28 100644 --- a/src/index.ts +++ b/src/index.ts @@ -165,6 +165,7 @@ export * from './identity/storage/WebIdAdapterFactory'; // Identity export * from './identity/IdentityProviderHttpHandler'; +export * from './identity/OidcHttpHandler'; // Init/Final export * from './init/final/Finalizable'; diff --git a/src/server/AuthorizingHttpHandler.ts b/src/server/AuthorizingHttpHandler.ts index 450d0026a..af8b56aff 100644 --- a/src/server/AuthorizingHttpHandler.ts +++ b/src/server/AuthorizingHttpHandler.ts @@ -58,7 +58,7 @@ export class AuthorizingHttpHandler extends OperationHttpHandler { this.operationHandler = args.operationHandler; } - public async handle(input: OperationHttpHandlerInput): Promise { + public async handle(input: OperationHttpHandlerInput): Promise { const { request, operation } = input; const credentials: CredentialSet = await this.credentialsExtractor.handleSafe(request); this.logger.verbose(`Extracted credentials: ${JSON.stringify(credentials)}`); diff --git a/src/server/OperationHttpHandler.ts b/src/server/OperationHttpHandler.ts index 8f685be35..31b4b1ddc 100644 --- a/src/server/OperationHttpHandler.ts +++ b/src/server/OperationHttpHandler.ts @@ -9,8 +9,6 @@ export interface OperationHttpHandlerInput extends HttpHandlerInput { /** * An HTTP handler that makes use of an already parsed Operation. - * Can either return a ResponseDescription to be resolved by the calling class, - * or undefined if this class handles the response itself. */ export abstract class OperationHttpHandler - extends AsyncHandler {} + extends AsyncHandler {} diff --git a/test/integration/Identity.test.ts b/test/integration/Identity.test.ts index ccd4b925d..36468643c 100644 --- a/test/integration/Identity.test.ts +++ b/test/integration/Identity.test.ts @@ -378,7 +378,7 @@ describe('A Solid server with IDP', (): void => { }); it('should return correct error output.', async(): Promise => { - const res = await fetch(`${baseUrl}idp/auth`); + const res = await fetch(`${baseUrl}.oidc/auth`); expect(res.status).toBe(400); await expect(res.text()).resolves.toContain('InvalidRequest: invalid_request'); }); diff --git a/test/integration/config/server-without-auth.json b/test/integration/config/server-without-auth.json index 963980f47..60a5b38c0 100644 --- a/test/integration/config/server-without-auth.json +++ b/test/integration/config/server-without-auth.json @@ -4,7 +4,7 @@ "files-scs:config/app/main/default.json", "files-scs:config/app/init/initialize-root.json", "files-scs:config/app/setup/disabled.json", - "files-scs:config/http/handler/default.json", + "files-scs:config/http/handler/simple.json", "files-scs:config/http/middleware/websockets.json", "files-scs:config/http/server-factory/websockets.json", "files-scs:config/http/static/default.json", @@ -26,9 +26,5 @@ "files-scs:config/util/variables/default.json" ], "@graph": [ - { - "@id": "urn:solid-server:default:IdentityProviderHandler", - "@type": "UnsupportedAsyncHandler" - } ] } diff --git a/test/unit/identity/IdentityProviderHttpHandler.test.ts b/test/unit/identity/IdentityProviderHttpHandler.test.ts index d11f8f367..4c246040c 100644 --- a/test/unit/identity/IdentityProviderHttpHandler.test.ts +++ b/test/unit/identity/IdentityProviderHttpHandler.test.ts @@ -17,6 +17,7 @@ import type { RepresentationConverter, RepresentationConverterArgs, } from '../../../src/storage/conversion/RepresentationConverter'; +import { NotFoundHttpError } from '../../../src/util/errors/NotFoundHttpError'; import { joinUrl } from '../../../src/util/PathUtil'; import { guardedStreamFrom, readableToString } from '../../../src/util/StreamUtil'; import { CONTENT_TYPE, SOLID_HTTP, SOLID_META } from '../../../src/util/Vocabularies'; @@ -46,7 +47,6 @@ describe('An IdentityProviderHttpHandler', (): void => { }; provider = { - callback: jest.fn(), interactionDetails: jest.fn(), } as any; @@ -113,11 +113,9 @@ describe('An IdentityProviderHttpHandler', (): void => { handler = new IdentityProviderHttpHandler(args); }); - it('calls the provider if there is no matching route.', async(): Promise => { + it('throws a 404 if there is no matching route.', async(): Promise => { operation.target.path = joinUrl(baseUrl, 'invalid'); - await expect(handler.handle({ request, response, operation })).resolves.toBeUndefined(); - expect(provider.callback).toHaveBeenCalledTimes(1); - expect(provider.callback).toHaveBeenLastCalledWith(request, response); + await expect(handler.handle({ request, response, operation })).rejects.toThrow(NotFoundHttpError); }); it('creates Representations for InteractionResponseResults.', async(): Promise => { diff --git a/test/unit/identity/OidcHttpHandler.test.ts b/test/unit/identity/OidcHttpHandler.test.ts new file mode 100644 index 000000000..61be21c83 --- /dev/null +++ b/test/unit/identity/OidcHttpHandler.test.ts @@ -0,0 +1,31 @@ +import type { Provider } from 'oidc-provider'; +import type { ProviderFactory } from '../../../src/identity/configuration/ProviderFactory'; +import { OidcHttpHandler } from '../../../src/identity/OidcHttpHandler'; +import type { HttpRequest } from '../../../src/server/HttpRequest'; +import type { HttpResponse } from '../../../src/server/HttpResponse'; + +describe('An OidcHttpHandler', (): void => { + const request: HttpRequest = {} as any; + const response: HttpResponse = {} as any; + let provider: jest.Mocked; + let providerFactory: jest.Mocked; + let handler: OidcHttpHandler; + + beforeEach(async(): Promise => { + provider = { + callback: jest.fn(), + } as any; + + providerFactory = { + getProvider: jest.fn().mockResolvedValue(provider), + }; + + handler = new OidcHttpHandler(providerFactory); + }); + + it('sends all requests to the OIDC library.', async(): Promise => { + await expect(handler.handle({ request, response })).resolves.toBeUndefined(); + expect(provider.callback).toHaveBeenCalledTimes(1); + expect(provider.callback).toHaveBeenLastCalledWith(request, response); + }); +}); diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index 3c8a0bcb0..6cb3c9024 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -12,23 +12,24 @@ jest.mock('oidc-provider', (): any => ({ })); const routes = { - authorization: '/foo/idp/auth', - check_session: '/foo/idp/session/check', - code_verification: '/foo/idp/device', - device_authorization: '/foo/idp/device/auth', - end_session: '/foo/idp/session/end', - introspection: '/foo/idp/token/introspection', - jwks: '/foo/idp/jwks', - pushed_authorization_request: '/foo/idp/request', - registration: '/foo/idp/reg', - revocation: '/foo/idp/token/revocation', - token: '/foo/idp/token', - userinfo: '/foo/idp/me', + authorization: '/foo/oidc/auth', + check_session: '/foo/oidc/session/check', + code_verification: '/foo/oidc/device', + device_authorization: '/foo/oidc/device/auth', + end_session: '/foo/oidc/session/end', + introspection: '/foo/oidc/token/introspection', + jwks: '/foo/oidc/jwks', + pushed_authorization_request: '/foo/oidc/request', + registration: '/foo/oidc/reg', + revocation: '/foo/oidc/token/revocation', + token: '/foo/oidc/token', + userinfo: '/foo/oidc/me', }; describe('An IdentityProviderFactory', (): void => { let baseConfig: Configuration; const baseUrl = 'http://test.com/foo/'; + const oidcPath = '/oidc'; const idpPath = '/idp'; const webId = 'http://alice.test.com/card#me'; let adapterFactory: jest.Mocked; @@ -59,6 +60,7 @@ describe('An IdentityProviderFactory', (): void => { factory = new IdentityProviderFactory(baseConfig, { adapterFactory, baseUrl, + oidcPath, idpPath, storage, errorHandler, @@ -70,6 +72,7 @@ describe('An IdentityProviderFactory', (): void => { expect((): any => new IdentityProviderFactory(baseConfig, { adapterFactory, baseUrl, + oidcPath, idpPath: 'idp', storage, errorHandler, @@ -127,6 +130,7 @@ describe('An IdentityProviderFactory', (): void => { factory = new IdentityProviderFactory(baseConfig, { adapterFactory, baseUrl, + oidcPath, idpPath, storage, errorHandler, @@ -148,6 +152,7 @@ describe('An IdentityProviderFactory', (): void => { const factory2 = new IdentityProviderFactory(baseConfig, { adapterFactory, baseUrl, + oidcPath, idpPath, storage, errorHandler,