fix: Return correct status code when deleting non-existent resource

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",

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<void> {
+    await this.source.canHandle(operation);
+  }
+
+  public async handle(operation: Operation): Promise<AccessMap> {
+    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;
+  }
+}

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';

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 ],
 

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<ModesExtractor>;
+  let resourceSet: jest.Mocked<ResourceSet>;
+  let identifierStrategy: jest.Mocked<IdentifierStrategy>;
+  let extractor: DeleteParentExtractor;
+
+  beforeEach(async(): Promise<void> => {
+    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<void> => {
+    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<void> => {
+    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<void> => {
+    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<void> => {
+    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<void> => {
+    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();
+  });
+});