From b0c50b8a7ba3443d8128fcd9967a6086993ebde2 Mon Sep 17 00:00:00 2001 From: Ruben Verborgh Date: Fri, 27 Nov 2020 00:17:40 +0100 Subject: [PATCH] change: Make credential extractors specialized. --- config/presets/ldp/credentials-extractor.json | 16 ++++-- index.ts | 1 + src/authentication/DPoPWebIdExtractor.ts | 37 ++++++++----- .../EmptyCredentialsExtractor.ts | 20 +++++++ src/authentication/UnsecureWebIdExtractor.ts | 17 +++++- .../AuthenticatedDataAccessorBasedConfig.ts | 4 +- test/configs/BasicConfig.ts | 4 +- test/configs/BasicHandlersConfig.ts | 4 +- test/configs/BasicHandlersWithAclConfig.ts | 4 +- test/configs/DataAccessorBasedConfig.ts | 4 +- .../authentication/DPoPWebIdExtractor.test.ts | 55 ++++++++++++------- .../EmptyCredentialsExtractor.test.ts | 20 +++++++ .../UnsecureWebIdExtractor.test.ts | 27 +++++++++ .../authentication/UnsecureWebIdExtractor.ts | 19 ------- 14 files changed, 164 insertions(+), 68 deletions(-) create mode 100644 src/authentication/EmptyCredentialsExtractor.ts create mode 100644 test/unit/authentication/EmptyCredentialsExtractor.test.ts create mode 100644 test/unit/authentication/UnsecureWebIdExtractor.test.ts delete mode 100644 test/unit/authentication/UnsecureWebIdExtractor.ts diff --git a/config/presets/ldp/credentials-extractor.json b/config/presets/ldp/credentials-extractor.json index d4e2b8ace..339544a16 100644 --- a/config/presets/ldp/credentials-extractor.json +++ b/config/presets/ldp/credentials-extractor.json @@ -3,10 +3,18 @@ "@graph": [ { "@id": "urn:solid-server:default:CredentialsExtractor", - "@type": "DPoPWebIdExtractor", - "DPoPWebIdExtractor:_targetExtractor": { - "@id": "urn:solid-server:default:TargetExtractor" - } + "@type": "FirstCompositeHandler", + "FirstCompositeHandler:_handlers": [ + { + "@type": "DPoPWebIdExtractor", + "DPoPWebIdExtractor:_targetExtractor": { + "@id": "urn:solid-server:default:TargetExtractor" + } + }, + { + "@type": "EmptyCredentialsExtractor" + } + ] } ] } diff --git a/index.ts b/index.ts index 189277523..717720e72 100644 --- a/index.ts +++ b/index.ts @@ -2,6 +2,7 @@ export * from './src/authentication/Credentials'; export * from './src/authentication/CredentialsExtractor'; export * from './src/authentication/DPoPWebIdExtractor'; +export * from './src/authentication/EmptyCredentialsExtractor'; export * from './src/authentication/UnsecureWebIdExtractor'; // Authorization diff --git a/src/authentication/DPoPWebIdExtractor.ts b/src/authentication/DPoPWebIdExtractor.ts index dc9675863..8b412e8a4 100644 --- a/src/authentication/DPoPWebIdExtractor.ts +++ b/src/authentication/DPoPWebIdExtractor.ts @@ -2,6 +2,8 @@ import { verify } from 'ts-dpop'; import type { TargetExtractor } from '../ldp/http/TargetExtractor'; import { getLoggerFor } from '../logging/LogUtil'; import type { HttpRequest } from '../server/HttpRequest'; +import { BadRequestHttpError } from '../util/errors/BadRequestHttpError'; +import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; import type { Credentials } from './Credentials'; import { CredentialsExtractor } from './CredentialsExtractor'; @@ -17,20 +19,27 @@ export class DPoPWebIdExtractor extends CredentialsExtractor { this.targetExtractor = targetExtractor; } - public async handle(request: HttpRequest): Promise { - let webID: string | undefined; - const authorizationHeader = request.headers.authorization; - const dpopHeader = request.headers.dpop as string; - if (authorizationHeader && dpopHeader) { - const method = request.method as any; - const resource = await this.targetExtractor.handleSafe(request); - try { - webID = await verify(authorizationHeader, dpopHeader, method, resource.path); - this.logger.info(`Extracted WebID via DPoP token: ${webID}`); - } catch (error: unknown) { - this.logger.warn(`Error verifying WebID via DPoP token: ${(error as Error).message}`); - } + public async canHandle({ headers }: HttpRequest): Promise { + const { authorization } = headers; + if (!authorization || !authorization.startsWith('DPoP ')) { + throw new NotImplementedHttpError('No DPoP Authorization header specified.'); + } + } + + public async handle(request: HttpRequest): Promise { + const { headers: { authorization, dpop }, method } = request; + if (!dpop) { + throw new BadRequestHttpError('No DPoP token specified.'); + } + const resource = await this.targetExtractor.handleSafe(request); + try { + const webID = await verify(authorization as string, dpop as string, method as any, resource.path); + this.logger.info(`Verified WebID via DPoP token: ${webID}`); + return { webID }; + } catch (error: unknown) { + const message = `Error verifying WebID via DPoP token: ${(error as Error).message}`; + this.logger.warn(message); + throw new BadRequestHttpError(message); } - return { webID }; } } diff --git a/src/authentication/EmptyCredentialsExtractor.ts b/src/authentication/EmptyCredentialsExtractor.ts new file mode 100644 index 000000000..d78c85291 --- /dev/null +++ b/src/authentication/EmptyCredentialsExtractor.ts @@ -0,0 +1,20 @@ +import type { HttpRequest } from '../server/HttpRequest'; +import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; +import type { Credentials } from './Credentials'; +import { CredentialsExtractor } from './CredentialsExtractor'; + +/** + * Extracts the empty credentials, indicating an unauthenticated agent. + */ +export class EmptyCredentialsExtractor extends CredentialsExtractor { + public async canHandle({ headers }: HttpRequest): Promise { + const { authorization } = headers; + if (authorization) { + throw new NotImplementedHttpError('Unexpected Authorization scheme.'); + } + } + + public async handle(): Promise { + return {}; + } +} diff --git a/src/authentication/UnsecureWebIdExtractor.ts b/src/authentication/UnsecureWebIdExtractor.ts index f47b98d67..6905128ec 100644 --- a/src/authentication/UnsecureWebIdExtractor.ts +++ b/src/authentication/UnsecureWebIdExtractor.ts @@ -1,4 +1,6 @@ +import { getLoggerFor } from '../logging/LogUtil'; import type { HttpRequest } from '../server/HttpRequest'; +import { NotImplementedHttpError } from '../util/errors/NotImplementedHttpError'; import type { Credentials } from './Credentials'; import { CredentialsExtractor } from './CredentialsExtractor'; @@ -6,7 +8,18 @@ import { CredentialsExtractor } from './CredentialsExtractor'; * Credentials extractor which simply interprets the contents of the Authorization header as a webID. */ export class UnsecureWebIdExtractor extends CredentialsExtractor { - public async handle(input: HttpRequest): Promise { - return { webID: input.headers.authorization }; + protected readonly logger = getLoggerFor(this); + + public async canHandle({ headers }: HttpRequest): Promise { + const { authorization } = headers; + if (!authorization || !authorization.startsWith('WebID ')) { + throw new NotImplementedHttpError('No WebID Authorization header specified.'); + } + } + + public async handle({ headers }: HttpRequest): Promise { + const webID = /^WebID\s+(.*)/u.exec(headers.authorization as string)![1]; + this.logger.info(`Agent unsecurely claims to be ${webID}`); + return { webID }; } } diff --git a/test/configs/AuthenticatedDataAccessorBasedConfig.ts b/test/configs/AuthenticatedDataAccessorBasedConfig.ts index c3005bc2a..2ad66f358 100644 --- a/test/configs/AuthenticatedDataAccessorBasedConfig.ts +++ b/test/configs/AuthenticatedDataAccessorBasedConfig.ts @@ -5,10 +5,10 @@ import type { } from '../../index'; import { AuthenticatedLdpHandler, + EmptyCredentialsExtractor, FirstCompositeHandler, MethodPermissionsExtractor, RdfToQuadConverter, - UnsecureWebIdExtractor, QuadToRdfConverter, } from '../../index'; import type { ServerConfig } from './ServerConfig'; @@ -43,7 +43,7 @@ export class AuthenticatedDataAccessorBasedConfig implements ServerConfig { public getHttpHandler(): HttpHandler { const requestParser = getBasicRequestParser(); - const credentialsExtractor = new UnsecureWebIdExtractor(); + const credentialsExtractor = new EmptyCredentialsExtractor(); const permissionsExtractor = new FirstCompositeHandler([ new MethodPermissionsExtractor(), ]); diff --git a/test/configs/BasicConfig.ts b/test/configs/BasicConfig.ts index 6948bb7fe..6dab8d0bb 100644 --- a/test/configs/BasicConfig.ts +++ b/test/configs/BasicConfig.ts @@ -3,8 +3,8 @@ import type { HttpHandler, import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, + EmptyCredentialsExtractor, MethodPermissionsExtractor, - UnsecureWebIdExtractor, } from '../../index'; import type { ServerConfig } from './ServerConfig'; import { getOperationHandler, getInMemoryResourceStore, getBasicRequestParser, getResponseWriter } from './Util'; @@ -26,7 +26,7 @@ export class BasicConfig implements ServerConfig { public getHttpHandler(): HttpHandler { const requestParser = getBasicRequestParser(); - const credentialsExtractor = new UnsecureWebIdExtractor(); + const credentialsExtractor = new EmptyCredentialsExtractor(); const permissionsExtractor = new MethodPermissionsExtractor(); const authorizer = new AllowEverythingAuthorizer(); diff --git a/test/configs/BasicHandlersConfig.ts b/test/configs/BasicHandlersConfig.ts index 4a0b405ce..ef141eb43 100644 --- a/test/configs/BasicHandlersConfig.ts +++ b/test/configs/BasicHandlersConfig.ts @@ -3,6 +3,7 @@ import type { HttpHandler, import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, + EmptyCredentialsExtractor, FirstCompositeHandler, MethodPermissionsExtractor, QuadToRdfConverter, @@ -10,7 +11,6 @@ import { RdfToQuadConverter, SparqlUpdateBodyParser, SparqlPatchPermissionsExtractor, - UnsecureWebIdExtractor, } from '../../index'; import type { ServerConfig } from './ServerConfig'; @@ -47,7 +47,7 @@ export class BasicHandlersConfig implements ServerConfig { new RawBodyParser(), ]); - const credentialsExtractor = new UnsecureWebIdExtractor(); + const credentialsExtractor = new EmptyCredentialsExtractor(); const permissionsExtractor = new FirstCompositeHandler([ new MethodPermissionsExtractor(), new SparqlPatchPermissionsExtractor(), diff --git a/test/configs/BasicHandlersWithAclConfig.ts b/test/configs/BasicHandlersWithAclConfig.ts index cb1fc6b8b..dfe4d2eb1 100644 --- a/test/configs/BasicHandlersWithAclConfig.ts +++ b/test/configs/BasicHandlersWithAclConfig.ts @@ -2,10 +2,10 @@ import type { HttpHandler, ResourceStore } from '../../index'; import { AuthenticatedLdpHandler, + EmptyCredentialsExtractor, FirstCompositeHandler, MethodPermissionsExtractor, RdfToQuadConverter, - UnsecureWebIdExtractor, QuadToRdfConverter, } from '../../index'; import type { ServerConfig } from './ServerConfig'; @@ -39,7 +39,7 @@ export class BasicHandlersWithAclConfig implements ServerConfig { public getHttpHandler(): HttpHandler { const requestParser = getBasicRequestParser(); - const credentialsExtractor = new UnsecureWebIdExtractor(); + const credentialsExtractor = new EmptyCredentialsExtractor(); const permissionsExtractor = new FirstCompositeHandler([ new MethodPermissionsExtractor(), ]); diff --git a/test/configs/DataAccessorBasedConfig.ts b/test/configs/DataAccessorBasedConfig.ts index 921d4281e..3e7f61d6b 100644 --- a/test/configs/DataAccessorBasedConfig.ts +++ b/test/configs/DataAccessorBasedConfig.ts @@ -6,12 +6,12 @@ import type { import { AllowEverythingAuthorizer, AuthenticatedLdpHandler, + EmptyCredentialsExtractor, FirstCompositeHandler, MethodPermissionsExtractor, QuadToRdfConverter, RawBodyParser, RdfToQuadConverter, - UnsecureWebIdExtractor, } from '../../index'; import type { ServerConfig } from './ServerConfig'; import { @@ -43,7 +43,7 @@ export class DataAccessorBasedConfig implements ServerConfig { // This is for the sake of test coverage, as it could also be just getBasicRequestParser() const requestParser = getBasicRequestParser([ new RawBodyParser() ]); - const credentialsExtractor = new UnsecureWebIdExtractor(); + const credentialsExtractor = new EmptyCredentialsExtractor(); const permissionsExtractor = new FirstCompositeHandler([ new MethodPermissionsExtractor(), ]); diff --git a/test/unit/authentication/DPoPWebIdExtractor.test.ts b/test/unit/authentication/DPoPWebIdExtractor.test.ts index c7deaad29..b50f53c62 100644 --- a/test/unit/authentication/DPoPWebIdExtractor.test.ts +++ b/test/unit/authentication/DPoPWebIdExtractor.test.ts @@ -1,17 +1,12 @@ import { verify } from 'ts-dpop'; import { DPoPWebIdExtractor } from '../../../src/authentication/DPoPWebIdExtractor'; -import { TargetExtractor } from '../../../src/ldp/http/TargetExtractor'; -import type { ResourceIdentifier } from '../../../src/ldp/representation/ResourceIdentifier'; import type { HttpRequest } from '../../../src/server/HttpRequest'; - -class DummyTargetExtractor extends TargetExtractor { - public async handle(): Promise { - return { path: 'http://example.org/foo/bar' }; - } -} +import { BadRequestHttpError } from '../../../src/util/errors/BadRequestHttpError'; +import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; +import { StaticAsyncHandler } from '../../util/StaticAsyncHandler'; describe('A DPoPWebIdExtractor', (): void => { - const targetExtractor = new DummyTargetExtractor(); + const targetExtractor = new StaticAsyncHandler(true, { path: 'http://example.org/foo/bar' }); const webIdExtractor = new DPoPWebIdExtractor(targetExtractor); beforeEach((): void => { @@ -27,8 +22,26 @@ describe('A DPoPWebIdExtractor', (): void => { }, } as any as HttpRequest; - it('returns empty credentials.', async(): Promise => { - await expect(webIdExtractor.handle(request)).resolves.toEqual({}); + it('throws an error.', async(): Promise => { + const result = webIdExtractor.handleSafe(request); + await expect(result).rejects.toThrow(NotImplementedHttpError); + await expect(result).rejects.toThrow('No DPoP Authorization header specified.'); + }); + }); + + describe('on a request with an Authorization header that does not start with DPoP', (): void => { + const request = { + method: 'GET', + headers: { + authorization: 'Other token-1234', + dpop: 'token-5678', + }, + } as any as HttpRequest; + + it('throws an error.', async(): Promise => { + const result = webIdExtractor.handleSafe(request); + await expect(result).rejects.toThrow(NotImplementedHttpError); + await expect(result).rejects.toThrow('No DPoP Authorization header specified.'); }); }); @@ -40,8 +53,10 @@ describe('A DPoPWebIdExtractor', (): void => { }, } as any as HttpRequest; - it('returns empty credentials.', async(): Promise => { - await expect(webIdExtractor.handle(request)).resolves.toEqual({}); + it('throws an error.', async(): Promise => { + const result = webIdExtractor.handleSafe(request); + await expect(result).rejects.toThrow(BadRequestHttpError); + await expect(result).rejects.toThrow('No DPoP token specified.'); }); }); @@ -55,20 +70,20 @@ describe('A DPoPWebIdExtractor', (): void => { } as any as HttpRequest; it('calls the target extractor with the correct parameters.', async(): Promise => { - await webIdExtractor.handle(request); + await webIdExtractor.handleSafe(request); expect(targetExtractor.handle).toHaveBeenCalledTimes(1); expect(targetExtractor.handle).toHaveBeenCalledWith(request); }); it('calls the DPoP verifier with the correct parameters.', async(): Promise => { - await webIdExtractor.handle(request); + await webIdExtractor.handleSafe(request); expect(verify).toHaveBeenCalledTimes(1); expect(verify).toHaveBeenCalledWith('DPoP token-1234', 'token-5678', 'GET', 'http://example.org/foo/bar'); }); it('returns the extracted WebID.', async(): Promise => { - await expect(webIdExtractor.handle(request)).resolves - .toEqual({ webID: 'http://alice.example/card#me' }); + const result = webIdExtractor.handleSafe(request); + await expect(result).resolves.toEqual({ webID: 'http://alice.example/card#me' }); }); }); @@ -87,8 +102,10 @@ describe('A DPoPWebIdExtractor', (): void => { }); }); - it('returns empty credentials.', async(): Promise => { - await expect(webIdExtractor.handle(request)).resolves.toEqual({}); + it('throws an error.', async(): Promise => { + const result = webIdExtractor.handleSafe(request); + await expect(result).rejects.toThrow(BadRequestHttpError); + await expect(result).rejects.toThrow('Error verifying WebID via DPoP token: invalid'); }); }); }); diff --git a/test/unit/authentication/EmptyCredentialsExtractor.test.ts b/test/unit/authentication/EmptyCredentialsExtractor.test.ts new file mode 100644 index 000000000..aa5fc1833 --- /dev/null +++ b/test/unit/authentication/EmptyCredentialsExtractor.test.ts @@ -0,0 +1,20 @@ +import { EmptyCredentialsExtractor } from '../../../src/authentication/EmptyCredentialsExtractor'; +import type { HttpRequest } from '../../../src/server/HttpRequest'; +import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; + +describe('An EmptyCredentialsExtractor', (): void => { + const extractor = new EmptyCredentialsExtractor(); + + it('throws an error if an Authorization header is specified.', async(): Promise => { + const headers = { authorization: 'Other http://alice.example/card#me' }; + const result = extractor.handleSafe({ headers } as HttpRequest); + await expect(result).rejects.toThrow(NotImplementedHttpError); + await expect(result).rejects.toThrow('Unexpected Authorization scheme.'); + }); + + it('returns the empty credentials.', async(): Promise => { + const headers = {}; + const result = extractor.handleSafe({ headers } as HttpRequest); + await expect(result).resolves.toEqual({}); + }); +}); diff --git a/test/unit/authentication/UnsecureWebIdExtractor.test.ts b/test/unit/authentication/UnsecureWebIdExtractor.test.ts new file mode 100644 index 000000000..15505fc2d --- /dev/null +++ b/test/unit/authentication/UnsecureWebIdExtractor.test.ts @@ -0,0 +1,27 @@ +import { UnsecureWebIdExtractor } from '../../../src/authentication/UnsecureWebIdExtractor'; +import type { HttpRequest } from '../../../src/server/HttpRequest'; +import { NotImplementedHttpError } from '../../../src/util/errors/NotImplementedHttpError'; + +describe('An UnsecureWebIdExtractor', (): void => { + const extractor = new UnsecureWebIdExtractor(); + + it('throws an error if no Authorization header is specified.', async(): Promise => { + const headers = {}; + const result = extractor.handleSafe({ headers } as HttpRequest); + await expect(result).rejects.toThrow(NotImplementedHttpError); + await expect(result).rejects.toThrow('No WebID Authorization header specified.'); + }); + + it('throws an error if a non-WebID Authorization header is specified.', async(): Promise => { + const headers = { authorization: 'Other http://alice.example/card#me' }; + const result = extractor.handleSafe({ headers } as HttpRequest); + await expect(result).rejects.toThrow(NotImplementedHttpError); + await expect(result).rejects.toThrow('No WebID Authorization header specified.'); + }); + + it('returns the authorization header as WebID if there is one.', async(): Promise => { + const headers = { authorization: 'WebID http://alice.example/card#me' }; + const result = extractor.handleSafe({ headers } as HttpRequest); + await expect(result).resolves.toEqual({ webID: 'http://alice.example/card#me' }); + }); +}); diff --git a/test/unit/authentication/UnsecureWebIdExtractor.ts b/test/unit/authentication/UnsecureWebIdExtractor.ts deleted file mode 100644 index 15f648b97..000000000 --- a/test/unit/authentication/UnsecureWebIdExtractor.ts +++ /dev/null @@ -1,19 +0,0 @@ -import { UnsecureWebIdExtractor } from '../../../src/authentication/UnsecureWebIdExtractor'; -import type { HttpRequest } from '../../../src/server/HttpRequest'; - -describe('An UnsecureWebIdExtractor', (): void => { - const extractor = new UnsecureWebIdExtractor(); - - it('can handle all input.', async(): Promise => { - await expect(extractor.canHandle({} as HttpRequest)).resolves.toBeUndefined(); - }); - - it('returns undefined if there is no input.', async(): Promise => { - await expect(extractor.handle({ headers: {}} as HttpRequest)).resolves.toEqual({}); - }); - - it('returns the authorization header as webID if there is one.', async(): Promise => { - await expect(extractor.handle({ headers: { authorization: 'test' }} as HttpRequest)) - .resolves.toEqual({ webID: 'test' }); - }); -});