From 2ae5924dde55c53c8c58e447a3e3734e993287fb Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 23 Feb 2022 15:19:20 +0100 Subject: [PATCH] feat: Pass access modes to PermissionReaders This allows PermissionReaders to potentially only check the necessary access modes for potential performance optimization. --- RELEASE_NOTES.md | 1 + src/authorization/PermissionReader.ts | 8 +++- src/authorization/WebAclReader.ts | 3 +- src/server/AuthorizingHttpHandler.ts | 2 +- .../authorization/AllStaticReader.test.ts | 4 +- .../authorization/AuxiliaryReader.test.ts | 29 +++++++------- .../OwnerPermissionReader.test.ts | 17 +++++--- .../authorization/PathBasedReader.test.ts | 1 + .../UnionPermissionReader.test.ts | 3 +- test/unit/authorization/WebAclReader.test.ts | 39 ++++++++++++------- .../server/AuthorizingHttpHandler.test.ts | 2 +- 11 files changed, 69 insertions(+), 40 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index f8f578f2a..66749ddee 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -20,6 +20,7 @@ The following changes are relevant for v3 custom configs that replaced certain f ### Interface changes These changes are relevant if you wrote custom modules for the server that depend on existing interfaces. - The output of `parseContentType` in `HeaderUtil` was changed to include parameters. +- `PermissionReader`s take an additional `modes` parameter as input. ## v3.0.0 ### New features diff --git a/src/authorization/PermissionReader.ts b/src/authorization/PermissionReader.ts index 7ec777c53..c615b6513 100644 --- a/src/authorization/PermissionReader.ts +++ b/src/authorization/PermissionReader.ts @@ -1,7 +1,7 @@ import type { CredentialSet } from '../authentication/Credentials'; import type { ResourceIdentifier } from '../http/representation/ResourceIdentifier'; import { AsyncHandler } from '../util/handlers/AsyncHandler'; -import type { PermissionSet } from './permissions/Permissions'; +import type { AccessMode, PermissionSet } from './permissions/Permissions'; export interface PermissionReaderInput { /** @@ -12,6 +12,12 @@ export interface PermissionReaderInput { * Identifier of the resource that will be read/modified. */ identifier: ResourceIdentifier; + /** + * This is the minimum set of access modes the output needs to contain, + * allowing the handler to limit its search space to this set. + * However, non-exhaustive information about other access modes can still be returned. + */ + modes: Set; } /** diff --git a/src/authorization/WebAclReader.ts b/src/authorization/WebAclReader.ts index abdd1aa24..4a2e91796 100644 --- a/src/authorization/WebAclReader.ts +++ b/src/authorization/WebAclReader.ts @@ -64,7 +64,8 @@ export class WebAclReader extends PermissionReader { const isAcl = this.aclStrategy.isAuxiliaryIdentifier(identifier); const mainIdentifier = isAcl ? this.aclStrategy.getSubjectIdentifier(identifier) : identifier; - // Determine the full authorization for the agent granted by the applicable ACL + // Determine the full authorization for the agent granted by the applicable ACL. + // Note that we don't filter on input modes as all results are needed for the WAC-Allow header. const acl = await this.getAclRecursive(mainIdentifier); return this.createPermissions(credentials, acl, isAcl); } diff --git a/src/server/AuthorizingHttpHandler.ts b/src/server/AuthorizingHttpHandler.ts index af8b56aff..b23ea3584 100644 --- a/src/server/AuthorizingHttpHandler.ts +++ b/src/server/AuthorizingHttpHandler.ts @@ -66,7 +66,7 @@ export class AuthorizingHttpHandler extends OperationHttpHandler { const modes = await this.modesExtractor.handleSafe(operation); this.logger.verbose(`Required modes are read: ${[ ...modes ].join(',')}`); - const permissionSet = await this.permissionReader.handleSafe({ credentials, identifier: operation.target }); + const permissionSet = await this.permissionReader.handleSafe({ credentials, identifier: operation.target, modes }); this.logger.verbose(`Available permissions are ${JSON.stringify(permissionSet)}`); try { diff --git a/test/unit/authorization/AllStaticReader.test.ts b/test/unit/authorization/AllStaticReader.test.ts index 0630b37c5..057eb69b5 100644 --- a/test/unit/authorization/AllStaticReader.test.ts +++ b/test/unit/authorization/AllStaticReader.test.ts @@ -23,12 +23,12 @@ describe('An AllStaticReader', (): void => { it('always returns permissions matching the given allow parameter.', async(): Promise => { let authorizer = new AllStaticReader(true); - await expect(authorizer.handle({ credentials, identifier })).resolves.toEqual({ + await expect(authorizer.handle({ credentials, identifier, modes: new Set() })).resolves.toEqual({ [CredentialGroup.agent]: getPermissions(true), }); authorizer = new AllStaticReader(false); - await expect(authorizer.handle({ credentials, identifier })).resolves.toEqual({ + await expect(authorizer.handle({ credentials, identifier, modes: new Set() })).resolves.toEqual({ [CredentialGroup.agent]: getPermissions(false), }); }); diff --git a/test/unit/authorization/AuxiliaryReader.test.ts b/test/unit/authorization/AuxiliaryReader.test.ts index 422b25402..6d35ea35a 100644 --- a/test/unit/authorization/AuxiliaryReader.test.ts +++ b/test/unit/authorization/AuxiliaryReader.test.ts @@ -1,7 +1,7 @@ import { CredentialGroup } from '../../../src/authentication/Credentials'; import { AuxiliaryReader } from '../../../src/authorization/AuxiliaryReader'; import type { PermissionReader } from '../../../src/authorization/PermissionReader'; -import type { PermissionSet } from '../../../src/authorization/permissions/Permissions'; +import type { AccessMode, PermissionSet } from '../../../src/authorization/permissions/Permissions'; import type { AuxiliaryStrategy } from '../../../src/http/auxiliary/AuxiliaryStrategy'; import type { ResourceIdentifier } from '../../../src/http/representation/ResourceIdentifier'; import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; @@ -9,6 +9,7 @@ import { NotImplementedHttpError } from '../../../src/util/errors/NotImplemented describe('An AuxiliaryReader', (): void => { const suffix = '.dummy'; const credentials = {}; + const modes = new Set(); const subjectIdentifier = { path: 'http://test.com/foo' }; const auxiliaryIdentifier = { path: 'http://test.com/foo.dummy' }; const permissionSet: PermissionSet = { [CredentialGroup.agent]: { read: true }}; @@ -33,50 +34,50 @@ describe('An AuxiliaryReader', (): void => { }); it('can handle auxiliary resources if the source supports the subject resource.', async(): Promise => { - await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials, modes })) .resolves.toBeUndefined(); expect(source.canHandle).toHaveBeenLastCalledWith( - { identifier: subjectIdentifier, credentials }, + { identifier: subjectIdentifier, credentials, modes }, ); - await expect(reader.canHandle({ identifier: subjectIdentifier, credentials })) + await expect(reader.canHandle({ identifier: subjectIdentifier, credentials, modes })) .rejects.toThrow(NotImplementedHttpError); strategy.usesOwnAuthorization.mockReturnValueOnce(true); - await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials, modes })) .rejects.toThrow(NotImplementedHttpError); source.canHandle.mockRejectedValue(new Error('no source support')); - await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials, modes })) .rejects.toThrow('no source support'); }); it('handles resources by sending the updated parameters to the source.', async(): Promise => { - await expect(reader.handle({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.handle({ identifier: auxiliaryIdentifier, credentials, modes })) .resolves.toBe(permissionSet); expect(source.handle).toHaveBeenLastCalledWith( - { identifier: subjectIdentifier, credentials }, + { identifier: subjectIdentifier, credentials, modes }, ); // Safety checks are not present when calling `handle` - await expect(reader.handle({ identifier: subjectIdentifier, credentials })) + await expect(reader.handle({ identifier: subjectIdentifier, credentials, modes })) .rejects.toThrow(NotImplementedHttpError); }); it('combines both checking and handling when calling handleSafe.', async(): Promise => { - await expect(reader.handleSafe({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.handleSafe({ identifier: auxiliaryIdentifier, credentials, modes })) .resolves.toBe(permissionSet); expect(source.handleSafe).toHaveBeenLastCalledWith( - { identifier: subjectIdentifier, credentials }, + { identifier: subjectIdentifier, credentials, modes }, ); - await expect(reader.handleSafe({ identifier: subjectIdentifier, credentials })) + await expect(reader.handleSafe({ identifier: subjectIdentifier, credentials, modes })) .rejects.toThrow(NotImplementedHttpError); strategy.usesOwnAuthorization.mockReturnValueOnce(true); - await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.canHandle({ identifier: auxiliaryIdentifier, credentials, modes })) .rejects.toThrow(NotImplementedHttpError); source.handleSafe.mockRejectedValue(new Error('no source support')); - await expect(reader.handleSafe({ identifier: auxiliaryIdentifier, credentials })) + await expect(reader.handleSafe({ identifier: auxiliaryIdentifier, credentials, modes })) .rejects.toThrow('no source support'); }); }); diff --git a/test/unit/authorization/OwnerPermissionReader.test.ts b/test/unit/authorization/OwnerPermissionReader.test.ts index b8b4dc3a2..765c13fd7 100644 --- a/test/unit/authorization/OwnerPermissionReader.test.ts +++ b/test/unit/authorization/OwnerPermissionReader.test.ts @@ -1,6 +1,8 @@ import type { CredentialSet } from '../../../src/authentication/Credentials'; import { CredentialGroup } from '../../../src/authentication/Credentials'; import { OwnerPermissionReader } from '../../../src/authorization/OwnerPermissionReader'; +import { AclMode } from '../../../src/authorization/permissions/AclPermission'; +import type { AccessMode } from '../../../src/authorization/permissions/Permissions'; import type { AuxiliaryIdentifierStrategy } from '../../../src/http/auxiliary/AuxiliaryIdentifierStrategy'; import type { ResourceIdentifier } from '../../../src/http/representation/ResourceIdentifier'; import type { @@ -13,6 +15,7 @@ describe('An OwnerPermissionReader', (): void => { const podBaseUrl = 'http://test.com/alice/'; let credentials: CredentialSet; let identifier: ResourceIdentifier; + let modes: Set; let settings: AccountSettings; let accountStore: jest.Mocked; let aclStrategy: jest.Mocked; @@ -23,6 +26,8 @@ describe('An OwnerPermissionReader', (): void => { identifier = { path: `${podBaseUrl}.acl` }; + modes = new Set([ AclMode.control ]) as Set; + settings = { useIdp: true, podBaseUrl, @@ -46,31 +51,31 @@ describe('An OwnerPermissionReader', (): void => { it('returns empty permissions for non-ACL resources.', async(): Promise => { identifier.path = podBaseUrl; - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({}); + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({}); }); it('returns empty permissions if there is no agent WebID.', async(): Promise => { credentials = {}; - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({}); + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({}); }); it('returns empty permissions if the agent has no account.', async(): Promise => { credentials.agent!.webId = 'http://test.com/someone/else'; - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({}); + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({}); }); it('returns empty permissions if the account has no pod.', async(): Promise => { delete settings.podBaseUrl; - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({}); + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({}); }); it('returns empty permissions if the target identifier is not in the pod.', async(): Promise => { identifier.path = 'http://somewhere.else/.acl'; - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({}); + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({}); }); it('returns full permissions if the owner is accessing an ACL resource in their pod.', async(): Promise => { - await expect(reader.handle({ credentials, identifier })).resolves.toEqual({ + await expect(reader.handle({ credentials, identifier, modes })).resolves.toEqual({ [CredentialGroup.agent]: { read: true, write: true, diff --git a/test/unit/authorization/PathBasedReader.test.ts b/test/unit/authorization/PathBasedReader.test.ts index 6b72d5047..d051aac4f 100644 --- a/test/unit/authorization/PathBasedReader.test.ts +++ b/test/unit/authorization/PathBasedReader.test.ts @@ -15,6 +15,7 @@ describe('A PathBasedReader', (): void => { input = { identifier: { path: `${baseUrl}first` }, credentials: {}, + modes: new Set(), }; readers = [ diff --git a/test/unit/authorization/UnionPermissionReader.test.ts b/test/unit/authorization/UnionPermissionReader.test.ts index c93452d2d..0b0bbea65 100644 --- a/test/unit/authorization/UnionPermissionReader.test.ts +++ b/test/unit/authorization/UnionPermissionReader.test.ts @@ -3,7 +3,8 @@ import type { PermissionReader, PermissionReaderInput } from '../../../src/autho import { UnionPermissionReader } from '../../../src/authorization/UnionPermissionReader'; describe('A UnionPermissionReader', (): void => { - const input: PermissionReaderInput = { credentials: {}, identifier: { path: 'http://test.com/foo' }}; + const input: PermissionReaderInput = + { credentials: {}, identifier: { path: 'http://test.com/foo' }, modes: new Set() }; let readers: jest.Mocked[]; let unionReader: UnionPermissionReader; diff --git a/test/unit/authorization/WebAclReader.test.ts b/test/unit/authorization/WebAclReader.test.ts index 6ee4fb2df..4e17388cc 100644 --- a/test/unit/authorization/WebAclReader.test.ts +++ b/test/unit/authorization/WebAclReader.test.ts @@ -1,7 +1,10 @@ import { DataFactory } from 'n3'; -import { CredentialGroup } from '../../../src/authentication/Credentials'; import type { CredentialSet } from '../../../src/authentication/Credentials'; +import { CredentialGroup } from '../../../src/authentication/Credentials'; import type { AccessChecker } from '../../../src/authorization/access/AccessChecker'; +import type { PermissionReaderInput } from '../../../src/authorization/PermissionReader'; +import { AclMode } from '../../../src/authorization/permissions/AclPermission'; +import { AccessMode } from '../../../src/authorization/permissions/Permissions'; import { WebAclReader } from '../../../src/authorization/WebAclReader'; import type { AuxiliaryIdentifierStrategy } from '../../../src/http/auxiliary/AuxiliaryIdentifierStrategy'; import { BasicRepresentation } from '../../../src/http/representation/BasicRepresentation'; @@ -31,12 +34,20 @@ describe('A WebAclReader', (): void => { const identifierStrategy = new SingleRootIdentifierStrategy('http://test.com/'); let credentials: CredentialSet; let identifier: ResourceIdentifier; + let modes: Set; + let input: PermissionReaderInput; let accessChecker: jest.Mocked; beforeEach(async(): Promise => { credentials = { [CredentialGroup.public]: {}, [CredentialGroup.agent]: {}}; identifier = { path: 'http://test.com/foo' }; + modes = new Set([ + AccessMode.read, AccessMode.write, AccessMode.append, AccessMode.create, AccessMode.delete, AclMode.control, + ]) as Set; + + input = { credentials, identifier, modes }; + store = { getRepresentation: jest.fn().mockResolvedValue(new BasicRepresentation([ quad(nn('auth'), nn(`${rdf}type`), nn(`${acl}Authorization`)), @@ -55,8 +66,8 @@ describe('A WebAclReader', (): void => { }); it('returns undefined permissions for undefined credentials.', async(): Promise => { - credentials = {}; - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + input.credentials = {}; + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: {}, [CredentialGroup.agent]: {}, }); @@ -69,7 +80,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), ]) } as Representation); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { read: true }, [CredentialGroup.agent]: { read: true }, }); @@ -82,7 +93,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}accessTo`), nn('somewhereElse')), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), ]) } as Representation); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: {}, [CredentialGroup.agent]: {}, }); @@ -96,7 +107,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}fakeMode1`)), ]) } as Representation); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { read: true }, [CredentialGroup.agent]: { read: true }, }); @@ -114,7 +125,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), ], INTERNAL_QUADS); }); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { read: true }, [CredentialGroup.agent]: { read: true }, }); @@ -122,14 +133,14 @@ describe('A WebAclReader', (): void => { it('re-throws ResourceStore errors as internal errors.', async(): Promise => { store.getRepresentation.mockRejectedValue(new Error('TEST!')); - const promise = reader.handle({ identifier, credentials }); + const promise = reader.handle(input); await expect(promise).rejects.toThrow(`Error reading ACL for ${identifier.path}: TEST!`); await expect(promise).rejects.toThrow(InternalServerError); }); it('errors if the root container has no corresponding acl document.', async(): Promise => { store.getRepresentation.mockRejectedValue(new NotFoundHttpError()); - const promise = reader.handle({ identifier, credentials }); + const promise = reader.handle(input); await expect(promise).rejects.toThrow('No ACL document found for root container'); await expect(promise).rejects.toThrow(ForbiddenHttpError); }); @@ -140,7 +151,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Write`)), ]) } as Representation); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { write: true, append: true, create: true, delete: true }, [CredentialGroup.agent]: { write: true, append: true, create: true, delete: true }, }); @@ -152,7 +163,8 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Control`)), ]) } as Representation); - await expect(reader.handle({ identifier: { path: `${identifier.path}.acl` }, credentials })).resolves.toEqual({ + input.identifier = { path: `${identifier.path}.acl` }; + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { read: true, write: true, append: true, create: true, delete: true, control: true }, [CredentialGroup.agent]: { read: true, write: true, append: true, create: true, delete: true, control: true }, }); @@ -164,7 +176,8 @@ describe('A WebAclReader', (): void => { quad(nn('auth'), nn(`${acl}accessTo`), nn(identifier.path)), quad(nn('auth'), nn(`${acl}mode`), nn(`${acl}Read`)), ]) } as Representation); - await expect(reader.handle({ identifier: { path: `${identifier.path}.acl` }, credentials })).resolves.toEqual({ + input.identifier = { path: `${identifier.path}.acl` }; + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: {}, [CredentialGroup.agent]: {}, }); @@ -185,7 +198,7 @@ describe('A WebAclReader', (): void => { quad(nn('auth2'), nn(`${acl}mode`), nn(`${acl}Control`)), ]) } as Representation); - await expect(reader.handle({ identifier, credentials })).resolves.toEqual({ + await expect(reader.handle(input)).resolves.toEqual({ [CredentialGroup.public]: { read: true }, [CredentialGroup.agent]: { control: true }, }); diff --git a/test/unit/server/AuthorizingHttpHandler.test.ts b/test/unit/server/AuthorizingHttpHandler.test.ts index 7d11ba6b8..0f2f91a25 100644 --- a/test/unit/server/AuthorizingHttpHandler.test.ts +++ b/test/unit/server/AuthorizingHttpHandler.test.ts @@ -62,7 +62,7 @@ describe('An AuthorizingHttpHandler', (): void => { expect(modesExtractor.handleSafe).toHaveBeenCalledTimes(1); expect(modesExtractor.handleSafe).toHaveBeenLastCalledWith(operation); expect(permissionReader.handleSafe).toHaveBeenCalledTimes(1); - expect(permissionReader.handleSafe).toHaveBeenLastCalledWith({ credentials, identifier: operation.target }); + expect(permissionReader.handleSafe).toHaveBeenLastCalledWith({ credentials, identifier: operation.target, modes }); expect(authorizer.handleSafe).toHaveBeenCalledTimes(1); expect(authorizer.handleSafe) .toHaveBeenLastCalledWith({ credentials, identifier: operation.target, modes, permissionSet });