feat(secret storage): keyId in SecretStorage.setDefaultKeyId can be set at null in order to delete an exising recovery key (#4615)

Author: florianduros

spec/unit/secret-storage.spec.ts

@@ -23,12 +23,14 @@ import {
     SecretStorageCallbacks,
     SecretStorageKeyDescriptionAesV1,
     SecretStorageKeyDescriptionCommon,
+    ServerSideSecretStorage,
     ServerSideSecretStorageImpl,
     trimTrailingEquals,
 } from "../../src/secret-storage";
 import { randomString } from "../../src/randomstring";
 import { SecretInfo } from "../../src/secret-storage.ts";
-import { AccountDataEvents } from "../../src";
+import { AccountDataEvents, ClientEvent, MatrixEvent, TypedEventEmitter } from "../../src";
+import { defer, IDeferred } from "../../src/utils";
 
 declare module "../../src/@types/event" {
     interface SecretStorageAccountDataEvents {
@@ -273,6 +275,78 @@ describe("ServerSideSecretStorageImpl", function () {
             expect(console.warn).toHaveBeenCalledWith(expect.stringContaining("unknown algorithm"));
         });
     });
+
+    describe("setDefaultKeyId", function () {
+        let secretStorage: ServerSideSecretStorage;
+        let accountDataAdapter: Mocked<AccountDataClient>;
+        let accountDataPromise: IDeferred<void>;
+        beforeEach(() => {
+            accountDataAdapter = mockAccountDataClient();
+            accountDataPromise = defer();
+            accountDataAdapter.setAccountData.mockImplementation(() => {
+                accountDataPromise.resolve();
+                return Promise.resolve({});
+            });
+
+            secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+        });
+
+        it("should set the default key id", async function () {
+            const setDefaultPromise = secretStorage.setDefaultKeyId("keyId");
+            await accountDataPromise.promise;
+
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith("m.secret_storage.default_key", {
+                key: "keyId",
+            });
+
+            accountDataAdapter.emit(
+                ClientEvent.AccountData,
+                new MatrixEvent({
+                    type: "m.secret_storage.default_key",
+                    content: { key: "keyId" },
+                }),
+            );
+            await setDefaultPromise;
+        });
+
+        it("should set the default key id with a null key id", async function () {
+            const setDefaultPromise = secretStorage.setDefaultKeyId(null);
+            await accountDataPromise.promise;
+
+            expect(accountDataAdapter.setAccountData).toHaveBeenCalledWith("m.secret_storage.default_key", {});
+
+            accountDataAdapter.emit(
+                ClientEvent.AccountData,
+                new MatrixEvent({
+                    type: "m.secret_storage.default_key",
+                    content: {},
+                }),
+            );
+            await setDefaultPromise;
+        });
+    });
+
+    describe("getDefaultKeyId", function () {
+        it("should return null when there is no key", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            expect(await secretStorage.getDefaultKeyId()).toBe(null);
+        });
+
+        it("should return the key id when there is a key", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            accountDataAdapter.getAccountDataFromServer.mockResolvedValue({ key: "keyId" });
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            expect(await secretStorage.getDefaultKeyId()).toBe("keyId");
+        });
+
+        it("should return null when an empty object is in the account data", async function () {
+            const accountDataAdapter = mockAccountDataClient();
+            accountDataAdapter.getAccountDataFromServer.mockResolvedValue({});
+            const secretStorage = new ServerSideSecretStorageImpl(accountDataAdapter, {});
+            expect(await secretStorage.getDefaultKeyId()).toBe(null);
+        });
+    });
 });
 
 describe("trimTrailingEquals", () => {
@@ -291,8 +365,13 @@ describe("trimTrailingEquals", () => {
 });
 
 function mockAccountDataClient(): Mocked<AccountDataClient> {
+    const eventEmitter = new TypedEventEmitter();
     return {
         getAccountDataFromServer: jest.fn().mockResolvedValue(null),
         setAccountData: jest.fn().mockResolvedValue({}),
+        on: eventEmitter.on.bind(eventEmitter),
+        off: eventEmitter.off.bind(eventEmitter),
+        removeListener: eventEmitter.removeListener.bind(eventEmitter),
+        emit: eventEmitter.emit.bind(eventEmitter),
     } as unknown as Mocked<AccountDataClient>;
 }

src/crypto/EncryptionSetup.ts

@@ -264,7 +264,10 @@ class AccountDataClientAdapter
         return event?.getContent<AccountDataEvents[K]>() ?? null;
     }
 
-    public setAccountData<K extends keyof AccountDataEvents>(type: K, content: AccountDataEvents[K]): Promise<{}> {
+    public setAccountData<K extends keyof AccountDataEvents>(
+        type: K,
+        content: AccountDataEvents[K] | Record<string, never>,
+    ): Promise<{}> {
         const event = new MatrixEvent({ type, content });
         const lastEvent = this.values.get(type);
         this.values.set(type, event);

src/secret-storage.ts

@@ -148,7 +148,10 @@ export interface AccountDataClient extends TypedEventEmitter<ClientEvent.Account
      * @param content - the content object to be set
      * @returns an empty object
      */
-    setAccountData: <K extends keyof AccountDataEvents>(eventType: K, content: AccountDataEvents[K]) => Promise<{}>;
+    setAccountData: <K extends keyof AccountDataEvents>(
+        eventType: K,
+        content: AccountDataEvents[K] | Record<string, never>,
+    ) => Promise<{}>;
 }
 
 /**
@@ -316,9 +319,12 @@ export interface ServerSideSecretStorage {
     /**
      * Set the default key ID for encrypting secrets.
      *
+     * If keyId is `null`, the default key id value in the account data will be set to an empty object.
+     * This is considered as "disabling" the default key.
+     *
      * @param keyId - The new default key ID
      */
-    setDefaultKeyId(keyId: string): Promise<void>;
+    setDefaultKeyId(keyId: string | null): Promise<void>;
 }
 
 /**
@@ -357,21 +363,33 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage {
     }
 
     /**
-     * Set the default key ID for encrypting secrets.
-     *
-     * @param keyId - The new default key ID
+     * Implementation of {@link ServerSideSecretStorage#setDefaultKeyId}.
      */
-    public setDefaultKeyId(keyId: string): Promise<void> {
+    public setDefaultKeyId(keyId: string | null): Promise<void> {
         return new Promise<void>((resolve, reject) => {
             const listener = (ev: MatrixEvent): void => {
-                if (ev.getType() === "m.secret_storage.default_key" && ev.getContent().key === keyId) {
+                if (ev.getType() !== "m.secret_storage.default_key") {
+                    //  Different account data item
+                    return;
+                }
+
+                // If keyId === null, the content should be an empty object.
+                // Otherwise, the `key` in the content object should match keyId.
+                const content = ev.getContent();
+                const isSameKey = keyId === null ? Object.keys(content).length === 0 : content.key === keyId;
+                if (isSameKey) {
                     this.accountDataAdapter.removeListener(ClientEvent.AccountData, listener);
                     resolve();
                 }
             };
             this.accountDataAdapter.on(ClientEvent.AccountData, listener);
 
-            this.accountDataAdapter.setAccountData("m.secret_storage.default_key", { key: keyId }).catch((e) => {
+            // The spec [1] says that the value of the account data entry should be an object with a `key` property.
+            // It doesn't specify how to delete the default key; we do it by setting the account data to an empty object.
+            //
+            // [1]: https://spec.matrix.org/v1.13/client-server-api/#key-storage
+            const newValue: Record<string, never> | { key: string } = keyId === null ? {} : { key: keyId };
+            this.accountDataAdapter.setAccountData("m.secret_storage.default_key", newValue).catch((e) => {
                 this.accountDataAdapter.removeListener(ClientEvent.AccountData, listener);
                 reject(e);
             });