From e86e0cf36bb69858c514352c84b5ddc1900633f7 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Mon, 28 Feb 2022 11:46:38 +0100 Subject: [PATCH] feat: Return 404 for read/delete requests if there is no resource --- RELEASE_NOTES.md | 2 +- config/ldp/handler/components/authorizer.json | 3 +- .../PermissionBasedAuthorizer.ts | 32 +++++++++++++++++-- .../PermissionBasedAuthorizer.test.ts | 17 +++++++++- 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index e9f3f8ef7b..305a1bed1f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -23,7 +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. +- Several `ModesExtractor`s `PermissionBasedAuthorizer` now take a `ResourceSet` as constructor parameter. ## v3.0.0 ### New features diff --git a/config/ldp/handler/components/authorizer.json b/config/ldp/handler/components/authorizer.json index a962ef9549..0272afa993 100644 --- a/config/ldp/handler/components/authorizer.json +++ b/config/ldp/handler/components/authorizer.json @@ -4,7 +4,8 @@ { "comment": "Matches requested permissions with those available.", "@id": "urn:solid-server:default:Authorizer", - "@type": "PermissionBasedAuthorizer" + "@type": "PermissionBasedAuthorizer", + "resourceSet": { "@id": "urn:solid-server:default:CachedResourceSet" } } ] } diff --git a/src/authorization/PermissionBasedAuthorizer.ts b/src/authorization/PermissionBasedAuthorizer.ts index a8153d903b..d71eacce3b 100644 --- a/src/authorization/PermissionBasedAuthorizer.ts +++ b/src/authorization/PermissionBasedAuthorizer.ts @@ -1,10 +1,13 @@ import type { CredentialSet } from '../authentication/Credentials'; import { getLoggerFor } from '../logging/LogUtil'; +import type { ResourceSet } from '../storage/ResourceSet'; import { ForbiddenHttpError } from '../util/errors/ForbiddenHttpError'; +import { NotFoundHttpError } from '../util/errors/NotFoundHttpError'; import { UnauthorizedHttpError } from '../util/errors/UnauthorizedHttpError'; import type { AuthorizerInput } from './Authorizer'; import { Authorizer } from './Authorizer'; -import type { AccessMode, PermissionSet } from './permissions/Permissions'; +import type { PermissionSet } from './permissions/Permissions'; +import { AccessMode } from './permissions/Permissions'; /** * Authorizer that bases its decision on the output it gets from its PermissionReader. @@ -15,14 +18,39 @@ import type { AccessMode, PermissionSet } from './permissions/Permissions'; export class PermissionBasedAuthorizer extends Authorizer { protected readonly logger = getLoggerFor(this); + private readonly resourceSet: ResourceSet; + + /** + * The existence of the target resource determines the output status code for certain situations. + * 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 handle(input: AuthorizerInput): Promise { const { credentials, modes, identifier, permissionSet } = input; const modeString = [ ...modes ].join(','); this.logger.debug(`Checking if ${credentials.agent?.webId} has ${modeString} permissions for ${identifier.path}`); + // Ensure all required modes are within the agent's permissions. for (const mode of modes) { - this.requireModePermission(credentials, permissionSet, mode); + try { + this.requireModePermission(credentials, permissionSet, mode); + } catch (error: unknown) { + // If we know the operation will return a 404 regardless (= resource does not exist and is not being created), + // and the agent is allowed to know about its existence (= the agent has Read permissions), + // then immediately send the 404 here, as it makes any other agent permissions irrelevant. + const exposeExistence = this.hasModePermission(permissionSet, AccessMode.read); + if (exposeExistence && !modes.has(AccessMode.create) && !await this.resourceSet.hasResource(identifier)) { + throw new NotFoundHttpError(); + } + // Otherwise, deny access based on existing grounds. + throw error; + } } this.logger.debug(`${JSON.stringify(credentials)} has ${modeString} permissions for ${identifier.path}`); } diff --git a/test/unit/authorization/PermissionBasedAuthorizer.test.ts b/test/unit/authorization/PermissionBasedAuthorizer.test.ts index b29aad2126..cae015848d 100644 --- a/test/unit/authorization/PermissionBasedAuthorizer.test.ts +++ b/test/unit/authorization/PermissionBasedAuthorizer.test.ts @@ -2,11 +2,14 @@ import { CredentialGroup } from '../../../src/authentication/Credentials'; import type { AuthorizerInput } from '../../../src/authorization/Authorizer'; import { PermissionBasedAuthorizer } from '../../../src/authorization/PermissionBasedAuthorizer'; import { AccessMode } from '../../../src/authorization/permissions/Permissions'; +import type { ResourceSet } from '../../../src/storage/ResourceSet'; import { ForbiddenHttpError } from '../../../src/util/errors/ForbiddenHttpError'; +import { NotFoundHttpError } from '../../../src/util/errors/NotFoundHttpError'; import { UnauthorizedHttpError } from '../../../src/util/errors/UnauthorizedHttpError'; describe('A PermissionBasedAuthorizer', (): void => { let input: AuthorizerInput; + let resourceSet: jest.Mocked; let authorizer: PermissionBasedAuthorizer; beforeEach(async(): Promise => { @@ -16,8 +19,11 @@ describe('A PermissionBasedAuthorizer', (): void => { permissionSet: {}, credentials: {}, }; + resourceSet = { + hasResource: jest.fn().mockResolvedValue(true), + }; - authorizer = new PermissionBasedAuthorizer(); + authorizer = new PermissionBasedAuthorizer(resourceSet); }); it('can handle any input.', async(): Promise => { @@ -53,4 +59,13 @@ describe('A PermissionBasedAuthorizer', (): void => { it('defaults to empty permissions for the Authorization.', async(): Promise => { await expect(authorizer.handle(input)).resolves.toBeUndefined(); }); + + it('throws a 404 in case the target resource does not exist and would not be written to.', async(): Promise => { + resourceSet.hasResource.mockResolvedValueOnce(false); + input.modes = new Set([ AccessMode.delete ]); + input.permissionSet = { + [CredentialGroup.public]: { read: true }, + }; + await expect(authorizer.handle(input)).rejects.toThrow(NotFoundHttpError); + }); });