From 42168ee39b82f7b18008274e712494faf66b6f0f Mon Sep 17 00:00:00 2001 From: Simone Persiani Date: Mon, 16 Aug 2021 10:50:47 +0200 Subject: [PATCH 1/6] feat: Add function readableToQuads Co-Authored-By: Ludovico Granata --- src/authorization/WebAclAuthorizer.ts | 8 ++---- src/storage/patch/SparqlUpdatePatchHandler.ts | 11 +++----- src/util/StreamUtil.ts | 14 ++++++++++ test/unit/util/StreamUtil.test.ts | 26 ++++++++++++++++++- 4 files changed, 45 insertions(+), 14 deletions(-) diff --git a/src/authorization/WebAclAuthorizer.ts b/src/authorization/WebAclAuthorizer.ts index de7ac53e2f..97650b0789 100644 --- a/src/authorization/WebAclAuthorizer.ts +++ b/src/authorization/WebAclAuthorizer.ts @@ -15,6 +15,7 @@ import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; import { UnauthorizedHttpError } from '../util/errors/UnauthorizedHttpError'; import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy'; +import { readableToQuads } from '../util/StreamUtil'; import { ACL, FOAF } from '../util/Vocabularies'; import type { AuthorizerArgs } from './Authorizer'; import { Authorizer } from './Authorizer'; @@ -254,12 +255,7 @@ export class WebAclAuthorizer extends Authorizer { */ private async filterData(data: Representation, predicate: string, object: string): Promise { // Import all triples from the representation into a queryable store - const quads = new Store(); - const importer = quads.import(data.data); - await new Promise((resolve, reject): void => { - importer.on('end', resolve); - importer.on('error', reject); - }); + const quads = await readableToQuads(data.data); // Find subjects that occur with a given predicate/object, and collect all their triples const subjectData = new Store(); diff --git a/src/storage/patch/SparqlUpdatePatchHandler.ts b/src/storage/patch/SparqlUpdatePatchHandler.ts index 379100ad68..07ffe9a445 100644 --- a/src/storage/patch/SparqlUpdatePatchHandler.ts +++ b/src/storage/patch/SparqlUpdatePatchHandler.ts @@ -15,7 +15,7 @@ import type { ResourceIdentifier } from '../../ldp/representation/ResourceIdenti import { getLoggerFor } from '../../logging/LogUtil'; import { INTERNAL_QUADS } from '../../util/ContentTypes'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; -import { readableToString } from '../../util/StreamUtil'; +import { readableToQuads, readableToString } from '../../util/StreamUtil'; import type { RepresentationConverter } from '../conversion/RepresentationConverter'; import { ConvertingPatchHandler } from './ConvertingPatchHandler'; import type { PatchHandlerArgs } from './PatchHandler'; @@ -119,19 +119,16 @@ export class SparqlUpdatePatchHandler extends ConvertingPatchHandler { */ protected async patch(input: PatchHandlerArgs, representation?: Representation): Promise { const { identifier, patch } = input; - const result = new Store(); + let result: Store; let metadata: RepresentationMetadata; if (representation) { ({ metadata } = representation); - const importEmitter = result.import(representation.data); - await new Promise((resolve, reject): void => { - importEmitter.on('end', resolve); - importEmitter.on('error', reject); - }); + result = await readableToQuads(representation.data); this.logger.debug(`${result.size} quads in ${identifier.path}.`); } else { metadata = new RepresentationMetadata(identifier, INTERNAL_QUADS); + result = new Store(); } // Run the query through Comunica diff --git a/src/util/StreamUtil.ts b/src/util/StreamUtil.ts index 1affe13b8c..ffc3a96179 100644 --- a/src/util/StreamUtil.ts +++ b/src/util/StreamUtil.ts @@ -3,6 +3,7 @@ import { Readable, Transform } from 'stream'; import { promisify } from 'util'; import arrayifyStream from 'arrayify-stream'; import eos from 'end-of-stream'; +import { Store } from 'n3'; import pump from 'pump'; import { getLoggerFor } from '../logging/LogUtil'; import { isHttpRequest } from '../server/HttpRequest'; @@ -23,6 +24,19 @@ export async function readableToString(stream: Readable): Promise { return (await arrayifyStream(stream)).join(''); } +/** + * Imports quads from a stream into a Store. + * @param stream - Stream of quads. + * + * @returns A Store containing all the quads. + */ +export async function readableToQuads(stream: Readable): Promise { + const quads = new Store(); + quads.import(stream); + await endOfStream(stream); + return quads; +} + // These error messages usually indicate expected behaviour so should not give a warning. // We compare against the error message instead of the code // since the second one is from an external library that does not assign an error code. diff --git a/test/unit/util/StreamUtil.test.ts b/test/unit/util/StreamUtil.test.ts index 7e02129f79..9417208913 100644 --- a/test/unit/util/StreamUtil.test.ts +++ b/test/unit/util/StreamUtil.test.ts @@ -1,9 +1,11 @@ import { PassThrough, Readable } from 'stream'; import arrayifyStream from 'arrayify-stream'; +import { Quad, NamedNode, Literal, BlankNode, Store } from 'n3'; import type { Logger } from '../../../src/logging/Logger'; import { getLoggerFor } from '../../../src/logging/LogUtil'; import { isHttpRequest } from '../../../src/server/HttpRequest'; -import { guardedStreamFrom, pipeSafely, transformSafely, readableToString } from '../../../src/util/StreamUtil'; +import { guardedStreamFrom, pipeSafely, transformSafely, + readableToString, readableToQuads } from '../../../src/util/StreamUtil'; jest.mock('../../../src/logging/LogUtil', (): any => { const logger: Logger = { warn: jest.fn(), log: jest.fn() } as any; @@ -23,6 +25,28 @@ describe('StreamUtil', (): void => { }); }); + describe('#readableToQuads', (): void => { + it('imports all quads from a Readable.', async(): Promise => { + const subject = new NamedNode('#subject'); + const property = new NamedNode('#property'); + const object = new NamedNode('#object'); + const literal = new Literal('abcde'); + const blankNode = new BlankNode('_1'); + const graph = new NamedNode('#graph'); + + const quad1 = new Quad(subject, property, object, graph); + const quad2 = new Quad(subject, property, literal, graph); + const quad3 = new Quad(subject, property, blankNode, graph); + const quads = new Store(); + quads.add(quad1); + quads.add(quad2); + quads.add(quad3); + + const stream = Readable.from([ quad1, quad2, quad3 ]); + await expect(readableToQuads(stream)).resolves.toEqual(quads); + }); + }); + describe('#pipeSafely', (): void => { beforeEach(async(): Promise => { jest.clearAllMocks(); From fe9d269d90994192a3ebfb612b6634a02f3d6780 Mon Sep 17 00:00:00 2001 From: Simone Persiani Date: Mon, 16 Aug 2021 10:58:28 +0200 Subject: [PATCH 2/6] feat: Add function promiseSome Co-Authored-By: Ludovico Granata --- src/index.ts | 1 + src/util/PromiseUtil.ts | 30 +++++++++++++++++++++++++++++ test/unit/util/PromiseUtil.test.ts | 31 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+) create mode 100644 src/util/PromiseUtil.ts create mode 100644 test/unit/util/PromiseUtil.test.ts diff --git a/src/index.ts b/src/index.ts index 1a7a3f1440..7bab76c46f 100644 --- a/src/index.ts +++ b/src/index.ts @@ -325,6 +325,7 @@ export * from './util/FetchUtil'; export * from './util/GuardedStream'; export * from './util/HeaderUtil'; export * from './util/PathUtil'; +export * from './util/PromiseUtil'; export * from './util/QuadUtil'; export * from './util/RecordObject'; export * from './util/ResourceUtil'; diff --git a/src/util/PromiseUtil.ts b/src/util/PromiseUtil.ts new file mode 100644 index 0000000000..7b72f82ee0 --- /dev/null +++ b/src/util/PromiseUtil.ts @@ -0,0 +1,30 @@ +// eslint-disable-next-line @typescript-eslint/no-empty-function +const infinitePromise = new Promise((): void => {}); + +/** + * A function that simulates the Array.some behaviour but on an array of Promises. + * Returns true if at least one promise returns true. + * Returns false if all promises return false or error. + * + * @remarks + * + * Predicates provided as input must be implemented considering + * the following points: + * 1. if they throw an error, it won't be propagated; + * 2. throwing an error should be logically equivalent to returning false. + */ +export async function promiseSome(predicates: Promise[]): Promise { + // These promises will only finish when their predicate returns true + const infinitePredicates = predicates.map(async(predicate): Promise => predicate.then( + async(value): Promise => value ? true : infinitePromise, + async(): Promise => infinitePromise, + )); + + // Returns after all predicates are resolved + const finalPromise = Promise.allSettled(predicates).then((results): boolean => + results.some((result): boolean => result.status === 'fulfilled' && result.value)); + + // Either one of the infinitePredicates will return true, + // or finalPromise will return the result if none of them did or finalPromise was faster + return Promise.race([ ...infinitePredicates, finalPromise ]); +} diff --git a/test/unit/util/PromiseUtil.test.ts b/test/unit/util/PromiseUtil.test.ts new file mode 100644 index 0000000000..e96a1113d5 --- /dev/null +++ b/test/unit/util/PromiseUtil.test.ts @@ -0,0 +1,31 @@ +import { promiseSome } from '../../../src/util/PromiseUtil'; + +describe('PromiseUtil', (): void => { + describe('#promiseSome', (): void => { + const resultTrue = Promise.resolve(true); + const resultFalse = Promise.resolve(false); + const resultError = Promise.reject(new Error('generic error')); + // eslint-disable-next-line @typescript-eslint/no-empty-function + const resultInfinite = new Promise((): void => {}); + + it('returns false if no promise is provided.', async(): Promise => { + await expect(promiseSome([])).resolves.toEqual(false); + }); + + it('returns false if no promise returns true.', async(): Promise => { + await expect(promiseSome([ resultFalse, resultFalse, resultFalse ])).resolves.toEqual(false); + }); + + it('returns true if at least a promise returns true.', async(): Promise => { + await expect(promiseSome([ resultFalse, resultTrue, resultFalse ])).resolves.toEqual(true); + }); + + it('does not propagate errors.', async(): Promise => { + await expect(promiseSome([ resultError, resultFalse, resultFalse ])).resolves.toEqual(false); + }); + + it('works with a combination of promises.', async(): Promise => { + await expect(promiseSome([ resultError, resultTrue, resultInfinite ])).resolves.toEqual(true); + }); + }); +}); From 88f943159b5ce1ed7a5acaa2a625d96f843529f0 Mon Sep 17 00:00:00 2001 From: Simone Persiani Date: Tue, 17 Aug 2021 17:15:21 +0200 Subject: [PATCH 3/6] feat: Add BooleanHandler Co-Authored-By: Ludovico Granata --- src/index.ts | 1 + src/util/handlers/BooleanHandler.ts | 45 ++++++++++ src/util/handlers/HandlerUtil.ts | 89 +++++++++++++++++++ src/util/handlers/WaterfallHandler.ts | 50 +---------- .../unit/util/handlers/BooleanHandler.test.ts | 56 ++++++++++++ test/unit/util/handlers/HandlerUtil.test.ts | 84 +++++++++++++++++ .../util/handlers/WaterfallHandler.test.ts | 41 --------- 7 files changed, 279 insertions(+), 87 deletions(-) create mode 100644 src/util/handlers/BooleanHandler.ts create mode 100644 src/util/handlers/HandlerUtil.ts create mode 100644 test/unit/util/handlers/BooleanHandler.test.ts create mode 100644 test/unit/util/handlers/HandlerUtil.test.ts diff --git a/src/index.ts b/src/index.ts index 7bab76c46f..acb8d16f71 100644 --- a/src/index.ts +++ b/src/index.ts @@ -292,6 +292,7 @@ export * from './util/errors/UnsupportedMediaTypeHttpError'; // Util/Handlers export * from './util/handlers/AsyncHandler'; +export * from './util/handlers/BooleanHandler'; export * from './util/handlers/ParallelHandler'; export * from './util/handlers/SequenceHandler'; export * from './util/handlers/UnsupportedAsyncHandler'; diff --git a/src/util/handlers/BooleanHandler.ts b/src/util/handlers/BooleanHandler.ts new file mode 100644 index 0000000000..010d71a9ee --- /dev/null +++ b/src/util/handlers/BooleanHandler.ts @@ -0,0 +1,45 @@ +import { getLoggerFor } from '../../logging/LogUtil'; +import { InternalServerError } from '../errors/InternalServerError'; +import { promiseSome } from '../PromiseUtil'; +import { AsyncHandler } from './AsyncHandler'; +import { filterHandlers } from './HandlerUtil'; + +/** + * A composite handler that returns true if any of its handlers can handle the input and return true. + * Handler errors are interpreted as false results. + */ +export class BooleanHandler extends AsyncHandler { + protected readonly logger = getLoggerFor(this); + + private readonly handlers: AsyncHandler[]; + + /** + * Creates a new BooleanHandler that stores the given handlers. + * @param handlers - Handlers over which it will run. + */ + public constructor(handlers: AsyncHandler[]) { + super(); + this.handlers = handlers; + } + + public async canHandle(input: TIn): Promise { + // We use this to generate an error if no handler supports the input + await filterHandlers(this.handlers, input); + } + + public async handleSafe(input: TIn): Promise { + const handlers = await filterHandlers(this.handlers, input); + return promiseSome(handlers.map(async(handler): Promise => handler.handle(input))); + } + + public async handle(input: TIn): Promise { + let handlers: AsyncHandler[]; + try { + handlers = await filterHandlers(this.handlers, input); + } catch (error: unknown) { + this.logger.warn('All handlers failed. This might be the consequence of calling handle before canHandle.'); + throw new InternalServerError('All handlers failed', { cause: error }); + } + return promiseSome(handlers.map(async(handler): Promise => handler.handle(input))); + } +} diff --git a/src/util/handlers/HandlerUtil.ts b/src/util/handlers/HandlerUtil.ts new file mode 100644 index 0000000000..3be746a0c7 --- /dev/null +++ b/src/util/handlers/HandlerUtil.ts @@ -0,0 +1,89 @@ +import { BadRequestHttpError } from '../errors/BadRequestHttpError'; +import { createErrorMessage, isError } from '../errors/ErrorUtil'; +import { HttpError } from '../errors/HttpError'; +import { InternalServerError } from '../errors/InternalServerError'; +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. + * + * @param handlers - List of handlers to search in. + * @param input - The input data. + * + * @returns A promise resolving to a handler that supports the data or otherwise rejecting. + */ +export async function findHandler(handlers: AsyncHandler[], input: TIn): +Promise> { + const errors: Error[] = []; + + for (const handler of handlers) { + try { + await handler.canHandle(input); + + return handler; + } catch (error: unknown) { + if (isError(error)) { + errors.push(error); + } else { + errors.push(new Error(createErrorMessage(error))); + } + } + } + + throw createAggregateError(errors); +} + +/** + * Filters a list of handlers to only keep those that can handle the input. + * Will error if no matching handlers are found. + * + * @param handlers - Handlers to filter. + * @param input - Input that needs to be supported. + */ +export async function filterHandlers(handlers: AsyncHandler[], input: TIn): +Promise[]> { + const results = await Promise.allSettled(handlers.map(async(handler): Promise> => { + await handler.canHandle(input); + return handler; + })); + const matches = results.filter((result): boolean => result.status === 'fulfilled') + .map((result): AsyncHandler => + (result as PromiseFulfilledResult>).value); + + if (matches.length > 0) { + return matches; + } + + // Generate error in case no matches were found + const errors = results.map((result): Error => (result as PromiseRejectedResult).reason); + + throw createAggregateError(errors); +} diff --git a/src/util/handlers/WaterfallHandler.ts b/src/util/handlers/WaterfallHandler.ts index de1ac5cd0e..b3ef570b67 100644 --- a/src/util/handlers/WaterfallHandler.ts +++ b/src/util/handlers/WaterfallHandler.ts @@ -1,9 +1,7 @@ import { getLoggerFor } from '../../logging/LogUtil'; -import { BadRequestHttpError } from '../errors/BadRequestHttpError'; -import { createErrorMessage } from '../errors/ErrorUtil'; -import { HttpError } from '../errors/HttpError'; import { InternalServerError } from '../errors/InternalServerError'; import type { AsyncHandler } from './AsyncHandler'; +import { findHandler } from './HandlerUtil'; /** * A composite handler that tries multiple handlers one by one @@ -31,7 +29,7 @@ export class WaterfallHandler implements AsyncHandler { * @returns A promise resolving if at least 1 handler supports to input, or rejecting if none do. */ public async canHandle(input: TIn): Promise { - await this.findHandler(input); + await findHandler(this.handlers, input); } /** @@ -45,7 +43,7 @@ export class WaterfallHandler implements AsyncHandler { let handler: AsyncHandler; try { - handler = await this.findHandler(input); + handler = await findHandler(this.handlers, input); } catch (error: unknown) { this.logger.warn('All handlers failed. This might be the consequence of calling handle before canHandle.'); throw new InternalServerError('All handlers failed', { cause: error }); @@ -63,48 +61,8 @@ export class WaterfallHandler implements AsyncHandler { * It rejects if no handlers support the given data. */ public async handleSafe(input: TIn): Promise { - const handler = await this.findHandler(input); + const handler = await findHandler(this.handlers, input); return handler.handle(input); } - - /** - * Finds a handler that can handle the given input data. - * Otherwise an error gets thrown. - * - * @param input - The input data. - * - * @returns A promise resolving to a handler that supports the data or otherwise rejecting. - */ - private async findHandler(input: TIn): Promise> { - const errors: HttpError[] = []; - - for (const handler of this.handlers) { - try { - await handler.canHandle(input); - - return handler; - } catch (error: unknown) { - if (HttpError.isInstance(error)) { - errors.push(error); - } else { - errors.push(new InternalServerError(createErrorMessage(error))); - } - } - } - - const joined = errors.map((error: Error): string => error.message).join(', '); - const message = `No handler supports the given input: [${joined}]`; - - // Check if all errors have the same status code - if (errors.length > 0 && errors.every((error): boolean => error.statusCode === errors[0].statusCode)) { - throw new HttpError(errors[0].statusCode, errors[0].name, message); - } - - // Find the error range (4xx or 5xx) - if (errors.some((error): boolean => error.statusCode >= 500)) { - throw new InternalServerError(message); - } - throw new BadRequestHttpError(message); - } } diff --git a/test/unit/util/handlers/BooleanHandler.test.ts b/test/unit/util/handlers/BooleanHandler.test.ts new file mode 100644 index 0000000000..1612f0e702 --- /dev/null +++ b/test/unit/util/handlers/BooleanHandler.test.ts @@ -0,0 +1,56 @@ +import { InternalServerError } from '../../../../src/util/errors/InternalServerError'; +import type { AsyncHandler } from '../../../../src/util/handlers/AsyncHandler'; +import { BooleanHandler } from '../../../../src/util/handlers/BooleanHandler'; +import { StaticAsyncHandler } from '../../../util/StaticAsyncHandler'; + +describe('A BooleanHandler', (): void => { + let handlerFalse: AsyncHandler; + let handlerTrue: AsyncHandler; + let handlerError: AsyncHandler; + let handlerCanNotHandle: AsyncHandler; + + beforeEach(async(): Promise => { + handlerFalse = new StaticAsyncHandler(true, false); + handlerTrue = new StaticAsyncHandler(true, true); + handlerError = new StaticAsyncHandler(true, null) as any; + handlerError.handle = (): never => { + throw new InternalServerError(); + }; + handlerCanNotHandle = new StaticAsyncHandler(false, null); + }); + + it('can handle the input if any of its handlers can.', async(): Promise => { + const handler = new BooleanHandler([ handlerFalse, handlerCanNotHandle ]); + await expect(handler.canHandle(null)).resolves.toBeUndefined(); + }); + + it('errors if none of its handlers supports the input.', async(): Promise => { + const handler = new BooleanHandler([ handlerCanNotHandle, handlerCanNotHandle ]); + await expect(handler.canHandle(null)).rejects.toThrow('[Not supported, Not supported]'); + }); + + it('returns true if any of its handlers returns true.', async(): Promise => { + const handler = new BooleanHandler([ handlerFalse, handlerTrue, handlerCanNotHandle ]); + await expect(handler.handle(null)).resolves.toBe(true); + }); + + it('returns false if none of its handlers returns true.', async(): Promise => { + const handler = new BooleanHandler([ handlerFalse, handlerError, handlerCanNotHandle ]); + await expect(handler.handle(null)).resolves.toBe(false); + }); + + it('throw an internal error when calling handle with unsupported input.', async(): Promise => { + const handler = new BooleanHandler([ handlerCanNotHandle, handlerCanNotHandle ]); + await expect(handler.handle(null)).rejects.toThrow(InternalServerError); + }); + + it('returns the same handle results with handleSafe.', async(): Promise => { + const handler = new BooleanHandler([ handlerFalse, handlerTrue, handlerCanNotHandle ]); + await expect(handler.handleSafe(null)).resolves.toBe(true); + }); + + it('throws the canHandle error when calling handleSafe with unsupported input.', async(): Promise => { + const handler = new BooleanHandler([ handlerCanNotHandle, handlerCanNotHandle ]); + await expect(handler.handleSafe(null)).rejects.toThrow('[Not supported, Not supported]'); + }); +}); diff --git a/test/unit/util/handlers/HandlerUtil.test.ts b/test/unit/util/handlers/HandlerUtil.test.ts new file mode 100644 index 0000000000..9cef18305a --- /dev/null +++ b/test/unit/util/handlers/HandlerUtil.test.ts @@ -0,0 +1,84 @@ +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 { 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 => { + expect(createAggregateError([ error401, error401 ])).toMatchObject({ + statusCode: 401, + name: 'UnauthorizedHttpError', + }); + }); + + it('throws an InternalServerError if one of the errors has status code 5xx.', async(): Promise => { + expect(createAggregateError([ error401, error501 ])).toMatchObject({ + statusCode: 500, + name: 'InternalServerError', + }); + }); + + it('throws an BadRequestHttpError if all handlers have 4xx status codes.', async(): Promise => { + expect(createAggregateError([ error401, error415 ])).toMatchObject({ + statusCode: 400, + name: 'BadRequestHttpError', + }); + }); + + it('interprets non-HTTP errors as internal errors.', async(): Promise => { + expect(createAggregateError([ error ])).toMatchObject({ + statusCode: 500, + name: 'InternalServerError', + }); + }); + }); + + describe('findHandler', (): void => { + let handlerTrue: AsyncHandler; + let handlerFalse: AsyncHandler; + + beforeEach(async(): Promise => { + handlerTrue = new StaticAsyncHandler(true, null); + handlerFalse = new StaticAsyncHandler(false, null); + }); + + it('finds a matching handler.', async(): Promise => { + await expect(findHandler([ handlerFalse, handlerTrue ], null)).resolves.toBe(handlerTrue); + }); + + it('errors if there is no matching handler.', async(): Promise => { + await expect(findHandler([ handlerFalse, handlerFalse ], null)).rejects.toThrow('[Not supported, Not supported]'); + }); + + it('supports non-native Errors.', async(): Promise => { + handlerFalse.canHandle = jest.fn().mockRejectedValue('apple'); + await expect(findHandler([ handlerFalse ], null)).rejects.toThrow('[Unknown error: apple]'); + }); + }); + + describe('filterHandlers', (): void => { + let handlerTrue: AsyncHandler; + let handlerFalse: AsyncHandler; + + beforeEach(async(): Promise => { + handlerTrue = new StaticAsyncHandler(true, null); + handlerFalse = new StaticAsyncHandler(false, null); + }); + + it('finds matching handlers.', async(): Promise => { + await expect(filterHandlers([ handlerTrue, handlerFalse, handlerTrue ], null)) + .resolves.toEqual([ handlerTrue, handlerTrue ]); + }); + + it('errors if there is no matching handler.', async(): Promise => { + await expect(filterHandlers([ handlerFalse, handlerFalse ], null)) + .rejects.toThrow('[Not supported, Not supported]'); + }); + }); +}); diff --git a/test/unit/util/handlers/WaterfallHandler.test.ts b/test/unit/util/handlers/WaterfallHandler.test.ts index 3a8f3eeb34..a424a3aa0d 100644 --- a/test/unit/util/handlers/WaterfallHandler.test.ts +++ b/test/unit/util/handlers/WaterfallHandler.test.ts @@ -1,5 +1,3 @@ -import { BadRequestHttpError } from '../../../../src/util/errors/BadRequestHttpError'; -import { HttpError } from '../../../../src/util/errors/HttpError'; import type { AsyncHandler } from '../../../../src/util/handlers/AsyncHandler'; import { WaterfallHandler } from '../../../../src/util/handlers/WaterfallHandler'; import { StaticAsyncHandler } from '../../../util/StaticAsyncHandler'; @@ -83,44 +81,5 @@ describe('A WaterfallHandler', (): void => { await expect(handler.handleSafe(null)).rejects.toThrow('[Not supported, Not supported]'); }); - - it('throws an error with matching status code if all handlers threw the same.', async(): Promise => { - handlerTrue.canHandle = async(): Promise => { - throw new HttpError(401, 'UnauthorizedHttpError'); - }; - const handler = new WaterfallHandler([ handlerTrue, handlerTrue ]); - - await expect(handler.canHandle(null)).rejects.toMatchObject({ - statusCode: 401, - name: 'UnauthorizedHttpError', - }); - }); - - it('throws an internal server error if one of the handlers threw one.', async(): Promise => { - handlerTrue.canHandle = async(): Promise => { - throw new HttpError(401, 'UnauthorizedHttpError'); - }; - handlerFalse.canHandle = async(): Promise => { - throw new Error('Server is crashing!'); - }; - const handler = new WaterfallHandler([ handlerTrue, handlerFalse ]); - - await expect(handler.canHandle(null)).rejects.toMatchObject({ - statusCode: 500, - name: 'InternalServerError', - }); - }); - - it('throws an BadRequestHttpError if handlers throw different errors.', async(): Promise => { - handlerTrue.canHandle = async(): Promise => { - throw new HttpError(401, 'UnauthorizedHttpError'); - }; - handlerFalse.canHandle = async(): Promise => { - throw new HttpError(415, 'UnsupportedMediaTypeHttpError'); - }; - const handler = new WaterfallHandler([ handlerTrue, handlerFalse ]); - - await expect(handler.canHandle(null)).rejects.toThrow(BadRequestHttpError); - }); }); }); From cbaad39be286d59df5fbcb881175da4d73b6833d Mon Sep 17 00:00:00 2001 From: Simone Persiani Date: Tue, 17 Aug 2021 16:51:50 +0200 Subject: [PATCH 4/6] refactor: Refactor WebAclAuthorizer Co-Authored-By: Ludovico Granata --- .../authorizers/access-checkers/agent.json | 9 ++ .../access-checkers/agentClass.json | 9 ++ config/ldp/authorization/authorizers/acl.json | 11 ++ src/authorization/WebAclAuthorizer.ts | 119 ++++++------------ .../access-checkers/AccessChecker.ts | 25 ++++ .../access-checkers/AgentAccessChecker.ts | 15 +++ .../AgentClassAccessChecker.ts | 18 +++ src/index.ts | 5 + src/util/Vocabularies.ts | 1 + .../authorization/WebAclAuthorizer.test.ts | 97 +++++++------- .../AgentAccessChecker.test.ts | 32 +++++ .../AgentClassAccessChecker.test.ts | 37 ++++++ 12 files changed, 246 insertions(+), 132 deletions(-) create mode 100644 config/ldp/authorization/authorizers/access-checkers/agent.json create mode 100644 config/ldp/authorization/authorizers/access-checkers/agentClass.json create mode 100644 src/authorization/access-checkers/AccessChecker.ts create mode 100644 src/authorization/access-checkers/AgentAccessChecker.ts create mode 100644 src/authorization/access-checkers/AgentClassAccessChecker.ts create mode 100644 test/unit/authorization/access-checkers/AgentAccessChecker.test.ts create mode 100644 test/unit/authorization/access-checkers/AgentClassAccessChecker.test.ts diff --git a/config/ldp/authorization/authorizers/access-checkers/agent.json b/config/ldp/authorization/authorizers/access-checkers/agent.json new file mode 100644 index 0000000000..0e883a2469 --- /dev/null +++ b/config/ldp/authorization/authorizers/access-checkers/agent.json @@ -0,0 +1,9 @@ +{ + "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", + "@graph": [ + { + "@id": "urn:solid-server:default:AgentAccessChecker", + "@type": "AgentAccessChecker" + } + ] +} diff --git a/config/ldp/authorization/authorizers/access-checkers/agentClass.json b/config/ldp/authorization/authorizers/access-checkers/agentClass.json new file mode 100644 index 0000000000..85c4a331e6 --- /dev/null +++ b/config/ldp/authorization/authorizers/access-checkers/agentClass.json @@ -0,0 +1,9 @@ +{ + "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", + "@graph": [ + { + "@id": "urn:solid-server:default:AgentClassAccessChecker", + "@type": "AgentClassAccessChecker" + } + ] +} diff --git a/config/ldp/authorization/authorizers/acl.json b/config/ldp/authorization/authorizers/acl.json index 4ede94e855..7a71a7b541 100644 --- a/config/ldp/authorization/authorizers/acl.json +++ b/config/ldp/authorization/authorizers/acl.json @@ -1,5 +1,9 @@ { "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", + "import": [ + "files-scs:config/ldp/authorization/authorizers/access-checkers/agent.json", + "files-scs:config/ldp/authorization/authorizers/access-checkers/agentClass.json" + ], "@graph": [ { "@id": "urn:solid-server:default:WebAclAuthorizer", @@ -12,6 +16,13 @@ }, "identifierStrategy": { "@id": "urn:solid-server:default:IdentifierStrategy" + }, + "accessChecker": { + "@type": "BooleanHandler", + "handlers": [ + { "@id": "urn:solid-server:default:AgentAccessChecker" }, + { "@id": "urn:solid-server:default:AgentClassAccessChecker" } + ] } } ] diff --git a/src/authorization/WebAclAuthorizer.ts b/src/authorization/WebAclAuthorizer.ts index 97650b0789..cc8db59071 100644 --- a/src/authorization/WebAclAuthorizer.ts +++ b/src/authorization/WebAclAuthorizer.ts @@ -16,11 +16,19 @@ import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError' import { UnauthorizedHttpError } from '../util/errors/UnauthorizedHttpError'; import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy'; import { readableToQuads } from '../util/StreamUtil'; -import { ACL, FOAF } from '../util/Vocabularies'; +import { ACL, RDF } from '../util/Vocabularies'; +import type { AccessChecker } from './access-checkers/AccessChecker'; import type { AuthorizerArgs } from './Authorizer'; import { Authorizer } from './Authorizer'; import { WebAclAuthorization } from './WebAclAuthorization'; +const modesMap: Record = { + [ACL.Read]: 'read', + [ACL.Write]: 'write', + [ACL.Append]: 'append', + [ACL.Control]: 'control', +} as const; + /** * Handles most web access control predicates such as * `acl:mode`, `acl:agentClass`, `acl:agent`, `acl:default` and `acl:accessTo`. @@ -32,13 +40,15 @@ export class WebAclAuthorizer extends Authorizer { private readonly aclStrategy: AuxiliaryIdentifierStrategy; private readonly resourceStore: ResourceStore; private readonly identifierStrategy: IdentifierStrategy; + private readonly accessChecker: AccessChecker; public constructor(aclStrategy: AuxiliaryIdentifierStrategy, resourceStore: ResourceStore, - identifierStrategy: IdentifierStrategy) { + identifierStrategy: IdentifierStrategy, accessChecker: AccessChecker) { super(); this.aclStrategy = aclStrategy; this.resourceStore = resourceStore; this.identifierStrategy = identifierStrategy; + this.accessChecker = accessChecker; } public async canHandle({ identifier }: AuthorizerArgs): Promise { @@ -59,7 +69,7 @@ export class WebAclAuthorizer extends Authorizer { // Determine the full authorization for the agent granted by the applicable ACL const acl = await this.getAclRecursive(identifier); - const authorization = this.createAuthorization(credentials, acl); + const authorization = await this.createAuthorization(credentials, acl); // Verify that the authorization allows all required modes for (const mode of modes) { @@ -82,11 +92,11 @@ export class WebAclAuthorizer extends Authorizer { * @param agent - Agent whose credentials will be used for the `user` field. * @param acl - Store containing all relevant authorization triples. */ - private createAuthorization(agent: Credentials, acl: Store): WebAclAuthorization { - const publicPermissions = this.determinePermissions({}, acl); - const userPermissions = this.determinePermissions(agent, acl); + private async createAuthorization(agent: Credentials, acl: Store): Promise { + const publicPermissions = await this.determinePermissions({}, acl); + const agentPermissions = await this.determinePermissions(agent, acl); - return new WebAclAuthorization(userPermissions, publicPermissions); + return new WebAclAuthorization(agentPermissions, publicPermissions); } /** @@ -94,16 +104,34 @@ export class WebAclAuthorizer extends Authorizer { * @param credentials - Credentials to find the permissions for. * @param acl - Store containing all relevant authorization triples. */ - private determinePermissions(credentials: Credentials, acl: Store): PermissionSet { - const permissions: PermissionSet = { + private async determinePermissions(credentials: Credentials, acl: Store): Promise { + const permissions = { read: false, write: false, append: false, control: false, }; - for (const mode of (Object.keys(permissions) as (keyof PermissionSet)[])) { - permissions[mode] = this.hasPermission(credentials, acl, mode); + + // Apply all ACL rules + const aclRules = acl.getSubjects(RDF.type, ACL.Authorization, null); + for (const rule of aclRules) { + const hasAccess = await this.accessChecker.handleSafe({ acl, rule, credentials }); + if (hasAccess) { + // Set all allowed modes to true + const modes = acl.getObjects(rule, ACL.mode, null); + for (const { value: mode } of modes) { + if (mode in modesMap) { + permissions[modesMap[mode]] = true; + } + } + } + } + + if (permissions.write) { + // Write permission implies Append permission + permissions.append = true; } + return permissions; } @@ -130,75 +158,6 @@ export class WebAclAuthorizer extends Authorizer { } } - /** - * Checks if the given agent has permission to execute the given mode based on the triples in the ACL. - * @param agent - Agent that wants access. - * @param acl - A store containing the relevant triples for authorization. - * @param mode - Which mode is requested. - */ - private hasPermission(agent: Credentials, acl: Store, mode: keyof PermissionSet): boolean { - // Collect all authorization blocks for this specific mode - const modeString = ACL[this.capitalize(mode) as 'Write' | 'Read' | 'Append' | 'Control']; - const auths = this.getModePermissions(acl, modeString); - - // Append permissions are implied by Write permissions - if (modeString === ACL.Append) { - auths.push(...this.getModePermissions(acl, ACL.Write)); - } - - // Check if any collected authorization block allows the specific agent - return auths.some((term): boolean => this.hasAccess(agent, term, acl)); - } - - /** - * Capitalizes the input string. - * @param mode - String to transform. - * - * @returns The capitalized string. - */ - private capitalize(mode: string): string { - return `${mode[0].toUpperCase()}${mode.slice(1).toLowerCase()}`; - } - - /** - * Returns the identifiers of all authorizations that grant the given mode access for a resource. - * @param acl - The store containing the quads of the ACL resource. - * @param aclMode - A valid acl mode (ACL.Write/Read/...) - */ - private getModePermissions(acl: Store, aclMode: string): Term[] { - return acl.getQuads(null, ACL.mode, aclMode, null).map((quad: Quad): Term => quad.subject); - } - - /** - * Checks if the given agent has access to the modes specified by the given authorization. - * @param agent - Credentials of agent that needs access. - * @param auth - acl:Authorization that needs to be checked. - * @param acl - A store containing the relevant triples of the authorization. - * - * @returns If the agent has access. - */ - private hasAccess(agent: Credentials, auth: Term, acl: Store): boolean { - // Check if public access is allowed - if (acl.countQuads(auth, ACL.agentClass, FOAF.Agent, null) !== 0) { - return true; - } - - // Check if authenticated access is allowed - if (this.isAuthenticated(agent)) { - // Check if any authenticated agent is allowed - if (acl.countQuads(auth, ACL.agentClass, ACL.AuthenticatedAgent, null) !== 0) { - return true; - } - // Check if this specific agent is allowed - if (acl.countQuads(auth, ACL.agent, agent.webId, null) !== 0) { - return true; - } - } - - // Neither unauthenticated nor authenticated access are allowed - return false; - } - /** * Returns the ACL triples that are relevant for the given identifier. * These can either be from a corresponding ACL document or an ACL document higher up with defaults. diff --git a/src/authorization/access-checkers/AccessChecker.ts b/src/authorization/access-checkers/AccessChecker.ts new file mode 100644 index 0000000000..30186b785d --- /dev/null +++ b/src/authorization/access-checkers/AccessChecker.ts @@ -0,0 +1,25 @@ +import type { Store, Term } from 'n3'; +import type { Credentials } from '../../authentication/Credentials'; +import { AsyncHandler } from '../../util/handlers/AsyncHandler'; + +/** + * Performs an authorization check against the given acl resource. + */ +export abstract class AccessChecker extends AsyncHandler {} + +export interface AccessCheckerArgs { + /** + * A store containing the relevant triples of the authorization. + */ + acl: Store; + + /** + * Authorization rule to be processed. + */ + rule: Term; + + /** + * Credentials of the entity that wants to use the resource. + */ + credentials: Credentials; +} diff --git a/src/authorization/access-checkers/AgentAccessChecker.ts b/src/authorization/access-checkers/AgentAccessChecker.ts new file mode 100644 index 0000000000..c5d9a6c43c --- /dev/null +++ b/src/authorization/access-checkers/AgentAccessChecker.ts @@ -0,0 +1,15 @@ +import { ACL } from '../../util/Vocabularies'; +import type { AccessCheckerArgs } from './AccessChecker'; +import { AccessChecker } from './AccessChecker'; + +/** + * Checks if the given WebID has been given access. + */ +export class AgentAccessChecker extends AccessChecker { + public async handle({ acl, rule, credentials }: AccessCheckerArgs): Promise { + if (typeof credentials.webId === 'string') { + return acl.countQuads(rule, ACL.terms.agent, credentials.webId, null) !== 0; + } + return false; + } +} diff --git a/src/authorization/access-checkers/AgentClassAccessChecker.ts b/src/authorization/access-checkers/AgentClassAccessChecker.ts new file mode 100644 index 0000000000..61e1620e18 --- /dev/null +++ b/src/authorization/access-checkers/AgentClassAccessChecker.ts @@ -0,0 +1,18 @@ +import { ACL, FOAF } from '../../util/Vocabularies'; +import type { AccessCheckerArgs } from './AccessChecker'; +import { AccessChecker } from './AccessChecker'; + +/** + * Checks access based on the agent class. + */ +export class AgentClassAccessChecker extends AccessChecker { + public async handle({ acl, rule, credentials }: AccessCheckerArgs): Promise { + if (acl.countQuads(rule, ACL.terms.agentClass, FOAF.terms.Agent, null) !== 0) { + return true; + } + if (typeof credentials.webId === 'string') { + return acl.countQuads(rule, ACL.terms.agentClass, ACL.terms.AuthenticatedAgent, null) !== 0; + } + return false; + } +} diff --git a/src/index.ts b/src/index.ts index acb8d16f71..6a9b7ce8ff 100644 --- a/src/index.ts +++ b/src/index.ts @@ -17,6 +17,11 @@ export * from './authorization/PathBasedAuthorizer'; export * from './authorization/WebAclAuthorization'; export * from './authorization/WebAclAuthorizer'; +// Authorization/access-checkers +export * from './authorization/access-checkers/AccessChecker'; +export * from './authorization/access-checkers/AgentAccessChecker'; +export * from './authorization/access-checkers/AgentClassAccessChecker'; + // Identity/Configuration export * from './identity/configuration/IdentityProviderFactory'; export * from './identity/configuration/ProviderFactory'; diff --git a/src/util/Vocabularies.ts b/src/util/Vocabularies.ts index 740567b102..a27bde91e1 100644 --- a/src/util/Vocabularies.ts +++ b/src/util/Vocabularies.ts @@ -60,6 +60,7 @@ export const ACL = createUriAndTermNamespace('http://www.w3.org/ns/auth/acl#', 'agent', 'agentClass', 'AuthenticatedAgent', + 'Authorization', 'default', 'mode', diff --git a/test/unit/authorization/WebAclAuthorizer.test.ts b/test/unit/authorization/WebAclAuthorizer.test.ts index 09590c409d..c72468aca6 100644 --- a/test/unit/authorization/WebAclAuthorizer.test.ts +++ b/test/unit/authorization/WebAclAuthorizer.test.ts @@ -1,5 +1,6 @@ import { namedNode, quad } from '@rdfjs/data-model'; import type { Credentials } from '../../../src/authentication/Credentials'; +import type { AccessChecker } from '../../../src/authorization/access-checkers/AccessChecker'; import { WebAclAuthorization } from '../../../src/authorization/WebAclAuthorization'; import { WebAclAuthorizer } from '../../../src/authorization/WebAclAuthorizer'; import type { AuxiliaryIdentifierStrategy } from '../../../src/ldp/auxiliary/AuxiliaryIdentifierStrategy'; @@ -18,6 +19,7 @@ import { guardedStreamFrom } from '../../../src/util/StreamUtil'; const nn = namedNode; const acl = 'http://www.w3.org/ns/auth/acl#'; +const rdf = 'http://www.w3.org/1999/02/22-rdf-syntax-ns#'; describe('A WebAclAuthorizer', (): void => { let authorizer: WebAclAuthorizer; @@ -26,12 +28,13 @@ describe('A WebAclAuthorizer', (): void => { isAuxiliaryIdentifier: (id: ResourceIdentifier): boolean => id.path.endsWith('.acl'), getAssociatedIdentifier: (id: ResourceIdentifier): ResourceIdentifier => ({ path: id.path.slice(0, -4) }), } as any; - let store: ResourceStore; + let store: jest.Mocked; const identifierStrategy = new SingleRootIdentifierStrategy('http://test.com/'); let permissions: PermissionSet; let credentials: Credentials; let identifier: ResourceIdentifier; let authorization: WebAclAuthorization; + let accessChecker: jest.Mocked; beforeEach(async(): Promise => { permissions = { @@ -60,18 +63,38 @@ describe('A WebAclAuthorizer', (): void => { store = { getRepresentation: jest.fn(), } as any; - authorizer = new WebAclAuthorizer(aclStrategy, store, identifierStrategy); + + accessChecker = { + handleSafe: jest.fn().mockResolvedValue(true), + } as any; + + authorizer = new WebAclAuthorizer(aclStrategy, store, identifierStrategy, accessChecker); }); it('handles all non-acl inputs.', async(): Promise => { - authorizer = new WebAclAuthorizer(aclStrategy, null as any, identifierStrategy); + authorizer = new WebAclAuthorizer(aclStrategy, null as any, identifierStrategy, accessChecker); await expect(authorizer.canHandle({ identifier } as any)).resolves.toBeUndefined(); await expect(authorizer.canHandle({ identifier: aclStrategy.getAuxiliaryIdentifier(identifier) } as any)) .rejects.toThrow(NotImplementedHttpError); }); + it('handles all valid modes and ignores other ones.', async(): Promise => { + credentials.webId = 'http://test.com/user'; + store.getRepresentation.mockResolvedValue({ data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), + quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), + quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), + quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), + quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}fakeMode1`)), + ]) } as Representation); + Object.assign(authorization.everyone, { read: true, write: true, append: true, control: false }); + Object.assign(authorization.user, { read: true, write: true, append: true, control: false }); + await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toEqual(authorization); + }); + it('allows access if the acl file allows all agents.', async(): Promise => { - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ + store.getRepresentation.mockResolvedValue({ data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), quad(nn('auth'), nn(`${acl}agentClass`), nn('http://xmlns.com/foaf/0.1/Agent')), quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), @@ -83,101 +106,71 @@ describe('A WebAclAuthorizer', (): void => { }); it('allows access if there is a parent acl file allowing all agents.', async(): Promise => { - store.getRepresentation = async(id: ResourceIdentifier): Promise => { + store.getRepresentation.mockImplementation(async(id: ResourceIdentifier): Promise => { if (id.path.endsWith('foo.acl')) { throw new NotFoundHttpError(); } return { data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), quad(nn('auth'), nn(`${acl}agentClass`), nn('http://xmlns.com/foaf/0.1/Agent')), quad(nn('auth'), nn(`${acl}default`), nn(identifierStrategy.getParentContainer(identifier).path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), ]), } as Representation; - }; + }); Object.assign(authorization.everyone, { read: true, write: true, append: true }); Object.assign(authorization.user, { read: true, write: true, append: true }); await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toEqual(authorization); }); - it('allows access to authorized agents if the acl files allows all authorized users.', async(): Promise => { - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ - quad(nn('auth'), nn(`${acl}agentClass`), nn(`${acl}AuthenticatedAgent`)), + it('throws a ForbiddenHttpError if access is not granted and credentials have a WebID.', async(): Promise => { + accessChecker.handleSafe.mockResolvedValue(false); + store.getRepresentation.mockResolvedValue({ data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), ]) } as Representation); - credentials.webId = 'http://test.com/user'; - Object.assign(authorization.user, { read: true, write: true, append: true }); - await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toEqual(authorization); + credentials.webId = 'http://test.com/alice/profile/card#me'; + await expect(authorizer.handle({ identifier, permissions, credentials })).rejects.toThrow(ForbiddenHttpError); }); - it('errors if authorization is required but the agent is not authorized.', async(): Promise => { - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ - quad(nn('auth'), nn(`${acl}agentClass`), nn(`${acl}AuthenticatedAgent`)), + it('throws an UnauthorizedHttpError if access is not granted there are no credentials.', async(): Promise => { + accessChecker.handleSafe.mockResolvedValue(false); + store.getRepresentation.mockResolvedValue({ data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), ]) } as Representation); await expect(authorizer.handle({ identifier, permissions, credentials })).rejects.toThrow(UnauthorizedHttpError); }); - it('allows access to specific agents if the acl files identifies them.', async(): Promise => { - credentials.webId = 'http://test.com/user'; - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ - quad(nn('auth'), nn(`${acl}agent`), nn(credentials.webId!)), - quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), - ]) } as Representation); - Object.assign(authorization.user, { read: true, write: true, append: true }); - await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toEqual(authorization); - }); - - it('errors if a specific agents wants to access files not assigned to them.', async(): Promise => { - credentials.webId = 'http://test.com/user'; - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ - quad(nn('auth'), nn(`${acl}agent`), nn('http://test.com/differentUser')), - quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), - quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), - ]) } as Representation); - await expect(authorizer.handle({ identifier, permissions, credentials })).rejects.toThrow(ForbiddenHttpError); - }); - it('re-throws ResourceStore errors as internal errors.', async(): Promise => { - store.getRepresentation = async(): Promise => { - throw new Error('TEST!'); - }; + store.getRepresentation.mockRejectedValue(new Error('TEST!')); const promise = authorizer.handle({ identifier, permissions, credentials }); await expect(promise).rejects.toThrow(`Error reading ACL for ${identifier.path}: TEST!`); await expect(promise).rejects.toThrow(InternalServerError); }); it('errors if the root container has no corresponding acl document.', async(): Promise => { - store.getRepresentation = async(): Promise => { - throw new NotFoundHttpError(); - }; + store.getRepresentation.mockRejectedValue(new NotFoundHttpError()); const promise = authorizer.handle({ identifier, permissions, credentials }); await expect(promise).rejects.toThrow('No ACL document found for root container'); await expect(promise).rejects.toThrow(ForbiddenHttpError); }); it('allows an agent to append if they have write access.', async(): Promise => { - credentials.webId = 'http://test.com/user'; - identifier.path = 'http://test.com/foo'; permissions = { read: false, write: false, append: true, control: false, }; - store.getRepresentation = async(): Promise => ({ data: guardedStreamFrom([ - quad(nn('auth'), nn(`${acl}agent`), nn(credentials.webId!)), + store.getRepresentation.mockResolvedValue({ data: guardedStreamFrom([ + quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), ]) } as Representation); + Object.assign(authorization.everyone, { write: true, append: true }); Object.assign(authorization.user, { write: true, append: true }); await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toEqual(authorization); }); diff --git a/test/unit/authorization/access-checkers/AgentAccessChecker.test.ts b/test/unit/authorization/access-checkers/AgentAccessChecker.test.ts new file mode 100644 index 0000000000..a1b02ad8c6 --- /dev/null +++ b/test/unit/authorization/access-checkers/AgentAccessChecker.test.ts @@ -0,0 +1,32 @@ +import { DataFactory, Store } from 'n3'; +import type { AccessCheckerArgs } from '../../../../src/authorization/access-checkers/AccessChecker'; +import { AgentAccessChecker } from '../../../../src/authorization/access-checkers/AgentAccessChecker'; +import { ACL } from '../../../../src/util/Vocabularies'; +import namedNode = DataFactory.namedNode; + +describe('A AgentAccessChecker', (): void => { + const webId = 'http://test.com/alice/profile/card#me'; + const acl = new Store(); + acl.addQuad(namedNode('match'), ACL.terms.agent, namedNode(webId)); + acl.addQuad(namedNode('noMatch'), ACL.terms.agent, namedNode('http://test.com/bob')); + const checker = new AgentAccessChecker(); + + it('can handle all requests.', async(): Promise => { + await expect(checker.canHandle(null as any)).resolves.toBeUndefined(); + }); + + it('returns true if a match is found for the given WebID.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('match'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(true); + }); + + it('returns false if no match is found.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('noMatch'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(false); + }); + + it('returns false if the credentials contain no WebID.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('match'), credentials: {}}; + await expect(checker.handle(input)).resolves.toBe(false); + }); +}); diff --git a/test/unit/authorization/access-checkers/AgentClassAccessChecker.test.ts b/test/unit/authorization/access-checkers/AgentClassAccessChecker.test.ts new file mode 100644 index 0000000000..b9e6ef285b --- /dev/null +++ b/test/unit/authorization/access-checkers/AgentClassAccessChecker.test.ts @@ -0,0 +1,37 @@ +import { DataFactory, Store } from 'n3'; +import type { AccessCheckerArgs } from '../../../../src/authorization/access-checkers/AccessChecker'; +import { AgentClassAccessChecker } from '../../../../src/authorization/access-checkers/AgentClassAccessChecker'; +import { ACL, FOAF } from '../../../../src/util/Vocabularies'; +import namedNode = DataFactory.namedNode; + +describe('An AgentClassAccessChecker', (): void => { + const webId = 'http://test.com/alice/profile/card#me'; + const acl = new Store(); + acl.addQuad(namedNode('agentMatch'), ACL.terms.agentClass, FOAF.terms.Agent); + acl.addQuad(namedNode('authenticatedMatch'), ACL.terms.agentClass, ACL.terms.AuthenticatedAgent); + const checker = new AgentClassAccessChecker(); + + it('can handle all requests.', async(): Promise => { + await expect(checker.canHandle(null as any)).resolves.toBeUndefined(); + }); + + it('returns true if the rule contains foaf:agent as supported class.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('agentMatch'), credentials: {}}; + await expect(checker.handle(input)).resolves.toBe(true); + }); + + it('returns true for authenticated users with an acl:AuthenticatedAgent rule.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('authenticatedMatch'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(true); + }); + + it('returns false for unauthenticated users with an acl:AuthenticatedAgent rule.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('authenticatedMatch'), credentials: {}}; + await expect(checker.handle(input)).resolves.toBe(false); + }); + + it('returns false if no class rule is found.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('noMatch'), credentials: {}}; + await expect(checker.handle(input)).resolves.toBe(false); + }); +}); From 2511771e2989e03c002dcb63a8dc4c03045a4da6 Mon Sep 17 00:00:00 2001 From: Simone Persiani Date: Tue, 17 Aug 2021 16:56:09 +0200 Subject: [PATCH 5/6] feat: Add support for agentGroup ACL rules Co-Authored-By: Ludovico Granata --- .../access-checkers/agentGroup.json | 10 ++++ config/ldp/authorization/authorizers/acl.json | 6 ++- src/authorization/WebAclAuthorizer.ts | 5 +- .../AgentGroupAccessChecker.ts | 49 ++++++++++++++++++ src/index.ts | 1 + src/util/Vocabularies.ts | 5 ++ .../AgentGroupAccessChecker.test.ts | 50 +++++++++++++++++++ 7 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 config/ldp/authorization/authorizers/access-checkers/agentGroup.json create mode 100644 src/authorization/access-checkers/AgentGroupAccessChecker.ts create mode 100644 test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts diff --git a/config/ldp/authorization/authorizers/access-checkers/agentGroup.json b/config/ldp/authorization/authorizers/access-checkers/agentGroup.json new file mode 100644 index 0000000000..fdc402bb48 --- /dev/null +++ b/config/ldp/authorization/authorizers/access-checkers/agentGroup.json @@ -0,0 +1,10 @@ +{ + "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", + "@graph": [ + { + "@id": "urn:solid-server:default:AgentGroupAccessChecker", + "@type": "AgentGroupAccessChecker", + "converter": { "@id": "urn:solid-server:default:RepresentationConverter" } + } + ] +} diff --git a/config/ldp/authorization/authorizers/acl.json b/config/ldp/authorization/authorizers/acl.json index 7a71a7b541..3e23fad65f 100644 --- a/config/ldp/authorization/authorizers/acl.json +++ b/config/ldp/authorization/authorizers/acl.json @@ -2,7 +2,8 @@ "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", "import": [ "files-scs:config/ldp/authorization/authorizers/access-checkers/agent.json", - "files-scs:config/ldp/authorization/authorizers/access-checkers/agentClass.json" + "files-scs:config/ldp/authorization/authorizers/access-checkers/agentClass.json", + "files-scs:config/ldp/authorization/authorizers/access-checkers/agentGroup.json" ], "@graph": [ { @@ -21,7 +22,8 @@ "@type": "BooleanHandler", "handlers": [ { "@id": "urn:solid-server:default:AgentAccessChecker" }, - { "@id": "urn:solid-server:default:AgentClassAccessChecker" } + { "@id": "urn:solid-server:default:AgentClassAccessChecker" }, + { "@id": "urn:solid-server:default:AgentGroupAccessChecker" } ] } } diff --git a/src/authorization/WebAclAuthorizer.ts b/src/authorization/WebAclAuthorizer.ts index cc8db59071..f41b146562 100644 --- a/src/authorization/WebAclAuthorizer.ts +++ b/src/authorization/WebAclAuthorizer.ts @@ -31,8 +31,9 @@ const modesMap: Record = { /** * Handles most web access control predicates such as - * `acl:mode`, `acl:agentClass`, `acl:agent`, `acl:default` and `acl:accessTo`. - * Does not support `acl:agentGroup`, `acl:origin` and `acl:trustedApp` yet. + * `acl:mode`, `acl:agentClass`, `acl:agent`, `acl:default`, + * `acl:accessTo` and `acl:agentGroup`. + * Does not support `acl:origin` and `acl:trustedApp` yet. */ export class WebAclAuthorizer extends Authorizer { protected readonly logger = getLoggerFor(this); diff --git a/src/authorization/access-checkers/AgentGroupAccessChecker.ts b/src/authorization/access-checkers/AgentGroupAccessChecker.ts new file mode 100644 index 0000000000..66074ebe5f --- /dev/null +++ b/src/authorization/access-checkers/AgentGroupAccessChecker.ts @@ -0,0 +1,49 @@ +import type { Term } from 'n3'; +import type { ResourceIdentifier } from '../../ldp/representation/ResourceIdentifier'; +import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter'; +import { fetchDataset } from '../../util/FetchUtil'; +import { promiseSome } from '../../util/PromiseUtil'; +import { readableToQuads } from '../../util/StreamUtil'; +import { ACL, VCARD } from '../../util/Vocabularies'; +import type { AccessCheckerArgs } from './AccessChecker'; +import { AccessChecker } from './AccessChecker'; + +/** + * Checks if the given WebID belongs to a group that has access. + */ +export class AgentGroupAccessChecker extends AccessChecker { + private readonly converter: RepresentationConverter; + + public constructor(converter: RepresentationConverter) { + super(); + this.converter = converter; + } + + public async handle({ acl, rule, credentials }: AccessCheckerArgs): Promise { + if (typeof credentials.webId === 'string') { + const { webId } = credentials; + const groups = acl.getObjects(rule, ACL.terms.agentGroup, null); + + return await promiseSome(groups.map(async(group: Term): Promise => + this.isMemberOfGroup(webId, group))); + } + return false; + } + + /** + * Checks if the given agent is member of a given vCard group. + * @param webId - WebID of the agent that needs access. + * @param group - URL of the vCard group that needs to be checked. + * + * @returns If the agent is member of the given vCard group. + */ + private async isMemberOfGroup(webId: string, group: Term): Promise { + const groupDocument: ResourceIdentifier = { path: /^[^#]*/u.exec(group.value)![0] }; + + // Fetch the required vCard group file + const dataset = await fetchDataset(groupDocument.path, this.converter); + + const quads = await readableToQuads(dataset.data); + return quads.countQuads(group, VCARD.terms.hasMember, webId, null) !== 0; + } +} diff --git a/src/index.ts b/src/index.ts index 6a9b7ce8ff..3994f8c37c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -21,6 +21,7 @@ export * from './authorization/WebAclAuthorizer'; export * from './authorization/access-checkers/AccessChecker'; export * from './authorization/access-checkers/AgentAccessChecker'; export * from './authorization/access-checkers/AgentClassAccessChecker'; +export * from './authorization/access-checkers/AgentGroupAccessChecker'; // Identity/Configuration export * from './identity/configuration/IdentityProviderFactory'; diff --git a/src/util/Vocabularies.ts b/src/util/Vocabularies.ts index a27bde91e1..060820e22e 100644 --- a/src/util/Vocabularies.ts +++ b/src/util/Vocabularies.ts @@ -59,6 +59,7 @@ export const ACL = createUriAndTermNamespace('http://www.w3.org/ns/auth/acl#', 'accessTo', 'agent', 'agentClass', + 'agentGroup', 'AuthenticatedAgent', 'Authorization', 'default', @@ -144,6 +145,10 @@ export const VANN = createUriAndTermNamespace('http://purl.org/vocab/vann/', 'preferredNamespacePrefix', ); +export const VCARD = createUriAndTermNamespace('http://www.w3.org/2006/vcard/ns#', + 'hasMember', +); + export const XSD = createUriAndTermNamespace('http://www.w3.org/2001/XMLSchema#', 'dateTime', 'integer', diff --git a/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts b/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts new file mode 100644 index 0000000000..826e02d60d --- /dev/null +++ b/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts @@ -0,0 +1,50 @@ +import { DataFactory, Store } from 'n3'; +import type { AccessCheckerArgs } from '../../../../src/authorization/access-checkers/AccessChecker'; +import { AgentGroupAccessChecker } from '../../../../src/authorization/access-checkers/AgentGroupAccessChecker'; +import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation'; +import type { Representation } from '../../../../src/ldp/representation/Representation'; +import type { RepresentationConverter } from '../../../../src/storage/conversion/RepresentationConverter'; +import { INTERNAL_QUADS } from '../../../../src/util/ContentTypes'; +import * as fetchUtil from '../../../../src/util/FetchUtil'; +import { ACL, VCARD } from '../../../../src/util/Vocabularies'; +const { namedNode, quad } = DataFactory; + +describe('An AgentGroupAccessChecker', (): void => { + const webId = 'http://test.com/alice/profile/card#me'; + const groupId = 'http://test.com/group'; + const acl = new Store(); + acl.addQuad(namedNode('groupMatch'), ACL.terms.agentGroup, namedNode(groupId)); + acl.addQuad(namedNode('noMatch'), ACL.terms.agentGroup, namedNode('badGroup')); + let fetchMock: jest.SpyInstance; + let representation: Representation; + const converter: RepresentationConverter = {} as any; + let checker: AgentGroupAccessChecker; + + beforeEach(async(): Promise => { + const groupQuads = [ quad(namedNode(groupId), VCARD.terms.hasMember, namedNode(webId)) ]; + representation = new BasicRepresentation(groupQuads, INTERNAL_QUADS, false); + fetchMock = jest.spyOn(fetchUtil, 'fetchDataset'); + fetchMock.mockResolvedValue(representation); + + checker = new AgentGroupAccessChecker(converter); + }); + + it('can handle all requests.', async(): Promise => { + await expect(checker.canHandle(null as any)).resolves.toBeUndefined(); + }); + + it('returns true if the WebID is a valid group member.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(true); + }); + + it('returns false if the WebID is not a valid group member.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('noMatch'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(false); + }); + + it('returns false if there are no WebID credentials.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: {}}; + await expect(checker.handle(input)).resolves.toBe(false); + }); +}); From d835885dcdc4dc718a926d127bdf0e1f4f047543 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 19 Aug 2021 15:37:51 +0200 Subject: [PATCH 6/6] feat: Add a cache to the AgentGroupAccessChecker --- .../access-checkers/agentGroup.json | 12 +++++- .../AgentGroupAccessChecker.ts | 38 ++++++++++++++++--- .../ownership/TokenOwnershipValidator.ts | 2 +- .../storage/ExpiringAdapterFactory.ts | 10 ++--- src/storage/keyvalue/ExpiringStorage.ts | 15 +++++++- .../keyvalue/WrappedExpiringStorage.ts | 5 ++- .../AgentGroupAccessChecker.test.ts | 14 ++++++- .../storage/ExpiringAdapterFactory.test.ts | 17 ++++----- .../keyvalue/WrappedExpiringStorage.test.ts | 22 +++++++---- 9 files changed, 103 insertions(+), 32 deletions(-) diff --git a/config/ldp/authorization/authorizers/access-checkers/agentGroup.json b/config/ldp/authorization/authorizers/access-checkers/agentGroup.json index fdc402bb48..6f28a234b4 100644 --- a/config/ldp/authorization/authorizers/access-checkers/agentGroup.json +++ b/config/ldp/authorization/authorizers/access-checkers/agentGroup.json @@ -4,7 +4,17 @@ { "@id": "urn:solid-server:default:AgentGroupAccessChecker", "@type": "AgentGroupAccessChecker", - "converter": { "@id": "urn:solid-server:default:RepresentationConverter" } + "converter": { "@id": "urn:solid-server:default:RepresentationConverter" }, + "cache": { + "@id": "urn:solid-server:default:ExpiringAclCache", + "@type": "WrappedExpiringStorage", + "source": { "@type": "MemoryMapStorage" } + } + }, + { + "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", + "@id": "urn:solid-server:default:Finalizer", + "ParallelFinalizer:_finalizers": [ { "@id": "urn:solid-server:default:ExpiringAclCache" } ] } ] } diff --git a/src/authorization/access-checkers/AgentGroupAccessChecker.ts b/src/authorization/access-checkers/AgentGroupAccessChecker.ts index 66074ebe5f..d1a506da0f 100644 --- a/src/authorization/access-checkers/AgentGroupAccessChecker.ts +++ b/src/authorization/access-checkers/AgentGroupAccessChecker.ts @@ -1,6 +1,8 @@ -import type { Term } from 'n3'; +import type { Store, Term } from 'n3'; + import type { ResourceIdentifier } from '../../ldp/representation/ResourceIdentifier'; import type { RepresentationConverter } from '../../storage/conversion/RepresentationConverter'; +import type { ExpiringStorage } from '../../storage/keyvalue/ExpiringStorage'; import { fetchDataset } from '../../util/FetchUtil'; import { promiseSome } from '../../util/PromiseUtil'; import { readableToQuads } from '../../util/StreamUtil'; @@ -10,13 +12,24 @@ import { AccessChecker } from './AccessChecker'; /** * Checks if the given WebID belongs to a group that has access. + * + * Fetched results will be stored in an ExpiringStorage. + * + * Requires a storage that can store JS objects. + * `expiration` parameter is how long entries in the cache should be stored in minutes, defaults to 60. */ export class AgentGroupAccessChecker extends AccessChecker { private readonly converter: RepresentationConverter; + private readonly cache: ExpiringStorage>; + private readonly expiration: number; - public constructor(converter: RepresentationConverter) { + public constructor(converter: RepresentationConverter, cache: ExpiringStorage>, + expiration = 60) { super(); this.converter = converter; + this.cache = cache; + // Convert minutes to milliseconds + this.expiration = expiration * 60 * 1000; } public async handle({ acl, rule, credentials }: AccessCheckerArgs): Promise { @@ -41,9 +54,24 @@ export class AgentGroupAccessChecker extends AccessChecker { const groupDocument: ResourceIdentifier = { path: /^[^#]*/u.exec(group.value)![0] }; // Fetch the required vCard group file - const dataset = await fetchDataset(groupDocument.path, this.converter); - - const quads = await readableToQuads(dataset.data); + const quads = await this.fetchCachedQuads(groupDocument.path); return quads.countQuads(group, VCARD.terms.hasMember, webId, null) !== 0; } + + /** + * Fetches quads from the given URL. + * Will cache the values for later re-use. + */ + private async fetchCachedQuads(url: string): Promise { + let result = await this.cache.get(url); + if (!result) { + const prom = (async(): Promise => { + const representation = await fetchDataset(url, this.converter); + return readableToQuads(representation.data); + })(); + await this.cache.set(url, prom, this.expiration); + result = await prom; + } + return result; + } } diff --git a/src/identity/ownership/TokenOwnershipValidator.ts b/src/identity/ownership/TokenOwnershipValidator.ts index 6ee1aca3c6..9447a93e5d 100644 --- a/src/identity/ownership/TokenOwnershipValidator.ts +++ b/src/identity/ownership/TokenOwnershipValidator.ts @@ -36,7 +36,7 @@ export class TokenOwnershipValidator extends OwnershipValidator { // No reason to fetch the WebId if we don't have a token yet if (!token) { token = this.generateToken(); - await this.storage.set(key, token, new Date(Date.now() + this.expiration)); + await this.storage.set(key, token, this.expiration); this.throwError(webId, token); } diff --git a/src/identity/storage/ExpiringAdapterFactory.ts b/src/identity/storage/ExpiringAdapterFactory.ts index 24d7028b60..d540ab7c5b 100644 --- a/src/identity/storage/ExpiringAdapterFactory.ts +++ b/src/identity/storage/ExpiringAdapterFactory.ts @@ -43,13 +43,13 @@ export class ExpiringAdapter implements Adapter { public async upsert(id: string, payload: AdapterPayload, expiresIn?: number): Promise { // Despite what the typings say, `expiresIn` can be undefined - const expires = expiresIn ? new Date(Date.now() + (expiresIn * 1000)) : undefined; + const expiration = expiresIn ? expiresIn * 1000 : undefined; const key = this.keyFor(id); this.logger.debug(`Storing payload data for ${id}`); const storagePromises: Promise[] = [ - this.storage.set(key, payload, expires), + this.storage.set(key, payload, expiration), ]; if (payload.grantId) { storagePromises.push( @@ -57,15 +57,15 @@ export class ExpiringAdapter implements Adapter { const grantKey = this.grantKeyFor(payload.grantId!); const grants = (await this.storage.get(grantKey) || []) as string[]; grants.push(key); - await this.storage.set(grantKey, grants, expires); + await this.storage.set(grantKey, grants, expiration); })(), ); } if (payload.userCode) { - storagePromises.push(this.storage.set(this.userCodeKeyFor(payload.userCode), id, expires)); + storagePromises.push(this.storage.set(this.userCodeKeyFor(payload.userCode), id, expiration)); } if (payload.uid) { - storagePromises.push(this.storage.set(this.uidKeyFor(payload.uid), id, expires)); + storagePromises.push(this.storage.set(this.uidKeyFor(payload.uid), id, expiration)); } await Promise.all(storagePromises); } diff --git a/src/storage/keyvalue/ExpiringStorage.ts b/src/storage/keyvalue/ExpiringStorage.ts index ac53e8faf9..ab83d8d42e 100644 --- a/src/storage/keyvalue/ExpiringStorage.ts +++ b/src/storage/keyvalue/ExpiringStorage.ts @@ -1,10 +1,23 @@ import type { KeyValueStorage } from './KeyValueStorage'; +/* eslint-disable @typescript-eslint/method-signature-style */ /** * A KeyValueStorage in which the values can expire. * Entries with no expiration date never expire. */ export interface ExpiringStorage extends KeyValueStorage { + /** + * Sets the value for the given key. + * Should error if the data is already expired. + * + * @param key - Key to set/update. + * @param value - Value to store. + * @param expiration - How long this data should stay valid in milliseconds. + * + * @returns The storage. + */ + set(key: TKey, value: TValue, expiration?: number): Promise; + /** * Sets the value for the given key. * Should error if the data is already expired. @@ -15,5 +28,5 @@ export interface ExpiringStorage extends KeyValueStorage Promise; + set(key: TKey, value: TValue, expires?: Date): Promise; } diff --git a/src/storage/keyvalue/WrappedExpiringStorage.ts b/src/storage/keyvalue/WrappedExpiringStorage.ts index 5fffdf48a3..6ac9cc3c8d 100644 --- a/src/storage/keyvalue/WrappedExpiringStorage.ts +++ b/src/storage/keyvalue/WrappedExpiringStorage.ts @@ -34,7 +34,10 @@ export class WrappedExpiringStorage implements ExpiringStorage { + public async set(key: TKey, value: TValue, expiration?: number): Promise; + public async set(key: TKey, value: TValue, expires?: Date): Promise; + public async set(key: TKey, value: TValue, expireValue?: number | Date): Promise { + const expires = typeof expireValue === 'number' ? new Date(Date.now() + expireValue) : expireValue; if (this.isExpired(expires)) { throw new InternalServerError('Value is already expired'); } diff --git a/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts b/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts index 826e02d60d..363a96fc0d 100644 --- a/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts +++ b/test/unit/authorization/access-checkers/AgentGroupAccessChecker.test.ts @@ -4,6 +4,7 @@ import { AgentGroupAccessChecker } from '../../../../src/authorization/access-ch import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation'; import type { Representation } from '../../../../src/ldp/representation/Representation'; import type { RepresentationConverter } from '../../../../src/storage/conversion/RepresentationConverter'; +import type { ExpiringStorage } from '../../../../src/storage/keyvalue/ExpiringStorage'; import { INTERNAL_QUADS } from '../../../../src/util/ContentTypes'; import * as fetchUtil from '../../../../src/util/FetchUtil'; import { ACL, VCARD } from '../../../../src/util/Vocabularies'; @@ -18,6 +19,7 @@ describe('An AgentGroupAccessChecker', (): void => { let fetchMock: jest.SpyInstance; let representation: Representation; const converter: RepresentationConverter = {} as any; + let cache: ExpiringStorage>; let checker: AgentGroupAccessChecker; beforeEach(async(): Promise => { @@ -25,8 +27,11 @@ describe('An AgentGroupAccessChecker', (): void => { representation = new BasicRepresentation(groupQuads, INTERNAL_QUADS, false); fetchMock = jest.spyOn(fetchUtil, 'fetchDataset'); fetchMock.mockResolvedValue(representation); + fetchMock.mockClear(); - checker = new AgentGroupAccessChecker(converter); + cache = new Map() as any; + + checker = new AgentGroupAccessChecker(converter, cache); }); it('can handle all requests.', async(): Promise => { @@ -47,4 +52,11 @@ describe('An AgentGroupAccessChecker', (): void => { const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: {}}; await expect(checker.handle(input)).resolves.toBe(false); }); + + it('caches fetched results.', async(): Promise => { + const input: AccessCheckerArgs = { acl, rule: namedNode('groupMatch'), credentials: { webId }}; + await expect(checker.handle(input)).resolves.toBe(true); + await expect(checker.handle(input)).resolves.toBe(true); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); }); diff --git a/test/unit/identity/storage/ExpiringAdapterFactory.test.ts b/test/unit/identity/storage/ExpiringAdapterFactory.test.ts index 6f2a50414a..58d1010e4d 100644 --- a/test/unit/identity/storage/ExpiringAdapterFactory.test.ts +++ b/test/unit/identity/storage/ExpiringAdapterFactory.test.ts @@ -15,8 +15,7 @@ describe('An ExpiringAdapterFactory', (): void => { let storage: ExpiringStorage; let adapter: ExpiringAdapter; let factory: ExpiringAdapterFactory; - const expiresIn = 333; - const expireDate = new Date(Date.now() + (expiresIn * 1000)); + const expiresIn = 333 * 1000; beforeEach(async(): Promise => { payload = { data: 'data!' }; @@ -35,7 +34,7 @@ describe('An ExpiringAdapterFactory', (): void => { it('can find payload by id.', async(): Promise => { await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined(); expect(storage.set).toHaveBeenCalledTimes(1); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn); await expect(adapter.find(id)).resolves.toBe(payload); }); @@ -50,8 +49,8 @@ describe('An ExpiringAdapterFactory', (): void => { payload.userCode = userCode; await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined(); expect(storage.set).toHaveBeenCalledTimes(2); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expireDate); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expiresIn); await expect(adapter.findByUserCode(userCode)).resolves.toBe(payload); }); @@ -60,8 +59,8 @@ describe('An ExpiringAdapterFactory', (): void => { payload.uid = uid; await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined(); expect(storage.set).toHaveBeenCalledTimes(2); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expireDate); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), id, expiresIn); await expect(adapter.findByUid(uid)).resolves.toBe(payload); }); @@ -69,8 +68,8 @@ describe('An ExpiringAdapterFactory', (): void => { payload.grantId = grantId; await expect(adapter.upsert(id, payload, 333)).resolves.toBeUndefined(); expect(storage.set).toHaveBeenCalledTimes(2); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expireDate); - expect(storage.set).toHaveBeenCalledWith(expect.anything(), [ expect.anything() ], expireDate); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), payload, expiresIn); + expect(storage.set).toHaveBeenCalledWith(expect.anything(), [ expect.anything() ], expiresIn); await expect(adapter.find(id)).resolves.toBe(payload); await expect(adapter.revokeByGrantId(grantId)).resolves.toBeUndefined(); expect(storage.delete).toHaveBeenCalledTimes(2); diff --git a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts index 729473d49d..b531edb775 100644 --- a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts +++ b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts @@ -17,7 +17,7 @@ describe('A WrappedExpiringStorage', (): void => { tomorrow.setDate(tomorrow.getDate() + 1); const yesterday = new Date(); yesterday.setDate(yesterday.getDate() - 1); - let source: KeyValueStorage; + let source: jest.Mocked>; let storage: WrappedExpiringStorage; beforeEach(async(): Promise => { @@ -42,12 +42,12 @@ describe('A WrappedExpiringStorage', (): void => { }); it('returns data if it has not expired.', async(): Promise => { - (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', tomorrow)); + source.get.mockResolvedValueOnce(createExpires('data!', tomorrow)); await expect(storage.get('key')).resolves.toEqual('data!'); }); it('deletes expired data when trying to get it.', async(): Promise => { - (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', yesterday)); + source.get.mockResolvedValueOnce(createExpires('data!', yesterday)); await expect(storage.get('key')).resolves.toBeUndefined(); expect(source.delete).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenLastCalledWith('key'); @@ -60,12 +60,12 @@ describe('A WrappedExpiringStorage', (): void => { }); it('true on `has` checks if there is non-expired data.', async(): Promise => { - (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', tomorrow)); + source.get.mockResolvedValueOnce(createExpires('data!', tomorrow)); await expect(storage.has('key')).resolves.toBe(true); }); it('deletes expired data when checking if it exists.', async(): Promise => { - (source.get as jest.Mock).mockResolvedValueOnce(createExpires('data!', yesterday)); + source.get.mockResolvedValueOnce(createExpires('data!', yesterday)); await expect(storage.has('key')).resolves.toBe(false); expect(source.delete).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenLastCalledWith('key'); @@ -77,6 +77,12 @@ describe('A WrappedExpiringStorage', (): void => { expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow)); }); + it('can store data with an expiration duration.', async(): Promise => { + await storage.set('key', 'data!', tomorrow.getTime() - Date.now()); + expect(source.set).toHaveBeenCalledTimes(1); + expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow)); + }); + it('can store data without expiry date.', async(): Promise => { await storage.set('key', 'data!'); expect(source.set).toHaveBeenCalledTimes(1); @@ -99,7 +105,7 @@ describe('A WrappedExpiringStorage', (): void => { [ 'key2', createExpires('data2', yesterday) ], [ 'key3', createExpires('data3') ], ]; - (source.entries as jest.Mock).mockImplementationOnce(function* (): any { + source.entries.mockImplementationOnce(function* (): any { yield* data; }); const it = storage.entries(); @@ -123,7 +129,7 @@ describe('A WrappedExpiringStorage', (): void => { [ 'key2', createExpires('data2', yesterday) ], [ 'key3', createExpires('data3') ], ]; - (source.entries as jest.Mock).mockImplementationOnce(function* (): any { + source.entries.mockImplementationOnce(function* (): any { yield* data; }); @@ -150,7 +156,7 @@ describe('A WrappedExpiringStorage', (): void => { [ 'key2', createExpires('data2', yesterday) ], [ 'key3', createExpires('data3') ], ]; - (source.entries as jest.Mock).mockImplementationOnce(function* (): any { + source.entries.mockImplementationOnce(function* (): any { yield* data; });