fix: Extract correct access modes from request

RELEASE_NOTES.md

@@ -23,6 +23,7 @@ These changes are relevant if you wrote custom modules for the server that depen
 - `PermissionReader`s take an additional `modes` parameter as input.
 - The `ResourceStore` function `resourceExists` has been renamed to `hasResource`
   and has been moved to a separate `ResourceSet` interface.
+- Several `ModesExtractor`s now take a `ResourceSet` as constructor parameter.
 
 ## v3.0.0
 ### New features

config/ldp/modes/default.json

@@ -12,7 +12,8 @@
         },
         {
           "comment": "Extract access modes based on the HTTP method.",
-          "@type": "MethodModesExtractor"
+          "@type": "MethodModesExtractor",
+          "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" }
         },
         {
           "@type": "StaticThrowHandler",
@@ -27,8 +28,14 @@
       "source": {
         "@type": "WaterfallHandler",
         "handlers": [
-          { "@type": "N3PatchModesExtractor" },
+          {
-          { "@type": "SparqlUpdateModesExtractor" },
+            "@type": "N3PatchModesExtractor",
+            "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" }
+          },
+          {
+            "@type": "SparqlUpdateModesExtractor",
+            "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" }
+          },
           {
             "@type": "StaticThrowHandler",
             "error": { "@type": "UnsupportedMediaTypeHttpError" }

src/authorization/WebAclReader.ts

@@ -1,7 +1,7 @@
 import type { Quad, Term } from 'n3';
 import { Store } from 'n3';
-import { CredentialGroup } from '../authentication/Credentials';
 import type { Credential, CredentialSet } from '../authentication/Credentials';
+import { CredentialGroup } from '../authentication/Credentials';
 import type { AuxiliaryIdentifierStrategy } from '../http/auxiliary/AuxiliaryIdentifierStrategy';
 import type { Representation } from '../http/representation/Representation';
 import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier';
@@ -20,14 +20,15 @@ import type { PermissionReaderInput } from './PermissionReader';
 import { PermissionReader } from './PermissionReader';
 import type { AclPermission } from './permissions/AclPermission';
 import { AclMode } from './permissions/AclPermission';
-import { AccessMode } from './permissions/Permissions';
 import type { PermissionSet } from './permissions/Permissions';
+import { AccessMode } from './permissions/Permissions';
 
-const modesMap: Record<string, keyof AclPermission> = {
+// Maps ACL modes to their associated general modes.
-  [ACL.Read]: AccessMode.read,
+const modesMap: Record<string, Readonly<(keyof AclPermission)[]>> = {
-  [ACL.Write]: AccessMode.write,
+  [ACL.Read]: [ AccessMode.read ],
-  [ACL.Append]: AccessMode.append,
+  [ACL.Write]: [ AccessMode.append, AccessMode.write, AccessMode.create, AccessMode.delete ],
-  [ACL.Control]: AclMode.control,
+  [ACL.Append]: [ AccessMode.append ],
+  [ACL.Control]: [ AclMode.control ],
 } as const;
 
 /**
@@ -106,17 +107,14 @@ export class WebAclReader extends PermissionReader {
       if (hasAccess) {
         // Set all allowed modes to true
         const modes = acl.getObjects(rule, ACL.mode, null);
-        for (const { value: mode } of modes) {
+        for (const { value: aclMode } of modes) {
-          if (mode in modesMap) {
+          if (aclMode in modesMap) {
-            aclPermissions[modesMap[mode]] = true;
+            for (const mode of modesMap[aclMode]) {
+              aclPermissions[mode] = true;
             }
           }
         }
       }
-
-    if (aclPermissions.write) {
-      // Write permission implies Append permission
-      aclPermissions.append = true;
     }
 
     return aclPermissions;

src/authorization/permissions/MethodModesExtractor.ts

@@ -1,37 +1,63 @@
 import type { Operation } from '../../http/Operation';
+import type { ResourceSet } from '../../storage/ResourceSet';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
+import { isContainerIdentifier } from '../../util/PathUtil';
 import { ModesExtractor } from './ModesExtractor';
 import { AccessMode } from './Permissions';
 
 const READ_METHODS = new Set([ 'GET', 'HEAD' ]);
-const WRITE_METHODS = new Set([ 'PUT', 'DELETE' ]);
+const SUPPORTED_METHODS = new Set([ ...READ_METHODS, 'PUT', 'POST', 'DELETE' ]);
-const APPEND_METHODS = new Set([ 'POST' ]);
-const SUPPORTED_METHODS = new Set([ ...READ_METHODS, ...WRITE_METHODS, ...APPEND_METHODS ]);
 
 /**
  * Generates permissions for the base set of methods that always require the same permissions.
  * Specifically: GET, HEAD, POST, PUT and DELETE.
  */
 export class MethodModesExtractor extends ModesExtractor {
+  private readonly resourceSet: ResourceSet;
+
+  /**
+   * Certain permissions depend on the existence of the target resource.
+   * The provided {@link ResourceSet} will be used for that.
+   * @param resourceSet - {@link ResourceSet} that can verify the target resource existence.
+   */
+  public constructor(resourceSet: ResourceSet) {
+    super();
+    this.resourceSet = resourceSet;
+  }
+
   public async canHandle({ method }: Operation): Promise<void> {
     if (!SUPPORTED_METHODS.has(method)) {
       throw new NotImplementedHttpError(`Cannot determine permissions of ${method}`);
     }
   }
 
-  public async handle({ method }: Operation): Promise<Set<AccessMode>> {
+  public async handle({ method, target }: Operation): Promise<Set<AccessMode>> {
-    const result = new Set<AccessMode>();
+    const modes = new Set<AccessMode>();
+    // Reading requires Read permissions on the resource
     if (READ_METHODS.has(method)) {
-      result.add(AccessMode.read);
+      modes.add(AccessMode.read);
     }
-    if (WRITE_METHODS.has(method)) {
+    // Setting a resource's representation requires Write permissions
-      result.add(AccessMode.write);
+    if (method === 'PUT') {
-      result.add(AccessMode.append);
+      modes.add(AccessMode.write);
-      result.add(AccessMode.create);
+      // …and, if the resource does not exist yet, Create permissions are required as well
-      result.add(AccessMode.delete);
+      if (!await this.resourceSet.hasResource(target)) {
-    } else if (APPEND_METHODS.has(method)) {
+        modes.add(AccessMode.create);
-      result.add(AccessMode.append);
       }
-    return result;
+    }
+    // Creating a new resource in a container requires Append access to that container
+    if (method === 'POST') {
+      modes.add(AccessMode.append);
+    }
+    // Deleting a resource requires Delete access
+    if (method === 'DELETE') {
+      modes.add(AccessMode.delete);
+      // …and, if the target is a container, Read permissions are required as well
+      // as this exposes if a container is empty or not
+      if (isContainerIdentifier(target)) {
+        modes.add(AccessMode.read);
+      }
+    }
+    return modes;
   }
 }

src/authorization/permissions/N3PatchModesExtractor.ts

@@ -1,6 +1,7 @@
 import type { Operation } from '../../http/Operation';
 import type { N3Patch } from '../../http/representation/N3Patch';
 import { isN3Patch } from '../../http/representation/N3Patch';
+import type { ResourceSet } from '../../storage/ResourceSet';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
 import { ModesExtractor } from './ModesExtractor';
 import { AccessMode } from './Permissions';
@@ -14,13 +15,25 @@ import { AccessMode } from './Permissions';
  * https://solid.github.io/specification/protocol#n3-patch
  */
 export class N3PatchModesExtractor extends ModesExtractor {
+  private readonly resourceSet: ResourceSet;
+
+  /**
+   * Certain permissions depend on the existence of the target resource.
+   * The provided {@link ResourceSet} will be used for that.
+   * @param resourceSet - {@link ResourceSet} that can verify the target resource existence.
+   */
+  public constructor(resourceSet: ResourceSet) {
+    super();
+    this.resourceSet = resourceSet;
+  }
+
   public async canHandle({ body }: Operation): Promise<void> {
     if (!isN3Patch(body)) {
       throw new NotImplementedHttpError('Can only determine permissions of N3 Patch documents.');
     }
   }
 
-  public async handle({ body }: Operation): Promise<Set<AccessMode>> {
+  public async handle({ body, target }: Operation): Promise<Set<AccessMode>> {
     const { deletes, inserts, conditions } = body as N3Patch;
 
     const accessModes = new Set<AccessMode>();
@@ -32,6 +45,9 @@ export class N3PatchModesExtractor extends ModesExtractor {
     // When ?insertions is non-empty, servers MUST (also) treat the request as an Append operation.
     if (inserts.length > 0) {
       accessModes.add(AccessMode.append);
+      if (!await this.resourceSet.hasResource(target)) {
+        accessModes.add(AccessMode.create);
+      }
     }
     // When ?deletions is non-empty, servers MUST treat the request as a Read and Write operation.
     if (deletes.length > 0) {

src/authorization/permissions/SparqlUpdateModesExtractor.ts

@@ -2,6 +2,7 @@ import { Algebra } from 'sparqlalgebrajs';
 import type { Operation } from '../../http/Operation';
 import type { Representation } from '../../http/representation/Representation';
 import type { SparqlUpdatePatch } from '../../http/representation/SparqlUpdatePatch';
+import type { ResourceSet } from '../../storage/ResourceSet';
 import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError';
 import { ModesExtractor } from './ModesExtractor';
 import { AccessMode } from './Permissions';
@@ -12,6 +13,18 @@ import { AccessMode } from './Permissions';
  * while DELETEs require write permissions as well.
  */
 export class SparqlUpdateModesExtractor extends ModesExtractor {
+  private readonly resourceSet: ResourceSet;
+
+  /**
+   * Certain permissions depend on the existence of the target resource.
+   * The provided {@link ResourceSet} will be used for that.
+   * @param resourceSet - {@link ResourceSet} that can verify the target resource existence.
+   */
+  public constructor(resourceSet: ResourceSet) {
+    super();
+    this.resourceSet = resourceSet;
+  }
+
   public async canHandle({ body }: Operation): Promise<void> {
     if (!this.isSparql(body)) {
       throw new NotImplementedHttpError('Cannot determine permissions of non-SPARQL patches.');
@@ -21,21 +34,31 @@ export class SparqlUpdateModesExtractor extends ModesExtractor {
     }
   }
 
-  public async handle({ body }: Operation): Promise<Set<AccessMode>> {
+  public async handle({ body, target }: Operation): Promise<Set<AccessMode>> {
     // Verified in `canHandle` call
     const update = (body as SparqlUpdatePatch).algebra as Algebra.DeleteInsert;
-    const result = new Set<AccessMode>();
+    const modes = new Set<AccessMode>();
 
-    // Since `append` is a specific type of write, it is true if `write` is true.
+    if (this.isNop(update)) {
-    if (this.needsWrite(update)) {
+      return modes;
-      result.add(AccessMode.write);
-      result.add(AccessMode.append);
-      result.add(AccessMode.create);
-      result.add(AccessMode.delete);
-    } else if (this.needsAppend(update)) {
-      result.add(AccessMode.append);
     }
-    return result;
+
+    // Access modes inspired by the requirements on N3 Patch requests
+    if (this.hasConditions(update)) {
+      modes.add(AccessMode.read);
+    }
+    if (this.hasInserts(update)) {
+      modes.add(AccessMode.append);
+      if (!await this.resourceSet.hasResource(target)) {
+        modes.add(AccessMode.create);
+      }
+    }
+    if (this.hasDeletes(update)) {
+      modes.add(AccessMode.read);
+      modes.add(AccessMode.write);
+    }
+
+    return modes;
   }
 
   private isSparql(data: Representation): data is SparqlUpdatePatch {
@@ -52,33 +75,32 @@ export class SparqlUpdateModesExtractor extends ModesExtractor {
     return false;
   }
 
-  private isDeleteInsert(op: Algebra.Update): op is Algebra.DeleteInsert {
+  private isDeleteInsert(op: Algebra.Operation): op is Algebra.DeleteInsert {
     return op.type === Algebra.types.DELETE_INSERT;
   }
 
-  private isNop(op: Algebra.Update): op is Algebra.Nop {
+  private isNop(op: Algebra.Operation): op is Algebra.Nop {
     return op.type === Algebra.types.NOP;
   }
 
-  private needsAppend(update: Algebra.Update): boolean {
+  private hasConditions(update: Algebra.Update): boolean {
-    if (this.isNop(update)) {
+    if (this.isDeleteInsert(update)) {
-      return false;
+      return Boolean(update.where && !this.isNop(update.where));
     }
+    return (update as Algebra.CompositeUpdate).updates.some((op): boolean => this.hasConditions(op));
+  }
+
+  private hasInserts(update: Algebra.Update): boolean {
     if (this.isDeleteInsert(update)) {
       return Boolean(update.insert && update.insert.length > 0);
     }
-
+    return (update as Algebra.CompositeUpdate).updates.some((op): boolean => this.hasInserts(op));
-    return (update as Algebra.CompositeUpdate).updates.some((op): boolean => this.needsAppend(op));
   }
 
-  private needsWrite(update: Algebra.Update): boolean {
+  private hasDeletes(update: Algebra.Update): boolean {
-    if (this.isNop(update)) {
-      return false;
-    }
     if (this.isDeleteInsert(update)) {
       return Boolean(update.delete && update.delete.length > 0);
     }
-
+    return (update as Algebra.CompositeUpdate).updates.some((op): boolean => this.hasDeletes(op));
-    return (update as Algebra.CompositeUpdate).updates.some((op): boolean => this.needsWrite(op));
   }
 }

test/unit/authorization/permissions/MethodModesExtractor.test.ts

@@ -1,10 +1,19 @@
 import { MethodModesExtractor } from '../../../../src/authorization/permissions/MethodModesExtractor';
 import { AccessMode } from '../../../../src/authorization/permissions/Permissions';
 import type { Operation } from '../../../../src/http/Operation';
+import type { ResourceSet } from '../../../../src/storage/ResourceSet';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
 
 describe('A MethodModesExtractor', (): void => {
-  const extractor = new MethodModesExtractor();
+  let resourceSet: jest.Mocked<ResourceSet>;
+  let extractor: MethodModesExtractor;
+
+  beforeEach(async(): Promise<void> => {
+    resourceSet = {
+      hasResource: jest.fn().mockResolvedValue(true),
+    };
+    extractor = new MethodModesExtractor(resourceSet);
+  });
 
   it('can handle HEAD/GET/POST/PUT/DELETE.', async(): Promise<void> => {
     await expect(extractor.canHandle({ method: 'HEAD' } as Operation)).resolves.toBeUndefined();
@@ -29,11 +38,22 @@ describe('A MethodModesExtractor', (): void => {
 
   it('requires write for PUT operations.', async(): Promise<void> => {
     await expect(extractor.handle({ method: 'PUT' } as Operation))
-      .resolves.toEqual(new Set([ AccessMode.append, AccessMode.write, AccessMode.create, AccessMode.delete ]));
+      .resolves.toEqual(new Set([ AccessMode.write ]));
   });
 
-  it('requires write for DELETE operations.', async(): Promise<void> => {
+  it('requires create for PUT operations if the target does not exist.', async(): Promise<void> => {
-    await expect(extractor.handle({ method: 'DELETE' } as Operation))
+    resourceSet.hasResource.mockResolvedValueOnce(false);
-      .resolves.toEqual(new Set([ AccessMode.append, AccessMode.write, AccessMode.create, AccessMode.delete ]));
+    await expect(extractor.handle({ method: 'PUT' } as Operation))
+      .resolves.toEqual(new Set([ AccessMode.write, AccessMode.create ]));
+  });
+
+  it('requires delete for DELETE operations.', async(): Promise<void> => {
+    await expect(extractor.handle({ method: 'DELETE', target: { path: 'http://example.com/foo' }} as Operation))
+      .resolves.toEqual(new Set([ AccessMode.delete ]));
+  });
+
+  it('also requires read for DELETE operations on containers.', async(): Promise<void> => {
+    await expect(extractor.handle({ method: 'DELETE', target: { path: 'http://example.com/foo/' }} as Operation))
+      .resolves.toEqual(new Set([ AccessMode.delete, AccessMode.read ]));
   });
 });

test/unit/authorization/permissions/N3PatchModesExtractor.test.ts

@@ -5,6 +5,7 @@ import { AccessMode } from '../../../../src/authorization/permissions/Permission
 import type { Operation } from '../../../../src/http/Operation';
 import { BasicRepresentation } from '../../../../src/http/representation/BasicRepresentation';
 import type { N3Patch } from '../../../../src/http/representation/N3Patch';
+import type { ResourceSet } from '../../../../src/storage/ResourceSet';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
 
 const { quad, namedNode } = DataFactory;
@@ -13,7 +14,8 @@ describe('An N3PatchModesExtractor', (): void => {
   const triple: Quad = quad(namedNode('a'), namedNode('b'), namedNode('c'));
   let patch: N3Patch;
   let operation: Operation;
-  const extractor = new N3PatchModesExtractor();
+  let resourceSet: jest.Mocked<ResourceSet>;
+  let extractor: N3PatchModesExtractor;
 
   beforeEach(async(): Promise<void> => {
     patch = new BasicRepresentation() as N3Patch;
@@ -27,6 +29,12 @@ describe('An N3PatchModesExtractor', (): void => {
       preferences: {},
       target: { path: 'http://example.com/foo' },
     };
+
+    resourceSet = {
+      hasResource: jest.fn().mockResolvedValue(true),
+    };
+
+    extractor = new N3PatchModesExtractor(resourceSet);
   });
 
   it('can only handle N3 Patch documents.', async(): Promise<void> => {
@@ -47,6 +55,12 @@ describe('An N3PatchModesExtractor', (): void => {
     await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append ]));
   });
 
+  it('requires create access when there are inserts and the resource does not exist.', async(): Promise<void> => {
+    resourceSet.hasResource.mockResolvedValueOnce(false);
+    patch.inserts = [ triple ];
+    await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append, AccessMode.create ]));
+  });
+
   it('requires read and write access when there are inserts.', async(): Promise<void> => {
     patch.deletes = [ triple ];
     await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.read, AccessMode.write ]));

test/unit/authorization/permissions/SparqlUpdateModesExtractor.test.ts

@@ -3,12 +3,21 @@ import { AccessMode } from '../../../../src/authorization/permissions/Permission
 import { SparqlUpdateModesExtractor } from '../../../../src/authorization/permissions/SparqlUpdateModesExtractor';
 import type { Operation } from '../../../../src/http/Operation';
 import type { SparqlUpdatePatch } from '../../../../src/http/representation/SparqlUpdatePatch';
+import type { ResourceSet } from '../../../../src/storage/ResourceSet';
 import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError';
 
 describe('A SparqlUpdateModesExtractor', (): void => {
-  const extractor = new SparqlUpdateModesExtractor();
+  let resourceSet: jest.Mocked<ResourceSet>;
+  let extractor: SparqlUpdateModesExtractor;
   const factory = new Factory();
 
+  beforeEach(async(): Promise<void> => {
+    resourceSet = {
+      hasResource: jest.fn().mockResolvedValue(true),
+    };
+    extractor = new SparqlUpdateModesExtractor(resourceSet);
+  });
+
   it('can only handle (composite) SPARQL DELETE/INSERT operations.', async(): Promise<void> => {
     const operation = { method: 'PATCH', body: { algebra: factory.createDeleteInsert() }} as unknown as Operation;
     await expect(extractor.canHandle(operation)).resolves.toBeUndefined();
@@ -43,7 +52,18 @@ describe('A SparqlUpdateModesExtractor', (): void => {
     await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append ]));
   });
 
-  it('requires write for DELETE operations.', async(): Promise<void> => {
+  it('requires create for INSERT operations if the resource does not exist.', async(): Promise<void> => {
+    resourceSet.hasResource.mockResolvedValueOnce(false);
+    const operation = {
+      method: 'PATCH',
+      body: { algebra: factory.createDeleteInsert(undefined, [
+        factory.createPattern(factory.createTerm('<s>'), factory.createTerm('<p>'), factory.createTerm('<o>')),
+      ]) },
+    } as unknown as Operation;
+    await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append, AccessMode.create ]));
+  });
+
+  it('requires read and write for DELETE operations.', async(): Promise<void> => {
     const operation = {
       method: 'PATCH',
       body: { algebra: factory.createDeleteInsert([
@@ -51,20 +71,22 @@ describe('A SparqlUpdateModesExtractor', (): void => {
       ]) },
     } as unknown as Operation;
     await expect(extractor.handle(operation))
-      .resolves.toEqual(new Set([ AccessMode.append, AccessMode.write, AccessMode.create, AccessMode.delete ]));
+      .resolves.toEqual(new Set([ AccessMode.read, AccessMode.write ]));
   });
 
-  it('requires append for composite operations with an insert.', async(): Promise<void> => {
+  it('requires read and append for composite operations with an insert and conditions.', async(): Promise<void> => {
     const operation = {
       method: 'PATCH',
       body: { algebra: factory.createCompositeUpdate([ factory.createDeleteInsert(undefined, [
         factory.createPattern(factory.createTerm('<s>'), factory.createTerm('<p>'), factory.createTerm('<o>')),
-      ]) ]) },
+      ], factory.createBgp([
+        factory.createPattern(factory.createTerm('<s>'), factory.createTerm('<p>'), factory.createTerm('<o>')),
+      ])) ]) },
     } as unknown as Operation;
-    await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append ]));
+    await expect(extractor.handle(operation)).resolves.toEqual(new Set([ AccessMode.append, AccessMode.read ]));
   });
 
-  it('requires write for composite operations with a delete.', async(): Promise<void> => {
+  it('requires read, write and append for composite operations with a delete and insert.', async(): Promise<void> => {
     const operation = {
       method: 'PATCH',
       body: { algebra: factory.createCompositeUpdate([ factory.createDeleteInsert(undefined, [
@@ -75,6 +97,6 @@ describe('A SparqlUpdateModesExtractor', (): void => {
       ]) ]) },
     } as unknown as Operation;
     await expect(extractor.handle(operation))
-      .resolves.toEqual(new Set([ AccessMode.append, AccessMode.write, AccessMode.create, AccessMode.delete ]));
+      .resolves.toEqual(new Set([ AccessMode.append, AccessMode.read, AccessMode.write ]));
   });
 });