From 0d42987bbd2d68bfbc81e5270e16870d994f322e Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 16 Aug 2021 17:35:23 +0200 Subject: [PATCH] feat: Verify conditions in DataAccessorBasedStore --- src/index.ts | 1 + src/storage/DataAccessorBasedStore.ts | 52 +++++++++++++++--- src/storage/PatchingStore.ts | 8 ++- .../errors/PreconditionFailedHttpError.ts | 15 ++++++ .../storage/DataAccessorBasedStore.test.ts | 54 +++++++++++++++++-- test/unit/storage/PatchingStore.test.ts | 17 ++++-- test/unit/util/errors/HttpError.test.ts | 3 +- 7 files changed, 133 insertions(+), 17 deletions(-) create mode 100644 src/util/errors/PreconditionFailedHttpError.ts diff --git a/src/index.ts b/src/index.ts index e890e2e06..c5dc0f233 100644 --- a/src/index.ts +++ b/src/index.ts @@ -285,6 +285,7 @@ export * from './util/errors/InternalServerError'; export * from './util/errors/MethodNotAllowedHttpError'; export * from './util/errors/NotFoundHttpError'; export * from './util/errors/NotImplementedHttpError'; +export * from './util/errors/PreconditionFailedHttpError'; export * from './util/errors/SystemError'; export * from './util/errors/UnauthorizedHttpError'; export * from './util/errors/UnsupportedMediaTypeHttpError'; diff --git a/src/storage/DataAccessorBasedStore.ts b/src/storage/DataAccessorBasedStore.ts index 08baa6be9..fd7b3b1ba 100644 --- a/src/storage/DataAccessorBasedStore.ts +++ b/src/storage/DataAccessorBasedStore.ts @@ -3,6 +3,7 @@ import { DataFactory } from 'n3'; import type { NamedNode, Quad, Term } from 'rdf-js'; import { v4 as uuid } from 'uuid'; import type { AuxiliaryStrategy } from '../ldp/auxiliary/AuxiliaryStrategy'; +import type { Patch } from '../ldp/http/Patch'; import { BasicRepresentation } from '../ldp/representation/BasicRepresentation'; import type { Representation } from '../ldp/representation/Representation'; import type { RepresentationMetadata } from '../ldp/representation/RepresentationMetadata'; @@ -16,6 +17,7 @@ import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError'; import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; +import { PreconditionFailedHttpError } from '../util/errors/PreconditionFailedHttpError'; import type { IdentifierStrategy } from '../util/identifiers/IdentifierStrategy'; import { ensureTrailingSlash, @@ -39,6 +41,7 @@ import { PREFERRED_PREFIX_TERM, } from '../util/Vocabularies'; import type { DataAccessor } from './accessors/DataAccessor'; +import type { Conditions } from './Conditions'; import type { ResourceStore } from './ResourceStore'; /** @@ -134,7 +137,8 @@ export class DataAccessorBasedStore implements ResourceStore { return representation; } - public async addResource(container: ResourceIdentifier, representation: Representation): Promise { + public async addResource(container: ResourceIdentifier, representation: Representation, conditions?: Conditions): + Promise { this.validateIdentifier(container); // Ensure the representation is supported by the accessor @@ -153,10 +157,12 @@ export class DataAccessorBasedStore implements ResourceStore { // Solid, §5: "Servers MUST respond with the 405 status code to requests using HTTP methods // that are not supported by the target resource." // https://solid.github.io/specification/protocol#reading-writing-resources - if (parentMetadata && !isContainerPath(parentMetadata.identifier.value)) { + if (!isContainerPath(parentMetadata.identifier.value)) { throw new MethodNotAllowedHttpError('The given path is not a container.'); } + this.validateConditions(conditions, parentMetadata); + // Solid, §5.1: "Servers MAY allow clients to suggest the URI of a resource created through POST, // using the HTTP Slug header as defined in [RFC5023]. // Clients who want the server to assign a URI of a resource, MUST use the POST request." @@ -169,8 +175,8 @@ export class DataAccessorBasedStore implements ResourceStore { return newID; } - public async setRepresentation(identifier: ResourceIdentifier, representation: Representation): - Promise { + public async setRepresentation(identifier: ResourceIdentifier, representation: Representation, + conditions?: Conditions): Promise { this.validateIdentifier(identifier); // Ensure the representation is supported by the accessor @@ -196,15 +202,31 @@ export class DataAccessorBasedStore implements ResourceStore { throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.'); } + this.validateConditions(conditions, oldMetadata); + // Potentially have to create containers if it didn't exist yet return this.writeData(identifier, representation, isContainer, !oldMetadata, Boolean(oldMetadata)); } - public async modifyResource(): Promise { + public async modifyResource(identifier: ResourceIdentifier, patch: Patch, + conditions?: Conditions): Promise { + if (conditions) { + let metadata: RepresentationMetadata | undefined; + try { + metadata = await this.accessor.getMetadata(identifier); + } catch (error: unknown) { + if (!NotFoundHttpError.isInstance(error)) { + throw error; + } + } + + this.validateConditions(conditions, metadata); + } + throw new NotImplementedHttpError('Patches are not supported by the default store.'); } - public async deleteResource(identifier: ResourceIdentifier): Promise { + public async deleteResource(identifier: ResourceIdentifier, conditions?: Conditions): Promise { this.validateIdentifier(identifier); const metadata = await this.accessor.getMetadata(identifier); // Solid, §5.4: "When a DELETE request targets storage’s root container or its associated ACL resource, @@ -229,8 +251,11 @@ export class DataAccessorBasedStore implements ResourceStore { if (isContainerIdentifier(identifier) && await this.hasProperChildren(identifier)) { throw new ConflictHttpError('Can only delete empty containers.'); } - // Solid, §5.4: "When a contained resource is deleted, the server MUST also delete the associated auxiliary - // resources" + + this.validateConditions(conditions, metadata); + + // Solid, §5.4: "When a contained resource is deleted, + // the server MUST also delete the associated auxiliary resources" // https://solid.github.io/specification/protocol#deleting-resources const deleted = [ identifier ]; if (!this.auxiliaryStrategy.isAuxiliaryIdentifier(identifier)) { @@ -261,6 +286,17 @@ export class DataAccessorBasedStore implements ResourceStore { } } + /** + * Verify if the given metadata matches the conditions. + */ + protected validateConditions(conditions?: Conditions, metadata?: RepresentationMetadata): void { + // The 412 (Precondition Failed) status code indicates + // that one or more conditions given in the request header fields evaluated to false when tested on the server. + if (conditions && !conditions.matchesMetadata(metadata)) { + throw new PreconditionFailedHttpError(); + } + } + /** * Returns the metadata matching the identifier, ignoring the presence of a trailing slash or not. * diff --git a/src/storage/PatchingStore.ts b/src/storage/PatchingStore.ts index d9770865b..b256867bf 100644 --- a/src/storage/PatchingStore.ts +++ b/src/storage/PatchingStore.ts @@ -1,5 +1,6 @@ import type { Patch } from '../ldp/http/Patch'; import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifier'; +import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; import type { Conditions } from './Conditions'; import { PassthroughStore } from './PassthroughStore'; import type { PatchHandler } from './patch/PatchHandler'; @@ -22,8 +23,11 @@ export class PatchingStore extends Pass conditions?: Conditions): Promise { try { return await this.source.modifyResource(identifier, patch, conditions); - } catch { - return this.patcher.handleSafe({ source: this.source, identifier, patch }); + } catch (error: unknown) { + if (NotImplementedHttpError.isInstance(error)) { + return this.patcher.handleSafe({ source: this.source, identifier, patch }); + } + throw error; } } } diff --git a/src/util/errors/PreconditionFailedHttpError.ts b/src/util/errors/PreconditionFailedHttpError.ts new file mode 100644 index 000000000..3e63e4e86 --- /dev/null +++ b/src/util/errors/PreconditionFailedHttpError.ts @@ -0,0 +1,15 @@ +import type { HttpErrorOptions } from './HttpError'; +import { HttpError } from './HttpError'; + +/** + * An error thrown when access was denied due to the conditions on the request. + */ +export class PreconditionFailedHttpError extends HttpError { + public constructor(message?: string, options?: HttpErrorOptions) { + super(412, 'PreconditionFailedHttpError', message, options); + } + + public static isInstance(error: any): error is PreconditionFailedHttpError { + return HttpError.isInstance(error) && error.statusCode === 412; + } +} diff --git a/test/unit/storage/DataAccessorBasedStore.test.ts b/test/unit/storage/DataAccessorBasedStore.test.ts index cd799399a..8aba035b5 100644 --- a/test/unit/storage/DataAccessorBasedStore.test.ts +++ b/test/unit/storage/DataAccessorBasedStore.test.ts @@ -9,6 +9,7 @@ import type { Representation } from '../../../src/ldp/representation/Representat import { RepresentationMetadata } from '../../../src/ldp/representation/RepresentationMetadata'; import type { ResourceIdentifier } from '../../../src/ldp/representation/ResourceIdentifier'; import type { DataAccessor } from '../../../src/storage/accessors/DataAccessor'; +import { BasicConditions } from '../../../src/storage/BasicConditions'; import { DataAccessorBasedStore } from '../../../src/storage/DataAccessorBasedStore'; import { INTERNAL_QUADS } from '../../../src/util/ContentTypes'; import { BadRequestHttpError } from '../../../src/util/errors/BadRequestHttpError'; @@ -17,6 +18,7 @@ import { ForbiddenHttpError } from '../../../src/util/errors/ForbiddenHttpError' import { MethodNotAllowedHttpError } from '../../../src/util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; +import { PreconditionFailedHttpError } from '../../../src/util/errors/PreconditionFailedHttpError'; import type { Guarded } from '../../../src/util/GuardedStream'; import { SingleRootIdentifierStrategy } from '../../../src/util/identifiers/SingleRootIdentifierStrategy'; import { trimTrailingSlashes } from '../../../src/util/PathUtil'; @@ -117,8 +119,8 @@ class SimpleSuffixStrategy implements AuxiliaryStrategy { } describe('A DataAccessorBasedStore', (): void => { - const now = new Date(1234567489); - const later = new Date(987654321); + const now = new Date(2020, 5, 12); + const later = new Date(2021, 6, 13); let mockDate: jest.SpyInstance; let store: DataAccessorBasedStore; let accessor: SimpleDataAccessor; @@ -233,6 +235,13 @@ describe('A DataAccessorBasedStore', (): void => { await expect(result).rejects.toThrow('The given path is not a container.'); }); + it('throws a 412 if the conditions are not matched.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.addResource(resourceID, representation, conditions)) + .rejects.toThrow(PreconditionFailedHttpError); + }); + it('errors when trying to create a container with non-RDF data.', async(): Promise => { const resourceID = { path: root }; representation.metadata.add(RDF.type, LDP.terms.Container); @@ -341,6 +350,13 @@ describe('A DataAccessorBasedStore', (): void => { await expect(prom).rejects.toThrow(ForbiddenHttpError); }); + it('throws a 412 if the conditions are not matched.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.setRepresentation(resourceID, representation, conditions)) + .rejects.toThrow(PreconditionFailedHttpError); + }); + // As discussed in #475, trimming the trailing slash of a root container in getNormalizedMetadata // can result in undefined behaviour since there is no parent container. it('will not trim the slash of root containers since there is no parent.', async(): Promise => { @@ -527,8 +543,33 @@ describe('A DataAccessorBasedStore', (): void => { }); describe('modifying a Representation', (): void => { + it('throws a 412 if the conditions are not matched.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.modifyResource(resourceID, representation, conditions)) + .rejects.toThrow(PreconditionFailedHttpError); + }); + + it('throws a 412 if the conditions are not matched on resources that do not exist.', async(): Promise => { + const resourceID = { path: `${root}notHere` }; + const conditions = new BasicConditions({ matchesETag: [ '*' ]}); + await expect(store.modifyResource(resourceID, representation, conditions)) + .rejects.toThrow(PreconditionFailedHttpError); + }); + + it('re-throws the error if something goes wrong accessing the metadata.', async(): Promise => { + accessor.getMetadata = jest.fn(async(): Promise => { + throw new Error('error'); + }); + + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.modifyResource(resourceID, representation, conditions)) + .rejects.toThrow('error'); + }); + it('is not supported.', async(): Promise => { - const result = store.modifyResource(); + const result = store.modifyResource({ path: root }, representation); await expect(result).rejects.toThrow(NotImplementedHttpError); await expect(result).rejects.toThrow('Patches are not supported by the default store.'); }); @@ -569,6 +610,13 @@ describe('A DataAccessorBasedStore', (): void => { await expect(result).rejects.toThrow('Can only delete empty containers.'); }); + it('throws a 412 if the conditions are not matched.', async(): Promise => { + const resourceID = { path: root }; + const conditions = new BasicConditions({ notMatchesETag: [ '*' ]}); + await expect(store.deleteResource(resourceID, conditions)) + .rejects.toThrow(PreconditionFailedHttpError); + }); + it('will delete resources.', async(): Promise => { accessor.data[`${root}resource`] = representation; await expect(store.deleteResource({ path: `${root}resource` })).resolves.toEqual([ diff --git a/test/unit/storage/PatchingStore.test.ts b/test/unit/storage/PatchingStore.test.ts index f77a8e62e..6857975ad 100644 --- a/test/unit/storage/PatchingStore.test.ts +++ b/test/unit/storage/PatchingStore.test.ts @@ -2,6 +2,7 @@ import type { Patch } from '../../../src/ldp/http/Patch'; import type { PatchHandler } from '../../../src/storage/patch/PatchHandler'; import { PatchingStore } from '../../../src/storage/PatchingStore'; import type { ResourceStore } from '../../../src/storage/ResourceStore'; +import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; describe('A PatchingStore', (): void => { let store: PatchingStore; @@ -26,15 +27,25 @@ describe('A PatchingStore', (): void => { expect(source.modifyResource).toHaveBeenLastCalledWith({ path: 'modifyPath' }, {}, undefined); }); - it('calls its patcher if modifyResource failed.', async(): Promise => { + it('calls its patcher if modifyResource is not implemented.', async(): Promise => { source.modifyResource = jest.fn(async(): Promise => { - throw new Error('dummy'); + throw new NotImplementedHttpError(); }); await expect(store.modifyResource({ path: 'modifyPath' }, {} as Patch)).resolves.toBe('patcher'); expect(source.modifyResource).toHaveBeenCalledTimes(1); expect(source.modifyResource).toHaveBeenLastCalledWith({ path: 'modifyPath' }, {}, undefined); - await expect((source.modifyResource as jest.Mock).mock.results[0].value).rejects.toThrow('dummy'); + await expect((source.modifyResource as jest.Mock).mock.results[0].value).rejects.toThrow(NotImplementedHttpError); expect(handleSafeFn).toHaveBeenCalledTimes(1); expect(handleSafeFn).toHaveBeenLastCalledWith({ source, identifier: { path: 'modifyPath' }, patch: {}}); }); + + it('rethrows source modifyResource errors.', async(): Promise => { + source.modifyResource = jest.fn(async(): Promise => { + throw new Error('dummy'); + }); + await expect(store.modifyResource({ path: 'modifyPath' }, {} as Patch)).rejects.toThrow('dummy'); + expect(source.modifyResource).toHaveBeenCalledTimes(1); + expect(source.modifyResource).toHaveBeenLastCalledWith({ path: 'modifyPath' }, {}, undefined); + expect(handleSafeFn).toHaveBeenCalledTimes(0); + }); }); diff --git a/test/unit/util/errors/HttpError.test.ts b/test/unit/util/errors/HttpError.test.ts index e263e849a..5853df215 100644 --- a/test/unit/util/errors/HttpError.test.ts +++ b/test/unit/util/errors/HttpError.test.ts @@ -7,6 +7,7 @@ import { InternalServerError } from '../../../../src/util/errors/InternalServerE import { MethodNotAllowedHttpError } from '../../../../src/util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError'; +import { PreconditionFailedHttpError } from '../../../../src/util/errors/PreconditionFailedHttpError'; import { UnauthorizedHttpError } from '../../../../src/util/errors/UnauthorizedHttpError'; import { UnsupportedMediaTypeHttpError } from '../../../../src/util/errors/UnsupportedMediaTypeHttpError'; @@ -25,7 +26,7 @@ describe('HttpError', (): void => { [ 'NotFoundHttpError', 404, NotFoundHttpError ], [ 'MethodNotAllowedHttpError', 405, MethodNotAllowedHttpError ], [ 'ConflictHttpError', 409, ConflictHttpError ], - [ 'MethodNotAllowedHttpError', 405, MethodNotAllowedHttpError ], + [ 'PreconditionFailedHttpError', 412, PreconditionFailedHttpError ], [ 'UnsupportedMediaTypeHttpError', 415, UnsupportedMediaTypeHttpError ], [ 'InternalServerError', 500, InternalServerError ], [ 'NotImplementedHttpError', 501, NotImplementedHttpError ],