From 7a44581406689c3ee3c6477832f7aa3bf4b76c8d Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 11 Oct 2023 10:42:19 +0200 Subject: [PATCH] fix: Ensure setup values are migrated correctly --- config/app/init/base/init.json | 2 +- config/app/init/migration/v6.json | 10 +- src/init/migration/V6MigrationInitializer.ts | 42 +++++--- test/integration/V6Migration.test.ts | 10 ++ .../migration/V6MigrationInitializer.test.ts | 97 ++++++++++++------- 5 files changed, 104 insertions(+), 57 deletions(-) diff --git a/config/app/init/base/init.json b/config/app/init/base/init.json index bd951d78c..3b48319f8 100644 --- a/config/app/init/base/init.json +++ b/config/app/init/base/init.json @@ -33,8 +33,8 @@ "@type":"SequenceHandler", "handlers": [ { "@id": "urn:solid-server:default:CleanupInitializer"}, - { "@id": "urn:solid-server:default:BaseUrlVerifier" }, { "@id": "urn:solid-server:default:MigrationInitializer" }, + { "@id": "urn:solid-server:default:BaseUrlVerifier" }, { "@id": "urn:solid-server:default:PrimaryParallelInitializer" }, { "@id": "urn:solid-server:default:SeededAccountInitializer" }, { "@id": "urn:solid-server:default:ModuleVersionVerifier" }, diff --git a/config/app/init/migration/v6.json b/config/app/init/migration/v6.json index fa90076d9..340e3c3ca 100644 --- a/config/app/init/migration/v6.json +++ b/config/app/init/migration/v6.json @@ -2,7 +2,7 @@ "@context": "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^6.0.0/components/context.jsonld", "@graph": [ { - "comment": "Handles migration of v6 account data.", + "comment": "Handles migration of v6 internal data. In a conditional handler to prevent issues if something fails between migration and writing the new version.", "@id": "urn:solid-server:default:V6MigrationHandler", "@type": "ConditionalHandler", "storageKey": "v6-migration", @@ -13,7 +13,7 @@ "@id": "urn:solid-server:default:V6MigrationInitializer", "@type": "V6MigrationInitializer", "versionKey": "current-server-version", - "versionStorage": { "@id": "urn:solid-server:default:V6MigrationSetupStorage" }, + "setupStorage": { "@id": "urn:solid-server:default:V6MigrationSetupStorage" }, "accountStorage": { "@id": "urn:solid-server:default:V6MigrationAccountStorage" }, "clientCredentialsStorage": { "@id": "urn:solid-server:default:V6MigrationClientCredentialsStorage" }, "cleanupStorages": [ @@ -23,10 +23,10 @@ { "@id": "urn:solid-server:default:V6MigrationKeyStorage" }, { "@id": "urn:solid-server:default:V6MigrationAdapterStorage" }, { "@id": "urn:solid-server:default:V6MigrationTokenStorage" }, - { "@id": "urn:solid-server:default:V6MigrationNotificationStorage" }, - { "@id": "urn:solid-server:default:V6MigrationSetupStorage" } + { "@id": "urn:solid-server:default:V6MigrationNotificationStorage" } ], - "newStorage": { "@id": "urn:solid-server:default:AccountStorage" }, + "newAccountStorage": { "@id": "urn:solid-server:default:AccountStorage" }, + "newSetupStorage": { "@id": "urn:solid-server:default:SetupStorage" }, "skipConfirmation": { "@id": "urn:solid-server:default:variable:confirmMigration" } } }, diff --git a/src/init/migration/V6MigrationInitializer.ts b/src/init/migration/V6MigrationInitializer.ts index e8c7f3baa..bf0cd0d80 100644 --- a/src/init/migration/V6MigrationInitializer.ts +++ b/src/init/migration/V6MigrationInitializer.ts @@ -50,11 +50,11 @@ const STORAGE_DESCRIPTION = { export interface V6MigrationInitializerArgs { /** - * The storage in which the version is saved that was stored last time the server was started. + * The storage in which all setup values are stored, including the version of the server. */ - versionStorage: KeyValueStorage; + setupStorage: KeyValueStorage; /** - * The key necessary to get the version from the `versionStorage`. + * The key necessary to get the version from the `setupStorage`. */ versionKey: string; /** @@ -72,7 +72,11 @@ export interface V6MigrationInitializerArgs { /** * The storage that will contain the account data in the new format. */ - newStorage: AccountLoginStorage; + newAccountStorage: AccountLoginStorage; + /** + * The storage that will contain the setup entries in the new format. + */ + newSetupStorage: KeyValueStorage; /** * If true, no confirmation prompt will be printed to the stdout. */ @@ -92,27 +96,29 @@ export class V6MigrationInitializer extends Initializer { private readonly skipConfirmation: boolean; private readonly versionKey: string; - private readonly versionStorage: KeyValueStorage; + private readonly setupStorage: KeyValueStorage; private readonly accountStorage: KeyValueStorage; private readonly clientCredentialsStorage: KeyValueStorage; private readonly cleanupStorages: KeyValueStorage[]; - private readonly newStorage: AccountLoginStorage; + private readonly newAccountStorage: AccountLoginStorage; + private readonly newSetupStorage: KeyValueStorage; public constructor(args: V6MigrationInitializerArgs) { super(); this.skipConfirmation = Boolean(args.skipConfirmation); this.versionKey = args.versionKey; - this.versionStorage = args.versionStorage; + this.setupStorage = args.setupStorage; this.accountStorage = args.accountStorage; this.clientCredentialsStorage = args.clientCredentialsStorage; this.cleanupStorages = args.cleanupStorages; - this.newStorage = args.newStorage; + this.newAccountStorage = args.newAccountStorage; + this.newSetupStorage = args.newSetupStorage; } public async handle(): Promise { - const previousVersion = await this.versionStorage.get(this.versionKey); + const previousVersion = await this.setupStorage.get(this.versionKey); if (!previousVersion) { // This happens if this is the first time the server is started this.logger.debug('No previous version found'); @@ -166,7 +172,13 @@ export class V6MigrationInitializer extends Initializer { this.logger.warn(`Unable to find account for client credentials ${label}. Skipping migration of this token.`); continue; } - await this.newStorage.create(CLIENT_CREDENTIALS_STORAGE_TYPE, { webId, label, secret, accountId }); + await this.newAccountStorage.create(CLIENT_CREDENTIALS_STORAGE_TYPE, { webId, label, secret, accountId }); + } + + this.logger.debug('Converting setup entries.'); + for await (const [ key, value ] of this.setupStorage.entries()) { + await this.newSetupStorage.set(key, value); + await this.setupStorage.delete(key); } // Cleanup all old entries @@ -206,17 +218,17 @@ export class V6MigrationInitializer extends Initializer { return; } - const { id: accountId } = await this.newStorage.create(ACCOUNT_TYPE, {}); + const { id: accountId } = await this.newAccountStorage.create(ACCOUNT_TYPE, {}); // The `toLowerCase` call is important here to have the expected value - await this.newStorage.create(PASSWORD_STORAGE_TYPE, + await this.newAccountStorage.create(PASSWORD_STORAGE_TYPE, { email: email.toLowerCase(), password, verified, accountId }); if (settings.useIdp) { - await this.newStorage.create(WEBID_STORAGE_TYPE, { webId, accountId }); + await this.newAccountStorage.create(WEBID_STORAGE_TYPE, { webId, accountId }); } if (settings.podBaseUrl) { - const { id: podId } = await this.newStorage.create(POD_STORAGE_TYPE, + const { id: podId } = await this.newAccountStorage.create(POD_STORAGE_TYPE, { baseUrl: settings.podBaseUrl, accountId }); - await this.newStorage.create(OWNER_STORAGE_TYPE, { webId, podId, visible: false }); + await this.newAccountStorage.create(OWNER_STORAGE_TYPE, { webId, podId, visible: false }); } return { accountId, webId }; diff --git a/test/integration/V6Migration.test.ts b/test/integration/V6Migration.test.ts index 5eb6acf08..e86688516 100644 --- a/test/integration/V6Migration.test.ts +++ b/test/integration/V6Migration.test.ts @@ -51,6 +51,7 @@ describe('A server migrating from v6', (): void => { it('can start the server to migrate the data.', async(): Promise => { // This is going to trigger the migration step await expect(app.start()).resolves.toBeUndefined(); + // If migration was successful, there should be no files left in these folders const accountDir = await readdir(joinFilePath(rootFilePath, '.internal/accounts/')); expect(accountDir).toEqual(expect.arrayContaining([ 'data', 'index', 'credentials' ])); @@ -58,6 +59,15 @@ describe('A server migrating from v6', (): void => { expect(credentialsDir).toEqual([]); const forgotDir = await readdir(joinFilePath(rootFilePath, '.internal/forgot-password/')); expect(forgotDir).toEqual([]); + + // Setup resources should have been migrated + const setupDir = await readdir(joinFilePath(rootFilePath, '.internal/setup/')); + expect(setupDir).toEqual([ + 'current-base-url$.json', + 'current-server-version$.json', + 'setupCompleted-2.0$.json', + 'v6-migration$.json', + ]); }); it('still allows existing accounts to log in.', async(): Promise => { diff --git a/test/unit/init/migration/V6MigrationInitializer.test.ts b/test/unit/init/migration/V6MigrationInitializer.test.ts index 3d893abe4..3dee5768c 100644 --- a/test/unit/init/migration/V6MigrationInitializer.test.ts +++ b/test/unit/init/migration/V6MigrationInitializer.test.ts @@ -42,11 +42,12 @@ describe('A V6MigrationInitializer', (): void => { let accounts: Record; let clientCredentials: Record; const versionKey = 'version'; - let versionStorage: jest.Mocked>; + let setupStorage: jest.Mocked>; let accountStorage: jest.Mocked>; let clientCredentialsStorage: jest.Mocked>; let forgotPasswordStorage: jest.Mocked>; - let newStorage: jest.Mocked>; + let newAccountStorage: jest.Mocked>; + let newSetupStorage: jest.Mocked>; let initializer: V6MigrationInitializer; beforeEach(async(): Promise => { @@ -62,8 +63,16 @@ describe('A V6MigrationInitializer', (): void => { token: { webId, secret: 'secret!' }, }; - versionStorage = { + setupStorage = { get: jest.fn().mockResolvedValue('6.0.0'), + delete: jest.fn(), + entries: jest.fn(async function* (): AsyncGenerator<[string, string]> { + yield [ 'version', '6.0.0' ]; + }), + } satisfies Partial> as any; + + newSetupStorage = { + set: jest.fn(), } satisfies Partial> as any; accountStorage = { @@ -89,17 +98,18 @@ describe('A V6MigrationInitializer', (): void => { }), } satisfies Partial> as any; - newStorage = { + newAccountStorage = { create: jest.fn((type): any => ({ id: `${type}-id` })), } satisfies Partial> as any; initializer = new V6MigrationInitializer({ versionKey, - versionStorage, + setupStorage, accountStorage, clientCredentialsStorage, cleanupStorages: [ accountStorage, clientCredentialsStorage, forgotPasswordStorage ], - newStorage, + newAccountStorage, + newSetupStorage, skipConfirmation: true, }); }); @@ -107,8 +117,8 @@ describe('A V6MigrationInitializer', (): void => { it('migrates the data.', async(): Promise => { await expect(initializer.handle()).resolves.toBeUndefined(); - expect(versionStorage.get).toHaveBeenCalledTimes(1); - expect(versionStorage.get).toHaveBeenLastCalledWith(versionKey); + expect(setupStorage.get).toHaveBeenCalledTimes(1); + expect(setupStorage.get).toHaveBeenLastCalledWith(versionKey); expect(accountStorage.get).toHaveBeenCalledTimes(2); expect(accountStorage.get).toHaveBeenCalledWith(webId); @@ -125,43 +135,50 @@ describe('A V6MigrationInitializer', (): void => { expect(forgotPasswordStorage.delete).toHaveBeenCalledTimes(1); expect(forgotPasswordStorage.delete).toHaveBeenCalledWith('forgot'); - expect(newStorage.create).toHaveBeenCalledTimes(11); - expect(newStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {}); - expect(newStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledTimes(11); + expect(newAccountStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {}); + expect(newAccountStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, { email: 'email@example.com', password: '123', verified: true, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, { email: 'email2@example.com', password: '1234', verified: true, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId: webId2, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test/', accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, { webId, podId: 'pod-id', visible: false }); - expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId, accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, + { webId: webId2, accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test/', accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, + { webId, podId: 'pod-id', visible: false }); + expect(newAccountStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, { webId: webId2, podId: 'pod-id', visible: false }); - expect(newStorage.create).toHaveBeenCalledWith(CLIENT_CREDENTIALS_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledWith(CLIENT_CREDENTIALS_STORAGE_TYPE, { label: 'token', secret: 'secret!', webId, accountId: 'account-id' }); + + expect(newSetupStorage.set).toHaveBeenCalledTimes(1); + expect(newSetupStorage.set).toHaveBeenLastCalledWith('version', '6.0.0'); + expect(setupStorage.delete).toHaveBeenCalledTimes(1); + expect(setupStorage.delete).toHaveBeenLastCalledWith('version'); }); it('does nothing if the server has no stored version number.', async(): Promise => { - versionStorage.get.mockResolvedValueOnce(undefined); + setupStorage.get.mockResolvedValueOnce(undefined); await expect(initializer.handle()).resolves.toBeUndefined(); expect(accountStorage.get).toHaveBeenCalledTimes(0); - expect(newStorage.create).toHaveBeenCalledTimes(0); + expect(newAccountStorage.create).toHaveBeenCalledTimes(0); }); it('does nothing if stored version is more than 6.', async(): Promise => { - versionStorage.get.mockResolvedValueOnce('7.0.0'); + setupStorage.get.mockResolvedValueOnce('7.0.0'); await expect(initializer.handle()).resolves.toBeUndefined(); expect(accountStorage.get).toHaveBeenCalledTimes(0); - expect(newStorage.create).toHaveBeenCalledTimes(0); + expect(newAccountStorage.create).toHaveBeenCalledTimes(0); }); it('ignores accounts and credentials for which it cannot find the settings.', async(): Promise => { delete settings[webId]; await expect(initializer.handle()).resolves.toBeUndefined(); - expect(versionStorage.get).toHaveBeenCalledTimes(1); - expect(versionStorage.get).toHaveBeenLastCalledWith(versionKey); + expect(setupStorage.get).toHaveBeenCalledTimes(1); + expect(setupStorage.get).toHaveBeenLastCalledWith(versionKey); expect(accountStorage.get).toHaveBeenCalledTimes(2); expect(accountStorage.get).toHaveBeenCalledWith(webId); @@ -171,14 +188,20 @@ describe('A V6MigrationInitializer', (): void => { expect(accountStorage.delete).toHaveBeenCalledWith('account'); expect(accountStorage.delete).toHaveBeenCalledWith('account2'); - expect(newStorage.create).toHaveBeenCalledTimes(5); - expect(newStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {}); - expect(newStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledTimes(5); + expect(newAccountStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {}); + expect(newAccountStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_TYPE, { email: 'email2@example.com', password: '1234', verified: true, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId: webId2, accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' }); - expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, + expect(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, + { webId: webId2, accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' }); + expect(newAccountStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, { webId: webId2, podId: 'pod-id', visible: false }); + + expect(newSetupStorage.set).toHaveBeenCalledTimes(1); + expect(newSetupStorage.set).toHaveBeenLastCalledWith('version', '6.0.0'); + expect(setupStorage.delete).toHaveBeenCalledTimes(1); + expect(setupStorage.delete).toHaveBeenLastCalledWith('version'); }); describe('with prompts enabled', (): void => { @@ -187,11 +210,12 @@ describe('A V6MigrationInitializer', (): void => { initializer = new V6MigrationInitializer({ versionKey, - versionStorage, + setupStorage, accountStorage, clientCredentialsStorage, cleanupStorages: [ accountStorage, clientCredentialsStorage, forgotPasswordStorage ], - newStorage, + newAccountStorage, + newSetupStorage, skipConfirmation: false, }); }); @@ -200,15 +224,16 @@ describe('A V6MigrationInitializer', (): void => { await expect(initializer.handle()).resolves.toBeUndefined(); expect(questionMock).toHaveBeenCalledTimes(1); - expect(questionMock.mock.invocationCallOrder[0]).toBeLessThan(newStorage.create.mock.invocationCallOrder[0]); + expect(questionMock.mock.invocationCallOrder[0]) + .toBeLessThan(newAccountStorage.create.mock.invocationCallOrder[0]); - expect(newStorage.create).toHaveBeenCalledTimes(11); + expect(newAccountStorage.create).toHaveBeenCalledTimes(11); }); it('throws an error to stop the server if no positive answer is received.', async(): Promise => { questionMock.mockImplementation((input, callback): void => callback('n')); await expect(initializer.handle()).rejects.toThrow('Stopping server as migration was cancelled.'); - expect(newStorage.create).toHaveBeenCalledTimes(0); + expect(newAccountStorage.create).toHaveBeenCalledTimes(0); }); }); });