feat: Parse Accept headers as early as possible

config/ldp/handler/components/error-handler.json

@@ -17,6 +17,7 @@
             "comment": "Converts an Error object into a representation for an HTTP response.",
             "@type": "ConvertingErrorHandler",
             "converter": { "@id": "urn:solid-server:default:UiEnabledConverter" },
+            "preferenceParser": { "@id": "urn:solid-server:default:PreferenceParser" },
             "showStackTrace": { "@id": "urn:solid-server:default:variable:showStackTrace" }
           }
         ]

config/ldp/handler/components/preferences.json

@@ -0,0 +1,9 @@
+{
+  "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^5.0.0/components/context.jsonld",
+  "@graph": [
+    {
+      "@id": "urn:solid-server:default:PreferenceParser",
+      "@type": "AcceptPreferenceParser"
+    }
+  ]
+}

config/ldp/handler/components/request-parser.json

@@ -11,7 +11,7 @@
         "args_identifierStrategy": { "@id": "urn:solid-server:default:IdentifierStrategy" },
         "args_includeQueryString": false
       },
-      "args_preferenceParser": { "@type": "AcceptPreferenceParser" },
+      "args_preferenceParser": { "@id": "urn:solid-server:default:PreferenceParser" },
       "args_metadataParser": { "@id": "urn:solid-server:default:MetadataParser" },
       "args_conditionsParser": { "@type": "BasicConditionsParser" },
       "args_bodyParser": {

config/ldp/handler/default.json

@@ -5,6 +5,7 @@
     "css:config/ldp/handler/components/error-handler.json",
     "css:config/ldp/handler/components/operation-handler.json",
     "css:config/ldp/handler/components/operation-metadata.json",
+    "css:config/ldp/handler/components/preferences.json",
     "css:config/ldp/handler/components/request-parser.json",
     "css:config/ldp/handler/components/response-writer.json"
   ],

src/http/input/BasicRequestParser.ts

@@ -22,19 +22,23 @@ export interface BasicRequestParserArgs {
 
 /**
  * Creates an {@link Operation} from an incoming {@link HttpRequest} by aggregating the results
- * of a {@link TargetExtractor}, {@link PreferenceParser}, {@link MetadataParser},
+ * of a {@link TargetExtractor}, {@link MetadataParser},
  * {@link ConditionsParser} and {@link BodyParser}.
  */
 export class BasicRequestParser extends RequestParser {
-  private readonly targetExtractor!: TargetExtractor;
+  private readonly targetExtractor: TargetExtractor;
-  private readonly preferenceParser!: PreferenceParser;
+  private readonly preferenceParser: PreferenceParser;
-  private readonly metadataParser!: MetadataParser;
+  private readonly metadataParser: MetadataParser;
-  private readonly conditionsParser!: ConditionsParser;
+  private readonly conditionsParser: ConditionsParser;
-  private readonly bodyParser!: BodyParser;
+  private readonly bodyParser: BodyParser;
 
   public constructor(args: BasicRequestParserArgs) {
     super();
-    Object.assign(this, args);
+    this.targetExtractor = args.targetExtractor;
+    this.preferenceParser = args.preferenceParser;
+    this.metadataParser = args.metadataParser;
+    this.conditionsParser = args.conditionsParser;
+    this.bodyParser = args.bodyParser;
   }
 
   public async handle(request: HttpRequest): Promise<Operation> {

src/http/output/error/ConvertingErrorHandler.ts

@@ -6,6 +6,7 @@ import { INTERNAL_ERROR } from '../../../util/ContentTypes';
 import { getStatusCode } from '../../../util/errors/HttpErrorUtil';
 import { toLiteral } from '../../../util/TermUtil';
 import { HTTP, XSD } from '../../../util/Vocabularies';
+import type { PreferenceParser } from '../../input/preferences/PreferenceParser';
 import { BasicRepresentation } from '../../representation/BasicRepresentation';
 import type { Representation } from '../../representation/Representation';
 import { RepresentationMetadata } from '../../representation/RepresentationMetadata';
@@ -25,22 +26,25 @@ type PreparedArguments = {
  */
 export class ConvertingErrorHandler extends ErrorHandler {
   private readonly converter: RepresentationConverter;
+  private readonly preferenceParser: PreferenceParser;
   private readonly showStackTrace: boolean;
 
-  public constructor(converter: RepresentationConverter, showStackTrace = false) {
+  public constructor(converter: RepresentationConverter, preferenceParser: PreferenceParser, showStackTrace = false) {
     super();
     this.converter = converter;
+    this.preferenceParser = preferenceParser;
     this.showStackTrace = showStackTrace;
   }
 
   public async canHandle(input: ErrorHandlerArgs): Promise<void> {
-    const { conversionArgs } = this.prepareArguments(input);
+    await this.preferenceParser.canHandle({ request: input.request });
+    const { conversionArgs } = await this.extractErrorDetails(input);
 
     await this.converter.canHandle(conversionArgs);
   }
 
   public async handle(input: ErrorHandlerArgs): Promise<ResponseDescription> {
-    const { statusCode, conversionArgs } = this.prepareArguments(input);
+    const { statusCode, conversionArgs } = await this.extractErrorDetails(input);
 
     const converted = await this.converter.handle(conversionArgs);
 
@@ -48,7 +52,8 @@ export class ConvertingErrorHandler extends ErrorHandler {
   }
 
   public async handleSafe(input: ErrorHandlerArgs): Promise<ResponseDescription> {
-    const { statusCode, conversionArgs } = this.prepareArguments(input);
+    await this.preferenceParser.canHandle({ request: input.request });
+    const { statusCode, conversionArgs } = await this.extractErrorDetails(input);
 
     const converted = await this.converter.handleSafe(conversionArgs);
 
@@ -58,10 +63,11 @@ export class ConvertingErrorHandler extends ErrorHandler {
   /**
    * Prepares the arguments used by all functions.
    */
-  private prepareArguments({ error, preferences }: ErrorHandlerArgs): PreparedArguments {
+  private async extractErrorDetails({ error, request }: ErrorHandlerArgs): Promise<PreparedArguments> {
     const statusCode = getStatusCode(error);
     const representation = this.toRepresentation(error, statusCode);
     const identifier = { path: representation.metadata.identifier.value };
+    const preferences = await this.preferenceParser.handle({ request });
     return { statusCode, conversionArgs: { identifier, representation, preferences }};
   }
 

src/http/output/error/ErrorHandler.ts

@@ -1,10 +1,10 @@
+import type { HttpRequest } from '../../../server/HttpRequest';
 import { AsyncHandler } from '../../../util/handlers/AsyncHandler';
-import type { RepresentationPreferences } from '../../representation/RepresentationPreferences';
 import type { ResponseDescription } from '../response/ResponseDescription';
 
 export interface ErrorHandlerArgs {
   error: Error;
-  preferences: RepresentationPreferences;
+  request: HttpRequest;
 }
 
 /**

src/identity/configuration/IdentityProviderFactory.ts

@@ -19,6 +19,7 @@ import { BasicRepresentation } from '../../http/representation/BasicRepresentati
 import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage';
 import { InternalServerError } from '../../util/errors/InternalServerError';
 import { RedirectHttpError } from '../../util/errors/RedirectHttpError';
+import { guardStream } from '../../util/GuardedStream';
 import { joinUrl } from '../../util/PathUtil';
 import type { ClientCredentials } from '../interaction/email-password/credentials/ClientCredentialsAdapterFactory';
 import type { InteractionHandler } from '../interaction/InteractionHandler';
@@ -325,7 +326,7 @@ export class IdentityProviderFactory implements ProviderFactory {
         error.message += ` - ${out.error_description}`;
       }
 
-      const result = await this.errorHandler.handleSafe({ error, preferences: { type: { 'text/plain': 1 }}});
+      const result = await this.errorHandler.handleSafe({ error, request: guardStream(ctx.req) });
       await this.responseWriter.handleSafe({ response: ctx.res, result });
     };
   }

src/server/ParsingHttpHandler.ts

@@ -3,12 +3,13 @@ import type { OperationMetadataCollector } from '../http/ldp/metadata/OperationM
 import type { ErrorHandler } from '../http/output/error/ErrorHandler';
 import type { ResponseDescription } from '../http/output/response/ResponseDescription';
 import type { ResponseWriter } from '../http/output/ResponseWriter';
-import type { RepresentationPreferences } from '../http/representation/RepresentationPreferences';
 import { getLoggerFor } from '../logging/LogUtil';
 import { assertError } from '../util/errors/ErrorUtil';
 import { HttpError } from '../util/errors/HttpError';
 import type { HttpHandlerInput } from './HttpHandler';
 import { HttpHandler } from './HttpHandler';
+import type { HttpRequest } from './HttpRequest';
+import type { HttpResponse } from './HttpResponse';
 import type { OperationHttpHandler } from './OperationHttpHandler';
 
 export interface ParsingHttpHandlerArgs {
@@ -35,9 +36,9 @@ export interface ParsingHttpHandlerArgs {
 }
 
 /**
- * Parses requests and sends the resulting Operation to wrapped operationHandler.
+ * Parses requests and sends the resulting {@link Operation} to the wrapped {@link OperationHttpHandler}.
- * Errors are caught and handled by the Errorhandler.
+ * Errors are caught and handled by the {@link ErrorHandler}.
- * In case the operationHandler returns a result it will be sent to the ResponseWriter.
+ * In case the {@link OperationHttpHandler} returns a result it will be sent to the {@link ResponseWriter}.
  */
 export class ParsingHttpHandler extends HttpHandler {
   private readonly logger = getLoggerFor(this);
@@ -58,30 +59,45 @@ export class ParsingHttpHandler extends HttpHandler {
   }
 
   public async handle({ request, response }: HttpHandlerInput): Promise<void> {
-    let result: ResponseDescription | undefined;
+    let result: ResponseDescription;
-    let preferences: RepresentationPreferences = { type: { 'text/plain': 1 }};
 
     try {
-      const operation = await this.requestParser.handleSafe(request);
+      result = await this.handleRequest(request, response);
-      ({ preferences } = operation);
-      result = await this.operationHandler.handleSafe({ operation, request, response });
-
-      if (result?.metadata) {
-        await this.metadataCollector.handleSafe({ operation, metadata: result.metadata });
-      }
-
-      this.logger.verbose(`Parsed ${operation.method} operation on ${operation.target.path}`);
     } catch (error: unknown) {
-      assertError(error);
+      result = await this.handleError(error, request);
-      result = await this.errorHandler.handleSafe({ error, preferences });
-      if (HttpError.isInstance(error) && result.metadata) {
-        const quads = error.generateMetadata(result.metadata.identifier);
-        result.metadata.addQuads(quads);
-      }
     }
 
     if (result) {
       await this.responseWriter.handleSafe({ response, result });
     }
   }
+
+  /**
+   * Interprets the request and passes the generated Operation object to the stored OperationHttpHandler.
+   */
+  protected async handleRequest(request: HttpRequest, response: HttpResponse):
+  Promise<ResponseDescription> {
+    const operation = await this.requestParser.handleSafe(request);
+    const result = await this.operationHandler.handleSafe({ operation, request, response });
+
+    if (result?.metadata) {
+      await this.metadataCollector.handleSafe({ operation, metadata: result.metadata });
+    }
+
+    this.logger.verbose(`Parsed ${operation.method} operation on ${operation.target.path}`);
+    return result;
+  }
+
+  /**
+   * Handles the error output correctly based on the preferences.
+   */
+  protected async handleError(error: unknown, request: HttpRequest): Promise<ResponseDescription> {
+    assertError(error);
+    const result = await this.errorHandler.handleSafe({ error, request });
+    if (HttpError.isInstance(error) && result.metadata) {
+      const quads = error.generateMetadata(result.metadata.identifier);
+      result.metadata.addQuads(quads);
+    }
+    return result;
+  }
 }

test/unit/http/output/error/ConvertingErrorHandler.test.ts

@@ -1,10 +1,12 @@
 import 'jest-rdf';
 import arrayifyStream from 'arrayify-stream';
 import { DataFactory } from 'n3';
+import type { PreferenceParser } from '../../../../../src/http/input/preferences/PreferenceParser';
 import { ConvertingErrorHandler } from '../../../../../src/http/output/error/ConvertingErrorHandler';
 import { BasicRepresentation } from '../../../../../src/http/representation/BasicRepresentation';
 import type { Representation } from '../../../../../src/http/representation/Representation';
 import type { RepresentationPreferences } from '../../../../../src/http/representation/RepresentationPreferences';
+import type { HttpRequest } from '../../../../../src/server/HttpRequest';
 import type {
   RepresentationConverter,
   RepresentationConverterArgs,
@@ -33,7 +35,9 @@ describe('A ConvertingErrorHandler', (): void => {
   // The error object can get modified by the handler
   let error: Error;
   let stack: string | undefined;
-  let converter: RepresentationConverter;
+  const request = {} as HttpRequest;
+  let converter: jest.Mocked<RepresentationConverter>;
+  let preferenceParser: jest.Mocked<PreferenceParser>;
   let handler: ConvertingErrorHandler;
 
   beforeEach(async(): Promise<void> => {
@@ -45,20 +49,33 @@ describe('A ConvertingErrorHandler', (): void => {
       handleSafe: jest.fn((): Representation => new BasicRepresentation('serialization', 'text/turtle', true)),
     } as any;
 
-    handler = new ConvertingErrorHandler(converter, true);
+    preferenceParser = {
+      canHandle: jest.fn(),
+      handle: jest.fn().mockResolvedValue(preferences),
+    } as any;
+
+    handler = new ConvertingErrorHandler(converter, preferenceParser, true);
   });
 
   it('rejects input not supported by the converter.', async(): Promise<void> => {
-    (converter.canHandle as jest.Mock).mockRejectedValueOnce(new Error('rejected'));
+    converter.canHandle.mockRejectedValueOnce(new Error('rejected'));
-    await expect(handler.canHandle({ error, preferences })).rejects.toThrow('rejected');
+    await expect(handler.canHandle({ error, request })).rejects.toThrow('rejected');
     expect(converter.canHandle).toHaveBeenCalledTimes(1);
-    const args = (converter.canHandle as jest.Mock).mock.calls[0][0] as RepresentationConverterArgs;
+    const args = converter.canHandle.mock.calls[0][0];
     expect(args.preferences).toBe(preferences);
     expect(args.representation.metadata.contentType).toBe('internal/error');
   });
 
+  it('rejects input not supported by the preference parser.', async(): Promise<void> => {
+    preferenceParser.canHandle.mockRejectedValueOnce(new Error('rejected'));
+    await expect(handler.canHandle({ error, request })).rejects.toThrow('rejected');
+    expect(preferenceParser.canHandle).toHaveBeenCalledTimes(1);
+    expect(preferenceParser.canHandle).toHaveBeenLastCalledWith({ request });
+    expect(converter.canHandle).toHaveBeenCalledTimes(0);
+  });
+
   it('accepts input supported by the converter.', async(): Promise<void> => {
-    await expect(handler.canHandle({ error, preferences })).resolves.toBeUndefined();
+    await expect(handler.canHandle({ error, request })).resolves.toBeUndefined();
     expect(converter.canHandle).toHaveBeenCalledTimes(1);
     const args = (converter.canHandle as jest.Mock).mock.calls[0][0] as RepresentationConverterArgs;
     expect(args.preferences).toBe(preferences);
@@ -66,7 +83,7 @@ describe('A ConvertingErrorHandler', (): void => {
   });
 
   it('returns the converted error response.', async(): Promise<void> => {
-    const prom = handler.handle({ error, preferences });
+    const prom = handler.handle({ error, request });
     await expect(prom).resolves.toMatchObject({ statusCode: 404 });
     expect((await prom).metadata?.contentType).toBe('text/turtle');
     expect(converter.handle).toHaveBeenCalledTimes(1);
@@ -75,7 +92,7 @@ describe('A ConvertingErrorHandler', (): void => {
   });
 
   it('uses the handleSafe function of the converter during its own handleSafe call.', async(): Promise<void> => {
-    const prom = handler.handleSafe({ error, preferences });
+    const prom = handler.handleSafe({ error, request });
     await expect(prom).resolves.toMatchObject({ statusCode: 404 });
     expect((await prom).metadata?.contentType).toBe('text/turtle');
     expect(converter.handleSafe).toHaveBeenCalledTimes(1);
@@ -84,8 +101,8 @@ describe('A ConvertingErrorHandler', (): void => {
   });
 
   it('hides the stack trace if the option is disabled.', async(): Promise<void> => {
-    handler = new ConvertingErrorHandler(converter);
+    handler = new ConvertingErrorHandler(converter, preferenceParser);
-    const prom = handler.handle({ error, preferences });
+    const prom = handler.handle({ error, request });
     await expect(prom).resolves.toMatchObject({ statusCode: 404 });
     expect((await prom).metadata?.contentType).toBe('text/turtle');
     expect(converter.handle).toHaveBeenCalledTimes(1);

test/unit/http/output/error/RedirectingErrorHandler.test.ts

@@ -1,24 +1,25 @@
 import { RedirectingErrorHandler } from '../../../../../src/http/output/error/RedirectingErrorHandler';
+import type { HttpRequest } from '../../../../../src/server/HttpRequest';
 import { BadRequestHttpError } from '../../../../../src/util/errors/BadRequestHttpError';
 import { FoundHttpError } from '../../../../../src/util/errors/FoundHttpError';
 import { NotImplementedHttpError } from '../../../../../src/util/errors/NotImplementedHttpError';
 import { SOLID_HTTP } from '../../../../../src/util/Vocabularies';
 
 describe('A RedirectingErrorHandler', (): void => {
-  const preferences = {};
+  const request = {} as HttpRequest;
   const handler = new RedirectingErrorHandler();
 
   it('only accepts redirect errors.', async(): Promise<void> => {
     const unsupportedError = new BadRequestHttpError();
-    await expect(handler.canHandle({ error: unsupportedError, preferences })).rejects.toThrow(NotImplementedHttpError);
+    await expect(handler.canHandle({ error: unsupportedError, request })).rejects.toThrow(NotImplementedHttpError);
 
     const supportedError = new FoundHttpError('http://test.com/foo/bar');
-    await expect(handler.canHandle({ error: supportedError, preferences })).resolves.toBeUndefined();
+    await expect(handler.canHandle({ error: supportedError, request })).resolves.toBeUndefined();
   });
 
   it('creates redirect responses.', async(): Promise<void> => {
     const error = new FoundHttpError('http://test.com/foo/bar');
-    const result = await handler.handle({ error, preferences });
+    const result = await handler.handle({ error, request });
     expect(result.statusCode).toBe(error.statusCode);
     expect(result.metadata?.get(SOLID_HTTP.terms.location)?.value).toBe(error.location);
   });

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

@@ -1,3 +1,4 @@
+import { Readable } from 'stream';
 import type { Configuration, KoaContextWithOIDC } from 'oidc-provider';
 import type { ErrorHandler } from '../../../../src/http/output/error/ErrorHandler';
 import type { ResponseWriter } from '../../../../src/http/output/ResponseWriter';
@@ -8,7 +9,6 @@ import type {
 } from '../../../../src/identity/interaction/email-password/credentials/ClientCredentialsAdapterFactory';
 import type { Interaction, InteractionHandler } from '../../../../src/identity/interaction/InteractionHandler';
 import type { AdapterFactory } from '../../../../src/identity/storage/AdapterFactory';
-import type { HttpResponse } from '../../../../src/server/HttpResponse';
 import type { KeyValueStorage } from '../../../../src/storage/keyvalue/KeyValueStorage';
 import { FoundHttpError } from '../../../../src/util/errors/FoundHttpError';
 
@@ -53,6 +53,8 @@ describe('An IdentityProviderFactory', (): void => {
 
     ctx = {
       method: 'GET',
+      req: Readable.from('data'),
+      res: {},
       request: {
         href: 'http://example.com/idp/',
       },
@@ -143,13 +145,12 @@ describe('An IdentityProviderFactory', (): void => {
     });
 
     // Test the renderError function
-    const response = { } as HttpResponse;
+    await expect((config.renderError as any)(ctx, {}, 'error!')).resolves.toBeUndefined();
-    await expect((config.renderError as any)({ res: response }, {}, 'error!')).resolves.toBeUndefined();
     expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1);
     expect(errorHandler.handleSafe)
-      .toHaveBeenLastCalledWith({ error: 'error!', preferences: { type: { 'text/plain': 1 }}});
+      .toHaveBeenLastCalledWith({ error: 'error!', request: ctx.req });
     expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1);
-    expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response, result: { statusCode: 500 }});
+    expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response: ctx.res, result: { statusCode: 500 }});
   });
 
   it('errors if there is no valid interaction redirect.', async(): Promise<void> => {
@@ -212,17 +213,16 @@ describe('An IdentityProviderFactory', (): void => {
   it('updates errors if there is more information.', async(): Promise<void> => {
     const provider = await factory.getProvider() as any;
     const { config } = provider as { config: Configuration };
-    const response = { } as HttpResponse;
 
     const error = new Error('bad data');
     const out = { error_description: 'more info' };
 
-    await expect((config.renderError as any)({ res: response }, out, error)).resolves.toBeUndefined();
+    await expect((config.renderError as any)(ctx, out, error)).resolves.toBeUndefined();
     expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1);
     expect(errorHandler.handleSafe)
-      .toHaveBeenLastCalledWith({ error, preferences: { type: { 'text/plain': 1 }}});
+      .toHaveBeenLastCalledWith({ error, request: ctx.req });
     expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1);
-    expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response, result: { statusCode: 500 }});
+    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');
   });

test/unit/server/ParsingHttpHandler.test.ts

@@ -16,9 +16,8 @@ import { HttpError } from '../../../src/util/errors/HttpError';
 describe('A ParsingHttpHandler', (): void => {
   const request: HttpRequest = {} as any;
   const response: HttpResponse = {} as any;
-  const preferences = { type: { 'text/html': 1 }};
   const body = new BasicRepresentation();
-  const operation: Operation = { method: 'GET', target: { path: 'http://test.com/foo' }, preferences, body };
+  const operation: Operation = { method: 'GET', target: { path: 'http://test.com/foo' }, preferences: {}, body };
   const errorResponse = new ResponseDescription(400);
   let requestParser: jest.Mocked<RequestParser>;
   let metadataCollector: jest.Mocked<OperationMetadataCollector>;
@@ -75,7 +74,7 @@ describe('A ParsingHttpHandler', (): void => {
     source.handleSafe.mockRejectedValueOnce(error);
     await expect(handler.handle({ request, response })).resolves.toBeUndefined();
     expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1);
-    expect(errorHandler.handleSafe).toHaveBeenLastCalledWith({ error, preferences });
+    expect(errorHandler.handleSafe).toHaveBeenLastCalledWith({ error, request });
     expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1);
     expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response, result: errorResponse });
   });
@@ -87,7 +86,7 @@ describe('A ParsingHttpHandler', (): void => {
     errorHandler.handleSafe.mockResolvedValueOnce(metaResponse);
     await expect(handler.handle({ request, response })).resolves.toBeUndefined();
     expect(errorHandler.handleSafe).toHaveBeenCalledTimes(1);
-    expect(errorHandler.handleSafe).toHaveBeenLastCalledWith({ error, preferences });
+    expect(errorHandler.handleSafe).toHaveBeenLastCalledWith({ error, request });
     expect(responseWriter.handleSafe).toHaveBeenCalledTimes(1);
     expect(responseWriter.handleSafe).toHaveBeenLastCalledWith({ response, result: metaResponse });
     expect(metaResponse.metadata?.quads()).toHaveLength(1);