From 50e3cf5036b7e42f83d992518c16a5064bfd0ca7 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 14 Jan 2021 11:59:24 +0100 Subject: [PATCH] fix: Throw correct errors and streamline in DataAccessorBasedStore --- src/storage/DataAccessorBasedStore.ts | 113 ++++++++++-------- .../storage/DataAccessorBasedStore.test.ts | 39 ++---- 2 files changed, 77 insertions(+), 75 deletions(-) diff --git a/src/storage/DataAccessorBasedStore.ts b/src/storage/DataAccessorBasedStore.ts index fafc07484..c2705a946 100644 --- a/src/storage/DataAccessorBasedStore.ts +++ b/src/storage/DataAccessorBasedStore.ts @@ -9,7 +9,7 @@ import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifie import { INTERNAL_QUADS } from '../util/ContentTypes'; import { BadRequestHttpError } from '../util/errors/BadRequestHttpError'; import { ConflictHttpError } from '../util/errors/ConflictHttpError'; -import { InternalServerError } from '../util/errors/InternalServerError'; +import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError'; import { MethodNotAllowedHttpError } from '../util/errors/MethodNotAllowedHttpError'; import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; @@ -65,7 +65,7 @@ export class DataAccessorBasedStore implements ResourceStore { const metadata = await this.accessor.getMetadata(identifier); let representation: Representation; - if (this.isExistingContainer(metadata)) { + if (isContainerPath(metadata.identifier.value)) { // Generate a container representation from the metadata const data = metadata.quads(); metadata.addQuad(DC.terms.namespace, VANN.terms.preferredNamespacePrefix, 'dc'); @@ -86,23 +86,31 @@ export class DataAccessorBasedStore implements ResourceStore { // Ensure the representation is supported by the accessor await this.accessor.canHandle(representation); - // Using the parent metadata as we can also use that later to check if the nested containers maybe need to be made const parentMetadata = await this.getSafeNormalizedMetadata(container); - // When a POST method request targets a resource without an existing representation, - // the server MUST respond with the 404 status code. + // Solid, §5.3: "When a POST method request targets a resource without an existing representation, + // the server MUST respond with the 404 status code." + // https://solid.github.io/specification/protocol#writing-resources if (!parentMetadata) { throw new NotFoundHttpError(); } - if (parentMetadata && !this.isExistingContainer(parentMetadata)) { + // Not using `container` since `getSafeNormalizedMetadata` might return metadata for a different identifier. + // 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)) { throw new MethodNotAllowedHttpError('The given path is not a container.'); } + // 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." + // https://solid.github.io/specification/protocol#resource-type-heuristics const newID = this.createSafeUri(container, representation.metadata, parentMetadata); - // Write the data. New containers will need to be created if there is no parent. - await this.writeData(newID, representation, isContainerIdentifier(newID), !parentMetadata); + // Write the data. New containers should never be made for a POST request. + await this.writeData(newID, representation, isContainerIdentifier(newID), false); return newID; } @@ -116,16 +124,19 @@ export class DataAccessorBasedStore implements ResourceStore { // Check if the resource already exists const oldMetadata = await this.getSafeNormalizedMetadata(identifier); - // Might want to redirect in the future + // Might want to redirect in the future. + // See #480 + // Solid, §3.1: "If two URIs differ only in the trailing slash, and the server has associated a resource with + // one of them, then the other URI MUST NOT correspond to another resource. Instead, the server MAY respond to + // requests for the latter URI with a 301 redirect to the former." + // https://solid.github.io/specification/protocol#uri-slash-semantics if (oldMetadata && oldMetadata.identifier.value !== identifier.path) { - throw new ConflictHttpError(`${identifier.path} conflicts with existing path ${oldMetadata.identifier.value}`); + throw new ForbiddenHttpError(`${identifier.path} conflicts with existing path ${oldMetadata.identifier.value}`); } - // If we already have a resource for the given identifier, make sure they match resource types const isContainer = this.isNewContainer(representation.metadata, identifier.path); - if (oldMetadata && isContainer !== this.isExistingContainer(oldMetadata)) { - throw new ConflictHttpError('Input resource type does not match existing resource type.'); - } + // Solid, §3.1: "Paths ending with a slash denote a container resource." + // https://solid.github.io/specification/protocol#uri-slash-semantics if (isContainer !== isContainerIdentifier(identifier)) { throw new BadRequestHttpError('Containers should have a `/` at the end of their path, resources should not.'); } @@ -141,12 +152,16 @@ export class DataAccessorBasedStore implements ResourceStore { public async deleteResource(identifier: ResourceIdentifier): Promise { this.validateIdentifier(identifier); const metadata = await this.accessor.getMetadata(identifier); - // "When a DELETE request targets storage’s root container or its associated ACL resource, - // the server MUST respond with the 405 status code." - // https://solid.github.io/specification/#deleting-resources + // Solid, §5.4: "When a DELETE request targets storage’s root container or its associated ACL resource, + // the server MUST respond with the 405 status code." + // https://solid.github.io/specification/protocol#deleting-resources if (this.isRootStorage(metadata)) { throw new MethodNotAllowedHttpError('Cannot delete a root storage container.'); } + // Solid, §5.4: "When a DELETE request is made to a container, the server MUST delete the container + // if it contains no resources. If the container contains resources, + // the server MUST respond with the 409 status code and response body describing the error." + // https://solid.github.io/specification/protocol#deleting-resources if (metadata.getAll(LDP.contains).length > 0) { throw new ConflictHttpError('Can only delete empty containers.'); } @@ -164,9 +179,11 @@ export class DataAccessorBasedStore implements ResourceStore { /** * Returns the metadata matching the identifier, ignoring the presence of a trailing slash or not. - * This is used to support the following part of the spec: - * "If two URIs differ only in the trailing slash, and the server has associated a resource with one of them, - * then the other URI MUST NOT correspond to another resource." + * + * Solid, §3.1: "If two URIs differ only in the trailing slash, + * and the server has associated a resource with one of them, + * then the other URI MUST NOT correspond to another resource." + * https://solid.github.io/specification/protocol#uri-slash-semantics * * First the identifier gets requested and if no result is found * the identifier with differing trailing slash is requested. @@ -177,11 +194,13 @@ export class DataAccessorBasedStore implements ResourceStore { try { return await this.accessor.getMetadata(identifier); } catch (error: unknown) { - // Trimming the trailing slash of a root container is undefined as there is no parent container - if (error instanceof NotFoundHttpError && !this.identifierStrategy.isRootContainer(identifier)) { - return this.accessor.getMetadata( - { path: hasSlash ? trimTrailingSlashes(identifier.path) : ensureTrailingSlash(identifier.path) }, - ); + if (error instanceof NotFoundHttpError) { + const otherIdentifier = + { path: hasSlash ? trimTrailingSlashes(identifier.path) : ensureTrailingSlash(identifier.path) }; + + // Only try to access other identifier if it is valid in the scope of the DataAccessor + this.validateIdentifier(otherIdentifier); + return this.accessor.getMetadata(otherIdentifier); } throw error; } @@ -222,6 +241,9 @@ export class DataAccessorBasedStore implements ResourceStore { } // Root container should not have a parent container + // Solid, §5.3: "Servers MUST create intermediate containers and include corresponding containment triples + // in container representations derived from the URI path component of PUT and PATCH requests." + // https://solid.github.io/specification/protocol#writing-resources if (createContainers && !this.identifierStrategy.isRootContainer(identifier)) { await this.createRecursiveContainers(this.identifierStrategy.getParentContainer(identifier)); } @@ -254,11 +276,11 @@ export class DataAccessorBasedStore implements ResourceStore { throw error; } - // Make sure there are no containment triples in the body - for (const quad of quads) { - if (quad.predicate.value === LDP.contains) { - throw new ConflictHttpError('Container bodies are not allowed to have containment triples.'); - } + // Solid, §5.3: "Servers MUST NOT allow HTTP POST, PUT and PATCH to update a container’s containment triples; + // if the server receives such a request, it MUST respond with a 409 status code." + // https://solid.github.io/specification/protocol#writing-resources + if (quads.some((quad): boolean => quad.predicate.value === LDP.contains)) { + throw new ConflictHttpError('Container bodies are not allowed to have containment triples.'); } // Input content type doesn't matter anymore @@ -270,6 +292,13 @@ export class DataAccessorBasedStore implements ResourceStore { /** * Generates a new URI for a resource in the given container, potentially using the given slug. + * + * Solid, §5.3: "Servers MUST allow creating new resources with a POST request to URI path ending `/`. + * Servers MUST create a resource with URI path ending `/{id}` in container `/`. + * Servers MUST create a container with URI path ending `/{id}/` in container `/` for requests + * including the HTTP Link header with rel="type" targeting a valid LDP container type." + * https://solid.github.io/specification/protocol#writing-resources + * * @param container - Parent container of the new URI. * @param isContainer - Does the new URI represent a container? * @param slug - Slug to use for the new URI. @@ -315,8 +344,6 @@ export class DataAccessorBasedStore implements ResourceStore { * @param suffix - Suffix of the URI. Can be the full URI, but only the last part is required. */ protected isNewContainer(metadata: RepresentationMetadata, suffix?: string): boolean { - // Should not use `isExistingContainer` since the metadata might contain unrelated type triples - // It's not because there is no container type triple that the new resource is not a container if (this.hasContainerType(metadata.getAll(RDF.type))) { return true; } @@ -324,19 +351,6 @@ export class DataAccessorBasedStore implements ResourceStore { return Boolean(slug && isContainerPath(slug)); } - /** - * Checks if the given metadata represents a container, purely based on metadata type triples. - * Since type metadata always gets generated when writing resources this should never fail on stored resources. - * @param metadata - Metadata to check. - */ - protected isExistingContainer(metadata: RepresentationMetadata): boolean { - const types = metadata.getAll(RDF.type); - if (!types.some((type): boolean => type.value === LDP.Resource)) { - throw new InternalServerError('Unknown resource type.'); - } - return this.hasContainerType(types); - } - /** * Checks in a list of types if any of them match a Container type. */ @@ -359,8 +373,13 @@ export class DataAccessorBasedStore implements ResourceStore { protected async createRecursiveContainers(container: ResourceIdentifier): Promise { try { const metadata = await this.getNormalizedMetadata(container); - if (!this.isExistingContainer(metadata)) { - throw new ConflictHttpError(`Creating container ${container.path} conflicts with an existing resource.`); + // See #480 + // Solid, §3.1: "If two URIs differ only in the trailing slash, and the server has associated a resource with + // one of them, then the other URI MUST NOT correspond to another resource. Instead, the server MAY respond to + // requests for the latter URI with a 301 redirect to the former." + // https://solid.github.io/specification/protocol#uri-slash-semantics + if (!isContainerPath(metadata.identifier.value)) { + throw new ForbiddenHttpError(`Creating container ${container.path} conflicts with an existing resource.`); } } catch (error: unknown) { if (error instanceof NotFoundHttpError) { diff --git a/test/unit/storage/DataAccessorBasedStore.test.ts b/test/unit/storage/DataAccessorBasedStore.test.ts index ba463be00..5e55d81fb 100644 --- a/test/unit/storage/DataAccessorBasedStore.test.ts +++ b/test/unit/storage/DataAccessorBasedStore.test.ts @@ -11,7 +11,7 @@ import { DataAccessorBasedStore } from '../../../src/storage/DataAccessorBasedSt import { INTERNAL_QUADS } from '../../../src/util/ContentTypes'; import { BadRequestHttpError } from '../../../src/util/errors/BadRequestHttpError'; import { ConflictHttpError } from '../../../src/util/errors/ConflictHttpError'; -import { InternalServerError } from '../../../src/util/errors/InternalServerError'; +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'; @@ -90,7 +90,9 @@ describe('A DataAccessorBasedStore', (): void => { DataFactory.namedNode(LDP.BasicContainer), ]}, ); - accessor.data[root] = { metadata: containerMetadata } as Representation; + const rootMetadata = new RepresentationMetadata(containerMetadata); + rootMetadata.identifier = namedNode(root); + accessor.data[root] = { metadata: rootMetadata } as Representation; representation = { binary: true, @@ -117,6 +119,7 @@ describe('A DataAccessorBasedStore', (): void => { it('will return a data stream that matches the metadata for containers.', async(): Promise => { const resourceID = { path: `${root}container/` }; + containerMetadata.identifier = namedNode(resourceID.path); accessor.data[resourceID.path] = { metadata: containerMetadata } as Representation; const metaQuads = containerMetadata.quads(); const result = await store.getRepresentation(resourceID); @@ -243,8 +246,9 @@ describe('A DataAccessorBasedStore', (): void => { const resourceID = { path: `${root}resource` }; accessor.data[`${resourceID.path}/`] = representation; representation.metadata.identifier = DataFactory.namedNode(`${resourceID.path}/`); - await expect(store.setRepresentation(resourceID, representation)) - .rejects.toThrow(`${resourceID.path} conflicts with existing path ${resourceID.path}/`); + const prom = store.setRepresentation(resourceID, representation); + await expect(prom).rejects.toThrow(`${resourceID.path} conflicts with existing path ${resourceID.path}/`); + await expect(prom).rejects.toThrow(ForbiddenHttpError); }); // As discussed in #475, trimming the trailing slash of a root container in getNormalizedMetadata @@ -268,17 +272,6 @@ describe('A DataAccessorBasedStore', (): void => { mock.mockRestore(); }); - it('will error if the target has a different resource type.', async(): Promise => { - const resourceID = { path: `${root}resource` }; - accessor.data[resourceID.path] = representation; - representation.metadata.identifier = DataFactory.namedNode(resourceID.path); - const newRepresentation = { ...representation }; - newRepresentation.metadata = new RepresentationMetadata(representation.metadata); - newRepresentation.metadata.add(RDF.type, LDP.terms.Container); - await expect(store.setRepresentation(resourceID, newRepresentation)) - .rejects.toThrow(new ConflictHttpError('Input resource type does not match existing resource type.')); - }); - it('will error if the ending slash does not match its resource type.', async(): Promise => { const resourceID = { path: `${root}resource` }; representation.metadata.add(RDF.type, LDP.terms.Container); @@ -287,16 +280,6 @@ describe('A DataAccessorBasedStore', (): void => { ); }); - it('will error if the DataAccessor did not store the required type triples.', async(): Promise => { - const resourceID = { path: `${root}resource` }; - accessor.data[resourceID.path] = representation; - representation.metadata.identifier = namedNode(resourceID.path); - representation.metadata.removeAll(RDF.type); - await expect(store.setRepresentation(resourceID, representation)).rejects.toThrow( - new InternalServerError('Unknown resource type.'), - ); - }); - it('errors when trying to create a container with non-RDF data.', async(): Promise => { const resourceID = { path: `${root}container/` }; representation.metadata.add(RDF.type, LDP.terms.Container); @@ -360,9 +343,9 @@ describe('A DataAccessorBasedStore', (): void => { it('errors when a recursive container overlaps with an existing resource.', async(): Promise => { const resourceID = { path: `${root}a/b/resource` }; accessor.data[`${root}a`] = representation; - await expect(store.setRepresentation(resourceID, representation)).rejects.toThrow( - new ConflictHttpError(`Creating container ${root}a/ conflicts with an existing resource.`), - ); + const prom = store.setRepresentation(resourceID, representation); + await expect(prom).rejects.toThrow(`Creating container ${root}a/ conflicts with an existing resource.`); + await expect(prom).rejects.toThrow(ForbiddenHttpError); }); it('can write to root if it does not exist.', async(): Promise => {