fix: Make UnionCredentialsExtractor tolerate failures.

Fixes https://github.com/solid/community-server/issues/1031

.eslintrc.js

@@ -44,6 +44,7 @@ module.exports = {
     // Problems with optional parameters
     '@typescript-eslint/no-unnecessary-condition': 'off',
     '@typescript-eslint/prefer-optional-chain': 'error',
+    '@typescript-eslint/promise-function-async': [ 'error', { checkArrowFunctions: false } ],
     '@typescript-eslint/space-before-function-paren': [ 'error', 'never' ],
     '@typescript-eslint/unbound-method': 'off',
     '@typescript-eslint/unified-signatures': 'off',

src/http/output/error/ConvertingErrorHandler.ts

@@ -3,7 +3,7 @@ import type {
   RepresentationConverterArgs,
 } from '../../../storage/conversion/RepresentationConverter';
 import { INTERNAL_ERROR } from '../../../util/ContentTypes';
-import { getStatusCode } from '../../../util/errors/ErrorUtil';
+import { getStatusCode } from '../../../util/errors/HttpErrorUtil';
 import { toLiteral } from '../../../util/TermUtil';
 import { HTTP, XSD } from '../../../util/Vocabularies';
 import { BasicRepresentation } from '../../representation/BasicRepresentation';

src/http/output/error/SafeErrorHandler.ts

@@ -1,5 +1,6 @@
 import { getLoggerFor } from '../../../logging/LogUtil';
-import { createErrorMessage, getStatusCode } from '../../../util/errors/ErrorUtil';
+import { createErrorMessage } from '../../../util/errors/ErrorUtil';
+import { getStatusCode } from '../../../util/errors/HttpErrorUtil';
 import { guardedStreamFrom } from '../../../util/StreamUtil';
 import { toLiteral } from '../../../util/TermUtil';
 import { HTTP, XSD } from '../../../util/Vocabularies';

src/index.ts

@@ -320,6 +320,7 @@ export * from './util/errors/ConflictHttpError';
 export * from './util/errors/ErrorUtil';
 export * from './util/errors/ForbiddenHttpError';
 export * from './util/errors/HttpError';
+export * from './util/errors/HttpErrorUtil';
 export * from './util/errors/InternalServerError';
 export * from './util/errors/MethodNotAllowedHttpError';
 export * from './util/errors/NotFoundHttpError';

src/util/PromiseUtil.ts

@@ -1,3 +1,5 @@
+import { createAggregateError } from './errors/HttpErrorUtil';
+
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 const infinitePromise = new Promise<boolean>((): void => {});
 
@@ -28,3 +30,26 @@ export async function promiseSome(predicates: Promise<boolean>[]): Promise<boole
   // or finalPromise will return the result if none of them did or finalPromise was faster
   return Promise.race([ ...infinitePredicates, finalPromise ]);
 }
+
+/**
+ * Obtains the values of all fulfilled promises.
+ * If there are rejections (and `ignoreErrors` is false), throws a combined error of all rejected promises.
+ */
+export async function allFulfilled<T>(promises: Promise<T> [], ignoreErrors = false): Promise<T[]> {
+  // Collect values and errors
+  const values: T[] = [];
+  const errors: Error[] = [];
+  for (const result of await Promise.allSettled(promises)) {
+    if (result.status === 'fulfilled') {
+      values.push(result.value);
+    } else if (!ignoreErrors) {
+      errors.push(result.reason);
+    }
+  }
+
+  // Either throw or return
+  if (errors.length > 0) {
+    throw createAggregateError(errors);
+  }
+  return values;
+}

src/util/errors/ErrorUtil.ts

@@ -1,5 +1,4 @@
 import { types } from 'util';
-import { HttpError } from './HttpError';
 
 /**
  * Checks if the input is an {@link Error}.
@@ -25,10 +24,3 @@ export function assertError(error: unknown): asserts error is Error {
 export function createErrorMessage(error: unknown): string {
   return isError(error) ? error.message : `Unknown error: ${error}`;
 }
-
-/**
- * Returns the HTTP status code corresponding to the error.
- */
-export function getStatusCode(error: Error): number {
-  return HttpError.isInstance(error) ? error.statusCode : 500;
-}

src/util/errors/HttpErrorUtil.ts

@@ -0,0 +1,38 @@
+import { BadRequestHttpError } from './BadRequestHttpError';
+import { createErrorMessage } from './ErrorUtil';
+import { HttpError } from './HttpError';
+import { InternalServerError } from './InternalServerError';
+
+/**
+ * Returns the HTTP status code corresponding to the error.
+ */
+export function getStatusCode(error: Error): number {
+  return HttpError.isInstance(error) ? error.statusCode : 500;
+}
+
+/**
+ * Combines a list of errors into a single HttpErrors.
+ * Status code depends on the input errors. If they all share the same status code that code will be re-used.
+ * If they are all within the 4xx range, 400 will be used, otherwise 500.
+ *
+ * @param errors - Errors to combine.
+ * @param messagePrefix - Prefix for the aggregate error message. Will be followed with an array of all the messages.
+ */
+export function createAggregateError(errors: Error[], messagePrefix = 'No handler supports the given input:'):
+HttpError {
+  const httpErrors = errors.map((error): HttpError =>
+    HttpError.isInstance(error) ? error : new InternalServerError(createErrorMessage(error)));
+  const joined = httpErrors.map((error: Error): string => error.message).join(', ');
+  const message = `${messagePrefix} [${joined}]`;
+
+  // Check if all errors have the same status code
+  if (httpErrors.length > 0 && httpErrors.every((error): boolean => error.statusCode === httpErrors[0].statusCode)) {
+    return new HttpError(httpErrors[0].statusCode, httpErrors[0].name, message);
+  }
+
+  // Find the error range (4xx or 5xx)
+  if (httpErrors.some((error): boolean => error.statusCode >= 500)) {
+    return new InternalServerError(message);
+  }
+  return new BadRequestHttpError(message);
+}

src/util/handlers/HandlerUtil.ts

@@ -1,36 +1,7 @@
-import { BadRequestHttpError } from '../errors/BadRequestHttpError';
 import { createErrorMessage, isError } from '../errors/ErrorUtil';
-import { HttpError } from '../errors/HttpError';
-import { InternalServerError } from '../errors/InternalServerError';
+import { createAggregateError } from '../errors/HttpErrorUtil';
 import type { AsyncHandler } from './AsyncHandler';
 
-/**
- * Combines a list of errors into a single HttpErrors.
- * Status code depends on the input errors. If they all share the same status code that code will be re-used.
- * If they are all within the 4xx range, 400 will be used, otherwise 500.
- *
- * @param errors - Errors to combine.
- * @param messagePrefix - Prefix for the aggregate error message. Will be followed with an array of all the messages.
- */
-export function createAggregateError(errors: Error[], messagePrefix = 'No handler supports the given input:'):
-HttpError {
-  const httpErrors = errors.map((error): HttpError =>
-    HttpError.isInstance(error) ? error : new InternalServerError(createErrorMessage(error)));
-  const joined = httpErrors.map((error: Error): string => error.message).join(', ');
-  const message = `${messagePrefix} [${joined}]`;
-
-  // Check if all errors have the same status code
-  if (httpErrors.length > 0 && httpErrors.every((error): boolean => error.statusCode === httpErrors[0].statusCode)) {
-    return new HttpError(httpErrors[0].statusCode, httpErrors[0].name, message);
-  }
-
-  // Find the error range (4xx or 5xx)
-  if (httpErrors.some((error): boolean => error.statusCode >= 500)) {
-    return new InternalServerError(message);
-  }
-  return new BadRequestHttpError(message);
-}
-
 /**
  * Finds a handler that can handle the given input data.
  * Otherwise an error gets thrown.

src/util/handlers/ParallelHandler.ts

@@ -12,12 +12,10 @@ export class ParallelHandler<TIn = void, TOut = void> extends AsyncHandler<TIn,
   }
 
   public async canHandle(input: TIn): Promise<void> {
-    // eslint-disable-next-line @typescript-eslint/promise-function-async
     await Promise.all(this.handlers.map((handler): Promise<void> => handler.canHandle(input)));
   }
 
   public async handle(input: TIn): Promise<TOut[]> {
-    // eslint-disable-next-line @typescript-eslint/promise-function-async
     return Promise.all(this.handlers.map((handler): Promise<TOut> => handler.handle(input)));
   }
 }

src/util/handlers/UnionHandler.ts

@@ -1,31 +1,41 @@
+import { allFulfilled } from '../PromiseUtil';
 import { AsyncHandler } from './AsyncHandler';
-import { createAggregateError, filterHandlers, findHandler } from './HandlerUtil';
+import { filterHandlers, findHandler } from './HandlerUtil';
 
 // Helper types to make sure the UnionHandler has the same in/out types as the AsyncHandler type it wraps
-type ThenArg<T> = T extends PromiseLike<infer U> ? U : T;
+type Awaited<T> = T extends PromiseLike<infer U> ? U : T;
 type InType<T extends AsyncHandler<any, any>> = Parameters<T['handle']>[0];
-type OutType<T extends AsyncHandler<any, any>> = ThenArg<ReturnType<T['handle']>>;
-type HandlerType<T extends AsyncHandler> = AsyncHandler<InType<T>, OutType<T>>;
+type OutType<T extends AsyncHandler<any, any>> = Awaited<ReturnType<T['handle']>>;
 
 /**
  * Utility handler that allows combining the results of multiple handlers into one.
- * Will run all the handlers and then call the abstract `combine` function with the results,
- * which should return the output of the class.
- *
- * If `requireAll` is true, the handler will fail if any of the handlers do not support the input.
- * If `requireAll` is false, only the handlers that support the input will be called,
- * only if all handlers reject the input will this handler reject as well.
- * With `requireAll` set to false, the length of the input array
- * for the `combine` function is variable (but always at least 1).
+ * Will run the handlers and then call the abstract `combine` function with the results,
+ * which then generates the handler's output.
  */
 export abstract class UnionHandler<T extends AsyncHandler<any, any>> extends AsyncHandler<InType<T>, OutType<T>> {
   protected readonly handlers: T[];
   private readonly requireAll: boolean;
+  private readonly ignoreErrors: boolean;
 
-  protected constructor(handlers: T[], requireAll = false) {
+  /**
+   * Creates a new `UnionHandler`.
+   *
+   * When `requireAll` is false or `ignoreErrors` is true,
+   * the length of the input to `combine` can vary;
+   * otherwise, it is exactly the number of handlers.
+   *
+   * @param handlers - The handlers whose output is to be combined.
+   * @param requireAll - If true, will fail if any of the handlers do not support the input.
+                         If false, only the handlers that support the input will be called;
+   *                     will fail only if none of the handlers can handle the input.
+   * @param ignoreErrors - If true, ignores handlers that fail by omitting their output;
+   *                       if false, fails when any handlers fail.
+   */
+  public constructor(handlers: T[], requireAll = false, ignoreErrors = !requireAll) {
     super();
     this.handlers = handlers;
     this.requireAll = requireAll;
+    this.ignoreErrors = ignoreErrors;
   }
 
   public async canHandle(input: InType<T>): Promise<void> {
@@ -38,57 +48,21 @@ export abstract class UnionHandler<T extends AsyncHandler<any, any>> extends Asy
   }
 
   public async handle(input: InType<T>): Promise<OutType<T>> {
-    let handlers: HandlerType<T>[];
-    if (this.requireAll) {
-      // Handlers were already checked in canHandle
-      // eslint-disable-next-line prefer-destructuring
-      handlers = this.handlers;
-    } else {
-      handlers = await filterHandlers(this.handlers, input);
-    }
-
-    const results = await Promise.all(
-      handlers.map(async(handler): Promise<OutType<T>> => handler.handle(input)),
-    );
-
-    return this.combine(results);
-  }
-
-  public async handleSafe(input: InType<T>): Promise<OutType<T>> {
-    let handlers: HandlerType<T>[];
-    if (this.requireAll) {
-      await this.allCanHandle(input);
-      // eslint-disable-next-line prefer-destructuring
-      handlers = this.handlers;
-    } else {
-      // This will error if no handler supports the input
-      handlers = await filterHandlers(this.handlers, input);
-    }
-
-    const results = await Promise.all(
-      handlers.map(async(handler): Promise<OutType<T>> => handler.handle(input)),
-    );
-
-    return this.combine(results);
+    const handlers = this.requireAll ? this.handlers : await filterHandlers(this.handlers, input);
+    const results = handlers.map((handler): Promise<OutType<T>> => handler.handle(input));
+    return this.combine(await allFulfilled(results, this.ignoreErrors));
   }
 
   /**
    * Checks if all handlers can handle the input.
    * If not, throw an error based on the errors of the failed handlers.
    */
-  private async allCanHandle(input: InType<T>): Promise<void> {
-    const results = await Promise.allSettled(this.handlers.map(async(handler): Promise<HandlerType<T>> => {
-      await handler.canHandle(input);
-      return handler;
-    }));
-    if (results.some(({ status }): boolean => status === 'rejected')) {
-      const errors = results.map((result): Error => (result as PromiseRejectedResult).reason);
-      throw createAggregateError(errors);
-    }
+  protected async allCanHandle(input: InType<T>): Promise<void> {
+    await allFulfilled(this.handlers.map((handler): Promise<void> => handler.canHandle(input)));
   }
 
   /**
-   * Combine the results of the handlers into a single output.
+   * Combines the results of the handlers into a single output.
    */
   protected abstract combine(results: OutType<T>[]): Promise<OutType<T>>;
 }

test/integration/Setup.test.ts

@@ -51,7 +51,7 @@ describe('A Solid server with setup', (): void => {
 
     // Root access disabled
     res = await fetch(baseUrl);
-    expect(res.status).toBe(403);
+    expect(res.status).toBe(401);
 
     // Registration still possible
     const registerParams = { email, podName, password, confirmPassword: password, createWebId: true };

test/unit/authentication/UnionCredentialsExtractor.test.ts

@@ -43,4 +43,11 @@ describe('A UnionCredentialsExtractor', (): void => {
       [CredentialGroup.public]: {},
     });
   });
+
+  it('skips erroring handlers.', async(): Promise<void> => {
+    extractors[0].handle.mockRejectedValueOnce(new Error('error'));
+    await expect(extractor.handle(request)).resolves.toEqual({
+      [CredentialGroup.public]: {},
+    });
+  });
 });

test/unit/util/errors/ErrorUtil.test.ts

@@ -1,5 +1,4 @@
-import { assertError, createErrorMessage, getStatusCode, isError } from '../../../../src/util/errors/ErrorUtil';
-import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
+import { assertError, createErrorMessage, isError } from '../../../../src/util/errors/ErrorUtil';
 
 describe('ErrorUtil', (): void => {
   describe('#isError', (): void => {
@@ -39,14 +38,4 @@ describe('ErrorUtil', (): void => {
       expect(createErrorMessage('apple')).toBe('Unknown error: apple');
     });
   });
-
-  describe('#getStatusCode', (): void => {
-    it('returns the corresponding status code for HttpErrors.', async(): Promise<void> => {
-      expect(getStatusCode(new NotFoundHttpError())).toBe(404);
-    });
-
-    it('returns 500 for other errors.', async(): Promise<void> => {
-      expect(getStatusCode(new Error('404'))).toBe(500);
-    });
-  });
 });

test/unit/util/errors/HttpErrorUtil.test.ts

@@ -0,0 +1,50 @@
+import { HttpError } from '../../../../src/util/errors/HttpError';
+import { createAggregateError, getStatusCode } from '../../../../src/util/errors/HttpErrorUtil';
+import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError';
+
+describe('ErrorUtil', (): void => {
+  describe('createAggregateError', (): void => {
+    const error401 = new HttpError(401, 'UnauthorizedHttpError');
+    const error415 = new HttpError(415, 'UnsupportedMediaTypeHttpError');
+    const error501 = new HttpError(501, 'NotImplementedHttpError');
+    const error = new Error('noStatusCode');
+
+    it('throws an error with matching status code if all errors have the same.', async(): Promise<void> => {
+      expect(createAggregateError([ error401, error401 ])).toMatchObject({
+        statusCode: 401,
+        name: 'UnauthorizedHttpError',
+      });
+    });
+
+    it('throws an InternalServerError if one of the errors has status code 5xx.', async(): Promise<void> => {
+      expect(createAggregateError([ error401, error501 ])).toMatchObject({
+        statusCode: 500,
+        name: 'InternalServerError',
+      });
+    });
+
+    it('throws an BadRequestHttpError if all handlers have 4xx status codes.', async(): Promise<void> => {
+      expect(createAggregateError([ error401, error415 ])).toMatchObject({
+        statusCode: 400,
+        name: 'BadRequestHttpError',
+      });
+    });
+
+    it('interprets non-HTTP errors as internal errors.', async(): Promise<void> => {
+      expect(createAggregateError([ error ])).toMatchObject({
+        statusCode: 500,
+        name: 'InternalServerError',
+      });
+    });
+  });
+
+  describe('#getStatusCode', (): void => {
+    it('returns the corresponding status code for HttpErrors.', async(): Promise<void> => {
+      expect(getStatusCode(new NotFoundHttpError())).toBe(404);
+    });
+
+    it('returns 500 for other errors.', async(): Promise<void> => {
+      expect(getStatusCode(new Error('404'))).toBe(500);
+    });
+  });
+});

test/unit/util/handlers/HandlerUtil.test.ts

@@ -1,44 +1,8 @@
-import { HttpError } from '../../../../src/util/errors/HttpError';
 import type { AsyncHandler } from '../../../../src/util/handlers/AsyncHandler';
-import { createAggregateError, filterHandlers, findHandler } from '../../../../src/util/handlers/HandlerUtil';
+import { filterHandlers, findHandler } from '../../../../src/util/handlers/HandlerUtil';
 import { StaticAsyncHandler } from '../../../util/StaticAsyncHandler';
 
 describe('HandlerUtil', (): void => {
-  describe('createAggregateError', (): void => {
-    const error401 = new HttpError(401, 'UnauthorizedHttpError');
-    const error415 = new HttpError(415, 'UnsupportedMediaTypeHttpError');
-    const error501 = new HttpError(501, 'NotImplementedHttpError');
-    const error = new Error('noStatusCode');
-
-    it('throws an error with matching status code if all errors have the same.', async(): Promise<void> => {
-      expect(createAggregateError([ error401, error401 ])).toMatchObject({
-        statusCode: 401,
-        name: 'UnauthorizedHttpError',
-      });
-    });
-
-    it('throws an InternalServerError if one of the errors has status code 5xx.', async(): Promise<void> => {
-      expect(createAggregateError([ error401, error501 ])).toMatchObject({
-        statusCode: 500,
-        name: 'InternalServerError',
-      });
-    });
-
-    it('throws an BadRequestHttpError if all handlers have 4xx status codes.', async(): Promise<void> => {
-      expect(createAggregateError([ error401, error415 ])).toMatchObject({
-        statusCode: 400,
-        name: 'BadRequestHttpError',
-      });
-    });
-
-    it('interprets non-HTTP errors as internal errors.', async(): Promise<void> => {
-      expect(createAggregateError([ error ])).toMatchObject({
-        statusCode: 500,
-        name: 'InternalServerError',
-      });
-    });
-  });
-
   describe('findHandler', (): void => {
     let handlerTrue: AsyncHandler<any, any>;
     let handlerFalse: AsyncHandler<any, any>;

test/unit/util/handlers/UnionHandler.test.ts

@@ -2,10 +2,6 @@ import type { AsyncHandler } from '../../../../src/util/handlers/AsyncHandler';
 import { UnionHandler } from '../../../../src/util/handlers/UnionHandler';
 
 class SimpleUnionHandler extends UnionHandler<AsyncHandler<any, string>> {
-  public constructor(handlers: AsyncHandler<any, any>[], requireAll?: boolean) {
-    super(handlers, requireAll);
-  }
-
   protected async combine(results: string[]): Promise<string> {
     return results.join('');
   }
@@ -61,4 +57,25 @@ describe('A UnionHandler', (): void => {
     handlers[0].canHandle.mockRejectedValue(new Error('bad request'));
     await expect(handler.handle(input)).resolves.toBe('ab');
   });
+
+  it('requires all handlers to succeed if requireAll is true.', async(): Promise<void> => {
+    handler = new SimpleUnionHandler(handlers, true);
+
+    handlers[0].handle.mockRejectedValue(new Error('bad request'));
+    await expect(handler.handleSafe(input)).rejects.toThrow('bad request');
+  });
+
+  it('does not require all handlers to succeed if ignoreErrors is true.', async(): Promise<void> => {
+    handler = new SimpleUnionHandler(handlers, true, true);
+
+    handlers[0].handle.mockRejectedValueOnce(new Error('bad request'));
+    await expect(handler.handleSafe(input)).resolves.toBe('b');
+
+    handlers[1].handle.mockRejectedValueOnce(new Error('bad request'));
+    await expect(handler.handleSafe(input)).resolves.toBe('a');
+
+    handlers[0].handle.mockRejectedValueOnce(new Error('bad request'));
+    handlers[1].handle.mockRejectedValueOnce(new Error('bad request'));
+    await expect(handler.handleSafe(input)).resolves.toBe('');
+  });
 });