feat: Move OIDC library behaviour to separate path

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.

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" }

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" }
+      }
+    }
+  ]
+}

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" }
     },
     {

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" },

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<string, string>;
 
   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):
+  public async handle({ operation, request, response }: OperationHttpHandlerInput): Promise<ResponseDescription> {
-  Promise<ResponseDescription | undefined> {
     // 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();
+      throw new NotFoundHttpError();
-      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;
     }
 
     // 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<InteractionRoute | undefined> {
     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);

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<void> {
+    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);
+  }
+}

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<string, unknown>;
   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;
   }
 
   /**

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';

src/server/AuthorizingHttpHandler.ts

@@ -58,7 +58,7 @@ export class AuthorizingHttpHandler extends OperationHttpHandler {
     this.operationHandler = args.operationHandler;
   }
 
-  public async handle(input: OperationHttpHandlerInput): Promise<ResponseDescription | undefined> {
+  public async handle(input: OperationHttpHandlerInput): Promise<ResponseDescription> {
     const { request, operation } = input;
     const credentials: CredentialSet = await this.credentialsExtractor.handleSafe(request);
     this.logger.verbose(`Extracted credentials: ${JSON.stringify(credentials)}`);

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<OperationHttpHandlerInput, ResponseDescription | undefined> {}
+  extends AsyncHandler<OperationHttpHandlerInput, ResponseDescription> {}

test/integration/Identity.test.ts

@@ -378,7 +378,7 @@ describe('A Solid server with IDP', (): void => {
     });
 
     it('should return correct error output.', async(): Promise<void> => {
-      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');
     });

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"
-    }
   ]
 }

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<void> => {
+  it('throws a 404 if there is no matching route.', async(): Promise<void> => {
     operation.target.path = joinUrl(baseUrl, 'invalid');
-    await expect(handler.handle({ request, response, operation })).resolves.toBeUndefined();
+    await expect(handler.handle({ request, response, operation })).rejects.toThrow(NotFoundHttpError);
-    expect(provider.callback).toHaveBeenCalledTimes(1);
-    expect(provider.callback).toHaveBeenLastCalledWith(request, response);
   });
 
   it('creates Representations for InteractionResponseResults.', async(): Promise<void> => {

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<Provider>;
+  let providerFactory: jest.Mocked<ProviderFactory>;
+  let handler: OidcHttpHandler;
+
+  beforeEach(async(): Promise<void> => {
+    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<void> => {
+    await expect(handler.handle({ request, response })).resolves.toBeUndefined();
+    expect(provider.callback).toHaveBeenCalledTimes(1);
+    expect(provider.callback).toHaveBeenLastCalledWith(request, response);
+  });
+});

test/unit/identity/configuration/IdentityProviderFactory.test.ts

@@ -12,23 +12,24 @@ jest.mock('oidc-provider', (): any => ({
 }));
 
 const routes = {
-  authorization: '/foo/idp/auth',
+  authorization: '/foo/oidc/auth',
-  check_session: '/foo/idp/session/check',
+  check_session: '/foo/oidc/session/check',
-  code_verification: '/foo/idp/device',
+  code_verification: '/foo/oidc/device',
-  device_authorization: '/foo/idp/device/auth',
+  device_authorization: '/foo/oidc/device/auth',
-  end_session: '/foo/idp/session/end',
+  end_session: '/foo/oidc/session/end',
-  introspection: '/foo/idp/token/introspection',
+  introspection: '/foo/oidc/token/introspection',
-  jwks: '/foo/idp/jwks',
+  jwks: '/foo/oidc/jwks',
-  pushed_authorization_request: '/foo/idp/request',
+  pushed_authorization_request: '/foo/oidc/request',
-  registration: '/foo/idp/reg',
+  registration: '/foo/oidc/reg',
-  revocation: '/foo/idp/token/revocation',
+  revocation: '/foo/oidc/token/revocation',
-  token: '/foo/idp/token',
+  token: '/foo/oidc/token',
-  userinfo: '/foo/idp/me',
+  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<AdapterFactory>;
@@ -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,