From 1e1edd5c672bab92e34f24336207e5c069c98326 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Tue, 31 Aug 2021 09:49:22 +0200 Subject: [PATCH] refactor: Clean up internal storage Each IDP class using storage now has a different storage. This way those classes don't have to worry about clashing keys anymore. All internal storage is now in the /.internal/ container, thereby making it easier to take the location of the internal data into account: only 1 path needs to be blocked and a regex router handling internal data differently only has to match 1 path as well. --- .../handler/account-store/default.json | 7 ++-- .../handler/adapter-factory/webid.json | 3 +- config/identity/handler/default.json | 1 - .../identity/handler/key-value/storage.json | 16 -------- .../handler/provider-factory/identity.json | 2 +- config/identity/ownership/token.json | 14 ++++++- config/storage/key-value/memory.json | 19 +++++++++- config/storage/key-value/resource-store.json | 38 ++++++++++++++----- .../configuration/IdentityProviderFactory.ts | 13 ++++--- .../storage/BaseAccountStore.ts | 18 +++------ .../storage/ExpiringAdapterFactory.ts | 27 +++++-------- .../IdentityProviderFactory.test.ts | 4 +- .../storage/BaseAccountStore.test.ts | 3 +- .../storage/ExpiringAdapterFactory.test.ts | 3 +- 14 files changed, 90 insertions(+), 78 deletions(-) delete mode 100644 config/identity/handler/key-value/storage.json diff --git a/config/identity/handler/account-store/default.json b/config/identity/handler/account-store/default.json index 01f59d4c0..04ed27675 100644 --- a/config/identity/handler/account-store/default.json +++ b/config/identity/handler/account-store/default.json @@ -5,10 +5,9 @@ "comment": "The storage adapter that persists usernames, passwords, etc.", "@id": "urn:solid-server:auth:password:AccountStore", "@type": "BaseAccountStore", - "args_storageName": "/idp/email-password-db", - "args_saltRounds": 10, - "args_storage": { - "@id": "urn:solid-server:default:IdpStorage" + "saltRounds": 10, + "storage": { + "@id": "urn:solid-server:default:AccountStorage" } } ] diff --git a/config/identity/handler/adapter-factory/webid.json b/config/identity/handler/adapter-factory/webid.json index 381b02115..c667eafa5 100644 --- a/config/identity/handler/adapter-factory/webid.json +++ b/config/identity/handler/adapter-factory/webid.json @@ -7,8 +7,7 @@ "@type": "WebIdAdapterFactory", "source": { "@type": "ExpiringAdapterFactory", - "args_storageName": "/idp/oidc", - "args_storage": { "@id": "urn:solid-server:default:ExpiringIdpStorage" } + "storage": { "@id": "urn:solid-server:default:IdpAdapterStorage" } }, "converter": { "@id": "urn:solid-server:default:RepresentationConverter" } } diff --git a/config/identity/handler/default.json b/config/identity/handler/default.json index 7c3972f1a..ad1c96efa 100644 --- a/config/identity/handler/default.json +++ b/config/identity/handler/default.json @@ -4,7 +4,6 @@ "files-scs:config/identity/handler/account-store/default.json", "files-scs:config/identity/handler/adapter-factory/webid.json", "files-scs:config/identity/handler/interaction/routes.json", - "files-scs:config/identity/handler/key-value/storage.json", "files-scs:config/identity/handler/provider-factory/identity.json" ], "@graph": [ diff --git a/config/identity/handler/key-value/storage.json b/config/identity/handler/key-value/storage.json deleted file mode 100644 index 868f07c29..000000000 --- a/config/identity/handler/key-value/storage.json +++ /dev/null @@ -1,16 +0,0 @@ -{ - "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^1.0.0/components/context.jsonld", - "@graph": [ - { - "comment": "Stores expiring data. This class has a `finalize` function that needs to be called after stopping the server.", - "@id": "urn:solid-server:default:ExpiringIdpStorage", - "@type": "WrappedExpiringStorage", - "source": { "@id": "urn:solid-server:default:IdpStorage" } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "ParallelFinalizer:_finalizers": [ { "@id": "urn:solid-server:default:ExpiringIdpStorage" } ] - } - ] -} diff --git a/config/identity/handler/provider-factory/identity.json b/config/identity/handler/provider-factory/identity.json index 66c54e2e9..2e4baee27 100644 --- a/config/identity/handler/provider-factory/identity.json +++ b/config/identity/handler/provider-factory/identity.json @@ -11,7 +11,7 @@ "args_adapterFactory": { "@id": "urn:solid-server:default:IdpAdapterFactory" }, "args_baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "args_idpPath": "/idp", - "args_storage": { "@id": "urn:solid-server:default:IdpStorage" }, + "args_storage": { "@id": "urn:solid-server:default:IdpKeyStorage" }, "args_errorHandler": { "@id": "urn:solid-server:default:ErrorHandler" }, "args_responseWriter": { "@id": "urn:solid-server:default:ResponseWriter" }, "config": { diff --git a/config/identity/ownership/token.json b/config/identity/ownership/token.json index a940e4730..de6743a40 100644 --- a/config/identity/ownership/token.json +++ b/config/identity/ownership/token.json @@ -6,7 +6,19 @@ "@id": "urn:solid-server:auth:password:OwnershipValidator", "@type": "TokenOwnershipValidator", "converter": { "@id": "urn:solid-server:default:RepresentationConverter" }, - "storage": { "@id": "urn:solid-server:default:ExpiringIdpStorage" } + "storage": { "@id": "urn:solid-server:default:ExpiringTokenStorage" } + }, + + { + "comment": "Stores expiring data. This class has a `finalize` function that needs to be called after stopping the server.", + "@id": "urn:solid-server:default:ExpiringTokenStorage", + "@type": "WrappedExpiringStorage", + "source": { "@id": "urn:solid-server:default:IdpTokenStorage" } + }, + { + "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", + "@id": "urn:solid-server:default:Finalizer", + "ParallelFinalizer:_finalizers": [ { "@id": "urn:solid-server:default:ExpiringTokenStorage" } ] } ] } diff --git a/config/storage/key-value/memory.json b/config/storage/key-value/memory.json index 2b707b61b..5777bb5f1 100644 --- a/config/storage/key-value/memory.json +++ b/config/storage/key-value/memory.json @@ -10,8 +10,23 @@ "@type": "MemoryMapStorage" }, { - "comment": "Storage used by the IDP component.", - "@id": "urn:solid-server:default:IdpStorage", + "comment": "Storage used by the IDP adapter.", + "@id": "urn:solid-server:default:IdpAdapterStorage", + "@type": "MemoryMapStorage" + }, + { + "comment": "Storage used for the IDP keys.", + "@id": "urn:solid-server:default:IdpKeyStorage", + "@type": "MemoryMapStorage" + }, + { + "comment": "Storage used for IDP ownership tokens.", + "@id": "urn:solid-server:default:IdpTokenStorage", + "@type": "MemoryMapStorage" + }, + { + "comment": "Storage used for account management.", + "@id": "urn:solid-server:default:AccountStorage", "@type": "MemoryMapStorage" } ] diff --git a/config/storage/key-value/resource-store.json b/config/storage/key-value/resource-store.json index b522ec7af..f6cbdcaa1 100644 --- a/config/storage/key-value/resource-store.json +++ b/config/storage/key-value/resource-store.json @@ -13,26 +13,46 @@ "@type": "JsonResourceStorage", "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, - "container": "/locks/" + "container": "/.internal/locks/" }, { - "comment": "Storage used by the IDP component.", - "@id": "urn:solid-server:default:IdpStorage", + "comment": "Storage used by the IDP adapter.", + "@id": "urn:solid-server:default:IdpAdapterStorage", "@type": "JsonResourceStorage", "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, - "container": "/idp/data/" + "container": "/.internal/idp/adapter/" + }, + { + "comment": "Storage used for the IDP keys.", + "@id": "urn:solid-server:default:IdpKeyStorage", + "@type": "JsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/idp/keys/" + }, + { + "comment": "Storage used for IDP ownership tokens.", + "@id": "urn:solid-server:default:IdpTokenStorage", + "@type": "JsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/idp/tokens/" + }, + { + "comment": "Storage used for account management.", + "@id": "urn:solid-server:default:AccountStorage", + "@type": "JsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/accounts/" }, { "comment": "Block external access to the storage containers to avoid exposing internal data.", "@id": "urn:solid-server:default:PathBasedAuthorizer", "PathBasedAuthorizer:_paths": [ { - "PathBasedAuthorizer:_paths_key": "^/locks(/.*)?$", - "PathBasedAuthorizer:_paths_value": { "@type": "DenyAllAuthorizer" } - }, - { - "PathBasedAuthorizer:_paths_key": "^/idp/data(/.*)?$", + "PathBasedAuthorizer:_paths_key": "^/.internal(/.*)?$", "PathBasedAuthorizer:_paths_value": { "@type": "DenyAllAuthorizer" } } ] diff --git a/src/identity/configuration/IdentityProviderFactory.ts b/src/identity/configuration/IdentityProviderFactory.ts index 0f00c5f13..96d1aa7a3 100644 --- a/src/identity/configuration/IdentityProviderFactory.ts +++ b/src/identity/configuration/IdentityProviderFactory.ts @@ -47,6 +47,9 @@ export interface IdentityProviderFactoryArgs { responseWriter: ResponseWriter; } +const JWKS_KEY = 'jwks'; +const COOKIES_KEY = 'cookie-secret'; + /** * Creates an OIDC Provider based on the provided configuration and parameters. * The provider will be cached and returned on subsequent calls. @@ -138,8 +141,7 @@ export class IdentityProviderFactory implements ProviderFactory { */ private async generateJwks(): Promise<{ keys: JWK[] }> { // Check to see if the keys are already saved - const key = `${this.idpPath}/jwks`; - const jwks = await this.storage.get(key) as { keys: JWK[] } | undefined; + const jwks = await this.storage.get(JWKS_KEY) as { keys: JWK[] } | undefined; if (jwks) { return jwks; } @@ -152,7 +154,7 @@ export class IdentityProviderFactory implements ProviderFactory { // which is why we convert it into a plain object here. // Potentially this can be changed at a later point in time to `{ keys: [ jwk ]}`. const newJwks = { keys: [{ ...jwk }]}; - await this.storage.set(key, newJwks); + await this.storage.set(JWKS_KEY, newJwks); return newJwks; } @@ -162,14 +164,13 @@ export class IdentityProviderFactory implements ProviderFactory { */ private async generateCookieKeys(): Promise { // Check to see if the keys are already saved - const key = `${this.idpPath}/cookie-secret`; - const cookieSecret = await this.storage.get(key); + const cookieSecret = await this.storage.get(COOKIES_KEY); if (Array.isArray(cookieSecret)) { return cookieSecret; } // If they are not, generate and save them const newCookieSecret = [ randomBytes(64).toString('hex') ]; - await this.storage.set(key, newCookieSecret); + await this.storage.set(COOKIES_KEY, newCookieSecret); return newCookieSecret; } diff --git a/src/identity/interaction/email-password/storage/BaseAccountStore.ts b/src/identity/interaction/email-password/storage/BaseAccountStore.ts index 8437c9282..844bebc29 100644 --- a/src/identity/interaction/email-password/storage/BaseAccountStore.ts +++ b/src/identity/interaction/email-password/storage/BaseAccountStore.ts @@ -25,39 +25,31 @@ export interface ForgotPasswordPayload { export type EmailPasswordData = AccountPayload | ForgotPasswordPayload; -export interface BaseAccountStoreArgs { - storageName: string; - storage: KeyValueStorage; - saltRounds: number; -} - /** * A EmailPasswordStore that uses a KeyValueStorage * to persist its information. */ export class BaseAccountStore implements AccountStore { - private readonly storageName: string; private readonly storage: KeyValueStorage; private readonly saltRounds: number; - public constructor(args: BaseAccountStoreArgs) { - this.storageName = args.storageName; - this.storage = args.storage; - this.saltRounds = args.saltRounds; + public constructor(storage: KeyValueStorage, saltRounds: number) { + this.storage = storage; + this.saltRounds = saltRounds; } /** * Generates a ResourceIdentifier to store data for the given email. */ private getAccountResourceIdentifier(email: string): string { - return `${this.storageName}/account/${encodeURIComponent(email)}`; + return `account/${encodeURIComponent(email)}`; } /** * Generates a ResourceIdentifier to store data for the given recordId. */ private getForgotPasswordRecordResourceIdentifier(recordId: string): string { - return `${this.storageName}/forgot-password-resource-identifier/${encodeURIComponent(recordId)}`; + return `forgot-password-resource-identifier/${encodeURIComponent(recordId)}`; } /** diff --git a/src/identity/storage/ExpiringAdapterFactory.ts b/src/identity/storage/ExpiringAdapterFactory.ts index d540ab7c5..7c313bda8 100644 --- a/src/identity/storage/ExpiringAdapterFactory.ts +++ b/src/identity/storage/ExpiringAdapterFactory.ts @@ -3,11 +3,6 @@ import { getLoggerFor } from '../../logging/LogUtil'; import type { ExpiringStorage } from '../../storage/keyvalue/ExpiringStorage'; import type { AdapterFactory } from './AdapterFactory'; -export interface ExpiringAdapterArgs { - storageName: string; - storage: ExpiringStorage; -} - /** * An IDP storage adapter that uses an ExpiringStorage * to persist data. @@ -15,30 +10,28 @@ export interface ExpiringAdapterArgs { export class ExpiringAdapter implements Adapter { protected readonly logger = getLoggerFor(this); - private readonly storageName: string; private readonly name: string; private readonly storage: ExpiringStorage; - public constructor(name: string, args: ExpiringAdapterArgs) { + public constructor(name: string, storage: ExpiringStorage) { this.name = name; - this.storageName = args.storageName; - this.storage = args.storage; + this.storage = storage; } private grantKeyFor(id: string): string { - return `${this.storageName}/grant/${encodeURIComponent(id)}`; + return `grant/${encodeURIComponent(id)}`; } private userCodeKeyFor(userCode: string): string { - return `${this.storageName}/user_code/${encodeURIComponent(userCode)}`; + return `user_code/${encodeURIComponent(userCode)}`; } private uidKeyFor(uid: string): string { - return `${this.storageName}/uid/${encodeURIComponent(uid)}`; + return `uid/${encodeURIComponent(uid)}`; } private keyFor(id: string): string { - return `${this.storageName}/${this.name}/${encodeURIComponent(id)}`; + return `${this.name}/${encodeURIComponent(id)}`; } public async upsert(id: string, payload: AdapterPayload, expiresIn?: number): Promise { @@ -117,13 +110,13 @@ export class ExpiringAdapter implements Adapter { * The factory for a ExpiringStorageAdapter */ export class ExpiringAdapterFactory implements AdapterFactory { - private readonly args: ExpiringAdapterArgs; + private readonly storage: ExpiringStorage; - public constructor(args: ExpiringAdapterArgs) { - this.args = args; + public constructor(storage: ExpiringStorage) { + this.storage = storage; } public createStorageAdapter(name: string): ExpiringAdapter { - return new ExpiringAdapter(name, this.args); + return new ExpiringAdapter(name, this.storage); } } diff --git a/test/unit/identity/configuration/IdentityProviderFactory.test.ts b/test/unit/identity/configuration/IdentityProviderFactory.test.ts index e523201af..d1446194a 100644 --- a/test/unit/identity/configuration/IdentityProviderFactory.test.ts +++ b/test/unit/identity/configuration/IdentityProviderFactory.test.ts @@ -158,7 +158,7 @@ describe('An IdentityProviderFactory', (): void => { expect(result1.config.jwks).toEqual(result2.config.jwks); expect(storage.get).toHaveBeenCalledTimes(4); expect(storage.set).toHaveBeenCalledTimes(2); - expect(storage.set).toHaveBeenCalledWith('/idp/jwks', result1.config.jwks); - expect(storage.set).toHaveBeenCalledWith('/idp/cookie-secret', result1.config.cookies?.keys); + expect(storage.set).toHaveBeenCalledWith('jwks', result1.config.jwks); + expect(storage.set).toHaveBeenCalledWith('cookie-secret', result1.config.cookies?.keys); }); }); diff --git a/test/unit/identity/interaction/email-password/storage/BaseAccountStore.test.ts b/test/unit/identity/interaction/email-password/storage/BaseAccountStore.test.ts index 4e82931bb..1a97af295 100644 --- a/test/unit/identity/interaction/email-password/storage/BaseAccountStore.test.ts +++ b/test/unit/identity/interaction/email-password/storage/BaseAccountStore.test.ts @@ -5,7 +5,6 @@ import { BaseAccountStore } from '../../../../../../src/identity/interaction/ema import type { KeyValueStorage } from '../../../../../../src/storage/keyvalue/KeyValueStorage'; describe('A BaseAccountStore', (): void => { - const storageName = '/mail/storage'; let storage: KeyValueStorage; const saltRounds = 11; let store: BaseAccountStore; @@ -21,7 +20,7 @@ describe('A BaseAccountStore', (): void => { delete: jest.fn((id: string): any => map.delete(id)), } as any; - store = new BaseAccountStore({ storageName, storage, saltRounds }); + store = new BaseAccountStore(storage, saltRounds); }); it('can create accounts.', async(): Promise => { diff --git a/test/unit/identity/storage/ExpiringAdapterFactory.test.ts b/test/unit/identity/storage/ExpiringAdapterFactory.test.ts index 58d1010e4..4063f8acf 100644 --- a/test/unit/identity/storage/ExpiringAdapterFactory.test.ts +++ b/test/unit/identity/storage/ExpiringAdapterFactory.test.ts @@ -7,7 +7,6 @@ import type { ExpiringStorage } from '../../../../src/storage/keyvalue/ExpiringS jest.useFakeTimers(); describe('An ExpiringAdapterFactory', (): void => { - const storageName = '/storage'; const name = 'nnaammee'; const id = 'http://alice.test.com/card#me'; const grantId = 'grant123456'; @@ -27,7 +26,7 @@ describe('An ExpiringAdapterFactory', (): void => { delete: jest.fn().mockImplementation((key: string): any => map.delete(key)), } as any; - factory = new ExpiringAdapterFactory({ storageName, storage }); + factory = new ExpiringAdapterFactory(storage); adapter = factory.createStorageAdapter(name); });