fix: Ensure setup values are migrated correctly

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" },

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:V6MigrationNotificationStorage" }
-          { "@id": "urn:solid-server:default:V6MigrationSetupStorage" }
         ],
-        "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" }
       }
     },

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<string, string>;
+  setupStorage: KeyValueStorage<string, string>;
   /**
-   * 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<any>;
+  newAccountStorage: AccountLoginStorage<any>;
+  /**
+   * The storage that will contain the setup entries in the new format.
+   */
+  newSetupStorage: KeyValueStorage<string, string>;
   /**
    * 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<string, string>;
+  private readonly setupStorage: KeyValueStorage<string, string>;
 
   private readonly accountStorage: KeyValueStorage<string, Account | Settings>;
   private readonly clientCredentialsStorage: KeyValueStorage<string, ClientCredentials>;
   private readonly cleanupStorages: KeyValueStorage<string, any>[];
 
-  private readonly newStorage: AccountLoginStorage<typeof STORAGE_DESCRIPTION>;
+  private readonly newAccountStorage: AccountLoginStorage<typeof STORAGE_DESCRIPTION>;
+  private readonly newSetupStorage: KeyValueStorage<string, string>;
 
   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<void> {
-    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 };

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<void> => {
     // 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<void> => {

test/unit/init/migration/V6MigrationInitializer.test.ts

@@ -42,11 +42,12 @@ describe('A V6MigrationInitializer', (): void => {
   let accounts: Record<string, Account>;
   let clientCredentials: Record<string, ClientCredentials>;
   const versionKey = 'version';
-  let versionStorage: jest.Mocked<KeyValueStorage<string, string>>;
+  let setupStorage: jest.Mocked<KeyValueStorage<string, string>>;
   let accountStorage: jest.Mocked<KeyValueStorage<string, Account | Settings>>;
   let clientCredentialsStorage: jest.Mocked<KeyValueStorage<string, ClientCredentials>>;
   let forgotPasswordStorage: jest.Mocked<KeyValueStorage<string, unknown>>;
-  let newStorage: jest.Mocked<AccountLoginStorage<any>>;
+  let newAccountStorage: jest.Mocked<AccountLoginStorage<any>>;
+  let newSetupStorage: jest.Mocked<KeyValueStorage<string, string>>;
   let initializer: V6MigrationInitializer;
 
   beforeEach(async(): Promise<void> => {
@@ -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<KeyValueStorage<string, string>> as any;
+
+    newSetupStorage = {
+      set: jest.fn(),
     } satisfies Partial<KeyValueStorage<string, string>> as any;
 
     accountStorage = {
@@ -89,17 +98,18 @@ describe('A V6MigrationInitializer', (): void => {
       }),
     } satisfies Partial<KeyValueStorage<string, any>> as any;
 
-    newStorage = {
+    newAccountStorage = {
       create: jest.fn((type): any => ({ id: `${type}-id` })),
     } satisfies Partial<AccountLoginStorage<any>> 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<void> => {
     await expect(initializer.handle()).resolves.toBeUndefined();
 
-    expect(versionStorage.get).toHaveBeenCalledTimes(1);
+    expect(setupStorage.get).toHaveBeenCalledTimes(1);
-    expect(versionStorage.get).toHaveBeenLastCalledWith(versionKey);
+    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(newAccountStorage.create).toHaveBeenCalledTimes(11);
-    expect(newStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {});
+    expect(newAccountStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {});
-    expect(newStorage.create).toHaveBeenCalledWith(PASSWORD_STORAGE_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(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId, accountId: 'account-id' });
-    expect(newStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE, { webId: webId2, accountId: 'account-id' });
+    expect(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE,
-    expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test/', accountId: 'account-id' });
+      { webId: webId2, accountId: 'account-id' });
-    expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' });
+    expect(newAccountStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test/', accountId: 'account-id' });
-    expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE, { webId, podId: 'pod-id', visible: false });
+    expect(newAccountStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' });
-    expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE,
+    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<void> => {
-    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<void> => {
-    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<void> => {
     delete settings[webId];
     await expect(initializer.handle()).resolves.toBeUndefined();
 
-    expect(versionStorage.get).toHaveBeenCalledTimes(1);
+    expect(setupStorage.get).toHaveBeenCalledTimes(1);
-    expect(versionStorage.get).toHaveBeenLastCalledWith(versionKey);
+    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(newAccountStorage.create).toHaveBeenCalledTimes(5);
-    expect(newStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {});
+    expect(newAccountStorage.create).toHaveBeenCalledWith(ACCOUNT_TYPE, {});
-    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: webId2, accountId: 'account-id' });
+    expect(newAccountStorage.create).toHaveBeenCalledWith(WEBID_STORAGE_TYPE,
-    expect(newStorage.create).toHaveBeenCalledWith(POD_STORAGE_TYPE, { baseUrl: 'http://example.com/test2/', accountId: 'account-id' });
+      { webId: webId2, accountId: 'account-id' });
-    expect(newStorage.create).toHaveBeenCalledWith(OWNER_STORAGE_TYPE,
+    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<void> => {
       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);
     });
   });
 });