From ee312910d7f6bc08bd3176168fd6876ffc3d0146 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 18 Nov 2020 16:48:12 +0100 Subject: [PATCH] fix: Correctly handle acl behaviour for acl identifiers --- src/authorization/AclManager.ts | 35 ++++++++++++------- src/authorization/UrlBasedAclManager.ts | 26 +++++++------- src/authorization/WebAclAuthorizer.ts | 8 +++-- src/init/Setup.ts | 2 +- .../authorization/UrlBasedAclManager.test.ts | 32 ++++++++++++----- .../authorization/WebAclAuthorizer.test.ts | 12 ++++--- test/unit/init/Setup.test.ts | 9 ++--- 7 files changed, 77 insertions(+), 47 deletions(-) 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; + getAclDocument: (id: ResourceIdentifier) => Promise; /** - * 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; + isAclDocument: (id: ResourceIdentifier) => Promise; + + /** + * 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; } 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 { - return await this.isAcl(id) ? id : { path: `${id.path}.acl` }; + public async getAclDocument(id: ResourceIdentifier): Promise { + return await this.isAclDocument(id) ? id : { path: `${id.path}.acl` }; } - public async isAcl(id: ResourceIdentifier): Promise { + public async isAclDocument(id: ResourceIdentifier): Promise { return /\.acl\/?/u.test(id.path); } + + public async getAclConstrainedResource(id: ResourceIdentifier): Promise { + 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 { 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 { 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, 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 => { - 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 => { + 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 => { + 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 => { - 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 => { + 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 => { - 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 => { + 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 => { + 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 => + getAclDocument: async(id: ResourceIdentifier): Promise => id.path.endsWith('.acl') ? id : { path: `${id.path}.acl` }, - isAcl: async(id: ResourceIdentifier): Promise => id.path.endsWith('.acl'), + isAclDocument: async(id: ResourceIdentifier): Promise => id.path.endsWith('.acl'), + getAclConstrainedResource: async(id: ResourceIdentifier): Promise => + !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 => { @@ -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 => { store = { setRepresentation: jest.fn(async(): Promise => undefined), }; aclManager = { - getAcl: jest.fn(async(): Promise => ({ path: 'http://test.com/.acl' })), - }; + getAclDocument: jest.fn(async(): Promise => ({ path: 'http://test.com/.acl' })), + } as any; serverFactory = { startServer: jest.fn(), }; @@ -27,7 +28,7 @@ describe('Setup', (): void => { it('invokes ACL initialization.', async(): Promise => { await setup.setup(); - expect(aclManager.getAcl).toHaveBeenCalledWith({ path: 'http://localhost:3000/' }); + expect(aclManager.getAclDocument).toHaveBeenCalledWith({ path: 'http://localhost:3000/' }); expect(store.setRepresentation).toHaveBeenCalledTimes(1); }); });