From ef48660b482cb954e5b08e9c3799acfdba6d6f23 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 31 Oct 2022 16:55:35 +0100 Subject: [PATCH] fix: Return correct status code when deleting non-existent resource --- config/ldp/modes/default.json | 12 ++- .../permissions/DeleteParentExtractor.ts | 44 +++++++++ src/index.ts | 1 + test/integration/PermissionTable.test.ts | 3 +- .../permissions/DeleteParentExtractor.test.ts | 94 +++++++++++++++++++ 5 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 src/authorization/permissions/DeleteParentExtractor.ts create mode 100644 test/unit/authorization/permissions/DeleteParentExtractor.test.ts diff --git a/config/ldp/modes/default.json b/config/ldp/modes/default.json index e2606fb7c..3b74b0061 100644 --- a/config/ldp/modes/default.json +++ b/config/ldp/modes/default.json @@ -19,9 +19,15 @@ "@id": "urn:solid-server:default:PatchModesExtractor" }, { - "comment": "Extract access modes based on the HTTP method.", - "@type": "MethodModesExtractor", - "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" } + "comment": "Requires read permissions on parent container when deleting non-existent resource for correct status code.", + "@type": "DeleteParentExtractor", + "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" }, + "identifierStrategy": { "@id": "urn:solid-server:default:IdentifierStrategy" }, + "source": { + "comment": "Extract access modes based on the HTTP method.", + "@type": "MethodModesExtractor", + "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" } + } }, { "@type": "StaticThrowHandler", diff --git a/src/authorization/permissions/DeleteParentExtractor.ts b/src/authorization/permissions/DeleteParentExtractor.ts new file mode 100644 index 000000000..57ae454ce --- /dev/null +++ b/src/authorization/permissions/DeleteParentExtractor.ts @@ -0,0 +1,44 @@ +import type { Operation } from '../../http/Operation'; +import type { ResourceSet } from '../../storage/ResourceSet'; +import type { IdentifierStrategy } from '../../util/identifiers/IdentifierStrategy'; +import { ModesExtractor } from './ModesExtractor'; +import type { AccessMap } from './Permissions'; +import { AccessMode } from './Permissions'; + +/** + * In case a resource is being deleted but does not exist, + * the server response code depends on the access modes the agent has on the parent container. + * In case the agent has read access on the parent container, a 404 should be returned, + * otherwise it should be 401/403. + * + * This class adds support for this by requiring read access on the parent container + * in case the target resource does not exist. + */ +export class DeleteParentExtractor extends ModesExtractor { + private readonly source: ModesExtractor; + private readonly resourceSet: ResourceSet; + private readonly identifierStrategy: IdentifierStrategy; + + public constructor(source: ModesExtractor, resourceSet: ResourceSet, identifierStrategy: IdentifierStrategy) { + super(); + this.source = source; + this.resourceSet = resourceSet; + this.identifierStrategy = identifierStrategy; + } + + public async canHandle(operation: Operation): Promise { + await this.source.canHandle(operation); + } + + public async handle(operation: Operation): Promise { + const accessMap = await this.source.handle(operation); + const { target } = operation; + if (accessMap.get(target)?.has(AccessMode.delete) && + !this.identifierStrategy.isRootContainer(target) && + !await this.resourceSet.hasResource(target)) { + const parent = this.identifierStrategy.getParentContainer(target); + accessMap.add(parent, new Set([ AccessMode.read ])); + } + return accessMap; + } +} diff --git a/src/index.ts b/src/index.ts index 16acd154d..895f2114e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -16,6 +16,7 @@ export * from './authorization/access/AgentGroupAccessChecker'; // Authorization/Permissions export * from './authorization/permissions/AclPermission'; +export * from './authorization/permissions/DeleteParentExtractor'; export * from './authorization/permissions/IntermediateCreateExtractor'; export * from './authorization/permissions/ModesExtractor'; export * from './authorization/permissions/MethodModesExtractor'; diff --git a/test/integration/PermissionTable.test.ts b/test/integration/PermissionTable.test.ts index 3d5d39ca3..269793723 100644 --- a/test/integration/PermissionTable.test.ts +++ b/test/integration/PermissionTable.test.ts @@ -104,8 +104,7 @@ const table: [string, string, AM[], AM[] | undefined, string, string, number, nu [ 'DELETE', 'C/R', [ AM.read ], undefined, '', '', 401, 404 ], [ 'DELETE', 'C/R', [ AM.append ], undefined, '', '', 401, 401 ], [ 'DELETE', 'C/R', [ AM.append ], [ AM.read ], '', '', 401, 404 ], - // We throw a 404 instead of 401 since we don't yet check if the parent container has read permissions - // [ 'DELETE', 'C/R', [ AM.write ], undefined, '', '', 205, 401 ], + [ 'DELETE', 'C/R', [ AM.write ], undefined, '', '', 205, 401 ], [ 'DELETE', 'C/R', [ AM.write ], [ AM.read ], '', '', 401, 404 ], [ 'DELETE', 'C/R', [ AM.write ], [ AM.append ], '', '', 401, 401 ], diff --git a/test/unit/authorization/permissions/DeleteParentExtractor.test.ts b/test/unit/authorization/permissions/DeleteParentExtractor.test.ts new file mode 100644 index 000000000..04ed9658a --- /dev/null +++ b/test/unit/authorization/permissions/DeleteParentExtractor.test.ts @@ -0,0 +1,94 @@ +import { DeleteParentExtractor } from '../../../../src/authorization/permissions/DeleteParentExtractor'; +import type { ModesExtractor } from '../../../../src/authorization/permissions/ModesExtractor'; +import type { AccessMap } from '../../../../src/authorization/permissions/Permissions'; +import { AccessMode } from '../../../../src/authorization/permissions/Permissions'; +import type { Operation } from '../../../../src/http/Operation'; +import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation'; +import type { ResourceSet } from '../../../../src/storage/ResourceSet'; +import type { IdentifierStrategy } from '../../../../src/util/identifiers/IdentifierStrategy'; +import { IdentifierSetMultiMap } from '../../../../src/util/map/IdentifierMap'; + +describe('A DeleteParentExtractor', (): void => { + const baseUrl = 'http://example.com/'; + const resource = 'http://example.com/foo'; + let operation: Operation; + let sourceMap: AccessMap; + let source: jest.Mocked; + let resourceSet: jest.Mocked; + let identifierStrategy: jest.Mocked; + let extractor: DeleteParentExtractor; + + beforeEach(async(): Promise => { + operation = { + target: { path: resource }, + method: 'DELETE', + preferences: {}, + body: new BasicRepresentation(), + }; + + sourceMap = new IdentifierSetMultiMap(); + + source = { + canHandle: jest.fn(), + handle: jest.fn().mockResolvedValue(sourceMap), + } as any; + + resourceSet = { + hasResource: jest.fn().mockResolvedValue(true), + }; + + identifierStrategy = { + isRootContainer: jest.fn().mockReturnValue(false), + getParentContainer: jest.fn().mockReturnValue({ path: baseUrl }), + } as any; + + extractor = new DeleteParentExtractor(source, resourceSet, identifierStrategy); + }); + + it('supports input its source supports.', async(): Promise => { + await expect(extractor.canHandle(operation)).resolves.toBeUndefined(); + + source.canHandle.mockRejectedValue(new Error('bad data')); + await expect(extractor.canHandle(operation)).rejects.toThrow('bad data'); + }); + + it('adds read permission requirements if all conditions are met.', async(): Promise => { + sourceMap.add({ path: resource }, AccessMode.delete); + identifierStrategy.isRootContainer.mockReturnValue(false); + resourceSet.hasResource.mockResolvedValue(false); + + const resultMap = await extractor.handle(operation); + expect([ ...resultMap.entries() ]).toHaveLength(2); + expect(resultMap.get({ path: baseUrl })).toContain(AccessMode.read); + }); + + it('does not change the results if no delete access is required.', async(): Promise => { + sourceMap.add({ path: resource }, AccessMode.read); + identifierStrategy.isRootContainer.mockReturnValue(false); + resourceSet.hasResource.mockResolvedValue(false); + + const resultMap = await extractor.handle(operation); + expect([ ...resultMap.entries() ]).toHaveLength(1); + expect(resultMap.get({ path: baseUrl })).toBeUndefined(); + }); + + it('does not change the results if the target is the root container.', async(): Promise => { + sourceMap.add({ path: resource }, AccessMode.delete); + identifierStrategy.isRootContainer.mockReturnValue(true); + resourceSet.hasResource.mockResolvedValue(false); + + const resultMap = await extractor.handle(operation); + expect([ ...resultMap.entries() ]).toHaveLength(1); + expect(resultMap.get({ path: baseUrl })).toBeUndefined(); + }); + + it('does not change the results if the target exists.', async(): Promise => { + sourceMap.add({ path: resource }, AccessMode.delete); + identifierStrategy.isRootContainer.mockReturnValue(false); + resourceSet.hasResource.mockResolvedValue(true); + + const resultMap = await extractor.handle(operation); + expect([ ...resultMap.entries() ]).toHaveLength(1); + expect(resultMap.get({ path: baseUrl })).toBeUndefined(); + }); +});