Repository: solid/community-server (ts)

Issue:
Correctly check control permissions when accessing acl file
When checking permissions for accessing `http://test.com/resource` the corresponding acl file gets checked for the `accessTo` field with `http://test.com/resource` as object value (or similarly for parent acl resources and the `default` field). We have an exception when accessing an acl file such as `http://test.com/resource.acl` to make sure the `control` permission gets checked instead of the other permissions, but we don't have an exception yet for that object value that is checked as mentioned above.

What this practically means is that when `http://test.com/resource.acl` is accessed, we look for an `accessTo` field with object value `http://test.com/resource.acl`, which is incorrect. We need to look for a field with a value corresponding to the resource identifier.

To do this we need
 * A function `getResource` in the `AclManager` interface that returns the corresponding resource identifier when given an acl identifier.
 * The `WebAclAuthorizer` to use that function to check for the correct object value when the input is an acl identifier.

Pull request:
fix: Correctly handle acl behaviour for acl identifiers
Resolves #315 

Patch:
diff --git a/config/presets/storage-wrapper.json b/config/presets/storage-wrapper.json
index c278c1b80..205164566 100644
--- a/config/presets/storage-wrapper.json
+++ b/config/presets/storage-wrapper.json
@@ -34,13 +34,13 @@
       "RepresentationConvertingStore:_source": {
         "@id": "urn:solid-server:default:RoutingResourceStore"
       },
-      "RepresentationConvertingStore:_outConverter": {
+      "RepresentationConvertingStore:_options_outConverter": {
         "@id": "urn:solid-server:default:RepresentationConverter"
       },
-      "RepresentationConvertingStore:_inConverter": {
+      "RepresentationConvertingStore:_options_inConverter": {
         "@id": "urn:solid-server:default:RepresentationConverter"
       },
-      "RepresentationConvertingStore:_inType": "text/turtle"
+      "RepresentationConvertingStore:_options_inType": "text/turtle"
     },
 
     {
@@ -49,7 +49,7 @@
       "RepresentationConvertingStore:_source": {
         "@id": "urn:solid-server:default:RoutingResourceStore"
       },
-      "RepresentationConvertingStore:_outConverter": {
+      "RepresentationConvertingStore:_options_outConverter": {
         "@id": "urn:solid-server:default:RepresentationConverter"
       }
     }
diff --git a/config/presets/storage/backend/storage-filesystem.json b/config/presets/storage/backend/storage-filesystem.json
index d52d1ee6e..971d404c3 100644
--- a/config/presets/storage/backend/storage-filesystem.json
+++ b/config/presets/storage/backend/storage-filesystem.json
@@ -10,8 +10,8 @@
       "ExtensionBasedMapper:_rootFilepath": {
         "@id": "urn:solid-server:default:variable:rootFilePath"
       },
-      "ExtensionBasedMapper:_acl": "text/turtle",
-      "ExtensionBasedMapper:_meta": "text/turtle"
+      "ExtensionBasedMapper:_overrideTypes_acl": "text/turtle",
+      "ExtensionBasedMapper:_overrideTypes_meta": "text/turtle"
     },
     {
       "@id": "urn:solid-server:default:FileDataAccessor",
diff --git a/src/authorization/AclManager.ts b/src/authorization/AclManager.ts
index 1bf7b679d..274a5c5cb 100644
--- a/src/authorization/AclManager.ts
+++ b/src/authorization/AclManager.ts
@@ -1,26 +1,37 @@
 import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifier';
 
 /**
- * Handles where acl files are stored.
+ * Handles where acl resources are stored.
  */
 export interface AclManager {
   /**
-   * Returns the identifier of the acl file corresponding to the given resource.
-   * This does not guarantee that this acl file exists.
-   * In the case the input is already an acl file that will also be the response.
-   * @param id - The ResourceIdentifier of which we need the corresponding acl file.
+   * Returns the identifier of the acl resource corresponding to the given resource.
+   * This does not guarantee that this acl resource exists.
+   * In the case the input is already an acl resource that will also be the response.
+   * @param id - The ResourceIdentifier of which we need the corresponding acl resource.
    *
-   * @returns The ResourceIdentifier of the corresponding acl file.
+   * @returns The ResourceIdentifier of the corresponding acl resource.
    */
-  getAcl: (id: ResourceIdentifier) => Promise<ResourceIdentifier>;
+  getAclDocument: (id: ResourceIdentifier) => Promise<ResourceIdentifier>;
 
   /**
-   * Checks if the input identifier corresponds to an acl file.
-   * This does not check if that acl file exists,
-   * only if the identifier indicates that there could be an acl file there.
+   * Checks if the input identifier corresponds to an acl resource.
+   * This does not check if that acl resource exists,
+   * only if the identifier indicates that there could be an acl resource there.
    * @param id - Identifier to check.
    *
-   * @returns true if the input identifier points to an acl file.
+   * @returns true if the input identifier points to an acl resource.
    */
-  isAcl: (id: ResourceIdentifier) => Promise<boolean>;
+  isAclDocument: (id: ResourceIdentifier) => Promise<boolean>;
+
+  /**
+   * Returns the identifier of the resource on which the acl constraints are placed.
+   * In general, this is the resource identifier when the input is a normal resource,
+   * or the non-acl version if the input is an acl resource.
+   * This does not guarantee that this resource exists.
+   * @param aclId - Identifier of the acl resource.
+   *
+   * @returns The ResourceIdentifier of the corresponding resource.
+   */
+  getAclConstrainedResource: (id: ResourceIdentifier) => Promise<ResourceIdentifier>;
 }
diff --git a/src/authorization/UrlBasedAclManager.ts b/src/authorization/UrlBasedAclManager.ts
index d7b2c1f92..17bb133f1 100644
--- a/src/authorization/UrlBasedAclManager.ts
+++ b/src/authorization/UrlBasedAclManager.ts
@@ -4,23 +4,23 @@ import type { AclManager } from './AclManager';
 /**
  * Generates acl URIs by adding an .acl file extension.
  *
- * What actually should happen in getAcl:
- * 1. Return id if it isAcl
- * 2. Check store if id exists
- * 3a. (true) Close/destroy data stream! To prevent potential locking issues.
- * 4a. Check metadata if it is a container or a resource.
- * 3b. (false) Use input metadata/heuristic to check if container or resource.
- * 5. Generate the correct identifier (.acl right of / for containers, left for resources if there is a /)
- *
- * It is potentially possible that an agent wants to generate the acl file before generating the actual file.
- * (Unless this is not allowed by the spec, need to verify).
+ * Needs to be updated according to issue #113.
  */
 export class UrlBasedAclManager implements AclManager {
-  public async getAcl(id: ResourceIdentifier): Promise<ResourceIdentifier> {
-    return await this.isAcl(id) ? id : { path: `${id.path}.acl` };
+  public async getAclDocument(id: ResourceIdentifier): Promise<ResourceIdentifier> {
+    return await this.isAclDocument(id) ? id : { path: `${id.path}.acl` };
   }
 
-  public async isAcl(id: ResourceIdentifier): Promise<boolean> {
+  public async isAclDocument(id: ResourceIdentifier): Promise<boolean> {
     return /\.acl\/?/u.test(id.path);
   }
+
+  public async getAclConstrainedResource(id: ResourceIdentifier): Promise<ResourceIdentifier> {
+    if (!await this.isAclDocument(id)) {
+      return id;
+    }
+
+    // Slice off `.acl`
+    return { path: id.path.slice(0, -4) };
+  }
 }
diff --git a/src/authorization/WebAclAuthorizer.ts b/src/authorization/WebAclAuthorizer.ts
index cb3393c5b..c6efdddaa 100644
--- a/src/authorization/WebAclAuthorizer.ts
+++ b/src/authorization/WebAclAuthorizer.ts
@@ -40,7 +40,7 @@ export class WebAclAuthorizer extends Authorizer {
    */
   public async handle(input: AuthorizerArgs): Promise<void> {
     const store = await this.getAclRecursive(input.identifier);
-    if (await this.aclManager.isAcl(input.identifier)) {
+    if (await this.aclManager.isAclDocument(input.identifier)) {
       this.checkPermission(input.credentials, store, 'control');
     } else {
       (Object.keys(input.permissions) as (keyof PermissionSet)[]).forEach((key): void => {
@@ -117,11 +117,13 @@ export class WebAclAuthorizer extends Authorizer {
   private async getAclRecursive(id: ResourceIdentifier, recurse?: boolean): Promise<Store> {
     this.logger.debug(`Trying to read the direct ACL document of ${id.path}`);
     try {
-      const acl = await this.aclManager.getAcl(id);
+      const acl = await this.aclManager.getAclDocument(id);
       this.logger.debug(`Trying to read the ACL document ${acl.path}`);
       const data = await this.resourceStore.getRepresentation(acl, { type: [{ value: INTERNAL_QUADS, weight: 1 }]});
       this.logger.info(`Reading ACL statements from ${acl.path}`);
-      return this.filterData(data, recurse ? ACL.default : ACL.accessTo, id.path);
+
+      const resourceId = await this.aclManager.getAclConstrainedResource(id);
+      return this.filterData(data, recurse ? ACL.default : ACL.accessTo, resourceId.path);
     } catch (error: unknown) {
       if (error instanceof NotFoundHttpError) {
         this.logger.debug(`No direct ACL document found for ${id.path}`);
diff --git a/src/init/Setup.ts b/src/init/Setup.ts
index db799fc7e..a8437a096 100644
--- a/src/init/Setup.ts
+++ b/src/init/Setup.ts
@@ -59,7 +59,7 @@ export class Setup {
     acl:mode        acl:Control;
     acl:accessTo    <${this.base}>;
     acl:default     <${this.base}>.`;
-      const baseAclId = await this.aclManager.getAcl({ path: this.base });
+      const baseAclId = await this.aclManager.getAclDocument({ path: this.base });
       const metadata = new RepresentationMetadata(baseAclId.path, { [CONTENT_TYPE]: TEXT_TURTLE });
       await this.store.setRepresentation(
         baseAclId,


Test patch:
diff --git a/test/deploy/validate-package.sh b/test/deploy/validate-package.sh
index e9e0ac5ae..c70ae8611 100755
--- a/test/deploy/validate-package.sh
+++ b/test/deploy/validate-package.sh
@@ -17,7 +17,7 @@ if [ -z $PID ]; then
 else
   for i in {1..10}; do
     sleep 1
-    if curl -s localhost:8888; then
+    if curl -s -f localhost:8888; then
       echo "Server reached"
       FAILURE=0
       break
diff --git a/test/unit/authorization/UrlBasedAclManager.test.ts b/test/unit/authorization/UrlBasedAclManager.test.ts
index 11b666f93..522579afb 100644
--- a/test/unit/authorization/UrlBasedAclManager.test.ts
+++ b/test/unit/authorization/UrlBasedAclManager.test.ts
@@ -3,18 +3,32 @@ import { UrlBasedAclManager } from '../../../src/authorization/UrlBasedAclManage
 describe('An UrlBasedAclManager', (): void => {
   const manager = new UrlBasedAclManager();
 
-  it('generates acl URLs by adding an .acl extension.', async(): Promise<void> => {
-    await expect(manager.getAcl({ path: '/foo/bar' })).resolves.toEqual({ path: '/foo/bar.acl' });
+  describe('#getAcl', (): void => {
+    it('generates acl URLs by adding an .acl extension.', async(): Promise<void> => {
+      await expect(manager.getAclDocument({ path: '/foo/bar' })).resolves.toEqual({ path: '/foo/bar.acl' });
+    });
+
+    it('returns the identifier if the input is already an acl resource.', async(): Promise<void> => {
+      await expect(manager.getAclDocument({ path: '/foo/bar.acl' })).resolves.toEqual({ path: '/foo/bar.acl' });
+    });
   });
 
-  it('returns the identifier if the input is already an acl file.', async(): Promise<void> => {
-    await expect(manager.getAcl({ path: '/foo/bar.acl' })).resolves.toEqual({ path: '/foo/bar.acl' });
+  describe('#isAcl', (): void => {
+    it('checks if a resource is an acl resource by looking at the extension.', async(): Promise<void> => {
+      await expect(manager.isAclDocument({ path: '/foo/bar' })).resolves.toBeFalsy();
+      await expect(manager.isAclDocument({ path: '/foo/bar/' })).resolves.toBeFalsy();
+      await expect(manager.isAclDocument({ path: '/foo/bar.acl' })).resolves.toBeTruthy();
+      await expect(manager.isAclDocument({ path: '/foo/bar.acl/' })).resolves.toBeTruthy();
+    });
   });
 
-  it('checks if a resource is an acl file by looking at the extension.', async(): Promise<void> => {
-    await expect(manager.isAcl({ path: '/foo/bar' })).resolves.toBeFalsy();
-    await expect(manager.isAcl({ path: '/foo/bar/' })).resolves.toBeFalsy();
-    await expect(manager.isAcl({ path: '/foo/bar.acl' })).resolves.toBeTruthy();
-    await expect(manager.isAcl({ path: '/foo/bar.acl/' })).resolves.toBeTruthy();
+  describe('#getResource', (): void => {
+    it('generates non-acl resource URLs by removing the .acl extension.', async(): Promise<void> => {
+      await expect(manager.getAclConstrainedResource({ path: '/foo/bar.acl' })).resolves.toEqual({ path: '/foo/bar' });
+    });
+
+    it('returns the identifier if the input is already a non-acl resource.', async(): Promise<void> => {
+      await expect(manager.getAclConstrainedResource({ path: '/foo/bar' })).resolves.toEqual({ path: '/foo/bar' });
+    });
   });
 });
diff --git a/test/unit/authorization/WebAclAuthorizer.test.ts b/test/unit/authorization/WebAclAuthorizer.test.ts
index e2364474e..fb9354cff 100644
--- a/test/unit/authorization/WebAclAuthorizer.test.ts
+++ b/test/unit/authorization/WebAclAuthorizer.test.ts
@@ -19,9 +19,11 @@ const acl = 'http://www.w3.org/ns/auth/acl#';
 describe('A WebAclAuthorizer', (): void => {
   let authorizer: WebAclAuthorizer;
   const aclManager: AclManager = {
-    getAcl: async(id: ResourceIdentifier): Promise<ResourceIdentifier> =>
+    getAclDocument: async(id: ResourceIdentifier): Promise<ResourceIdentifier> =>
       id.path.endsWith('.acl') ? id : { path: `${id.path}.acl` },
-    isAcl: async(id: ResourceIdentifier): Promise<boolean> => id.path.endsWith('.acl'),
+    isAclDocument: async(id: ResourceIdentifier): Promise<boolean> => id.path.endsWith('.acl'),
+    getAclConstrainedResource: async(id: ResourceIdentifier): Promise<ResourceIdentifier> =>
+      !id.path.endsWith('.acl') ? id : { path: id.path.slice(0, -4) },
   };
   let permissions: PermissionSet;
   let credentials: Credentials;
@@ -134,9 +136,9 @@ describe('A WebAclAuthorizer', (): void => {
         quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Control`)),
       ]) } as Representation),
     } as unknown as ResourceStore;
-    identifier = await aclManager.getAcl(identifier);
+    const aclIdentifier = await aclManager.getAclDocument(identifier);
     authorizer = new WebAclAuthorizer(aclManager, store);
-    await expect(authorizer.handle({ identifier, permissions, credentials })).resolves.toBeUndefined();
+    await expect(authorizer.handle({ identifier: aclIdentifier, permissions, credentials })).resolves.toBeUndefined();
   });
 
   it('errors if an agent tries to edit the acl file without control permissions.', async(): Promise<void> => {
@@ -149,7 +151,7 @@ describe('A WebAclAuthorizer', (): void => {
         quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)),
       ]) } as Representation),
     } as unknown as ResourceStore;
-    identifier = await aclManager.getAcl(identifier);
+    identifier = await aclManager.getAclDocument(identifier);
     authorizer = new WebAclAuthorizer(aclManager, store);
     await expect(authorizer.handle({ identifier, permissions, credentials })).rejects.toThrow(ForbiddenHttpError);
   });
diff --git a/test/unit/init/Setup.test.ts b/test/unit/init/Setup.test.ts
index 724be4164..822cfa15c 100644
--- a/test/unit/init/Setup.test.ts
+++ b/test/unit/init/Setup.test.ts
@@ -1,3 +1,4 @@
+import type { AclManager } from '../../../src/authorization/AclManager';
 import { Setup } from '../../../src/init/Setup';
 import type { ResourceIdentifier } from '../../../src/ldp/representation/ResourceIdentifier';
 import { VoidLoggerFactory } from '../../../src/logging/VoidLoggerFactory';
@@ -5,15 +6,15 @@ import { VoidLoggerFactory } from '../../../src/logging/VoidLoggerFactory';
 describe('Setup', (): void => {
   let serverFactory: any;
   let store: any;
-  let aclManager: any;
+  let aclManager: AclManager;
   let setup: Setup;
   beforeEach(async(): Promise<void> => {
     store = {
       setRepresentation: jest.fn(async(): Promise<void> => undefined),
     };
     aclManager = {
-      getAcl: jest.fn(async(): Promise<ResourceIdentifier> => ({ path: 'http://test.com/.acl' })),
-    };
+      getAclDocument: jest.fn(async(): Promise<ResourceIdentifier> => ({ path: 'http://test.com/.acl' })),
+    } as any;
     serverFactory = {
       startServer: jest.fn(),
     };
@@ -27,7 +28,7 @@ describe('Setup', (): void => {
 
   it('invokes ACL initialization.', async(): Promise<void> => {
     await setup.setup();
-    expect(aclManager.getAcl).toHaveBeenCalledWith({ path: 'http://localhost:3000/' });
+    expect(aclManager.getAclDocument).toHaveBeenCalledWith({ path: 'http://localhost:3000/' });
     expect(store.setRepresentation).toHaveBeenCalledTimes(1);
   });
 });