From 328b85e53d1d206ecd9406456adac580af044922 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 29 Nov 2023 09:56:52 +0100 Subject: [PATCH 01/24] initial commit --- spec/integ/crypto/megolm-backup.spec.ts | 134 ++++- .../PerSessionKeyBackupDownloader.spec.ts | 538 ++++++++++++++++++ .../PerSessionKeyBackupDownloader.ts | 485 ++++++++++++++++ src/rust-crypto/backup.ts | 2 +- src/rust-crypto/rust-crypto.ts | 188 +++--- 5 files changed, 1267 insertions(+), 80 deletions(-) create mode 100644 spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts create mode 100644 src/rust-crypto/PerSessionKeyBackupDownloader.ts diff --git a/spec/integ/crypto/megolm-backup.spec.ts b/spec/integ/crypto/megolm-backup.spec.ts index d12b7e5486e..f4da98d021d 100644 --- a/spec/integ/crypto/megolm-backup.spec.ts +++ b/spec/integ/crypto/megolm-backup.spec.ts @@ -18,7 +18,7 @@ import fetchMock from "fetch-mock-jest"; import "fake-indexeddb/auto"; import { IDBFactory } from "fake-indexeddb"; -import { createClient, CryptoEvent, ICreateClientOpts, MatrixClient, TypedEventEmitter } from "../../../src"; +import { createClient, CryptoEvent, ICreateClientOpts, IEvent, MatrixClient, TypedEventEmitter } from "../../../src"; import { SyncResponder } from "../../test-utils/SyncResponder"; import { E2EKeyReceiver } from "../../test-utils/E2EKeyReceiver"; import { E2EKeyResponder } from "../../test-utils/E2EKeyResponder"; @@ -34,6 +34,7 @@ import * as testData from "../../test-utils/test-data"; import { KeyBackupInfo } from "../../../src/crypto-api/keybackup"; import { IKeyBackup } from "../../../src/crypto/backup"; import { flushPromises } from "../../test-utils/flushPromises"; +import { defer, IDeferred } from "../../../src/utils"; const ROOM_ID = testData.TEST_ROOM_ID; @@ -888,6 +889,137 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe }); }); + // describe("Backup Changed from other sessions", () => { + // beforeEach(async () => { + // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + // + // // ignore requests to send room key requests + // fetchMock.put("express:/_matrix/client/v3/sendToDevice/m.room_key_request/:request_id", {}); + // + // aliceClient = await initTestClient(); + // const aliceCrypto = aliceClient.getCrypto()!; + // await aliceCrypto.storeSessionBackupPrivateKey( + // Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"), + // testData.SIGNED_BACKUP_DATA.version!, + // ); + // + // // start after saving the private key + // await aliceClient.startClient(); + // + // // tell Alice to trust the dummy device that signed the backup, and re-check the backup. + // // XXX: should we automatically re-check after a device becomes verified? + // await waitForDeviceList(); + // await aliceClient.getCrypto()!.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID); + // await aliceClient.getCrypto()!.checkKeyBackupAndEnable(); + // }); + // + // // let aliceClient: MatrixClient; + // + // const SYNC_RESPONSE = { + // next_batch: 1, + // rooms: { join: { [ROOM_ID]: { timeline: { events: [testData.ENCRYPTED_EVENT] } } } }, + // }; + // + // it("If current backup has changed, the manager should switch to the new one on UTD", async () => { + // // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + // + // // aliceClient = await initTestClient(); + // + // // ===== + // // First ensure that the client checks for keys using the backup version 1 + // /// ===== + // + // fetchMock.get("express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", (url, request) => { + // // check that the version is correct + // const version = new URLSearchParams(new URL(url).search).get("version"); + // if (version == "1") { + // return testData.CURVE25519_KEY_BACKUP_DATA; + // } else { + // return { + // status: 403, + // body: { + // current_version: "1", + // errcode: "M_WRONG_ROOM_KEYS_VERSION", + // error: "Wrong backup version.", + // }, + // }; + // } + // }); + // + // // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the backup. + // syncResponder.sendOrQueueSyncResponse(SYNC_RESPONSE); + // await syncPromise(aliceClient); + // + // const room = aliceClient.getRoom(ROOM_ID)!; + // const event = room.getLiveTimeline().getEvents()[0]; + // await advanceTimersUntil(awaitDecryption(event, { waitOnDecryptionFailure: true })); + // + // expect(event.getContent()).toEqual(testData.CLEAR_EVENT.content); + // + // // ===== + // // Second suppose now that the backup has changed to version 2 + // /// ===== + // + // const awaitHasQueriedOldBackup: IDeferred = defer(); + // const awaitHasQueriedNewBackup: IDeferred = defer(); + // + // fetchMock.get( + // "express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", + // (url, request) => { + // // check that the version is correct + // const version = new URLSearchParams(new URL(url).search).get("version"); + // if (version == "2") { + // awaitHasQueriedNewBackup.resolve(); + // return testData.CURVE25519_KEY_BACKUP_DATA; + // } else { + // awaitHasQueriedOldBackup.resolve(); + // return { + // status: 403, + // body: { + // current_version: "2", + // errcode: "M_WRONG_ROOM_KEYS_VERSION", + // error: "Wrong backup version.", + // }, + // }; + // } + // }, + // { overwriteRoutes: true }, + // ); + // + // const backupVersion2 = { version: "2", ...testData.SIGNED_BACKUP_DATA }; + // fetchMock.get("path:/_matrix/client/v3/room_keys/version", backupVersion2, { overwriteRoutes: true }); + // + // console.log(`Updated backup ${JSON.stringify(backupVersion2)}`); + // // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the new backup. + // const newMessage: Partial = { + // type: "m.room.encrypted", + // room_id: "!room:id", + // sender: "@alice:localhost", + // content: { + // algorithm: "m.megolm.v1.aes-sha2", + // ciphertext: + // "AwgAEpABKvf9FqPW52zeHfeVTn90a3jlBLlx7g6VDEkc2089RQUJoWpSJRiK13E83rN41wgGFJccyfoCr7ZDGJeuGYMGETTrgnLQhLs6JmyPf37JYkzxW8uS8rGUKEqTFQriKhibHVLvVacOlSIObUiKU/V3r176XuixqZF/4eyK9A22JNpInbgI10ZUT6LnApH9LR3FpZbE2zImf1uNPuvp7r0xQbW7CcJjqpH+qTPBD5zFdFnMkc2SnbXCsIOaX11Dm0krWfQz7iA26ZnI1nyZnyh7XPrCnJCRsuQH", + // device_id: "WVMJGTSSVB", + // sender_key: "E5RiY/YCIrHWaF4u416CqvblC6udK2jt9SJ/h1QeLS0", + // session_id: "ybnW+LGdUhoS4fHm1DAEphukO3sZ1GCqZD7UQz7L+GA", + // }, + // event_id: "$event2", + // origin_server_ts: 1507753887000, + // }; + // + // const nextSyncResponse = { + // next_batch: 2, + // rooms: { join: { [ROOM_ID]: { timeline: { events: [newMessage] } } } }, + // }; + // syncResponder.sendOrQueueSyncResponse(nextSyncResponse); + // await syncPromise(aliceClient); + // + // await awaitHasQueriedOldBackup.promise; + // + // await awaitHasQueriedNewBackup.promise; + // }); + // }); + /** make sure that the client knows about the dummy device */ async function waitForDeviceList(): Promise { // Completing the initial sync will make the device list download outdated device lists (of which our own diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts new file mode 100644 index 00000000000..7daffde7c51 --- /dev/null +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -0,0 +1,538 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { Mocked } from "jest-mock"; +import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; + +import { + KeyDownloaderEvent, + KeyDownloadError, + OnDemandBackupDelegate, + PerSessionKeyBackupDownloader, +} from "../../../src/rust-crypto/PerSessionKeyBackupDownloader"; +import { logger } from "../../../src/logger"; +import { defer } from "../../../src/utils"; +import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupDecryptor } from "../../../src/rust-crypto/backup"; +import * as TestData from "../../test-utils/test-data"; +import { ConnectionError, CryptoEvent, MatrixError, TypedEventEmitter } from "../../../src"; + +describe("PerSessionKeyBackupDownloader", () => { + /** The downloader under test */ + let downloader: PerSessionKeyBackupDownloader; + + let delegate: Mocked; + + const BACKOFF_TIME = 2000; + + const mockEmitter = new TypedEventEmitter() as TypedEventEmitter; + + async function expectConfigurationError(error: KeyDownloadError): Promise { + return new Promise((resolve) => { + downloader.on(KeyDownloaderEvent.QueryKeyError, (err) => { + if (err === error) { + resolve(); + } + }); + }); + } + async function expectLoopStatus(expectedLoopRunning: boolean): Promise { + return new Promise((resolve) => { + downloader.on(KeyDownloaderEvent.DownloadLoopStateUpdate, (loopRunning) => { + if (expectedLoopRunning == loopRunning) { + resolve(); + } + }); + }); + } + + async function expectSessionImported(roomId: string, sessionId: string): Promise { + return new Promise((resolve) => { + downloader.on(KeyDownloaderEvent.KeyImported, (r, s) => { + if (roomId == r && sessionId == s) { + resolve(); + } + }); + }); + } + + beforeEach(async () => { + delegate = { + getActiveBackupVersion: jest.fn(), + getBackupDecryptionKey: jest.fn(), + requestRoomKeyFromBackup: jest.fn(), + importRoomKeys: jest.fn(), + createBackupDecryptor: jest.fn(), + requestKeyBackupVersion: jest.fn(), + getCryptoEventEmitter: jest.fn(), + } as unknown as Mocked; + + delegate.getCryptoEventEmitter.mockReturnValue(mockEmitter); + + downloader = new PerSessionKeyBackupDownloader(delegate, logger, BACKOFF_TIME); + + jest.useFakeTimers(); + }); + + afterEach(() => { + downloader.stop(); + jest.useRealTimers(); + }); + + describe("Given valid backup available", () => { + beforeEach(async () => { + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: TestData.SIGNED_BACKUP_DATA.version!, + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + delegate.createBackupDecryptor.mockReturnValue({ + decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), + } as unknown as RustBackupDecryptor); + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + }); + + it("Should download and import a missing key from backup", async () => { + const awaitKeyImported = defer(); + + delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + delegate.importRoomKeys.mockImplementation(async (keys) => { + awaitKeyImported.resolve(); + }); + + downloader.onDecryptionKeyMissingError("roomId", "sessionId"); + + await awaitKeyImported.promise; + + expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledWith("1", "roomId", "sessionId"); + expect(delegate.createBackupDecryptor).toHaveBeenCalledTimes(1); + }); + + it("Should not hammer the backup if the key is requested repeatedly", async () => { + const blockOnServerRequest = defer(); + // simulate a key not being in the backup + delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { + await blockOnServerRequest.promise; + return TestData.CURVE25519_KEY_BACKUP_DATA; + }); + + // Call 3 times + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId2"); + + const session2Imported = new Promise((resolve) => { + downloader.on(KeyDownloaderEvent.KeyImported, (roomId, sessionId) => { + if (sessionId === "sessionId2") { + resolve(); + } + }); + }); + blockOnServerRequest.resolve(); + + await session2Imported; + expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(2); + }); + + it("should continue to next key if current not in backup", async () => { + delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + + delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { + if (session == "sessionA0") { + throw new MatrixError( + { + errcode: "M_NOT_FOUND", + error: "No room_keys found", + }, + 404, + ); + } else if (session == "sessionA1") { + return TestData.CURVE25519_KEY_BACKUP_DATA; + } + }); + + const expectImported = expectSessionImported("!roomA", "sessionA1"); + const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + + await expectNotFound; + await expectImported; + }); + + it("Should not query repeatedly for a key not in backup", async () => { + delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + + delegate.requestRoomKeyFromBackup.mockRejectedValue( + new MatrixError( + { + errcode: "M_NOT_FOUND", + error: "No room_keys found", + }, + 404, + ), + ); + + const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + + await expectNotFound; + + const currentCallCount = delegate.requestRoomKeyFromBackup.mock.calls.length; + + // Should not query again for a key not in backup + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(currentCallCount); + + // advance time to retry + jest.advanceTimersByTime(BACKOFF_TIME + 10); + + const expectNotFoundSecondAttempt = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await expectNotFoundSecondAttempt; + }); + + it("Should stop properly", async () => { + // Simulate a call to stop while request is in flight + const blockOnServerRequest = defer(); + const requestRoomKeyCalled = defer(); + + // Mock the request to block + delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { + requestRoomKeyCalled.resolve(); + await blockOnServerRequest.promise; + return TestData.CURVE25519_KEY_BACKUP_DATA; + }); + + const expectStopped = expectConfigurationError(KeyDownloadError.STOPPED); + const expectLoopStarted = expectLoopStatus(true); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA2"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA3"); + + await expectLoopStarted; + await requestRoomKeyCalled.promise; + downloader.stop(); + + blockOnServerRequest.resolve(); + await expectStopped; + expect(delegate.importRoomKeys).not.toHaveBeenCalled(); + expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(1); + }); + }); + + describe("Given no usable backup available", () => { + let loopPausedPromise: Promise; + let configurationErrorPromise: Promise; + + beforeEach(async () => { + delegate.getActiveBackupVersion.mockResolvedValue(null); + delegate.getBackupDecryptionKey.mockResolvedValue(null); + + loopPausedPromise = expectLoopStatus(false); + + configurationErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); + }); + + it("Should not query server if no backup", async () => { + delegate.requestKeyBackupVersion.mockResolvedValue(null); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + await loopPausedPromise; + await configurationErrorPromise; + }); + + it("Should not query server if backup not active", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // but it's not active + delegate.getActiveBackupVersion.mockResolvedValue(null); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + await loopPausedPromise; + await configurationErrorPromise; + }); + + it("Should stop if backup key is not cached", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // it is active + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + // but the key is not cached + delegate.getBackupDecryptionKey.mockResolvedValue(null); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + await loopPausedPromise; + await configurationErrorPromise; + }); + + it("Should stop if backup key cached as wrong version", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // it is active + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + // but the cached key has the wrong version + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: "0", + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + await loopPausedPromise; + await configurationErrorPromise; + }); + + it("Should stop if backup key version does not match the active one", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // it is active + delegate.getActiveBackupVersion.mockResolvedValue("0"); + // key for old backup cached + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: "0", + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + + await loopPausedPromise; + await configurationErrorPromise; + }); + }); + + describe("Given Backup state update", () => { + it("After initial sync, when backup become trusted it should request keys for past requests", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + + // but at this point it's not trusted and we don't have the key + delegate.getActiveBackupVersion.mockResolvedValue(null); + delegate.getBackupDecryptionKey.mockResolvedValue(null); + + const configErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); + + const a0Imported = expectSessionImported("!roomA", "sessionA0"); + const a1Imported = expectSessionImported("!roomA", "sessionA1"); + const b1Imported = expectSessionImported("!roomB", "sessionB1"); + const c1Imported = expectSessionImported("!roomC", "sessionC1"); + + // During initial sync several keys are requested + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + downloader.onDecryptionKeyMissingError("!roomB", "sessionB1"); + downloader.onDecryptionKeyMissingError("!roomC", "sessionC1"); + + await configErrorPromise; + + // Now the backup becomes trusted + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + // And we have the key in cache + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: TestData.SIGNED_BACKUP_DATA.version!, + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + delegate.createBackupDecryptor.mockReturnValue({ + decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), + } as unknown as RustBackupDecryptor); + + const loopShouldResume = expectLoopStatus(true); + // In that case the sdk would fire a backup status update + mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); + + await loopShouldResume; + + await a0Imported; + await a1Imported; + await b1Imported; + await c1Imported; + }); + + it("If reset from other session, loop should stop until new decryption key is known", async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // It's trusted + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + // And we have the key in cache + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: TestData.SIGNED_BACKUP_DATA.version!, + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + delegate.createBackupDecryptor.mockReturnValue({ + decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), + } as unknown as RustBackupDecryptor); + + const a0Imported = expectSessionImported("!roomA", "sessionA0"); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + + await a0Imported; + + // Now some other session resets the backup and there is a new version + // the room_keys/keys endpoint will throw + delegate.requestRoomKeyFromBackup.mockRejectedValue( + new MatrixError( + { + errcode: "M_NOT_FOUND", + error: "Unknown backup version", + }, + 404, + ), + ); + + const loopPausedPromise = expectLoopStatus(false); + const expectMismatch = expectConfigurationError(KeyDownloadError.VERSION_MISMATCH); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + + await loopPausedPromise; + await expectMismatch; + + // The new backup is detected, the loop should resume but the cached key is still the old one + + const loopResumed = expectLoopStatus(false); + const cacheMismatch = expectConfigurationError(KeyDownloadError.VERSION_MISMATCH); + + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue({ version: "2", ...TestData.SIGNED_BACKUP_DATA }); + // It's trusted + delegate.getActiveBackupVersion.mockResolvedValue("2"); + + mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); + + await loopResumed; + await cacheMismatch; + + // Now the new key is cached + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: "2", + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + + const a1Imported = expectSessionImported("!roomA", "sessionA1"); + + mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); + + await a1Imported; + }); + }); + + describe("Error cases", () => { + beforeEach(async () => { + // there is a backup + delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + // It's trusted + delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + // And we have the key in cache + delegate.getBackupDecryptionKey.mockResolvedValue({ + backupVersion: TestData.SIGNED_BACKUP_DATA.version!, + decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), + } as unknown as RustSdkCryptoJs.BackupKeys); + + delegate.createBackupDecryptor.mockReturnValue({ + decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), + } as unknown as RustBackupDecryptor); + + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + it("Should wait on rate limit error", async () => { + // simulate rate limit error + delegate.requestRoomKeyFromBackup + .mockImplementationOnce(async () => { + throw new MatrixError( + { + errcode: "M_LIMIT_EXCEEDED", + error: "Too many requests", + retry_after_ms: 5000, + }, + 429, + ); + }) + .mockImplementationOnce(async () => TestData.CURVE25519_KEY_BACKUP_DATA); + + const errorPromise = expectConfigurationError(KeyDownloadError.RATE_LIMITED); + const keyImported = expectSessionImported("!roomA", "sessionA0"); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + + await errorPromise; + // The loop should resume after the retry_after_ms + jest.advanceTimersByTime(5000 + 100); + await jest.runAllTimersAsync(); + + await keyImported; + }); + + it("After a network error the same key is retried", async () => { + // simulate connectivity error + delegate.requestRoomKeyFromBackup + .mockImplementationOnce(async () => { + throw new ConnectionError("fetch failed", new Error("fetch failed")); + }) + .mockImplementationOnce(async () => TestData.CURVE25519_KEY_BACKUP_DATA); + + const errorPromise = expectConfigurationError(KeyDownloadError.NETWORK_ERROR); + const keyImported = expectSessionImported("!roomA", "sessionA0"); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + + await errorPromise; + // The loop should resume after the retry_after_ms + jest.advanceTimersByTime(BACKOFF_TIME + 100); + await jest.runAllTimersAsync(); + + await keyImported; + }); + + it("On Unknown error on import skip the key and continue", async () => { + delegate.importRoomKeys + .mockImplementationOnce(async () => { + throw new Error("Didn't work"); + }) + .mockImplementationOnce(async () => { + return; + }); + + const errorPromise = expectConfigurationError(KeyDownloadError.UNKNOWN_ERROR); + const keyImported = expectSessionImported("!roomA", "sessionA1"); + + downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + + await errorPromise; + + await keyImported; + }); + }); +}); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts new file mode 100644 index 00000000000..4f74795a104 --- /dev/null +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -0,0 +1,485 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; +import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; + +import { Curve25519AuthData, KeyBackupInfo, KeyBackupSession } from "../crypto-api/keybackup"; +import { IMegolmSessionData } from "../crypto"; +import { Logger } from "../logger"; +import { ClientPrefix, IHttpOpts, MatrixError, MatrixHttpApi, Method } from "../http-api"; +import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupDecryptor, RustBackupManager } from "./backup"; +import { CryptoEvent, TypedEventEmitter } from "../matrix"; +import { encodeUri, sleep } from "../utils"; +import { RustCrypto } from "./rust-crypto"; + +/** + * Extract the dependices of the OnDemandKeyBackupDownloader, main reason is to make testing easier. + */ +export interface OnDemandBackupDelegate { + getActiveBackupVersion(): Promise; + + getBackupDecryptionKey(): Promise; + + requestRoomKeyFromBackup(version: string, rooomId: string, sessionId: string): Promise; + + importRoomKeys(keys: IMegolmSessionData[]): Promise; + + createBackupDecryptor(key: RustSdkCryptoJs.BackupDecryptionKey): RustBackupDecryptor; + + requestKeyBackupVersion(): Promise; + + /** + * The backup downloader will listen to these events to know when to check for backup status changes in order to + * resume or stop querying. + */ + getCryptoEventEmitter(): TypedEventEmitter; +} + +/** + * Utility to create a delegate for the OnDemandKeyBackupDownloader that is usable by rust crypto. + * @param rustCrypto - The rust crypto instance. + * @param backupManager - The backup manager instance. + * @param olmMachine - The olm machine instance. + * @param http - The http instance. + */ +export function createDelegate( + rustCrypto: RustCrypto, + backupManager: RustBackupManager, + olmMachine: OlmMachine, + http: MatrixHttpApi, +): OnDemandBackupDelegate { + return { + getActiveBackupVersion(): Promise { + return backupManager.getActiveBackupVersion(); + }, + + async getBackupDecryptionKey(): Promise { + try { + return await olmMachine.getBackupKeys(); + } catch (e) { + return null; + } + }, + + async requestRoomKeyFromBackup(version: string, roomId: string, sessionId: string): Promise { + const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { + $roomId: roomId, + $sessionId: sessionId, + }); + + return await http.authedRequest(Method.Get, path, { version }, undefined, { + prefix: ClientPrefix.V3, + }); + }, + + async importRoomKeys(keys: IMegolmSessionData[]): Promise { + return rustCrypto.importRoomKeys(keys); + }, + + createBackupDecryptor: (key: RustSdkCryptoJs.BackupDecryptionKey): RustBackupDecryptor => { + return new RustBackupDecryptor(key); + }, + + async requestKeyBackupVersion(): Promise { + return await backupManager.requestKeyBackupVersion(); + }, + + getCryptoEventEmitter(): TypedEventEmitter { + return rustCrypto; + }, + }; +} + +export enum KeyDownloadError { + VERSION_MISMATCH = "VERSION_MISMATCH", + MISSING_DECRYPTION_KEY = "MISSING_DECRYPTION_KEY", + NETWORK_ERROR = "NETWORK_ERROR", + STOPPED = "STOPPED", + UNKNOWN_ERROR = "UNKNOWN_ERROR", + RATE_LIMITED = "RATE_LIMITED", + CONFIGURATION_ERROR = "CONFIGURATION_ERROR", +} + +type SessionInfo = { roomId: string; megolmSessionId: string }; + +type KeyDownloadResult = + | { ok: true; value: KeyBackupSession } + | { ok: false; error: KeyDownloadError; [key: string]: any }; + +type Configuration = { + backupVersion: string; + decryptor: RustBackupDecryptor; +}; + +export enum KeyDownloaderEvent { + DownloadLoopStateUpdate = "download_loop_started", + DownLoopStep = "download_loop_step", + QueryKeyError = "query_key_error", + KeyImported = "key_imported", +} +export type KeyDownloaderEventMap = { + [KeyDownloaderEvent.DownloadLoopStateUpdate]: (loopRunning: boolean) => void; + [KeyDownloaderEvent.DownLoopStep]: (remaining: number) => void; + [KeyDownloaderEvent.QueryKeyError]: (errCode: KeyDownloadError) => void; + [KeyDownloaderEvent.KeyImported]: (roomId: string, sessionId: string) => void; +}; + +/** + * When an unable to decrypt error is encountered, the client will call this + * in order to try to download the key from the backup. + * + */ +export class PerSessionKeyBackupDownloader extends TypedEventEmitter { + private stopped = false; + + private configuration: Configuration | null = null; + + /** We remember when a session was requested and not found in backup to avoid query again too soon. */ + private sessionLastCheckAttemptedTime: Record = {}; + + public constructor( + private readonly delegate: OnDemandBackupDelegate, + private readonly logger: Logger, + private readonly maxTimeBetweenRetry: number, + ) { + super(); + + const emitter = this.delegate.getCryptoEventEmitter(); + + emitter.on(CryptoEvent.KeyBackupStatus, (ev) => { + this.logger.info(`Key backup status changed, check configuration`); + // we want to check configuration + this.onBackupStatusChanged(); + }); + + emitter.on(CryptoEvent.KeyBackupFailed, (ev) => { + this.logger.info(`Key backup status changed, check configuration`); + // we want to check configuration + this.onBackupStatusChanged(); + }); + } + + public stop(): void { + this.stopped = true; + } + + private onBackupStatusChanged(): void { + // we want to check configuration + this.hasConfigurationProblem = false; + this.getOrCreateBackupDecryptor(true).then((decryptor) => { + if (decryptor) { + this.downloadKeysLoop(); + } + }); + } + + private downloadLoopRunning = false; + + private queuedRequests: SessionInfo[] = []; + + private isAlreadyInQueue(roomId: string, megolmSessionId: string): boolean { + return ( + this.queuedRequests.findIndex((info) => { + return info.roomId == roomId && info.megolmSessionId == megolmSessionId; + }) != -1 + ); + } + + private markAsNotFoundInBackup(megolmSessionId: string): void { + const now = Date.now(); + this.sessionLastCheckAttemptedTime[megolmSessionId] = now; + // if too big make some cleaning to keep under control + if (Object.keys(this.sessionLastCheckAttemptedTime).length > 100) { + for (const key in this.sessionLastCheckAttemptedTime) { + if (Math.max(now - this.sessionLastCheckAttemptedTime[key], 0) > this.maxTimeBetweenRetry) { + delete this.sessionLastCheckAttemptedTime[key]; + } + } + } + } + + private wasRequestedRecently(megolmSessionId: string): boolean { + const lastCheck = this.sessionLastCheckAttemptedTime[megolmSessionId]; + if (!lastCheck) return false; + return Math.max(Date.now() - lastCheck, 0) < this.maxTimeBetweenRetry; + } + + private hasConfigurationProblem = false; + + private pauseLoop(): void { + this.downloadLoopRunning = false; + this.emit(KeyDownloaderEvent.DownloadLoopStateUpdate, false); + } + /** + * Called when a MissingRoomKey or UnknownMessageIndex decryption error is encountered. + * + * This will try to download the key from the backup if there is a trusted active backup. + * In case of success the key will be imported and the onRoomKeysUpdated callback will be called + * internally by the rust-sdk and decrytion will be retried. + * + * @param roomId - The room ID of the room where the error occurred. + * @param megolmSessionId - The megolm session ID that is missing. + */ + public onDecryptionKeyMissingError(roomId: string, megolmSessionId: string): void { + // Several messages encrypted with the same session may be decrypted at the same time, + // so we need to be resistant and not query several time the same session. + if (this.isAlreadyInQueue(roomId, megolmSessionId)) { + // There is already a request queued for this session, no need to queue another one. + this.logger.trace(`Not checking key backup for session ${megolmSessionId} as it is already queued`); + return; + } + + if (this.wasRequestedRecently(megolmSessionId)) { + // We already tried to download this session recently, no need to try again. + this.logger.trace( + `Not checking key backup for session ${megolmSessionId} as it was already requested recently`, + ); + return; + } + + // We always add the request to the queue, even if we have a configuration problem (can't access backup). + // This is to make sure that if the configuration problem is resolved, we will try to download the key. + // This will happen after an initial sync, at this point the backup will not yet be trusted, but it will be + // just after the verification. + // We don't need to persist it because currently on refresh the sdk will retry to decrypt the messages. + this.queuedRequests.push({ roomId, megolmSessionId }); + + // Start the download loop if it's not already running. + this.downloadKeysLoop(); + } + + private async downloadKeysLoop(): Promise { + if (this.downloadLoopRunning) return; + + // If we have a configuration problem, we don't want to try to download. + // If any configuration change is detected, we will retry and restart the loop. + if (this.hasConfigurationProblem) return; + + this.downloadLoopRunning = true; + this.emit(KeyDownloaderEvent.DownloadLoopStateUpdate, true); + + while (this.queuedRequests.length > 0) { + this.emit(KeyDownloaderEvent.DownLoopStep, this.queuedRequests.length); + // we just peek the first one without removing it, so if a new request for same key comes in while we're + // processing this one, it won't queue another request. + const request = this.queuedRequests[0]; + const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); + if (this.stopped) { + this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.STOPPED); + return; + } + if (result.ok) { + // We got the encrypted key from backup, let's try to decrypt and import it. + try { + await this.decryptAndImport(request, result.value); + this.emit(KeyDownloaderEvent.KeyImported, request.roomId, request.megolmSessionId); + } catch (e) { + this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.UNKNOWN_ERROR); + this.logger.error( + `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, + e, + ); + } + // now remove the request from the queue as we've processed it. + this.queuedRequests.shift(); + } else { + this.emit(KeyDownloaderEvent.QueryKeyError, result.error); + this.logger.debug( + `Error while downloading key backup for session ${request.megolmSessionId}: ${result.error}`, + ); + switch (result.error) { + case KeyDownloadError.VERSION_MISMATCH: { + // We don't have the correct decryption key, so stop the loop. + // If we get the key later, we will retry. + this.pauseLoop(); + return; + } + case KeyDownloadError.MISSING_DECRYPTION_KEY: { + this.markAsNotFoundInBackup(request.megolmSessionId); + // continue for next one + this.queuedRequests.shift(); + break; + } + case KeyDownloadError.CONFIGURATION_ERROR: { + // Backup is not configured correctly, so stop the loop. + this.pauseLoop(); + return; + } + case KeyDownloadError.RATE_LIMITED: { + // we want to retry + await sleep(result.retryAfterMs ?? this.maxTimeBetweenRetry); + break; + } + case KeyDownloadError.NETWORK_ERROR: { + // We don't want to hammer if there is a problem, so wait a bit. + await sleep(this.maxTimeBetweenRetry); + break; + } + case KeyDownloadError.STOPPED: + // If the downloader was stopped, we don't want to retry. + this.pauseLoop(); + return; + } + } + } + this.pauseLoop(); + } + /** + * Query the backup for a key. + * + * @param targetRoomId - ID of the room that the session is used in. + * @param targetSessionId - ID of the session for which to check backup. + */ + private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { + const configuration = await this.getOrCreateBackupDecryptor(false); + if (!configuration) { + return { ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }; + } + + this.logger.debug(`Checking key backup for session ${targetSessionId}`); + + let res: KeyBackupSession; + + try { + res = await this.delegate.requestRoomKeyFromBackup( + configuration.backupVersion, + targetRoomId, + targetSessionId, + ); + } catch (e) { + if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; + + this.logger.info(`No luck requesting key backup for session ${targetSessionId}: ${e}`); + if (e instanceof MatrixError) { + const errCode = e.data.errcode; + if (errCode == "M_NOT_FOUND") { + // Unfortunately the spec doesn't give us a way to differentiate between a missing key and a wrong version. + // Synapse will return: + // - "error": "Unknown backup version" if the version is wrong. + // - "error": "No room_keys found" if the key is missing. + // For now we check the error message, but this is not ideal. + // It's useful to know if the key is missing or if the version is wrong. + if (e.data.error == "Unknown backup version") { + return { ok: false, error: KeyDownloadError.VERSION_MISMATCH }; + } + return { ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }; + } + if (errCode == "M_LIMIT_EXCEEDED") { + const waitTime = e.data.retry_after_ms; + if (waitTime > 0) { + this.logger.info(`Rate limited by server, waiting ${waitTime}ms`); + return { ok: false, error: KeyDownloadError.RATE_LIMITED, retryAfterMs: waitTime }; + } else { + // apply a backoff time + return { + ok: false, + error: KeyDownloadError.RATE_LIMITED, + retryAfterMs: this.maxTimeBetweenRetry, + }; + } + } + } + return { ok: false, error: KeyDownloadError.NETWORK_ERROR }; + } + + if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; + + return { + ok: true, + value: res, + }; + } + + private async getOrCreateBackupDecryptor(forceCheck: boolean): Promise { + if (this.configuration) { + return this.configuration; + } + + if (this.hasConfigurationProblem && !forceCheck) { + return null; + } + + const currentServerVersion = await this.delegate.requestKeyBackupVersion(); + + if (currentServerVersion?.algorithm != "m.megolm_backup.v1.curve25519-aes-sha2") { + this.logger.info(`getBackupDecryptor Unsupported algorithm ${currentServerVersion?.algorithm}`); + this.hasConfigurationProblem = true; + return null; + } + + if (!currentServerVersion?.version) { + this.logger.info(`No current key backup`); + this.hasConfigurationProblem = true; + return null; + } + + const activeVersion = await this.delegate.getActiveBackupVersion(); + if (activeVersion == null || currentServerVersion.version != activeVersion) { + // case when the server side current version is not trusted or is out of sync with the client side active version. + this.logger.info( + `The current backup version ${currentServerVersion.version} is not trusted. Active version=${activeVersion}`, + ); + this.hasConfigurationProblem = true; + return null; + } + + const authData = currentServerVersion.auth_data; + + const backupKeys = await this.delegate.getBackupDecryptionKey(); + if (!backupKeys?.decryptionKey) { + this.logger.debug(`Not checking key backup for session(no decryption key)`); + this.hasConfigurationProblem = true; + return null; + } + + if (activeVersion != backupKeys.backupVersion) { + this.logger.debug(`Cached key version doesn't match active backup version`); + this.hasConfigurationProblem = true; + return null; + } + + if (authData.public_key != backupKeys.decryptionKey.megolmV1PublicKey.publicKeyBase64) { + this.logger.debug(`getBackupDecryptor key mismatch error`); + this.hasConfigurationProblem = true; + return null; + } + + const backupDecryptor = this.delegate.createBackupDecryptor(backupKeys.decryptionKey); + this.hasConfigurationProblem = false; + this.configuration = { + decryptor: backupDecryptor, + backupVersion: activeVersion, + }; + return this.configuration; + } + + private async decryptAndImport(sessionInfo: SessionInfo, data: KeyBackupSession): Promise { + const configuration = await this.getOrCreateBackupDecryptor(false); + + if (!configuration) { + throw new Error("Backup: No configuration"); + } + + const sessionsToImport: Record = { [sessionInfo.megolmSessionId]: data }; + + const keys = await configuration!.decryptor.decryptSessions(sessionsToImport); + for (const k of keys) { + k.room_id = sessionInfo.roomId; + } + await this.delegate.importRoomKeys(keys); + } +} diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index a040b37ed55..46d8597f2ad 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -325,7 +325,7 @@ export class RustBackupManager extends TypedEventEmitter { + public async requestKeyBackupVersion(): Promise { try { return await this.http.authedRequest( Method.Get, diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index f0c3c8973e0..7f2c144bf06 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -25,11 +25,11 @@ import { Room } from "../models/room"; import { RoomMember } from "../models/room-member"; import { BackupDecryptor, CryptoBackend, OnSyncCompletedData } from "../common-crypto/CryptoBackend"; import { Logger } from "../logger"; -import { ClientPrefix, IHttpOpts, MatrixHttpApi, Method } from "../http-api"; +import { IHttpOpts, MatrixHttpApi, Method } from "../http-api"; import { RoomEncryptor } from "./RoomEncryptor"; import { OutgoingRequestProcessor } from "./OutgoingRequestProcessor"; import { KeyClaimManager } from "./KeyClaimManager"; -import { encodeUri, MapWithDefault } from "../utils"; +import { MapWithDefault } from "../utils"; import { BackupTrustInfo, BootstrapCrossSigningOpts, @@ -48,7 +48,6 @@ import { ImportRoomKeysOpts, KeyBackupCheck, KeyBackupInfo, - KeyBackupSession, UserVerificationStatus, VerificationRequest, } from "../crypto-api"; @@ -73,6 +72,7 @@ import { ISignatures } from "../@types/signed"; import { encodeBase64 } from "../base64"; import { DecryptionError } from "../crypto/algorithms"; import { OutgoingRequestsManager } from "./OutgoingRequestsManager"; +import { createDelegate, PerSessionKeyBackupDownloader } from "./PerSessionKeyBackupDownloader"; const ALL_VERIFICATION_METHODS = ["m.sas.v1", "m.qr_code.scan.v1", "m.qr_code.show.v1", "m.reciprocate.v1"]; @@ -81,7 +81,7 @@ interface ISignableObject { unsigned?: object; } -const KEY_BACKUP_CHECK_RATE_LIMIT = 5000; // ms +// const KEY_BACKUP_CHECK_RATE_LIMIT = 5000; // ms /** * An implementation of {@link CryptoBackend} using the Rust matrix-sdk-crypto. @@ -104,7 +104,9 @@ export class RustCrypto extends TypedEventEmitter = {}; // When did we last try to check the server for a given session id? + private readonly perSessionBackupDownloader: PerSessionKeyBackupDownloader; + + // private sessionLastCheckAttemptedTime: Record = {}; // When did we last try to check the server for a given session id? private readonly reemitter = new TypedReEmitter(this); @@ -142,9 +144,17 @@ export class RustCrypto extends TypedEventEmitter KEY_BACKUP_CHECK_RATE_LIMIT) { - this.sessionLastCheckAttemptedTime[targetSessionId!] = now; - this.queryKeyBackup(targetRoomId, targetSessionId).catch((e) => { - this.logger.error(`Unhandled error while checking key backup for session ${targetSessionId}`, e); - }); - } else { - const lastCheckStr = new Date(lastCheck).toISOString(); - this.logger.debug( - `Not checking key backup for session ${targetSessionId} (last checked at ${lastCheckStr})`, - ); - } - } - - /** - * Helper for {@link RustCrypto#startQueryKeyBackupRateLimited}. - * - * Requests the backup and imports it. Doesn't do any rate-limiting. - * - * @param targetRoomId - ID of the room that the session is used in. - * @param targetSessionId - ID of the session for which to check backup. - */ - private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { - const backupKeys: RustSdkCryptoJs.BackupKeys = await this.olmMachine.getBackupKeys(); - if (!backupKeys.decryptionKey) { - this.logger.debug(`Not checking key backup for session ${targetSessionId} (no decryption key)`); - return; - } - - this.logger.debug(`Checking key backup for session ${targetSessionId}`); - - const version = backupKeys.backupVersion; - const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { - $roomId: targetRoomId, - $sessionId: targetSessionId, - }); - - let res: KeyBackupSession; - try { - res = await this.http.authedRequest(Method.Get, path, { version }, undefined, { - prefix: ClientPrefix.V3, - }); - } catch (e) { - this.logger.info(`No luck requesting key backup for session ${targetSessionId}: ${e}`); - return; - } - - if (this.stopped) return; - - const backupDecryptor = new RustBackupDecryptor(backupKeys.decryptionKey); - const sessionsToImport: Record = { [targetSessionId]: res }; - const keys = await backupDecryptor.decryptSessions(sessionsToImport); - for (const k of keys) { - k.room_id = targetRoomId; - } - await this.importRoomKeys(keys); - } + // + // /** + // * Starts an attempt to retrieve a session from a key backup, if enough time + // * has elapsed since the last check for this session id. + // * + // * If a backup is found, it is decrypted and imported. + // * + // * @param targetRoomId - ID of the room that the session is used in. + // * @param targetSessionId - ID of the session for which to check backup. + // */ + // public startQueryKeyBackupRateLimited(targetRoomId: string, targetSessionId: string): void { + // const now = new Date().getTime(); + // const lastCheck = this.sessionLastCheckAttemptedTime[targetSessionId]; + // if (!lastCheck || now - lastCheck > KEY_BACKUP_CHECK_RATE_LIMIT) { + // this.sessionLastCheckAttemptedTime[targetSessionId!] = now; + // this.queryKeyBackup(targetRoomId, targetSessionId).catch((e) => { + // this.logger.error(`Unhandled error while checking key backup for session ${targetSessionId}`, e); + // }); + // } else { + // const lastCheckStr = new Date(lastCheck).toISOString(); + // this.logger.debug( + // `Not checking key backup for session ${targetSessionId} (last checked at ${lastCheckStr})`, + // ); + // } + // } + + // /** + // * Helper for {@link RustCrypto#startQueryKeyBackupRateLimited}. + // * + // * Requests the backup and imports it. Doesn't do any rate-limiting. + // * + // * @param targetRoomId - ID of the room that the session is used in. + // * @param targetSessionId - ID of the session for which to check backup. + // */ + // private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { + // const backupKeys: RustSdkCryptoJs.BackupKeys = await this.olmMachine.getBackupKeys(); + // if (!backupKeys.decryptionKey) { + // this.logger.debug(`Not checking key backup for session ${targetSessionId} (no decryption key)`); + // return; + // } + // + // const activeBackup = await this.backupManager.getActiveBackupVersion(); + // if (!activeBackup) { + // this.logger.warn(`No active key backup, or current backup not trusted aborting key backup download`); + // return; + // } + // + // if(activeBackup != backupKeys.backupVersion) { + // this.logger.warn(`Cached backup version ${backupKeys.backupVersion} does not match active backup version ${activeBackup}`); + // // The cached decryption key is out of sync, the user need to resync with 4S or request missing secrets to other sessions. + // return; + // } + // + // this.logger.debug(`Checking key backup for session ${targetSessionId}`); + // + // const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { + // $roomId: targetRoomId, + // $sessionId: targetSessionId, + // }); + // + // let res: KeyBackupSession; + // try { + // res = await this.http.authedRequest(Method.Get, path, { activeBackup }, undefined, { + // prefix: ClientPrefix.V3, + // }); + // } catch (e) { + // this.logger.info(`No luck requesting key backup for session ${targetSessionId}: ${e}`); + // if (e instanceof MatrixError) { + // const errCode = e.data.errcode; + // if (errCode == "M_NOT_FOUND" || errCode == "M_WRONG_ROOM_KEYS_VERSION") { + // // The active backup version is out of sync, force a check to sync with server. + // try { + // this.backupManager.checkKeyBackupAndEnable(true); + // } catch(e) { + // this.logger.error(`Error while checking key backup: ${e}`); + // } + // } + // } + // return; + // } + // + // if (this.stopped) return; + // + // const backupDecryptor = new RustBackupDecryptor(backupKeys.decryptionKey); + // const sessionsToImport: Record = { [targetSessionId]: res }; + // const keys = await backupDecryptor.decryptSessions(sessionsToImport); + // for (const k of keys) { + // k.room_id = targetRoomId; + // } + // await this.importRoomKeys(keys); + // } /** * Return the OlmMachine only if {@link RustCrypto#stop} has not been called. @@ -1556,7 +1588,7 @@ class EventDecryptor { public constructor( private readonly logger: Logger, private readonly olmMachine: RustSdkCryptoJs.OlmMachine, - private readonly crypto: RustCrypto, + private readonly perSessionBackupDownloader: PerSessionKeyBackupDownloader, ) {} public async attemptEventDecryption(event: MatrixEvent): Promise { @@ -1597,7 +1629,7 @@ class EventDecryptor { session: content.sender_key + "|" + content.session_id, }, ); - this.crypto.startQueryKeyBackupRateLimited( + this.perSessionBackupDownloader.onDecryptionKeyMissingError( event.getRoomId()!, event.getWireContent().session_id!, ); @@ -1611,7 +1643,7 @@ class EventDecryptor { session: content.sender_key + "|" + content.session_id, }, ); - this.crypto.startQueryKeyBackupRateLimited( + this.perSessionBackupDownloader.onDecryptionKeyMissingError( event.getRoomId()!, event.getWireContent().session_id!, ); From 00df2732eb850d48c481245041fb9a616fb572de Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 29 Nov 2023 11:36:11 +0100 Subject: [PATCH 02/24] new interation test --- spec/integ/crypto/megolm-backup.spec.ts | 270 +++++++++--------- .../PerSessionKeyBackupDownloader.ts | 10 +- 2 files changed, 149 insertions(+), 131 deletions(-) diff --git a/spec/integ/crypto/megolm-backup.spec.ts b/spec/integ/crypto/megolm-backup.spec.ts index f4da98d021d..b57db363808 100644 --- a/spec/integ/crypto/megolm-backup.spec.ts +++ b/spec/integ/crypto/megolm-backup.spec.ts @@ -889,136 +889,146 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe }); }); - // describe("Backup Changed from other sessions", () => { - // beforeEach(async () => { - // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); - // - // // ignore requests to send room key requests - // fetchMock.put("express:/_matrix/client/v3/sendToDevice/m.room_key_request/:request_id", {}); - // - // aliceClient = await initTestClient(); - // const aliceCrypto = aliceClient.getCrypto()!; - // await aliceCrypto.storeSessionBackupPrivateKey( - // Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"), - // testData.SIGNED_BACKUP_DATA.version!, - // ); - // - // // start after saving the private key - // await aliceClient.startClient(); - // - // // tell Alice to trust the dummy device that signed the backup, and re-check the backup. - // // XXX: should we automatically re-check after a device becomes verified? - // await waitForDeviceList(); - // await aliceClient.getCrypto()!.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID); - // await aliceClient.getCrypto()!.checkKeyBackupAndEnable(); - // }); - // - // // let aliceClient: MatrixClient; - // - // const SYNC_RESPONSE = { - // next_batch: 1, - // rooms: { join: { [ROOM_ID]: { timeline: { events: [testData.ENCRYPTED_EVENT] } } } }, - // }; - // - // it("If current backup has changed, the manager should switch to the new one on UTD", async () => { - // // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); - // - // // aliceClient = await initTestClient(); - // - // // ===== - // // First ensure that the client checks for keys using the backup version 1 - // /// ===== - // - // fetchMock.get("express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", (url, request) => { - // // check that the version is correct - // const version = new URLSearchParams(new URL(url).search).get("version"); - // if (version == "1") { - // return testData.CURVE25519_KEY_BACKUP_DATA; - // } else { - // return { - // status: 403, - // body: { - // current_version: "1", - // errcode: "M_WRONG_ROOM_KEYS_VERSION", - // error: "Wrong backup version.", - // }, - // }; - // } - // }); - // - // // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the backup. - // syncResponder.sendOrQueueSyncResponse(SYNC_RESPONSE); - // await syncPromise(aliceClient); - // - // const room = aliceClient.getRoom(ROOM_ID)!; - // const event = room.getLiveTimeline().getEvents()[0]; - // await advanceTimersUntil(awaitDecryption(event, { waitOnDecryptionFailure: true })); - // - // expect(event.getContent()).toEqual(testData.CLEAR_EVENT.content); - // - // // ===== - // // Second suppose now that the backup has changed to version 2 - // /// ===== - // - // const awaitHasQueriedOldBackup: IDeferred = defer(); - // const awaitHasQueriedNewBackup: IDeferred = defer(); - // - // fetchMock.get( - // "express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", - // (url, request) => { - // // check that the version is correct - // const version = new URLSearchParams(new URL(url).search).get("version"); - // if (version == "2") { - // awaitHasQueriedNewBackup.resolve(); - // return testData.CURVE25519_KEY_BACKUP_DATA; - // } else { - // awaitHasQueriedOldBackup.resolve(); - // return { - // status: 403, - // body: { - // current_version: "2", - // errcode: "M_WRONG_ROOM_KEYS_VERSION", - // error: "Wrong backup version.", - // }, - // }; - // } - // }, - // { overwriteRoutes: true }, - // ); - // - // const backupVersion2 = { version: "2", ...testData.SIGNED_BACKUP_DATA }; - // fetchMock.get("path:/_matrix/client/v3/room_keys/version", backupVersion2, { overwriteRoutes: true }); - // - // console.log(`Updated backup ${JSON.stringify(backupVersion2)}`); - // // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the new backup. - // const newMessage: Partial = { - // type: "m.room.encrypted", - // room_id: "!room:id", - // sender: "@alice:localhost", - // content: { - // algorithm: "m.megolm.v1.aes-sha2", - // ciphertext: - // "AwgAEpABKvf9FqPW52zeHfeVTn90a3jlBLlx7g6VDEkc2089RQUJoWpSJRiK13E83rN41wgGFJccyfoCr7ZDGJeuGYMGETTrgnLQhLs6JmyPf37JYkzxW8uS8rGUKEqTFQriKhibHVLvVacOlSIObUiKU/V3r176XuixqZF/4eyK9A22JNpInbgI10ZUT6LnApH9LR3FpZbE2zImf1uNPuvp7r0xQbW7CcJjqpH+qTPBD5zFdFnMkc2SnbXCsIOaX11Dm0krWfQz7iA26ZnI1nyZnyh7XPrCnJCRsuQH", - // device_id: "WVMJGTSSVB", - // sender_key: "E5RiY/YCIrHWaF4u416CqvblC6udK2jt9SJ/h1QeLS0", - // session_id: "ybnW+LGdUhoS4fHm1DAEphukO3sZ1GCqZD7UQz7L+GA", - // }, - // event_id: "$event2", - // origin_server_ts: 1507753887000, - // }; - // - // const nextSyncResponse = { - // next_batch: 2, - // rooms: { join: { [ROOM_ID]: { timeline: { events: [newMessage] } } } }, - // }; - // syncResponder.sendOrQueueSyncResponse(nextSyncResponse); - // await syncPromise(aliceClient); - // - // await awaitHasQueriedOldBackup.promise; - // - // await awaitHasQueriedNewBackup.promise; - // }); - // }); + describe("Backup Changed from other sessions", () => { + beforeEach(async () => { + fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + + // ignore requests to send room key requests + fetchMock.put("express:/_matrix/client/v3/sendToDevice/m.room_key_request/:request_id", {}); + + aliceClient = await initTestClient(); + const aliceCrypto = aliceClient.getCrypto()!; + await aliceCrypto.storeSessionBackupPrivateKey( + Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"), + testData.SIGNED_BACKUP_DATA.version!, + ); + + // start after saving the private key + await aliceClient.startClient(); + + // tell Alice to trust the dummy device that signed the backup, and re-check the backup. + // XXX: should we automatically re-check after a device becomes verified? + await waitForDeviceList(); + await aliceClient.getCrypto()!.setDeviceVerified(testData.TEST_USER_ID, testData.TEST_DEVICE_ID); + await aliceClient.getCrypto()!.checkKeyBackupAndEnable(); + }); + + // let aliceClient: MatrixClient; + + const SYNC_RESPONSE = { + next_batch: 1, + rooms: { join: { [ROOM_ID]: { timeline: { events: [testData.ENCRYPTED_EVENT] } } } }, + }; + + it("If current backup has changed, the manager should switch to the new one on UTD", async () => { + // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + + // aliceClient = await initTestClient(); + + // ===== + // First ensure that the client checks for keys using the backup version 1 + /// ===== + + fetchMock.get("express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", (url, request) => { + // check that the version is correct + const version = new URLSearchParams(new URL(url).search).get("version"); + if (version == "1") { + return testData.CURVE25519_KEY_BACKUP_DATA; + } else { + return { + status: 403, + body: { + current_version: "1", + errcode: "M_WRONG_ROOM_KEYS_VERSION", + error: "Wrong backup version.", + }, + }; + } + },{ overwriteRoutes: true }); + + // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the backup. + syncResponder.sendOrQueueSyncResponse(SYNC_RESPONSE); + await syncPromise(aliceClient); + + const room = aliceClient.getRoom(ROOM_ID)!; + const event = room.getLiveTimeline().getEvents()[0]; + await advanceTimersUntil(awaitDecryption(event, { waitOnDecryptionFailure: true })); + + expect(event.getContent()).toEqual(testData.CLEAR_EVENT.content); + + + // ===== + // Second suppose now that the backup has changed to version 2 + /// ===== + + const newBackup = { + ...testData.SIGNED_BACKUP_DATA, + version: "2", + }; + + fetchMock.get("path:/_matrix/client/v3/room_keys/version", newBackup, { overwriteRoutes: true }); + // suppose the new key is now known + const aliceCrypto = aliceClient.getCrypto()!; + await aliceCrypto.storeSessionBackupPrivateKey( + Buffer.from(testData.BACKUP_DECRYPTION_KEY_BASE64, "base64"), + newBackup.version, + ); + + // A check backup should happen at some point + await aliceCrypto.checkKeyBackupAndEnable(); + + const awaitHasQueriedNewBackup: IDeferred = defer(); + + fetchMock.get( + "express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", + (url, request) => { + // check that the version is correct + const version = new URLSearchParams(new URL(url).search).get("version"); + if (version == newBackup.version) { + awaitHasQueriedNewBackup.resolve(); + return testData.CURVE25519_KEY_BACKUP_DATA; + } else { + // awaitHasQueriedOldBackup.resolve(); + return { + status: 403, + body: { + current_version: "2", + errcode: "M_WRONG_ROOM_KEYS_VERSION", + error: "Wrong backup version.", + }, + }; + } + }, + { overwriteRoutes: true }, + ); + + // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the new backup. + const newMessage: Partial = { + type: "m.room.encrypted", + room_id: "!room:id", + sender: "@alice:localhost", + content: { + algorithm: "m.megolm.v1.aes-sha2", + ciphertext: + "AwgAEpABKvf9FqPW52zeHfeVTn90a3jlBLlx7g6VDEkc2089RQUJoWpSJRiK13E83rN41wgGFJccyfoCr7ZDGJeuGYMGETTrgnLQhLs6JmyPf37JYkzxW8uS8rGUKEqTFQriKhibHVLvVacOlSIObUiKU/V3r176XuixqZF/4eyK9A22JNpInbgI10ZUT6LnApH9LR3FpZbE2zImf1uNPuvp7r0xQbW7CcJjqpH+qTPBD5zFdFnMkc2SnbXCsIOaX11Dm0krWfQz7iA26ZnI1nyZnyh7XPrCnJCRsuQH", + device_id: "WVMJGTSSVB", + sender_key: "E5RiY/YCIrHWaF4u416CqvblC6udK2jt9SJ/h1QeLS0", + session_id: "ybnW+LGdUhoS4fHm1DAEphukO3sZ1GCqZD7UQz7L+GA", + }, + event_id: "$event2", + origin_server_ts: 1507753887000, + }; + + const nextSyncResponse = { + next_batch: 2, + rooms: { join: { [ROOM_ID]: { timeline: { events: [newMessage] } } } }, + }; + syncResponder.sendOrQueueSyncResponse(nextSyncResponse); + await syncPromise(aliceClient); + + await awaitHasQueriedNewBackup.promise; + }); + }); /** make sure that the client knows about the dummy device */ async function waitForDeviceList(): Promise { diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 4f74795a104..0a92845d3ba 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -167,10 +167,17 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { - this.logger.info(`Key backup status changed, check configuration`); + this.logger.info(`Key backup upload failed, check configuration`); // we want to check configuration this.onBackupStatusChanged(); }); + + /// TODO When the PR that adds signaling when the decryption is merged, we can use it to trigger a refresh + // emitter.on(CryptoEvent.KeyBackupPrivateKeyCached, (ev) => { + // this.logger.info(`Key backup decryption key is known, check configuration`); + // // we want to check configuration + // this.onBackupStatusChanged(); + // }); } public stop(): void { @@ -180,6 +187,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { if (decryptor) { this.downloadKeysLoop(); From 3f8c99eb590a6cfec513b41f9991a5704e058843 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 29 Nov 2023 11:49:26 +0100 Subject: [PATCH 03/24] more comments --- spec/integ/crypto/megolm-backup.spec.ts | 37 +++++++------- .../PerSessionKeyBackupDownloader.ts | 51 ++++++++++++++++++- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/spec/integ/crypto/megolm-backup.spec.ts b/spec/integ/crypto/megolm-backup.spec.ts index b57db363808..e8f7b84a544 100644 --- a/spec/integ/crypto/megolm-backup.spec.ts +++ b/spec/integ/crypto/megolm-backup.spec.ts @@ -929,22 +929,26 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe // First ensure that the client checks for keys using the backup version 1 /// ===== - fetchMock.get("express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", (url, request) => { - // check that the version is correct - const version = new URLSearchParams(new URL(url).search).get("version"); - if (version == "1") { - return testData.CURVE25519_KEY_BACKUP_DATA; - } else { - return { - status: 403, - body: { - current_version: "1", - errcode: "M_WRONG_ROOM_KEYS_VERSION", - error: "Wrong backup version.", - }, - }; - } - },{ overwriteRoutes: true }); + fetchMock.get( + "express:/_matrix/client/v3/room_keys/keys/:room_id/:session_id", + (url, request) => { + // check that the version is correct + const version = new URLSearchParams(new URL(url).search).get("version"); + if (version == "1") { + return testData.CURVE25519_KEY_BACKUP_DATA; + } else { + return { + status: 403, + body: { + current_version: "1", + errcode: "M_WRONG_ROOM_KEYS_VERSION", + error: "Wrong backup version.", + }, + }; + } + }, + { overwriteRoutes: true }, + ); // Send Alice a message that she won't be able to decrypt, and check that she fetches the key from the backup. syncResponder.sendOrQueueSyncResponse(SYNC_RESPONSE); @@ -956,7 +960,6 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe expect(event.getContent()).toEqual(testData.CLEAR_EVENT.content); - // ===== // Second suppose now that the backup has changed to version 2 /// ===== diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 0a92845d3ba..07b049cbf04 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -27,19 +27,40 @@ import { encodeUri, sleep } from "../utils"; import { RustCrypto } from "./rust-crypto"; /** - * Extract the dependices of the OnDemandKeyBackupDownloader, main reason is to make testing easier. + * Extract the dependence of the PerSessionKeyBackupDownloader, main reason is to make testing easier. */ export interface OnDemandBackupDelegate { + /** Gets the current trusted backup if any.*/ getActiveBackupVersion(): Promise; + /** Gets the current cached backup decryption key if any.*/ getBackupDecryptionKey(): Promise; - requestRoomKeyFromBackup(version: string, rooomId: string, sessionId: string): Promise; + /** + * Performs the server request to fetch the key from backup. + * + * @param version - The backup version to use. + * @param roomId - The room id of the session. + * @param sessionId - The session id of the session. + * + */ + requestRoomKeyFromBackup(version: string, roomId: string, sessionId: string): Promise; + /** + * Imports the given keys into the crypto store. + * @param keys - The keys to import. + */ importRoomKeys(keys: IMegolmSessionData[]): Promise; + /** + * Creates a backup decryptor that can decrypt the key retrieved from backup. + * @param key - The cached backup decryption key. + */ createBackupDecryptor(key: RustSdkCryptoJs.BackupDecryptionKey): RustBackupDecryptor; + /** + * Requests the current key backup version from the server. + */ requestKeyBackupVersion(): Promise; /** @@ -104,27 +125,45 @@ export function createDelegate( }; } +/** + * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. + */ export enum KeyDownloadError { + /** The backup version in use is out of sync with the server version. */ VERSION_MISMATCH = "VERSION_MISMATCH", + /** The requested key is not in the backup. */ MISSING_DECRYPTION_KEY = "MISSING_DECRYPTION_KEY", + /** A network error occurred while trying to get the key. */ NETWORK_ERROR = "NETWORK_ERROR", + /** The loop as been stopped. */ STOPPED = "STOPPED", + /** An unknown error occurred while decrypting/importing the key */ UNKNOWN_ERROR = "UNKNOWN_ERROR", + /** The server is rate limiting us. */ RATE_LIMITED = "RATE_LIMITED", + /** The backup is not configured correctly, can be that there is no backup, that it's not trusted + * , that we don't have the correct key in cache... */ CONFIGURATION_ERROR = "CONFIGURATION_ERROR", } +/** Helper type for requested session*/ type SessionInfo = { roomId: string; megolmSessionId: string }; +/** Helper type for the result of a key download. */ type KeyDownloadResult = | { ok: true; value: KeyBackupSession } | { ok: false; error: KeyDownloadError; [key: string]: any }; +/** Holds the current backup decryptor and version that should be used. */ type Configuration = { backupVersion: string; decryptor: RustBackupDecryptor; }; +/** + * Signaling for the Downloader loop. + * Not yet used by API, yet useful for testing. + */ export enum KeyDownloaderEvent { DownloadLoopStateUpdate = "download_loop_started", DownLoopStep = "download_loop_step", @@ -142,6 +181,11 @@ export type KeyDownloaderEventMap = { * When an unable to decrypt error is encountered, the client will call this * in order to try to download the key from the backup. * + * The current backup API is quite limited, there is no pagination, so it can take very long to + * get all keys for history. Downloading keys on demand is a way to avoid this problem. + * It will create a lot of requests, but form the user point of view, it's better to have that than + * waiting for a long time before being able to decrypt a message. + * */ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { private stopped = false; @@ -381,6 +425,9 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter Date: Fri, 1 Dec 2023 09:07:05 +0100 Subject: [PATCH 04/24] fix test, quick refactor on request version --- .../PerSessionKeyBackupDownloader.spec.ts | 9 +- .../PerSessionKeyBackupDownloader.ts | 70 +++++++++----- src/rust-crypto/rust-crypto.ts | 91 ------------------- 3 files changed, 53 insertions(+), 117 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 7daffde7c51..f9d655ee94a 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -414,18 +414,17 @@ describe("PerSessionKeyBackupDownloader", () => { // The new backup is detected, the loop should resume but the cached key is still the old one - const loopResumed = expectLoopStatus(false); - const cacheMismatch = expectConfigurationError(KeyDownloadError.VERSION_MISMATCH); + const configurationError = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue({ version: "2", ...TestData.SIGNED_BACKUP_DATA }); + delegate.requestKeyBackupVersion.mockResolvedValue({ ...TestData.SIGNED_BACKUP_DATA, version: "2" }); // It's trusted delegate.getActiveBackupVersion.mockResolvedValue("2"); mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - await loopResumed; - await cacheMismatch; + // await loopResumed; + await configurationError; // Now the new key is cached delegate.getBackupDecryptionKey.mockResolvedValue({ diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 07b049cbf04..5f395303a34 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -195,46 +195,46 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter = {}; + private readonly configurationChangeHandler = () => { + this.onBackupStatusChanged(); + }; + + private readonly logger: Logger; + public constructor( private readonly delegate: OnDemandBackupDelegate, - private readonly logger: Logger, + logger: Logger, private readonly maxTimeBetweenRetry: number, ) { super(); + this.logger = logger.getChild("[PerSessionKeyBackupDownloader]"); const emitter = this.delegate.getCryptoEventEmitter(); - emitter.on(CryptoEvent.KeyBackupStatus, (ev) => { - this.logger.info(`Key backup status changed, check configuration`); - // we want to check configuration - this.onBackupStatusChanged(); - }); - - emitter.on(CryptoEvent.KeyBackupFailed, (ev) => { - this.logger.info(`Key backup upload failed, check configuration`); - // we want to check configuration - this.onBackupStatusChanged(); - }); - - /// TODO When the PR that adds signaling when the decryption is merged, we can use it to trigger a refresh - // emitter.on(CryptoEvent.KeyBackupPrivateKeyCached, (ev) => { - // this.logger.info(`Key backup decryption key is known, check configuration`); - // // we want to check configuration - // this.onBackupStatusChanged(); - // }); + emitter.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + emitter.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + emitter.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } public stop(): void { this.stopped = true; + this.delegate.getCryptoEventEmitter().off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + this.delegate.getCryptoEventEmitter().off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + this.delegate + .getCryptoEventEmitter() + .off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } private onBackupStatusChanged(): void { + this.logger.info(`Key backup status change => check configuration`); // we want to check configuration this.hasConfigurationProblem = false; this.configuration = null; this.getOrCreateBackupDecryptor(true).then((decryptor) => { if (decryptor) { this.downloadKeysLoop(); + } else { + this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.CONFIGURATION_ERROR); } }); } @@ -459,6 +459,8 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter | null = null; + private async getOrCreateBackupDecryptor(forceCheck: boolean): Promise { if (this.configuration) { return this.configuration; @@ -468,7 +470,31 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { + let currentServerVersion = null; + try { + currentServerVersion = await this.delegate.requestKeyBackupVersion(); + } catch (e) { + this.logger.debug(`Backup: error while checking server version: ${e}`); + this.hasConfigurationProblem = true; + return null; + } + this.logger.debug(`Got current version from server:${currentServerVersion?.version}`); if (currentServerVersion?.algorithm != "m.megolm_backup.v1.curve25519-aes-sha2") { this.logger.info(`getBackupDecryptor Unsupported algorithm ${currentServerVersion?.algorithm}`); @@ -502,7 +528,9 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter doesn't match active backup version <${activeVersion}>`, + ); this.hasConfigurationProblem = true; return null; } diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index db26bfd450f..f1f44fa876c 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -168,97 +168,6 @@ export class RustCrypto extends TypedEventEmitter KEY_BACKUP_CHECK_RATE_LIMIT) { - // this.sessionLastCheckAttemptedTime[targetSessionId!] = now; - // this.queryKeyBackup(targetRoomId, targetSessionId).catch((e) => { - // this.logger.error(`Unhandled error while checking key backup for session ${targetSessionId}`, e); - // }); - // } else { - // const lastCheckStr = new Date(lastCheck).toISOString(); - // this.logger.debug( - // `Not checking key backup for session ${targetSessionId} (last checked at ${lastCheckStr})`, - // ); - // } - // } - - // /** - // * Helper for {@link RustCrypto#startQueryKeyBackupRateLimited}. - // * - // * Requests the backup and imports it. Doesn't do any rate-limiting. - // * - // * @param targetRoomId - ID of the room that the session is used in. - // * @param targetSessionId - ID of the session for which to check backup. - // */ - // private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { - // const backupKeys: RustSdkCryptoJs.BackupKeys = await this.olmMachine.getBackupKeys(); - // if (!backupKeys.decryptionKey) { - // this.logger.debug(`Not checking key backup for session ${targetSessionId} (no decryption key)`); - // return; - // } - // - // const activeBackup = await this.backupManager.getActiveBackupVersion(); - // if (!activeBackup) { - // this.logger.warn(`No active key backup, or current backup not trusted aborting key backup download`); - // return; - // } - // - // if(activeBackup != backupKeys.backupVersion) { - // this.logger.warn(`Cached backup version ${backupKeys.backupVersion} does not match active backup version ${activeBackup}`); - // // The cached decryption key is out of sync, the user need to resync with 4S or request missing secrets to other sessions. - // return; - // } - // - // this.logger.debug(`Checking key backup for session ${targetSessionId}`); - // - // const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { - // $roomId: targetRoomId, - // $sessionId: targetSessionId, - // }); - // - // let res: KeyBackupSession; - // try { - // res = await this.http.authedRequest(Method.Get, path, { activeBackup }, undefined, { - // prefix: ClientPrefix.V3, - // }); - // } catch (e) { - // this.logger.info(`No luck requesting key backup for session ${targetSessionId}: ${e}`); - // if (e instanceof MatrixError) { - // const errCode = e.data.errcode; - // if (errCode == "M_NOT_FOUND" || errCode == "M_WRONG_ROOM_KEYS_VERSION") { - // // The active backup version is out of sync, force a check to sync with server. - // try { - // this.backupManager.checkKeyBackupAndEnable(true); - // } catch(e) { - // this.logger.error(`Error while checking key backup: ${e}`); - // } - // } - // } - // return; - // } - // - // if (this.stopped) return; - // - // const backupDecryptor = new RustBackupDecryptor(backupKeys.decryptionKey); - // const sessionsToImport: Record = { [targetSessionId]: res }; - // const keys = await backupDecryptor.decryptSessions(sessionsToImport); - // for (const k of keys) { - // k.room_id = targetRoomId; - // } - // await this.importRoomKeys(keys); - // } /** * Return the OlmMachine only if {@link RustCrypto#stop} has not been called. From c58f3d1ea8409f36b70ea323cbc46523df549c8e Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 1 Dec 2023 10:34:49 +0100 Subject: [PATCH 05/24] cleaning and logs --- .../PerSessionKeyBackupDownloader.ts | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 5f395303a34..d15738406b5 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -195,7 +195,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter = {}; - private readonly configurationChangeHandler = () => { + private readonly configurationChangeHandler = (): void => { this.onBackupStatusChanged(); }; @@ -402,16 +402,20 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter Checking key backup for session ${targetSessionId}`); try { - res = await this.delegate.requestRoomKeyFromBackup( + const res = await this.delegate.requestRoomKeyFromBackup( configuration.backupVersion, targetRoomId, targetSessionId, ); + if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; + this.logger.debug(`<-- Got key from backup ${targetSessionId}`); + return { + ok: true, + value: res, + }; } catch (e) { if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; @@ -450,13 +454,6 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter | null = null; From f091251c978809d5c6d19bb529b79ea93b5db007 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 1 Dec 2023 12:00:14 +0100 Subject: [PATCH 06/24] fix type --- spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index f9d655ee94a..ab2c2809e86 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -162,6 +162,8 @@ describe("PerSessionKeyBackupDownloader", () => { ); } else if (session == "sessionA1") { return TestData.CURVE25519_KEY_BACKUP_DATA; + } else { + throw new Error("Unexpected session"); } }); From f1235c8527a7eb9ddcdb4a1babfcd53d92ecb7b0 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 1 Dec 2023 12:12:21 +0100 Subject: [PATCH 07/24] cleaning --- .../PerSessionKeyBackupDownloader.ts | 33 ++++++++++++------- src/rust-crypto/rust-crypto.ts | 7 ++-- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index d15738406b5..c93e4d5ce23 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -71,7 +71,8 @@ export interface OnDemandBackupDelegate { } /** - * Utility to create a delegate for the OnDemandKeyBackupDownloader that is usable by rust crypto. + * Utility to create a delegate for the PerSessionKeyBackupDownloader that is usable by rust crypto. + * * @param rustCrypto - The rust crypto instance. * @param backupManager - The backup manager instance. * @param olmMachine - The olm machine instance. @@ -178,13 +179,14 @@ export type KeyDownloaderEventMap = { }; /** - * When an unable to decrypt error is encountered, the client will call this - * in order to try to download the key from the backup. + * This function is called when an 'unable to decrypt' error occurs. It attempts to download the key from the backup. * - * The current backup API is quite limited, there is no pagination, so it can take very long to - * get all keys for history. Downloading keys on demand is a way to avoid this problem. - * It will create a lot of requests, but form the user point of view, it's better to have that than - * waiting for a long time before being able to decrypt a message. + * The current backup API lacks pagination, which can lead to lengthy key retrieval times for large histories (several 10s of minutes). + * To mitigate this, keys are downloaded on demand as decryption errors occurs. + * While this approach may result in numerous requests, it improves user experience by reducing wait times for message decryption. + * + * The PerSessionKeyBackupDownloader is resistant to backup configuration changes, it will automatically resume querying when + * the backup is configured correctly. * */ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { @@ -201,6 +203,14 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter = {}; // When did we last try to check the server for a given session id? - private readonly reemitter = new TypedReEmitter(this); public constructor( @@ -151,7 +149,7 @@ export class RustCrypto extends TypedEventEmitter Date: Tue, 5 Dec 2023 14:12:54 +0100 Subject: [PATCH 08/24] remove delegate stuff --- .../PerSessionKeyBackupDownloader.spec.ts | 354 +++++++++++------- .../PerSessionKeyBackupDownloader.ts | 174 +++------ src/rust-crypto/backup.ts | 22 ++ src/rust-crypto/rust-crypto.ts | 17 +- 4 files changed, 284 insertions(+), 283 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index ab2c2809e86..ba2631705db 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -16,29 +16,48 @@ limitations under the License. import { Mocked } from "jest-mock"; import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; +import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; +import fetchMock from "fetch-mock-jest"; import { KeyDownloaderEvent, KeyDownloadError, - OnDemandBackupDelegate, PerSessionKeyBackupDownloader, } from "../../../src/rust-crypto/PerSessionKeyBackupDownloader"; import { logger } from "../../../src/logger"; import { defer } from "../../../src/utils"; -import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupDecryptor } from "../../../src/rust-crypto/backup"; +import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupManager } from "../../../src/rust-crypto/backup"; import * as TestData from "../../test-utils/test-data"; -import { ConnectionError, CryptoEvent, MatrixError, TypedEventEmitter } from "../../../src"; +import { + ConnectionError, + CryptoEvent, + HttpApiEvent, + HttpApiEventHandlerMap, + IHttpOpts, + IMegolmSessionData, + MatrixHttpApi, + TypedEventEmitter, +} from "../../../src"; +import * as testData from "../../test-utils/test-data"; +import { BackupDecryptor } from "../../../src/common-crypto/CryptoBackend"; +import { KeyBackupSession } from "../../../src/crypto-api/keybackup"; describe("PerSessionKeyBackupDownloader", () => { /** The downloader under test */ let downloader: PerSessionKeyBackupDownloader; - let delegate: Mocked; + const mockCipherKey: Mocked = {} as unknown as Mocked; + // let delegate: Mocked; const BACKOFF_TIME = 2000; const mockEmitter = new TypedEventEmitter() as TypedEventEmitter; + let mockHttp: MatrixHttpApi; + let mockRustBackupManager: Mocked; + let mockOlmMachine: Mocked; + let mockBackupDecryptor: Mocked; + async function expectConfigurationError(error: KeyDownloadError): Promise { return new Promise((resolve) => { downloader.on(KeyDownloaderEvent.QueryKeyError, (err) => { @@ -68,104 +87,133 @@ describe("PerSessionKeyBackupDownloader", () => { }); } + function mockClearSession(sessionId: string): Mocked { + return { + session_id: sessionId, + } as unknown as Mocked; + } + beforeEach(async () => { - delegate = { + mockHttp = new MatrixHttpApi(new TypedEventEmitter(), { + baseUrl: "http://server/", + prefix: "", + onlyData: true, + }); + + mockBackupDecryptor = { + decryptSessions: jest.fn(), + } as unknown as Mocked; + + mockBackupDecryptor.decryptSessions.mockImplementation(async (ciphertexts) => { + const sessionId = Object.keys(ciphertexts)[0]; + return [mockClearSession(sessionId)]; + }); + + mockRustBackupManager = { getActiveBackupVersion: jest.fn(), getBackupDecryptionKey: jest.fn(), - requestRoomKeyFromBackup: jest.fn(), - importRoomKeys: jest.fn(), - createBackupDecryptor: jest.fn(), requestKeyBackupVersion: jest.fn(), - getCryptoEventEmitter: jest.fn(), - } as unknown as Mocked; - - delegate.getCryptoEventEmitter.mockReturnValue(mockEmitter); - - downloader = new PerSessionKeyBackupDownloader(delegate, logger, BACKOFF_TIME); + importRoomKeys: jest.fn(), + createBackupDecryptor: jest.fn().mockReturnValue(mockBackupDecryptor), + } as unknown as Mocked; + + mockOlmMachine = { + getBackupKeys: jest.fn(), + } as unknown as Mocked; + + downloader = new PerSessionKeyBackupDownloader( + mockRustBackupManager, + mockOlmMachine, + mockHttp, + mockEmitter, + logger, + BACKOFF_TIME, + ); jest.useFakeTimers(); }); afterEach(() => { downloader.stop(); + fetchMock.mockReset(); jest.useRealTimers(); }); describe("Given valid backup available", () => { beforeEach(async () => { - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: TestData.SIGNED_BACKUP_DATA.version!, decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - delegate.createBackupDecryptor.mockReturnValue({ - decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), - } as unknown as RustBackupDecryptor); - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + + mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); }); it("Should download and import a missing key from backup", async () => { const awaitKeyImported = defer(); - - delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); - delegate.importRoomKeys.mockImplementation(async (keys) => { + const roomId = "!roomId"; + const sessionId = "sessionId"; + const expectAPICall = new Promise((resolve) => { + fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/${roomId}/${sessionId}`, (url, request) => { + resolve(); + return TestData.CURVE25519_KEY_BACKUP_DATA; + }); + }); + mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { awaitKeyImported.resolve(); }); + mockBackupDecryptor.decryptSessions.mockResolvedValue([TestData.MEGOLM_SESSION_DATA]); - downloader.onDecryptionKeyMissingError("roomId", "sessionId"); + downloader.onDecryptionKeyMissingError(roomId, sessionId); + await expectAPICall; await awaitKeyImported.promise; - - expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledWith("1", "roomId", "sessionId"); - expect(delegate.createBackupDecryptor).toHaveBeenCalledTimes(1); + expect(mockRustBackupManager.createBackupDecryptor).toHaveBeenCalledTimes(1); }); it("Should not hammer the backup if the key is requested repeatedly", async () => { const blockOnServerRequest = defer(); - // simulate a key not being in the backup - delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { + + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/!roomId/:session_id`, async (url, request) => { await blockOnServerRequest.promise; - return TestData.CURVE25519_KEY_BACKUP_DATA; + return [mockCipherKey]; + }); + + const awaitKey2Imported = defer(); + + mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { + if (keys[0].session_id === "sessionId2") { + awaitKey2Imported.resolve(); + } }); - // Call 3 times + const spy = jest.spyOn(downloader, "queryKeyBackup"); + + // Call 3 times for same key downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); + // Call again for a different key downloader.onDecryptionKeyMissingError("!roomId", "sessionId2"); - const session2Imported = new Promise((resolve) => { - downloader.on(KeyDownloaderEvent.KeyImported, (roomId, sessionId) => { - if (sessionId === "sessionId2") { - resolve(); - } - }); - }); + // Allow the first server request to complete blockOnServerRequest.resolve(); - await session2Imported; - expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(2); + await awaitKey2Imported.promise; + expect(spy).toHaveBeenCalledTimes(2); }); it("should continue to next key if current not in backup", async () => { - delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); - - delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { - if (session == "sessionA0") { - throw new MatrixError( - { - errcode: "M_NOT_FOUND", - error: "No room_keys found", - }, - 404, - ); - } else if (session == "sessionA1") { - return TestData.CURVE25519_KEY_BACKUP_DATA; - } else { - throw new Error("Unexpected session"); - } + fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA0`, { + status: 404, + body: { + errcode: "M_NOT_FOUND", + error: "No backup found", + }, }); + fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA1`, mockCipherKey); const expectImported = expectSessionImported("!roomA", "sessionA1"); const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); @@ -178,17 +226,15 @@ describe("PerSessionKeyBackupDownloader", () => { }); it("Should not query repeatedly for a key not in backup", async () => { - delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA0`, { + status: 404, + body: { + errcode: "M_NOT_FOUND", + error: "No backup found", + }, + }); - delegate.requestRoomKeyFromBackup.mockRejectedValue( - new MatrixError( - { - errcode: "M_NOT_FOUND", - error: "No room_keys found", - }, - 404, - ), - ); + const spy = jest.spyOn(downloader, "queryKeyBackup"); const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); @@ -196,18 +242,19 @@ describe("PerSessionKeyBackupDownloader", () => { await expectNotFound; - const currentCallCount = delegate.requestRoomKeyFromBackup.mock.calls.length; - // Should not query again for a key not in backup - downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); - expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(currentCallCount); + + expect(spy).toHaveBeenCalledTimes(1); // advance time to retry jest.advanceTimersByTime(BACKOFF_TIME + 10); const expectNotFoundSecondAttempt = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + + expect(spy).toHaveBeenCalledTimes(2); + await expectNotFoundSecondAttempt; }); @@ -216,11 +263,13 @@ describe("PerSessionKeyBackupDownloader", () => { const blockOnServerRequest = defer(); const requestRoomKeyCalled = defer(); + let callCount = 0; // Mock the request to block - delegate.requestRoomKeyFromBackup.mockImplementation(async (version, room, session) => { + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, async (url, request) => { requestRoomKeyCalled.resolve(); await blockOnServerRequest.promise; - return TestData.CURVE25519_KEY_BACKUP_DATA; + callCount++; + return mockCipherKey; }); const expectStopped = expectConfigurationError(KeyDownloadError.STOPPED); @@ -237,8 +286,8 @@ describe("PerSessionKeyBackupDownloader", () => { blockOnServerRequest.resolve(); await expectStopped; - expect(delegate.importRoomKeys).not.toHaveBeenCalled(); - expect(delegate.requestRoomKeyFromBackup).toHaveBeenCalledTimes(1); + expect(mockRustBackupManager.importRoomKeys).not.toHaveBeenCalled(); + expect(callCount).toStrictEqual(1); }); }); @@ -247,16 +296,23 @@ describe("PerSessionKeyBackupDownloader", () => { let configurationErrorPromise: Promise; beforeEach(async () => { - delegate.getActiveBackupVersion.mockResolvedValue(null); - delegate.getBackupDecryptionKey.mockResolvedValue(null); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); + mockOlmMachine.getBackupKeys.mockResolvedValue(null); loopPausedPromise = expectLoopStatus(false); configurationErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); }); + afterEach(async () => { + fetchMock.mockClear(); + }); + it("Should not query server if no backup", async () => { - delegate.requestKeyBackupVersion.mockResolvedValue(null); + fetchMock.get("path:/_matrix/client/v3/room_keys/version", { + status: 404, + body: { errcode: "M_NOT_FOUND", error: "No current backup version." }, + }); downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); @@ -266,9 +322,10 @@ describe("PerSessionKeyBackupDownloader", () => { it("Should not query server if backup not active", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); - // but it's not active - delegate.getActiveBackupVersion.mockResolvedValue(null); + fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + + // but it's not trusted + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); @@ -278,11 +335,11 @@ describe("PerSessionKeyBackupDownloader", () => { it("Should stop if backup key is not cached", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); - // it is active - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + // it is trusted + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); // but the key is not cached - delegate.getBackupDecryptionKey.mockResolvedValue(null); + mockOlmMachine.getBackupKeys.mockResolvedValue(null); downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); @@ -292,11 +349,11 @@ describe("PerSessionKeyBackupDownloader", () => { it("Should stop if backup key cached as wrong version", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); - // it is active - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + // it is trusted + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); // but the cached key has the wrong version - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: "0", decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); @@ -309,11 +366,11 @@ describe("PerSessionKeyBackupDownloader", () => { it("Should stop if backup key version does not match the active one", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); - // it is active - delegate.getActiveBackupVersion.mockResolvedValue("0"); + fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); + // The sdk is out of sync, the trusted version is the old one + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue("0"); // key for old backup cached - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: "0", decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); @@ -328,11 +385,13 @@ describe("PerSessionKeyBackupDownloader", () => { describe("Given Backup state update", () => { it("After initial sync, when backup become trusted it should request keys for past requests", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); // but at this point it's not trusted and we don't have the key - delegate.getActiveBackupVersion.mockResolvedValue(null); - delegate.getBackupDecryptionKey.mockResolvedValue(null); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); + mockOlmMachine.getBackupKeys.mockResolvedValue(null); + + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey); const configErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); @@ -350,17 +409,13 @@ describe("PerSessionKeyBackupDownloader", () => { await configErrorPromise; // Now the backup becomes trusted - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); // And we have the key in cache - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: TestData.SIGNED_BACKUP_DATA.version!, decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - delegate.createBackupDecryptor.mockReturnValue({ - decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), - } as unknown as RustBackupDecryptor); - const loopShouldResume = expectLoopStatus(true); // In that case the sdk would fire a backup status update mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); @@ -375,18 +430,18 @@ describe("PerSessionKeyBackupDownloader", () => { it("If reset from other session, loop should stop until new decryption key is known", async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); // It's trusted - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); // And we have the key in cache - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: TestData.SIGNED_BACKUP_DATA.version!, decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - delegate.createBackupDecryptor.mockReturnValue({ - decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), - } as unknown as RustBackupDecryptor); + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { + overwriteRoutes: true, + }); const a0Imported = expectSessionImported("!roomA", "sessionA0"); @@ -396,14 +451,16 @@ describe("PerSessionKeyBackupDownloader", () => { // Now some other session resets the backup and there is a new version // the room_keys/keys endpoint will throw - delegate.requestRoomKeyFromBackup.mockRejectedValue( - new MatrixError( - { + fetchMock.get( + `express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, + { + status: 404, + body: { errcode: "M_NOT_FOUND", error: "Unknown backup version", }, - 404, - ), + }, + { overwriteRoutes: true }, ); const loopPausedPromise = expectLoopStatus(false); @@ -415,13 +472,15 @@ describe("PerSessionKeyBackupDownloader", () => { await expectMismatch; // The new backup is detected, the loop should resume but the cached key is still the old one - const configurationError = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue({ ...TestData.SIGNED_BACKUP_DATA, version: "2" }); + mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue({ + ...TestData.SIGNED_BACKUP_DATA, + version: "2", + }); // It's trusted - delegate.getActiveBackupVersion.mockResolvedValue("2"); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue("2"); mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); @@ -429,12 +488,14 @@ describe("PerSessionKeyBackupDownloader", () => { await configurationError; // Now the new key is cached - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: "2", decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - delegate.requestRoomKeyFromBackup.mockResolvedValue(TestData.CURVE25519_KEY_BACKUP_DATA); + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { + overwriteRoutes: true, + }); const a1Imported = expectSessionImported("!roomA", "sessionA1"); @@ -447,19 +508,15 @@ describe("PerSessionKeyBackupDownloader", () => { describe("Error cases", () => { beforeEach(async () => { // there is a backup - delegate.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); + mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); // It's trusted - delegate.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); + mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); // And we have the key in cache - delegate.getBackupDecryptionKey.mockResolvedValue({ + mockOlmMachine.getBackupKeys.mockResolvedValue({ backupVersion: TestData.SIGNED_BACKUP_DATA.version!, decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - delegate.createBackupDecryptor.mockReturnValue({ - decryptSessions: jest.fn().mockResolvedValue([TestData.MEGOLM_SESSION_DATA]), - } as unknown as RustBackupDecryptor); - jest.useFakeTimers(); }); @@ -469,39 +526,44 @@ describe("PerSessionKeyBackupDownloader", () => { it("Should wait on rate limit error", async () => { // simulate rate limit error - delegate.requestRoomKeyFromBackup - .mockImplementationOnce(async () => { - throw new MatrixError( - { - errcode: "M_LIMIT_EXCEEDED", - error: "Too many requests", - retry_after_ms: 5000, - }, - 429, - ); - }) - .mockImplementationOnce(async () => TestData.CURVE25519_KEY_BACKUP_DATA); + fetchMock.get( + `express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, + { + status: 429, + body: { + errcode: "M_LIMIT_EXCEEDED", + error: "Too many requests", + retry_after_ms: 5000, + }, + }, + { overwriteRoutes: true }, + ); const errorPromise = expectConfigurationError(KeyDownloadError.RATE_LIMITED); const keyImported = expectSessionImported("!roomA", "sessionA0"); + const spy = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await errorPromise; + + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { + overwriteRoutes: true, + }); + // The loop should resume after the retry_after_ms jest.advanceTimersByTime(5000 + 100); await jest.runAllTimersAsync(); + expect(spy).toHaveBeenCalledTimes(2); await keyImported; }); it("After a network error the same key is retried", async () => { // simulate connectivity error - delegate.requestRoomKeyFromBackup - .mockImplementationOnce(async () => { - throw new ConnectionError("fetch failed", new Error("fetch failed")); - }) - .mockImplementationOnce(async () => TestData.CURVE25519_KEY_BACKUP_DATA); + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, () => { + throw new ConnectionError("fetch failed", new Error("fetch failed")); + }); const errorPromise = expectConfigurationError(KeyDownloadError.NETWORK_ERROR); const keyImported = expectSessionImported("!roomA", "sessionA0"); @@ -509,6 +571,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await errorPromise; + + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { + overwriteRoutes: true, + }); // The loop should resume after the retry_after_ms jest.advanceTimersByTime(BACKOFF_TIME + 100); await jest.runAllTimersAsync(); @@ -517,7 +583,7 @@ describe("PerSessionKeyBackupDownloader", () => { }); it("On Unknown error on import skip the key and continue", async () => { - delegate.importRoomKeys + mockRustBackupManager.importRoomKeys .mockImplementationOnce(async () => { throw new Error("Didn't work"); }) @@ -525,6 +591,10 @@ describe("PerSessionKeyBackupDownloader", () => { return; }); + fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { + overwriteRoutes: true, + }); + const errorPromise = expectConfigurationError(KeyDownloadError.UNKNOWN_ERROR); const keyImported = expectSessionImported("!roomA", "sessionA1"); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index c93e4d5ce23..664c47516a9 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -17,114 +17,12 @@ limitations under the License. import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; -import { Curve25519AuthData, KeyBackupInfo, KeyBackupSession } from "../crypto-api/keybackup"; -import { IMegolmSessionData } from "../crypto"; +import { Curve25519AuthData, KeyBackupSession } from "../crypto-api/keybackup"; import { Logger } from "../logger"; import { ClientPrefix, IHttpOpts, MatrixError, MatrixHttpApi, Method } from "../http-api"; import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupDecryptor, RustBackupManager } from "./backup"; import { CryptoEvent, TypedEventEmitter } from "../matrix"; import { encodeUri, sleep } from "../utils"; -import { RustCrypto } from "./rust-crypto"; - -/** - * Extract the dependence of the PerSessionKeyBackupDownloader, main reason is to make testing easier. - */ -export interface OnDemandBackupDelegate { - /** Gets the current trusted backup if any.*/ - getActiveBackupVersion(): Promise; - - /** Gets the current cached backup decryption key if any.*/ - getBackupDecryptionKey(): Promise; - - /** - * Performs the server request to fetch the key from backup. - * - * @param version - The backup version to use. - * @param roomId - The room id of the session. - * @param sessionId - The session id of the session. - * - */ - requestRoomKeyFromBackup(version: string, roomId: string, sessionId: string): Promise; - - /** - * Imports the given keys into the crypto store. - * @param keys - The keys to import. - */ - importRoomKeys(keys: IMegolmSessionData[]): Promise; - - /** - * Creates a backup decryptor that can decrypt the key retrieved from backup. - * @param key - The cached backup decryption key. - */ - createBackupDecryptor(key: RustSdkCryptoJs.BackupDecryptionKey): RustBackupDecryptor; - - /** - * Requests the current key backup version from the server. - */ - requestKeyBackupVersion(): Promise; - - /** - * The backup downloader will listen to these events to know when to check for backup status changes in order to - * resume or stop querying. - */ - getCryptoEventEmitter(): TypedEventEmitter; -} - -/** - * Utility to create a delegate for the PerSessionKeyBackupDownloader that is usable by rust crypto. - * - * @param rustCrypto - The rust crypto instance. - * @param backupManager - The backup manager instance. - * @param olmMachine - The olm machine instance. - * @param http - The http instance. - */ -export function createDelegate( - rustCrypto: RustCrypto, - backupManager: RustBackupManager, - olmMachine: OlmMachine, - http: MatrixHttpApi, -): OnDemandBackupDelegate { - return { - getActiveBackupVersion(): Promise { - return backupManager.getActiveBackupVersion(); - }, - - async getBackupDecryptionKey(): Promise { - try { - return await olmMachine.getBackupKeys(); - } catch (e) { - return null; - } - }, - - async requestRoomKeyFromBackup(version: string, roomId: string, sessionId: string): Promise { - const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { - $roomId: roomId, - $sessionId: sessionId, - }); - - return await http.authedRequest(Method.Get, path, { version }, undefined, { - prefix: ClientPrefix.V3, - }); - }, - - async importRoomKeys(keys: IMegolmSessionData[]): Promise { - return rustCrypto.importRoomKeys(keys); - }, - - createBackupDecryptor: (key: RustSdkCryptoJs.BackupDecryptionKey): RustBackupDecryptor => { - return new RustBackupDecryptor(key); - }, - - async requestKeyBackupVersion(): Promise { - return await backupManager.requestKeyBackupVersion(); - }, - - getCryptoEventEmitter(): TypedEventEmitter { - return rustCrypto; - }, - }; -} /** * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. @@ -184,7 +82,7 @@ export type KeyDownloaderEventMap = { * The current backup API lacks pagination, which can lead to lengthy key retrieval times for large histories (several 10s of minutes). * To mitigate this, keys are downloaded on demand as decryption errors occurs. * While this approach may result in numerous requests, it improves user experience by reducing wait times for message decryption. - * + * * The PerSessionKeyBackupDownloader is resistant to backup configuration changes, it will automatically resume querying when * the backup is configured correctly. * @@ -205,34 +103,37 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter, + private readonly cryptoEventEmitter: TypedEventEmitter, logger: Logger, private readonly maxTimeBetweenRetry: number, ) { super(); this.logger = logger.getChild("[PerSessionKeyBackupDownloader]"); - const emitter = this.delegate.getCryptoEventEmitter(); - emitter.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - emitter.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - emitter.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + cryptoEventEmitter.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + cryptoEventEmitter.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + cryptoEventEmitter.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } public stop(): void { this.stopped = true; - this.delegate.getCryptoEventEmitter().off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - this.delegate.getCryptoEventEmitter().off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - this.delegate - .getCryptoEventEmitter() - .off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + this.cryptoEventEmitter.off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + this.cryptoEventEmitter.off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + this.cryptoEventEmitter.off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } private onBackupStatusChanged(): void { @@ -287,6 +188,29 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { + try { + return await this.olmMachine.getBackupKeys(); + } catch (e) { + return null; + } + } + + private async requestRoomKeyFromBackup( + version: string, + roomId: string, + sessionId: string, + ): Promise { + const path = encodeUri("/room_keys/keys/$roomId/$sessionId", { + $roomId: roomId, + $sessionId: sessionId, + }); + + return await this.http.authedRequest(Method.Get, path, { version }, undefined, { + prefix: ClientPrefix.V3, + }); + } + /** * Called when a MissingRoomKey or UnknownMessageIndex decryption error is encountered. * @@ -316,7 +240,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter Checking key backup for session ${targetSessionId}`); try { - const res = await this.delegate.requestRoomKeyFromBackup( - configuration.backupVersion, - targetRoomId, - targetSessionId, - ); + const res = await this.requestRoomKeyFromBackup(configuration.backupVersion, targetRoomId, targetSessionId); if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; this.logger.debug(`<-- Got key from backup ${targetSessionId}`); return { @@ -496,7 +416,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { let currentServerVersion = null; try { - currentServerVersion = await this.delegate.requestKeyBackupVersion(); + currentServerVersion = await this.backupManager.requestKeyBackupVersion(); } catch (e) { this.logger.debug(`Backup: error while checking server version: ${e}`); this.hasConfigurationProblem = true; @@ -516,7 +436,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmittercurrentServerVersion.auth_data; - const backupKeys = await this.delegate.getBackupDecryptionKey(); + const backupKeys = await this.getBackupDecryptionKey(); if (!backupKeys?.decryptionKey) { this.logger.debug(`Not checking key backup for session(no decryption key)`); this.hasConfigurationProblem = true; @@ -549,7 +469,7 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { + // TODO when backup support will be added we would need to expose the `from_backup` flag in the bindings + const jsonKeys = JSON.stringify(keys); + await this.olmMachine.importRoomKeys(jsonKeys, (progress: BigInt, total: BigInt) => { + const importOpt: ImportRoomKeyProgressData = { + total: Number(total), + successes: Number(progress), + stage: "load_keys", + failures: 0, + }; + opts?.progressCallback?.(importOpt); + }); + } private keyBackupCheckInProgress: Promise | null = null; @@ -440,6 +454,14 @@ export class RustBackupManager extends TypedEventEmitter { - // TODO when backup support will be added we would need to expose the `from_backup` flag in the bindings - const jsonKeys = JSON.stringify(keys); - await this.olmMachine.importRoomKeys(jsonKeys, (progress: BigInt, total: BigInt) => { - const importOpt: ImportRoomKeyProgressData = { - total: Number(total), - successes: Number(progress), - stage: "load_keys", - failures: 0, - }; - opts?.progressCallback?.(importOpt); - }); + return await this.backupManager.importRoomKeys(keys, opts); } /** @@ -1191,7 +1180,7 @@ export class RustCrypto extends TypedEventEmitter Date: Tue, 5 Dec 2023 18:48:10 +0100 Subject: [PATCH 09/24] remove events and use timer mocks --- .../PerSessionKeyBackupDownloader.spec.ts | 248 +++++++++++------- .../PerSessionKeyBackupDownloader.ts | 49 +--- src/rust-crypto/rust-crypto.ts | 6 +- 3 files changed, 174 insertions(+), 129 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index ba2631705db..aa4b2184c9f 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -14,18 +14,17 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { Mocked } from "jest-mock"; +import { Mocked, SpyInstance } from "jest-mock"; import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; import fetchMock from "fetch-mock-jest"; import { - KeyDownloaderEvent, KeyDownloadError, PerSessionKeyBackupDownloader, } from "../../../src/rust-crypto/PerSessionKeyBackupDownloader"; import { logger } from "../../../src/logger"; -import { defer } from "../../../src/utils"; +import { defer, IDeferred } from "../../../src/utils"; import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupManager } from "../../../src/rust-crypto/backup"; import * as TestData from "../../test-utils/test-data"; import { @@ -58,33 +57,15 @@ describe("PerSessionKeyBackupDownloader", () => { let mockOlmMachine: Mocked; let mockBackupDecryptor: Mocked; - async function expectConfigurationError(error: KeyDownloadError): Promise { - return new Promise((resolve) => { - downloader.on(KeyDownloaderEvent.QueryKeyError, (err) => { - if (err === error) { - resolve(); - } - }); - }); - } - async function expectLoopStatus(expectedLoopRunning: boolean): Promise { - return new Promise((resolve) => { - downloader.on(KeyDownloaderEvent.DownloadLoopStateUpdate, (loopRunning) => { - if (expectedLoopRunning == loopRunning) { - resolve(); - } - }); - }); - } + let expectedSession: { [roomId: string]: { [sessionId: string]: IDeferred } }; - async function expectSessionImported(roomId: string, sessionId: string): Promise { - return new Promise((resolve) => { - downloader.on(KeyDownloaderEvent.KeyImported, (r, s) => { - if (roomId == r && sessionId == s) { - resolve(); - } - }); - }); + async function expectSessionImported(roomId: string, sessionId: string) { + const deferred = defer(); + if (!expectedSession[roomId]) { + expectedSession[roomId] = {}; + } + expectedSession[roomId][sessionId] = deferred; + return deferred.promise; } function mockClearSession(sessionId: string): Mocked { @@ -115,6 +96,12 @@ describe("PerSessionKeyBackupDownloader", () => { requestKeyBackupVersion: jest.fn(), importRoomKeys: jest.fn(), createBackupDecryptor: jest.fn().mockReturnValue(mockBackupDecryptor), + on: jest.fn().mockImplementation((event, listener) => { + mockEmitter.on(event, listener); + }), + off: jest.fn().mockImplementation((event, listener) => { + mockEmitter.off(event, listener); + }), } as unknown as Mocked; mockOlmMachine = { @@ -125,15 +112,25 @@ describe("PerSessionKeyBackupDownloader", () => { mockRustBackupManager, mockOlmMachine, mockHttp, - mockEmitter, logger, BACKOFF_TIME, ); + expectedSession = {}; + mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { + const roomId = keys[0].room_id; + const sessionId = keys[0].session_id; + const deferred = expectedSession[roomId] && expectedSession[roomId][sessionId]; + if (deferred) { + deferred.resolve(); + } + }); + jest.useFakeTimers(); }); afterEach(() => { + expectedSession = {}; downloader.stop(); fetchMock.mockReset(); jest.useRealTimers(); @@ -215,13 +212,21 @@ describe("PerSessionKeyBackupDownloader", () => { }); fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA1`, mockCipherKey); + const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); + const expectImported = expectSessionImported("!roomA", "sessionA1"); - const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await jest.runAllTimersAsync(); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), + ); + downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + await jest.runAllTimersAsync(); + expect(spy).toHaveBeenCalledTimes(2); - await expectNotFound; await expectImported; }); @@ -234,28 +239,32 @@ describe("PerSessionKeyBackupDownloader", () => { }, }); - const spy = jest.spyOn(downloader, "queryKeyBackup"); - - const expectNotFound = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); + const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await jest.runAllTimersAsync(); - await expectNotFound; + expect(spy).toHaveReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), + ); + expect(spy).toHaveBeenCalledTimes(1); // Should not query again for a key not in backup downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await jest.runAllTimersAsync(); expect(spy).toHaveBeenCalledTimes(1); // advance time to retry jest.advanceTimersByTime(BACKOFF_TIME + 10); - const expectNotFoundSecondAttempt = expectConfigurationError(KeyDownloadError.MISSING_DECRYPTION_KEY); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await jest.runAllTimersAsync(); expect(spy).toHaveBeenCalledTimes(2); - - await expectNotFoundSecondAttempt; + expect(spy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), + ); }); it("Should stop properly", async () => { @@ -272,36 +281,34 @@ describe("PerSessionKeyBackupDownloader", () => { return mockCipherKey; }); - const expectStopped = expectConfigurationError(KeyDownloadError.STOPPED); - const expectLoopStarted = expectLoopStatus(true); - downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA2"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA3"); - await expectLoopStarted; await requestRoomKeyCalled.promise; downloader.stop(); blockOnServerRequest.resolve(); - await expectStopped; + + // let the first request complete + await jest.runAllTimersAsync(); + expect(mockRustBackupManager.importRoomKeys).not.toHaveBeenCalled(); expect(callCount).toStrictEqual(1); }); }); describe("Given no usable backup available", () => { - let loopPausedPromise: Promise; - let configurationErrorPromise: Promise; + // let loopPausedPromise: Promise; + // let configurationErrorPromise: Promise; + let keyQuerySpy: SpyInstance; beforeEach(async () => { mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); mockOlmMachine.getBackupKeys.mockResolvedValue(null); - loopPausedPromise = expectLoopStatus(false); - - configurationErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); + keyQuerySpy = jest.spyOn(downloader, "queryKeyBackup"); }); afterEach(async () => { @@ -316,8 +323,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); - await loopPausedPromise; - await configurationErrorPromise; + await jest.runAllTimersAsync(); + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), + ); }); it("Should not query server if backup not active", async () => { @@ -329,8 +338,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); - await loopPausedPromise; - await configurationErrorPromise; + await jest.runAllTimersAsync(); + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), + ); }); it("Should stop if backup key is not cached", async () => { @@ -343,8 +354,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); - await loopPausedPromise; - await configurationErrorPromise; + await jest.runAllTimersAsync(); + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), + ); }); it("Should stop if backup key cached as wrong version", async () => { @@ -360,8 +373,11 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); - await loopPausedPromise; - await configurationErrorPromise; + await jest.runAllTimersAsync(); + // await loopPausedPromise; + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), + ); }); it("Should stop if backup key version does not match the active one", async () => { @@ -377,8 +393,11 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); - await loopPausedPromise; - await configurationErrorPromise; + await jest.runAllTimersAsync(); + // await loopPausedPromise; + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), + ); }); }); @@ -393,8 +412,6 @@ describe("PerSessionKeyBackupDownloader", () => { fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey); - const configErrorPromise = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); - const a0Imported = expectSessionImported("!roomA", "sessionA0"); const a1Imported = expectSessionImported("!roomA", "sessionA1"); const b1Imported = expectSessionImported("!roomB", "sessionB1"); @@ -405,8 +422,11 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); downloader.onDecryptionKeyMissingError("!roomB", "sessionB1"); downloader.onDecryptionKeyMissingError("!roomC", "sessionC1"); + await jest.runAllTimersAsync(); - await configErrorPromise; + // @ts-ignore + expect(downloader.hasConfigurationProblem).toEqual(true); + // expect(keyQuerySpy).toHaveReturnedWith(Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR })); // Now the backup becomes trusted mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); @@ -416,11 +436,10 @@ describe("PerSessionKeyBackupDownloader", () => { decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - const loopShouldResume = expectLoopStatus(true); // In that case the sdk would fire a backup status update mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - await loopShouldResume; + await jest.runAllTimersAsync(); await a0Imported; await a1Imported; @@ -442,6 +461,7 @@ describe("PerSessionKeyBackupDownloader", () => { fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, }); + const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const a0Imported = expectSessionImported("!roomA", "sessionA0"); @@ -463,16 +483,12 @@ describe("PerSessionKeyBackupDownloader", () => { { overwriteRoutes: true }, ); - const loopPausedPromise = expectLoopStatus(false); - const expectMismatch = expectConfigurationError(KeyDownloadError.VERSION_MISMATCH); - downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); - await loopPausedPromise; - await expectMismatch; - - // The new backup is detected, the loop should resume but the cached key is still the old one - const configurationError = expectConfigurationError(KeyDownloadError.CONFIGURATION_ERROR); + await jest.runAllTimersAsync(); + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.VERSION_MISMATCH }), + ); // there is a backup mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue({ @@ -482,10 +498,12 @@ describe("PerSessionKeyBackupDownloader", () => { // It's trusted mockRustBackupManager.getActiveBackupVersion.mockResolvedValue("2"); + // The new backup is detected, the loop should resume but the cached key is still the old one mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - // await loopResumed; - await configurationError; + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.VERSION_MISMATCH }), + ); // Now the new key is cached mockOlmMachine.getBackupKeys.mockResolvedValue({ @@ -539,24 +557,47 @@ describe("PerSessionKeyBackupDownloader", () => { { overwriteRoutes: true }, ); - const errorPromise = expectConfigurationError(KeyDownloadError.RATE_LIMITED); + // const errorPromise = expectConfigurationError(KeyDownloadError.RATE_LIMITED); const keyImported = expectSessionImported("!roomA", "sessionA0"); - const spy = jest.spyOn(downloader, "queryKeyBackup"); + // @ts-ignore + const originalImplementation = downloader.queryKeyBackup.bind(downloader); + + const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); + const rateDeferred = defer(); + keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { + const result = await originalImplementation(targetRoomId, targetSessionId); + if (!result.ok && result.error === KeyDownloadError.RATE_LIMITED) { + rateDeferred.resolve(); + } + return result; + }); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); - await errorPromise; + await rateDeferred.promise; + expect(keyQuerySpy).toHaveBeenCalledTimes(1); + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.RATE_LIMITED }), + ); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, }); + // Advance less than the retry_after_ms + jest.advanceTimersByTime(100); + // let any pending callbacks in PromiseJobs run + await Promise.resolve(); + // no additional call should have been made + expect(keyQuerySpy).toHaveBeenCalledTimes(1); + // The loop should resume after the retry_after_ms - jest.advanceTimersByTime(5000 + 100); - await jest.runAllTimersAsync(); + jest.advanceTimersByTime(5000); + // let any pending callbacks in PromiseJobs run + await Promise.resolve(); - expect(spy).toHaveBeenCalledTimes(2); await keyImported; + expect(keyQuerySpy).toHaveBeenCalledTimes(2); }); it("After a network error the same key is retried", async () => { @@ -565,29 +606,58 @@ describe("PerSessionKeyBackupDownloader", () => { throw new ConnectionError("fetch failed", new Error("fetch failed")); }); - const errorPromise = expectConfigurationError(KeyDownloadError.NETWORK_ERROR); + // @ts-ignore + const originalImplementation = downloader.queryKeyBackup.bind(downloader); + + const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); + const errorDeferred = defer(); + keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { + const result = await originalImplementation(targetRoomId, targetSessionId); + if (!result.ok && result.error === KeyDownloadError.NETWORK_ERROR) { + errorDeferred.resolve(); + } + return result; + }); const keyImported = expectSessionImported("!roomA", "sessionA0"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); + await errorDeferred.promise; + await Promise.resolve(); - await errorPromise; + expect(keyQuerySpy).toHaveReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.NETWORK_ERROR }), + ); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, }); + + // Advance less than the retry_after_ms + jest.advanceTimersByTime(100); + // let any pending callbacks in PromiseJobs run + await Promise.resolve(); + // no additional call should have been made + expect(keyQuerySpy).toHaveBeenCalledTimes(1); + // The loop should resume after the retry_after_ms jest.advanceTimersByTime(BACKOFF_TIME + 100); - await jest.runAllTimersAsync(); + await Promise.resolve(); await keyImported; }); it("On Unknown error on import skip the key and continue", async () => { + const keyImported = defer(); mockRustBackupManager.importRoomKeys .mockImplementationOnce(async () => { throw new Error("Didn't work"); }) - .mockImplementationOnce(async () => { + .mockImplementationOnce(async (sessions) => { + const roomId = sessions[0].room_id; + const sessionId = sessions[0].session_id; + if (roomId === "!roomA" && sessionId === "sessionA1") { + keyImported.resolve(); + } return; }); @@ -595,15 +665,17 @@ describe("PerSessionKeyBackupDownloader", () => { overwriteRoutes: true, }); - const errorPromise = expectConfigurationError(KeyDownloadError.UNKNOWN_ERROR); - const keyImported = expectSessionImported("!roomA", "sessionA1"); + const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); + await jest.runAllTimersAsync(); - await errorPromise; + expect(keyQuerySpy).toHaveLastReturnedWith( + Promise.resolve({ ok: false, error: KeyDownloadError.UNKNOWN_ERROR }), + ); - await keyImported; + await keyImported.promise; }); }); }); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 664c47516a9..a7327c96a44 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -20,8 +20,8 @@ import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; import { Curve25519AuthData, KeyBackupSession } from "../crypto-api/keybackup"; import { Logger } from "../logger"; import { ClientPrefix, IHttpOpts, MatrixError, MatrixHttpApi, Method } from "../http-api"; -import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupDecryptor, RustBackupManager } from "./backup"; -import { CryptoEvent, TypedEventEmitter } from "../matrix"; +import { RustBackupDecryptor, RustBackupManager } from "./backup"; +import { CryptoEvent } from "../matrix"; import { encodeUri, sleep } from "../utils"; /** @@ -59,23 +59,6 @@ type Configuration = { decryptor: RustBackupDecryptor; }; -/** - * Signaling for the Downloader loop. - * Not yet used by API, yet useful for testing. - */ -export enum KeyDownloaderEvent { - DownloadLoopStateUpdate = "download_loop_started", - DownLoopStep = "download_loop_step", - QueryKeyError = "query_key_error", - KeyImported = "key_imported", -} -export type KeyDownloaderEventMap = { - [KeyDownloaderEvent.DownloadLoopStateUpdate]: (loopRunning: boolean) => void; - [KeyDownloaderEvent.DownLoopStep]: (remaining: number) => void; - [KeyDownloaderEvent.QueryKeyError]: (errCode: KeyDownloadError) => void; - [KeyDownloaderEvent.KeyImported]: (roomId: string, sessionId: string) => void; -}; - /** * This function is called when an 'unable to decrypt' error occurs. It attempts to download the key from the backup. * @@ -87,7 +70,7 @@ export type KeyDownloaderEventMap = { * the backup is configured correctly. * */ -export class PerSessionKeyBackupDownloader extends TypedEventEmitter { +export class PerSessionKeyBackupDownloader { private stopped = false; private configuration: Configuration | null = null; @@ -107,7 +90,6 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter, - private readonly cryptoEventEmitter: TypedEventEmitter, logger: Logger, private readonly maxTimeBetweenRetry: number, ) { - super(); - this.logger = logger.getChild("[PerSessionKeyBackupDownloader]"); - cryptoEventEmitter.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - cryptoEventEmitter.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - cryptoEventEmitter.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + backupManager.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + backupManager.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + backupManager.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } public stop(): void { this.stopped = true; - this.cryptoEventEmitter.off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - this.cryptoEventEmitter.off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - this.cryptoEventEmitter.off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + this.backupManager.off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); + this.backupManager.off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); + this.backupManager.off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); } private onBackupStatusChanged(): void { @@ -144,8 +123,6 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { if (decryptor) { this.downloadKeysLoop(); - } else { - this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.CONFIGURATION_ERROR); } }); } @@ -185,7 +162,6 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter { @@ -257,25 +233,21 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter 0) { - this.emit(KeyDownloaderEvent.DownLoopStep, this.queuedRequests.length); + // this.emit(KeyDownloaderEvent.DownLoopStep, this.queuedRequests.length); // we just peek the first one without removing it, so if a new request for same key comes in while we're // processing this one, it won't queue another request. const request = this.queuedRequests[0]; const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); if (this.stopped) { - this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.STOPPED); return; } if (result.ok) { // We got the encrypted key from backup, let's try to decrypt and import it. try { await this.decryptAndImport(request, result.value); - this.emit(KeyDownloaderEvent.KeyImported, request.roomId, request.megolmSessionId); } catch (e) { - this.emit(KeyDownloaderEvent.QueryKeyError, KeyDownloadError.UNKNOWN_ERROR); this.logger.error( `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, e, @@ -284,7 +256,6 @@ export class PerSessionKeyBackupDownloader extends TypedEventEmitter Date: Tue, 5 Dec 2023 18:55:26 +0100 Subject: [PATCH 10/24] fix import --- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index a7327c96a44..6858ff8e6c7 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -20,9 +20,10 @@ import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; import { Curve25519AuthData, KeyBackupSession } from "../crypto-api/keybackup"; import { Logger } from "../logger"; import { ClientPrefix, IHttpOpts, MatrixError, MatrixHttpApi, Method } from "../http-api"; -import { RustBackupDecryptor, RustBackupManager } from "./backup"; +import { RustBackupManager } from "./backup"; import { CryptoEvent } from "../matrix"; import { encodeUri, sleep } from "../utils"; +import { BackupDecryptor } from "../common-crypto/CryptoBackend"; /** * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. @@ -56,7 +57,7 @@ type KeyDownloadResult = /** Holds the current backup decryptor and version that should be used. */ type Configuration = { backupVersion: string; - decryptor: RustBackupDecryptor; + decryptor: BackupDecryptor; }; /** From 26ea4aed42f828c5a108fb9a4d5b72677d7d2331 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 5 Dec 2023 18:55:40 +0100 Subject: [PATCH 11/24] ts ignore in tests --- .../PerSessionKeyBackupDownloader.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index aa4b2184c9f..5244b45d3da 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -185,6 +185,7 @@ describe("PerSessionKeyBackupDownloader", () => { } }); + // @ts-ignore const spy = jest.spyOn(downloader, "queryKeyBackup"); // Call 3 times for same key @@ -212,6 +213,7 @@ describe("PerSessionKeyBackupDownloader", () => { }); fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA1`, mockCipherKey); + // @ts-ignore const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const expectImported = expectSessionImported("!roomA", "sessionA1"); @@ -239,6 +241,7 @@ describe("PerSessionKeyBackupDownloader", () => { }, }); + // @ts-ignore const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); @@ -308,6 +311,7 @@ describe("PerSessionKeyBackupDownloader", () => { mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); mockOlmMachine.getBackupKeys.mockResolvedValue(null); + // @ts-ignore keyQuerySpy = jest.spyOn(downloader, "queryKeyBackup"); }); @@ -461,6 +465,8 @@ describe("PerSessionKeyBackupDownloader", () => { fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, }); + + // @ts-ignore const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const a0Imported = expectSessionImported("!roomA", "sessionA0"); @@ -563,8 +569,10 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore const originalImplementation = downloader.queryKeyBackup.bind(downloader); + // @ts-ignore const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const rateDeferred = defer(); + // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { const result = await originalImplementation(targetRoomId, targetSessionId); if (!result.ok && result.error === KeyDownloadError.RATE_LIMITED) { @@ -609,8 +617,11 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore const originalImplementation = downloader.queryKeyBackup.bind(downloader); + // @ts-ignore const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const errorDeferred = defer(); + + // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { const result = await originalImplementation(targetRoomId, targetSessionId); if (!result.ok && result.error === KeyDownloadError.NETWORK_ERROR) { @@ -665,6 +676,7 @@ describe("PerSessionKeyBackupDownloader", () => { overwriteRoutes: true, }); + // @ts-ignore const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); From 2951a9a3e08c03836e3b9ec1dd75bf683f5d156b Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 5 Dec 2023 20:07:47 +0100 Subject: [PATCH 12/24] Quick cleaning --- .../PerSessionKeyBackupDownloader.spec.ts | 64 +++++-------------- .../PerSessionKeyBackupDownloader.ts | 9 +-- 2 files changed, 21 insertions(+), 52 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 5244b45d3da..1a7b56f65c1 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -19,10 +19,7 @@ import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm"; import { OlmMachine } from "@matrix-org/matrix-sdk-crypto-wasm"; import fetchMock from "fetch-mock-jest"; -import { - KeyDownloadError, - PerSessionKeyBackupDownloader, -} from "../../../src/rust-crypto/PerSessionKeyBackupDownloader"; +import { PerSessionKeyBackupDownloader } from "../../../src/rust-crypto/PerSessionKeyBackupDownloader"; import { logger } from "../../../src/logger"; import { defer, IDeferred } from "../../../src/utils"; import { RustBackupCryptoEventMap, RustBackupCryptoEvents, RustBackupManager } from "../../../src/rust-crypto/backup"; @@ -46,7 +43,6 @@ describe("PerSessionKeyBackupDownloader", () => { let downloader: PerSessionKeyBackupDownloader; const mockCipherKey: Mocked = {} as unknown as Mocked; - // let delegate: Mocked; const BACKOFF_TIME = 2000; @@ -221,9 +217,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await jest.runAllTimersAsync(); expect(spy).toHaveBeenCalledTimes(1); - expect(spy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), - ); + expect(spy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "MISSING_DECRYPTION_KEY" })); downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); await jest.runAllTimersAsync(); @@ -247,9 +241,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await jest.runAllTimersAsync(); - expect(spy).toHaveReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), - ); + expect(spy).toHaveReturnedWith(Promise.resolve({ ok: false, error: "MISSING_DECRYPTION_KEY" })); expect(spy).toHaveBeenCalledTimes(1); // Should not query again for a key not in backup @@ -265,9 +257,7 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); expect(spy).toHaveBeenCalledTimes(2); - expect(spy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }), - ); + expect(spy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "MISSING_DECRYPTION_KEY" })); }); it("Should stop properly", async () => { @@ -328,9 +318,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); }); it("Should not query server if backup not active", async () => { @@ -343,9 +331,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); }); it("Should stop if backup key is not cached", async () => { @@ -359,9 +345,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); }); it("Should stop if backup key cached as wrong version", async () => { @@ -379,9 +363,7 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); // await loopPausedPromise; - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); }); it("Should stop if backup key version does not match the active one", async () => { @@ -399,9 +381,7 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); // await loopPausedPromise; - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); }); }); @@ -430,7 +410,6 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore expect(downloader.hasConfigurationProblem).toEqual(true); - // expect(keyQuerySpy).toHaveReturnedWith(Promise.resolve({ ok: false, error: KeyDownloadError.CONFIGURATION_ERROR })); // Now the backup becomes trusted mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); @@ -492,9 +471,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.VERSION_MISMATCH }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "VERSION_MISMATCH" })); // there is a backup mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue({ @@ -507,9 +484,7 @@ describe("PerSessionKeyBackupDownloader", () => { // The new backup is detected, the loop should resume but the cached key is still the old one mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.VERSION_MISMATCH }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "VERSION_MISMATCH" })); // Now the new key is cached mockOlmMachine.getBackupKeys.mockResolvedValue({ @@ -563,7 +538,6 @@ describe("PerSessionKeyBackupDownloader", () => { { overwriteRoutes: true }, ); - // const errorPromise = expectConfigurationError(KeyDownloadError.RATE_LIMITED); const keyImported = expectSessionImported("!roomA", "sessionA0"); // @ts-ignore @@ -575,7 +549,7 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { const result = await originalImplementation(targetRoomId, targetSessionId); - if (!result.ok && result.error === KeyDownloadError.RATE_LIMITED) { + if (!result.ok && result.error === "RATE_LIMITED") { rateDeferred.resolve(); } return result; @@ -584,9 +558,7 @@ describe("PerSessionKeyBackupDownloader", () => { await rateDeferred.promise; expect(keyQuerySpy).toHaveBeenCalledTimes(1); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.RATE_LIMITED }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "RATE_LIMITED" })); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, @@ -624,7 +596,7 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { const result = await originalImplementation(targetRoomId, targetSessionId); - if (!result.ok && result.error === KeyDownloadError.NETWORK_ERROR) { + if (!result.ok && result.error === "NETWORK_ERROR") { errorDeferred.resolve(); } return result; @@ -635,9 +607,7 @@ describe("PerSessionKeyBackupDownloader", () => { await errorDeferred.promise; await Promise.resolve(); - expect(keyQuerySpy).toHaveReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.NETWORK_ERROR }), - ); + expect(keyQuerySpy).toHaveReturnedWith(Promise.resolve({ ok: false, error: "NETWORK_ERROR" })); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, @@ -683,9 +653,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith( - Promise.resolve({ ok: false, error: KeyDownloadError.UNKNOWN_ERROR }), - ); + expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "UNKNOWN_ERROR" })); await keyImported.promise; }); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 6858ff8e6c7..2dbfc8de38e 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -28,7 +28,7 @@ import { BackupDecryptor } from "../common-crypto/CryptoBackend"; /** * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. */ -export enum KeyDownloadError { +enum KeyDownloadError { /** The backup version in use is out of sync with the server version. */ VERSION_MISMATCH = "VERSION_MISMATCH", /** The requested key is not in the backup. */ @@ -61,13 +61,13 @@ type Configuration = { }; /** - * This function is called when an 'unable to decrypt' error occurs. It attempts to download the key from the backup. + * Used when an 'unable to decrypt' error occurs. It attempts to download the key from the backup. * * The current backup API lacks pagination, which can lead to lengthy key retrieval times for large histories (several 10s of minutes). * To mitigate this, keys are downloaded on demand as decryption errors occurs. * While this approach may result in numerous requests, it improves user experience by reducing wait times for message decryption. * - * The PerSessionKeyBackupDownloader is resistant to backup configuration changes, it will automatically resume querying when + * The PerSessionKeyBackupDownloader is resistant to backup configuration changes: it will automatically resume querying when * the backup is configured correctly. * */ @@ -92,7 +92,7 @@ export class PerSessionKeyBackupDownloader { * @param olmMachine - The olm machine to use. * @param http - The http instance to use. * @param logger - The logger to use. - * @param maxTimeBetweenRetry - The maximum time to wait between two retries. This is to avoid hammering the server. + * @param maxTimeBetweenRetry - The maximum time to wait between two retries in case of errors. To avoid hammering the server. * */ public constructor( @@ -159,6 +159,7 @@ export class PerSessionKeyBackupDownloader { return Math.max(Date.now() - lastCheck, 0) < this.maxTimeBetweenRetry; } + // Remembers if we have a configuration problem. private hasConfigurationProblem = false; private pauseLoop(): void { From db3c736fcede25d5072ca6f3cc0b3e5d6e588de0 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 6 Dec 2023 14:04:34 +0100 Subject: [PATCH 13/24] code review --- .../PerSessionKeyBackupDownloader.spec.ts | 77 +---- .../PerSessionKeyBackupDownloader.ts | 278 ++++++++++-------- src/rust-crypto/backup.ts | 8 + src/rust-crypto/rust-crypto.ts | 6 +- 4 files changed, 161 insertions(+), 208 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 1a7b56f65c1..d263ba258f8 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -105,10 +105,10 @@ describe("PerSessionKeyBackupDownloader", () => { } as unknown as Mocked; downloader = new PerSessionKeyBackupDownloader( - mockRustBackupManager, + logger, mockOlmMachine, mockHttp, - logger, + mockRustBackupManager, BACKOFF_TIME, ); @@ -429,79 +429,6 @@ describe("PerSessionKeyBackupDownloader", () => { await b1Imported; await c1Imported; }); - - it("If reset from other session, loop should stop until new decryption key is known", async () => { - // there is a backup - mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); - // It's trusted - mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA.version!); - // And we have the key in cache - mockOlmMachine.getBackupKeys.mockResolvedValue({ - backupVersion: TestData.SIGNED_BACKUP_DATA.version!, - decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), - } as unknown as RustSdkCryptoJs.BackupKeys); - - fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { - overwriteRoutes: true, - }); - - // @ts-ignore - const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); - - const a0Imported = expectSessionImported("!roomA", "sessionA0"); - - downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); - - await a0Imported; - - // Now some other session resets the backup and there is a new version - // the room_keys/keys endpoint will throw - fetchMock.get( - `express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, - { - status: 404, - body: { - errcode: "M_NOT_FOUND", - error: "Unknown backup version", - }, - }, - { overwriteRoutes: true }, - ); - - downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); - - await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "VERSION_MISMATCH" })); - - // there is a backup - mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue({ - ...TestData.SIGNED_BACKUP_DATA, - version: "2", - }); - // It's trusted - mockRustBackupManager.getActiveBackupVersion.mockResolvedValue("2"); - - // The new backup is detected, the loop should resume but the cached key is still the old one - mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "VERSION_MISMATCH" })); - - // Now the new key is cached - mockOlmMachine.getBackupKeys.mockResolvedValue({ - backupVersion: "2", - decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), - } as unknown as RustSdkCryptoJs.BackupKeys); - - fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { - overwriteRoutes: true, - }); - - const a1Imported = expectSessionImported("!roomA", "sessionA1"); - - mockEmitter.emit(CryptoEvent.KeyBackupStatus, true); - - await a1Imported; - }); }); describe("Error cases", () => { diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 2dbfc8de38e..36fcdc995b2 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -29,24 +29,27 @@ import { BackupDecryptor } from "../common-crypto/CryptoBackend"; * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. */ enum KeyDownloadError { - /** The backup version in use is out of sync with the server version. */ - VERSION_MISMATCH = "VERSION_MISMATCH", /** The requested key is not in the backup. */ MISSING_DECRYPTION_KEY = "MISSING_DECRYPTION_KEY", /** A network error occurred while trying to get the key. */ NETWORK_ERROR = "NETWORK_ERROR", - /** The loop as been stopped. */ + /** The loop has been stopped. */ STOPPED = "STOPPED", /** An unknown error occurred while decrypting/importing the key */ UNKNOWN_ERROR = "UNKNOWN_ERROR", /** The server is rate limiting us. */ RATE_LIMITED = "RATE_LIMITED", - /** The backup is not configured correctly, can be that there is no backup, that it's not trusted - * , that we don't have the correct key in cache... */ + /** The backup is not configured correctly. + * + * Example problems can include: + * * There is no backup + * * Backup is not trusted + * * We don't have the correct key in cache + */ CONFIGURATION_ERROR = "CONFIGURATION_ERROR", } -/** Helper type for requested session*/ +/** Details of a megolm session whose key we are trying to fetch. */ type SessionInfo = { roomId: string; megolmSessionId: string }; /** Helper type for the result of a key download. */ @@ -69,22 +72,32 @@ type Configuration = { * * The PerSessionKeyBackupDownloader is resistant to backup configuration changes: it will automatically resume querying when * the backup is configured correctly. - * */ export class PerSessionKeyBackupDownloader { private stopped = false; + /** The version and decryption key to use with current backup if all setup correctly */ private configuration: Configuration | null = null; - /** We remember when a session was requested and not found in backup to avoid query again too soon. */ - private sessionLastCheckAttemptedTime: Record = {}; - - private readonly configurationChangeHandler = (): void => { - this.onBackupStatusChanged(); - }; + /** We remember when a session was requested and not found in backup to avoid query again too soon. + * Map of session_id to timestamp */ + private sessionLastCheckAttemptedTime: Map = new Map(); + /** The logger to use */ private readonly logger: Logger; + /** Whether the download loop is running. */ + private downloadLoopRunning = false; + + /** The list of requests that are queued. */ + private queuedRequests: SessionInfo[] = []; + + // Remembers if we have a configuration problem. + private hasConfigurationProblem = false; + + /** The current server backup version check promise. To avoid doing a server call if one is in flight. */ + private currentBackupVersionCheck: Promise | null = null; + /** * Creates a new instance of PerSessionKeyBackupDownloader. * @@ -92,76 +105,114 @@ export class PerSessionKeyBackupDownloader { * @param olmMachine - The olm machine to use. * @param http - The http instance to use. * @param logger - The logger to use. - * @param maxTimeBetweenRetry - The maximum time to wait between two retries in case of errors. To avoid hammering the server. + * @param backoffDuration - The minimum time to wait between two retries in case of errors. To avoid hammering the server. * */ public constructor( - private readonly backupManager: RustBackupManager, + logger: Logger, private readonly olmMachine: OlmMachine, private readonly http: MatrixHttpApi, - logger: Logger, - private readonly maxTimeBetweenRetry: number, + private readonly backupManager: RustBackupManager, + private readonly backoffDuration: number, ) { this.logger = logger.getChild("[PerSessionKeyBackupDownloader]"); - backupManager.on(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - backupManager.on(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - backupManager.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + backupManager.on(CryptoEvent.KeyBackupStatus, this.onBackupStatusChanged); + backupManager.on(CryptoEvent.KeyBackupFailed, this.onBackupStatusChanged); + backupManager.on(CryptoEvent.KeyBackupDecryptionKeyCached, this.onBackupStatusChanged); + } + + /** + * Called when a MissingRoomKey or UnknownMessageIndex decryption error is encountered. + * + * This will try to download the key from the backup if there is a trusted active backup. + * In case of success the key will be imported and the onRoomKeysUpdated callback will be called + * internally by the rust-sdk and decrytion will be retried. + * + * @param roomId - The room ID of the room where the error occurred. + * @param megolmSessionId - The megolm session ID that is missing. + */ + public onDecryptionKeyMissingError(roomId: string, megolmSessionId: string): void { + // Several messages encrypted with the same session may be decrypted at the same time, + // so we need to be resistant and not query several time the same session. + if (this.isAlreadyInQueue(roomId, megolmSessionId)) { + // There is already a request queued for this session, no need to queue another one. + this.logger.trace(`Not checking key backup for session ${megolmSessionId} as it is already queued`); + return; + } + + if (this.wasRequestedRecently(megolmSessionId)) { + // We already tried to download this session recently and it was not in backup, no need to try again. + this.logger.trace( + `Not checking key backup for session ${megolmSessionId} as it was already requested recently`, + ); + return; + } + + // We always add the request to the queue, even if we have a configuration problem (can't access backup). + // This is to make sure that if the configuration problem is resolved, we will try to download the key. + // This will happen after an initial sync, at this point the backup will not yet be trusted and the decryption + // key will not be available, but it will be just after the verification. + // We don't need to persist it because currently on refresh the sdk will retry to decrypt the messages in error. + this.queuedRequests.push({ roomId, megolmSessionId }); + + // Start the download loop if it's not already running. + this.downloadKeysLoop(); } public stop(): void { this.stopped = true; - this.backupManager.off(CryptoEvent.KeyBackupStatus, this.configurationChangeHandler); - this.backupManager.off(CryptoEvent.KeyBackupFailed, this.configurationChangeHandler); - this.backupManager.off(CryptoEvent.KeyBackupDecryptionKeyCached, this.configurationChangeHandler); + this.backupManager.off(CryptoEvent.KeyBackupStatus, this.onBackupStatusChanged); + this.backupManager.off(CryptoEvent.KeyBackupFailed, this.onBackupStatusChanged); + this.backupManager.off(CryptoEvent.KeyBackupDecryptionKeyCached, this.onBackupStatusChanged); } - private onBackupStatusChanged(): void { - this.logger.info(`Key backup status change => check configuration`); + /** + * Called when the backup status changes (CryptoEvents) + * This will trigger a check of the backup configuration. + */ + private onBackupStatusChanged = (): void => { // we want to check configuration this.hasConfigurationProblem = false; this.configuration = null; - this.getOrCreateBackupDecryptor(true).then((decryptor) => { + this.getOrCreateBackupConfiguration(true).then((decryptor) => { if (decryptor) { this.downloadKeysLoop(); } }); - } - - private downloadLoopRunning = false; - - private queuedRequests: SessionInfo[] = []; + }; + /** Returns true if the megolm session is already queued for download. */ private isAlreadyInQueue(roomId: string, megolmSessionId: string): boolean { - return ( - this.queuedRequests.findIndex((info) => { - return info.roomId == roomId && info.megolmSessionId == megolmSessionId; - }) != -1 - ); + return this.queuedRequests.some((info) => { + return info.roomId == roomId && info.megolmSessionId == megolmSessionId; + }); } + /** + * Marks the session as not found in backup, to avoid retrying to soon for a key not in backup + * @param megolmSessionId - The megolm session ID that is missing. + * */ private markAsNotFoundInBackup(megolmSessionId: string): void { const now = Date.now(); - this.sessionLastCheckAttemptedTime[megolmSessionId] = now; + this.sessionLastCheckAttemptedTime.set(megolmSessionId, now); // if too big make some cleaning to keep under control - if (Object.keys(this.sessionLastCheckAttemptedTime).length > 100) { - for (const key in this.sessionLastCheckAttemptedTime) { - if (Math.max(now - this.sessionLastCheckAttemptedTime[key], 0) > this.maxTimeBetweenRetry) { - delete this.sessionLastCheckAttemptedTime[key]; - } - } + if (this.sessionLastCheckAttemptedTime.size > 100) { + this.sessionLastCheckAttemptedTime = new Map( + Array.from(this.sessionLastCheckAttemptedTime).filter((sid, ts) => { + return Math.max(now - ts, 0) < this.backoffDuration; + }), + ); } } + /** Returns true if the session was requested recently. */ private wasRequestedRecently(megolmSessionId: string): boolean { - const lastCheck = this.sessionLastCheckAttemptedTime[megolmSessionId]; + const lastCheck = this.sessionLastCheckAttemptedTime.get(megolmSessionId); if (!lastCheck) return false; - return Math.max(Date.now() - lastCheck, 0) < this.maxTimeBetweenRetry; + return Math.max(Date.now() - lastCheck, 0) < this.backoffDuration; } - // Remembers if we have a configuration problem. - private hasConfigurationProblem = false; - private pauseLoop(): void { this.downloadLoopRunning = false; } @@ -174,6 +225,13 @@ export class PerSessionKeyBackupDownloader { } } + /** + * Requests a key from the server side backup. + * @param version - The backup version to use. + * @param roomId - The room ID of the room where the error occurred. + * @param sessionId - The megolm session ID that is missing. + * + */ private async requestRoomKeyFromBackup( version: string, roomId: string, @@ -189,44 +247,6 @@ export class PerSessionKeyBackupDownloader { }); } - /** - * Called when a MissingRoomKey or UnknownMessageIndex decryption error is encountered. - * - * This will try to download the key from the backup if there is a trusted active backup. - * In case of success the key will be imported and the onRoomKeysUpdated callback will be called - * internally by the rust-sdk and decrytion will be retried. - * - * @param roomId - The room ID of the room where the error occurred. - * @param megolmSessionId - The megolm session ID that is missing. - */ - public onDecryptionKeyMissingError(roomId: string, megolmSessionId: string): void { - // Several messages encrypted with the same session may be decrypted at the same time, - // so we need to be resistant and not query several time the same session. - if (this.isAlreadyInQueue(roomId, megolmSessionId)) { - // There is already a request queued for this session, no need to queue another one. - this.logger.trace(`Not checking key backup for session ${megolmSessionId} as it is already queued`); - return; - } - - if (this.wasRequestedRecently(megolmSessionId)) { - // We already tried to download this session recently and it was not in backup, no need to try again. - this.logger.trace( - `Not checking key backup for session ${megolmSessionId} as it was already requested recently`, - ); - return; - } - - // We always add the request to the queue, even if we have a configuration problem (can't access backup). - // This is to make sure that if the configuration problem is resolved, we will try to download the key. - // This will happen after an initial sync, at this point the backup will not yet be trusted and the decryption - // key will not be available, but it will be just after the verification. - // We don't need to persist it because currently on refresh the sdk will retry to decrypt the messages in error. - this.queuedRequests.push({ roomId, megolmSessionId }); - - // Start the download loop if it's not already running. - this.downloadKeysLoop(); - } - private async downloadKeysLoop(): Promise { if (this.downloadLoopRunning) return; @@ -237,7 +257,6 @@ export class PerSessionKeyBackupDownloader { this.downloadLoopRunning = true; while (this.queuedRequests.length > 0) { - // this.emit(KeyDownloaderEvent.DownLoopStep, this.queuedRequests.length); // we just peek the first one without removing it, so if a new request for same key comes in while we're // processing this one, it won't queue another request. const request = this.queuedRequests[0]; @@ -262,12 +281,6 @@ export class PerSessionKeyBackupDownloader { `Error while downloading key backup for session ${request.megolmSessionId}: ${result.error}`, ); switch (result.error) { - case KeyDownloadError.VERSION_MISMATCH: { - // We don't have the correct decryption key, so stop the loop. - // If we get the key later, we will retry. - this.pauseLoop(); - return; - } case KeyDownloadError.MISSING_DECRYPTION_KEY: { this.markAsNotFoundInBackup(request.megolmSessionId); // continue for next one @@ -281,12 +294,12 @@ export class PerSessionKeyBackupDownloader { } case KeyDownloadError.RATE_LIMITED: { // we want to retry - await sleep(result.retryAfterMs ?? this.maxTimeBetweenRetry); + await sleep(result.retryAfterMs ?? this.backoffDuration); break; } case KeyDownloadError.NETWORK_ERROR: { // We don't want to hammer if there is a problem, so wait a bit. - await sleep(this.maxTimeBetweenRetry); + await sleep(this.backoffDuration); break; } case KeyDownloadError.STOPPED: @@ -298,6 +311,7 @@ export class PerSessionKeyBackupDownloader { } this.pauseLoop(); } + /** * Query the backup for a key. * @@ -305,12 +319,12 @@ export class PerSessionKeyBackupDownloader { * @param targetSessionId - ID of the session for which to check backup. */ private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { - const configuration = await this.getOrCreateBackupDecryptor(false); + const configuration = await this.getOrCreateBackupConfiguration(false); if (!configuration) { return { ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }; } - this.logger.debug(`--> Checking key backup for session ${targetSessionId}`); + this.logger.debug(`Checking key backup for session ${targetSessionId}`); try { const res = await this.requestRoomKeyFromBackup(configuration.backupVersion, targetRoomId, targetSessionId); @@ -333,12 +347,8 @@ export class PerSessionKeyBackupDownloader { // - "error": "No room_keys found" if the key is missing. // For now we check the error message, but this is not ideal. // It's useful to know if the key is missing or if the version is wrong. - // As it's not spec'ed, will work only with synapse, but it's better than nothing? - // Other implementations will consider this as a missing key, but soon after a backup status - // change will trigger a configuration check for future keys (this one won't be retied though) - if (e.data.error == "Unknown backup version") { - return { ok: false, error: KeyDownloadError.VERSION_MISMATCH }; - } + // As it's not spec'ed, we fallback on considering the key has not in backup, + // notice that this request will be lost if the backup is not configured correctly. return { ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }; } if (errCode == "M_LIMIT_EXCEEDED") { @@ -351,7 +361,7 @@ export class PerSessionKeyBackupDownloader { return { ok: false, error: KeyDownloadError.RATE_LIMITED, - retryAfterMs: this.maxTimeBetweenRetry, + retryAfterMs: this.backoffDuration, }; } } @@ -360,9 +370,33 @@ export class PerSessionKeyBackupDownloader { } } - private currentBackupVersionCheck: Promise | null = null; + private async decryptAndImport(sessionInfo: SessionInfo, data: KeyBackupSession): Promise { + const configuration = await this.getOrCreateBackupConfiguration(false); - private async getOrCreateBackupDecryptor(forceCheck: boolean): Promise { + if (!configuration) { + throw new Error("Backup: No configuration"); + } + + const sessionsToImport: Record = { [sessionInfo.megolmSessionId]: data }; + + const keys = await configuration!.decryptor.decryptSessions(sessionsToImport); + for (const k of keys) { + k.room_id = sessionInfo.roomId; + } + await this.backupManager.importRoomKeys(keys); + } + + /** + * Gets the current backup configuration or create one if it doesn't exist. + * + * When a valid configuration is found it is cached and returned for subsequent calls. + * If a check is forced or a check has not yet been done, a new check is done. + * + * @param forceCheck - If true, force a check of the backup configuration. + * + * @returns The current backup configuration or null if there is a configuration problem. + */ + private async getOrCreateBackupConfiguration(forceCheck: boolean): Promise { if (this.configuration) { return this.configuration; } @@ -374,7 +408,7 @@ export class PerSessionKeyBackupDownloader { // This method can be called rapidly by several emitted CryptoEvent, so we need to make sure that we don't // query the server several times. if (this.currentBackupVersionCheck != null) { - this.logger.debug(`Backup: already checking server version, use current promise`); + this.logger.debug(`Already checking server version, use current promise`); return await this.currentBackupVersionCheck; } @@ -395,10 +429,10 @@ export class PerSessionKeyBackupDownloader { this.hasConfigurationProblem = true; return null; } - this.logger.debug(`Got current version from server:${currentServerVersion?.version}`); + this.logger.debug(`Got current backup version from server: ${currentServerVersion?.version}`); if (currentServerVersion?.algorithm != "m.megolm_backup.v1.curve25519-aes-sha2") { - this.logger.info(`getBackupDecryptor Unsupported algorithm ${currentServerVersion?.algorithm}`); + this.logger.info(`Unsupported algorithm ${currentServerVersion?.algorithm}`); this.hasConfigurationProblem = true; return null; } @@ -411,26 +445,26 @@ export class PerSessionKeyBackupDownloader { const activeVersion = await this.backupManager.getActiveBackupVersion(); if (activeVersion == null || currentServerVersion.version != activeVersion) { - // case when the server side current version is not trusted or is out of sync with the client side active version. + // Either the current backup version on server side is not trusted, or it is out of sync with the active version on the client side. this.logger.info( - `The current backup version ${currentServerVersion.version} is not trusted. Active version=${activeVersion}`, + `The current backup version on the server (${currentServerVersion.version}) is not trusted. Version we are currently backing up to: ${activeVersion}`, ); this.hasConfigurationProblem = true; return null; } - const authData = currentServerVersion.auth_data; + const authData = currentServerVersion.auth_data as Curve25519AuthData; const backupKeys = await this.getBackupDecryptionKey(); if (!backupKeys?.decryptionKey) { - this.logger.debug(`Not checking key backup for session(no decryption key)`); + this.logger.debug(`Not checking key backup for session (no decryption key)`); this.hasConfigurationProblem = true; return null; } if (activeVersion != backupKeys.backupVersion) { this.logger.debug( - `Cached key version <${backupKeys.backupVersion}> doesn't match active backup version <${activeVersion}>`, + `Version for which we have a decryption key (${backupKeys.backupVersion}) doesn't match the version we are backing up to (${activeVersion})`, ); this.hasConfigurationProblem = true; return null; @@ -450,20 +484,4 @@ export class PerSessionKeyBackupDownloader { }; return this.configuration; } - - private async decryptAndImport(sessionInfo: SessionInfo, data: KeyBackupSession): Promise { - const configuration = await this.getOrCreateBackupDecryptor(false); - - if (!configuration) { - throw new Error("Backup: No configuration"); - } - - const sessionsToImport: Record = { [sessionInfo.megolmSessionId]: data }; - - const keys = await configuration!.decryptor.decryptSessions(sessionsToImport); - for (const k of keys) { - k.room_id = sessionInfo.roomId; - } - await this.backupManager.importRoomKeys(keys); - } } diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index 7cdd8b51177..12495a30922 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -173,6 +173,14 @@ export class RustBackupManager extends TypedEventEmitter { // TODO when backup support will be added we would need to expose the `from_backup` flag in the bindings const jsonKeys = JSON.stringify(keys); diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 94a7c0584fb..9349511edbb 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -146,10 +146,10 @@ export class RustCrypto extends TypedEventEmitter Date: Thu, 7 Dec 2023 11:51:28 +0100 Subject: [PATCH 14/24] Use Errors instead of Results --- .../PerSessionKeyBackupDownloader.spec.ts | 66 ++++++--- .../PerSessionKeyBackupDownloader.ts | 128 +++++++++--------- 2 files changed, 106 insertions(+), 88 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index d263ba258f8..0bf46cf6f0a 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -241,8 +241,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await jest.runAllTimersAsync(); - expect(spy).toHaveReturnedWith(Promise.resolve({ ok: false, error: "MISSING_DECRYPTION_KEY" })); expect(spy).toHaveBeenCalledTimes(1); + const returnedPromise = spy.mock.results[0].value; + await expect(returnedPromise).rejects.toThrow("Failed to get key from backup: MISSING_DECRYPTION_KEY"); // Should not query again for a key not in backup downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); @@ -257,7 +258,9 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); expect(spy).toHaveBeenCalledTimes(2); - expect(spy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "MISSING_DECRYPTION_KEY" })); + await expect(spy.mock.results[1].value).rejects.toThrow( + "Failed to get key from backup: MISSING_DECRYPTION_KEY", + ); }); it("Should stop properly", async () => { @@ -318,7 +321,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: CONFIGURATION_ERROR", + ); }); it("Should not query server if backup not active", async () => { @@ -331,7 +336,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: CONFIGURATION_ERROR", + ); }); it("Should stop if backup key is not cached", async () => { @@ -345,7 +352,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: CONFIGURATION_ERROR", + ); }); it("Should stop if backup key cached as wrong version", async () => { @@ -362,8 +371,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - // await loopPausedPromise; - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); + + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: CONFIGURATION_ERROR", + ); }); it("Should stop if backup key version does not match the active one", async () => { @@ -380,8 +391,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - // await loopPausedPromise; - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "CONFIGURATION_ERROR" })); + + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: CONFIGURATION_ERROR", + ); }); }); @@ -475,17 +488,22 @@ describe("PerSessionKeyBackupDownloader", () => { const rateDeferred = defer(); // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { - const result = await originalImplementation(targetRoomId, targetSessionId); - if (!result.ok && result.error === "RATE_LIMITED") { - rateDeferred.resolve(); + try { + return await originalImplementation(targetRoomId, targetSessionId); + } catch (err: any) { + if (err.name === "KeyDownloadRateLimit") { + rateDeferred.resolve(); + } + throw err; } - return result; }); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await rateDeferred.promise; expect(keyQuerySpy).toHaveBeenCalledTimes(1); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "RATE_LIMITED" })); + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: rate limited", + ); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, @@ -522,11 +540,14 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { - const result = await originalImplementation(targetRoomId, targetSessionId); - if (!result.ok && result.error === "NETWORK_ERROR") { - errorDeferred.resolve(); + try { + return await originalImplementation(targetRoomId, targetSessionId); + } catch (err: any) { + if (err.name === "KeyDownloadError") { + errorDeferred.resolve(); + } + throw err; } - return result; }); const keyImported = expectSessionImported("!roomA", "sessionA0"); @@ -534,7 +555,9 @@ describe("PerSessionKeyBackupDownloader", () => { await errorDeferred.promise; await Promise.resolve(); - expect(keyQuerySpy).toHaveReturnedWith(Promise.resolve({ ok: false, error: "NETWORK_ERROR" })); + await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( + "Failed to get key from backup: NETWORK_ERROR", + ); fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, mockCipherKey, { overwriteRoutes: true, @@ -580,9 +603,10 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomA", "sessionA1"); await jest.runAllTimersAsync(); - expect(keyQuerySpy).toHaveLastReturnedWith(Promise.resolve({ ok: false, error: "UNKNOWN_ERROR" })); - await keyImported.promise; + + expect(keyQuerySpy).toHaveBeenCalledTimes(2); + expect(mockRustBackupManager.importRoomKeys).toHaveBeenCalledTimes(2); }); }); }); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 36fcdc995b2..4f80ccfd225 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -28,17 +28,13 @@ import { BackupDecryptor } from "../common-crypto/CryptoBackend"; /** * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. */ -enum KeyDownloadError { +enum KeyDownloadErrorCode { /** The requested key is not in the backup. */ MISSING_DECRYPTION_KEY = "MISSING_DECRYPTION_KEY", /** A network error occurred while trying to get the key. */ NETWORK_ERROR = "NETWORK_ERROR", /** The loop has been stopped. */ STOPPED = "STOPPED", - /** An unknown error occurred while decrypting/importing the key */ - UNKNOWN_ERROR = "UNKNOWN_ERROR", - /** The server is rate limiting us. */ - RATE_LIMITED = "RATE_LIMITED", /** The backup is not configured correctly. * * Example problems can include: @@ -49,14 +45,23 @@ enum KeyDownloadError { CONFIGURATION_ERROR = "CONFIGURATION_ERROR", } +class KeyDownloadError extends Error { + public constructor(public readonly code: KeyDownloadErrorCode) { + super(`Failed to get key from backup: ${code}`); + this.name = "KeyDownloadError"; + } +} + +class KeyDownloadRateLimit extends Error { + public constructor(public readonly retryMillis: number) { + super(`Failed to get key from backup: rate limited`); + this.name = "KeyDownloadRateLimit"; + } +} + /** Details of a megolm session whose key we are trying to fetch. */ type SessionInfo = { roomId: string; megolmSessionId: string }; -/** Helper type for the result of a key download. */ -type KeyDownloadResult = - | { ok: true; value: KeyBackupSession } - | { ok: false; error: KeyDownloadError; [key: string]: any }; - /** Holds the current backup decryptor and version that should be used. */ type Configuration = { backupVersion: string; @@ -177,6 +182,7 @@ export class PerSessionKeyBackupDownloader { this.configuration = null; this.getOrCreateBackupConfiguration(true).then((decryptor) => { if (decryptor) { + // restart the download loop if it was stopped this.downloadKeysLoop(); } }); @@ -260,14 +266,15 @@ export class PerSessionKeyBackupDownloader { // we just peek the first one without removing it, so if a new request for same key comes in while we're // processing this one, it won't queue another request. const request = this.queuedRequests[0]; - const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); - if (this.stopped) { - return; - } - if (result.ok) { + try { + const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); + + if (this.stopped) { + return; + } // We got the encrypted key from backup, let's try to decrypt and import it. try { - await this.decryptAndImport(request, result.value); + await this.decryptAndImport(request, result); } catch (e) { this.logger.error( `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, @@ -276,39 +283,34 @@ export class PerSessionKeyBackupDownloader { } // now remove the request from the queue as we've processed it. this.queuedRequests.shift(); - } else { - this.logger.debug( - `Error while downloading key backup for session ${request.megolmSessionId}: ${result.error}`, - ); - switch (result.error) { - case KeyDownloadError.MISSING_DECRYPTION_KEY: { - this.markAsNotFoundInBackup(request.megolmSessionId); - // continue for next one - this.queuedRequests.shift(); - break; - } - case KeyDownloadError.CONFIGURATION_ERROR: { - // Backup is not configured correctly, so stop the loop. - this.pauseLoop(); - return; + } catch (err) { + if (err instanceof KeyDownloadError) { + switch (err.code) { + case KeyDownloadErrorCode.MISSING_DECRYPTION_KEY: + this.markAsNotFoundInBackup(request.megolmSessionId); + // continue for next one + this.queuedRequests.shift(); + break; + case KeyDownloadErrorCode.NETWORK_ERROR: + // We don't want to hammer if there is a problem, so wait a bit. + await sleep(this.backoffDuration); + break; + case KeyDownloadErrorCode.STOPPED: + // If the downloader was stopped, we don't want to retry. + this.pauseLoop(); + return; + case KeyDownloadErrorCode.CONFIGURATION_ERROR: + // Backup is not configured correctly, so stop the loop. + this.pauseLoop(); + return; } - case KeyDownloadError.RATE_LIMITED: { - // we want to retry - await sleep(result.retryAfterMs ?? this.backoffDuration); - break; - } - case KeyDownloadError.NETWORK_ERROR: { - // We don't want to hammer if there is a problem, so wait a bit. - await sleep(this.backoffDuration); - break; - } - case KeyDownloadError.STOPPED: - // If the downloader was stopped, we don't want to retry. - this.pauseLoop(); - return; + } else if (err instanceof KeyDownloadRateLimit) { + // we want to retry after the backoff time + await sleep(err.retryMillis); } } } + // all pending request have been processed, we can stop the loop. this.pauseLoop(); } @@ -318,24 +320,20 @@ export class PerSessionKeyBackupDownloader { * @param targetRoomId - ID of the room that the session is used in. * @param targetSessionId - ID of the session for which to check backup. */ - private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { + private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { const configuration = await this.getOrCreateBackupConfiguration(false); if (!configuration) { - return { ok: false, error: KeyDownloadError.CONFIGURATION_ERROR }; + throw new KeyDownloadError(KeyDownloadErrorCode.CONFIGURATION_ERROR); } this.logger.debug(`Checking key backup for session ${targetSessionId}`); - + if (this.stopped) throw new KeyDownloadError(KeyDownloadErrorCode.STOPPED); try { const res = await this.requestRoomKeyFromBackup(configuration.backupVersion, targetRoomId, targetSessionId); - if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; - this.logger.debug(`<-- Got key from backup ${targetSessionId}`); - return { - ok: true, - value: res, - }; + this.logger.debug(`Got key from backup for sessionId:${targetSessionId}`); + return res; } catch (e) { - if (this.stopped) return { ok: false, error: KeyDownloadError.STOPPED }; + if (this.stopped) throw new KeyDownloadError(KeyDownloadErrorCode.STOPPED); this.logger.info(`No luck requesting key backup for session ${targetSessionId}: ${e}`); if (e instanceof MatrixError) { @@ -347,26 +345,22 @@ export class PerSessionKeyBackupDownloader { // - "error": "No room_keys found" if the key is missing. // For now we check the error message, but this is not ideal. // It's useful to know if the key is missing or if the version is wrong. - // As it's not spec'ed, we fallback on considering the key has not in backup, - // notice that this request will be lost if the backup is not configured correctly. - return { ok: false, error: KeyDownloadError.MISSING_DECRYPTION_KEY }; + // As it's not spec'ed, we fall back on considering the key is not in backup, + // notice that this request will be lost if instead the backup got out of sync (updated from other session). + throw new KeyDownloadError(KeyDownloadErrorCode.MISSING_DECRYPTION_KEY); } if (errCode == "M_LIMIT_EXCEEDED") { const waitTime = e.data.retry_after_ms; if (waitTime > 0) { this.logger.info(`Rate limited by server, waiting ${waitTime}ms`); - return { ok: false, error: KeyDownloadError.RATE_LIMITED, retryAfterMs: waitTime }; + throw new KeyDownloadRateLimit(waitTime); } else { - // apply a backoff time - return { - ok: false, - error: KeyDownloadError.RATE_LIMITED, - retryAfterMs: this.backoffDuration, - }; + // apply the default backoff time + throw new KeyDownloadRateLimit(this.backoffDuration); } } } - return { ok: false, error: KeyDownloadError.NETWORK_ERROR }; + throw new KeyDownloadError(KeyDownloadErrorCode.NETWORK_ERROR); } } @@ -394,7 +388,7 @@ export class PerSessionKeyBackupDownloader { * * @param forceCheck - If true, force a check of the backup configuration. * - * @returns The current backup configuration or null if there is a configuration problem. + * @returns The backup configuration to use or null if there is a configuration problem. */ private async getOrCreateBackupConfiguration(forceCheck: boolean): Promise { if (this.configuration) { From 2245bc2f4b789a350a870203865dbcfd1fcf0dae Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 12:18:36 +0100 Subject: [PATCH 15/24] cleaning --- spec/integ/crypto/megolm-backup.spec.ts | 4 ---- .../unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts | 2 -- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 7 +++---- 3 files changed, 3 insertions(+), 10 deletions(-) diff --git a/spec/integ/crypto/megolm-backup.spec.ts b/spec/integ/crypto/megolm-backup.spec.ts index e8f7b84a544..d7f8644c8e0 100644 --- a/spec/integ/crypto/megolm-backup.spec.ts +++ b/spec/integ/crypto/megolm-backup.spec.ts @@ -921,10 +921,6 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe }; it("If current backup has changed, the manager should switch to the new one on UTD", async () => { - // fetchMock.get("path:/_matrix/client/v3/room_keys/version", testData.SIGNED_BACKUP_DATA); - - // aliceClient = await initTestClient(); - // ===== // First ensure that the client checks for keys using the backup version 1 /// ===== diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 0bf46cf6f0a..38366a55a66 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -296,8 +296,6 @@ describe("PerSessionKeyBackupDownloader", () => { }); describe("Given no usable backup available", () => { - // let loopPausedPromise: Promise; - // let configurationErrorPromise: Promise; let keyQuerySpy: SpyInstance; beforeEach(async () => { diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 4f80ccfd225..a056e87ef77 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -31,7 +31,7 @@ import { BackupDecryptor } from "../common-crypto/CryptoBackend"; enum KeyDownloadErrorCode { /** The requested key is not in the backup. */ MISSING_DECRYPTION_KEY = "MISSING_DECRYPTION_KEY", - /** A network error occurred while trying to get the key. */ + /** A network error occurred while trying to download the key from backup. */ NETWORK_ERROR = "NETWORK_ERROR", /** The loop has been stopped. */ STOPPED = "STOPPED", @@ -343,10 +343,9 @@ export class PerSessionKeyBackupDownloader { // Synapse will return: // - "error": "Unknown backup version" if the version is wrong. // - "error": "No room_keys found" if the key is missing. - // For now we check the error message, but this is not ideal. // It's useful to know if the key is missing or if the version is wrong. - // As it's not spec'ed, we fall back on considering the key is not in backup, - // notice that this request will be lost if instead the backup got out of sync (updated from other session). + // As it's not spec'ed, we fall back on considering the key is not in backup. + // Notice that this request will be lost if instead the backup got out of sync (updated from other session). throw new KeyDownloadError(KeyDownloadErrorCode.MISSING_DECRYPTION_KEY); } if (errCode == "M_LIMIT_EXCEEDED") { From c0cd300eb49e46adfb3818cc860b887f3b90569f Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:08:16 +0100 Subject: [PATCH 16/24] review --- .../PerSessionKeyBackupDownloader.spec.ts | 29 +++++-------------- .../PerSessionKeyBackupDownloader.ts | 18 ++++++------ src/rust-crypto/rust-crypto.ts | 3 -- 3 files changed, 16 insertions(+), 34 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 38366a55a66..42be5eeb4b3 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -44,7 +44,8 @@ describe("PerSessionKeyBackupDownloader", () => { const mockCipherKey: Mocked = {} as unknown as Mocked; - const BACKOFF_TIME = 2000; + // matches the const in PerSessionKeyBackupDownloader + const BACKOFF_TIME = 5000; const mockEmitter = new TypedEventEmitter() as TypedEventEmitter; @@ -104,13 +105,7 @@ describe("PerSessionKeyBackupDownloader", () => { getBackupKeys: jest.fn(), } as unknown as Mocked; - downloader = new PerSessionKeyBackupDownloader( - logger, - mockOlmMachine, - mockHttp, - mockRustBackupManager, - BACKOFF_TIME, - ); + downloader = new PerSessionKeyBackupDownloader(logger, mockOlmMachine, mockHttp, mockRustBackupManager); expectedSession = {}; mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { @@ -268,12 +263,10 @@ describe("PerSessionKeyBackupDownloader", () => { const blockOnServerRequest = defer(); const requestRoomKeyCalled = defer(); - let callCount = 0; // Mock the request to block fetchMock.get(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`, async (url, request) => { requestRoomKeyCalled.resolve(); await blockOnServerRequest.promise; - callCount++; return mockCipherKey; }); @@ -291,7 +284,9 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); expect(mockRustBackupManager.importRoomKeys).not.toHaveBeenCalled(); - expect(callCount).toStrictEqual(1); + expect( + fetchMock.calls(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`).length, + ).toStrictEqual(1); }); }); @@ -306,10 +301,6 @@ describe("PerSessionKeyBackupDownloader", () => { keyQuerySpy = jest.spyOn(downloader, "queryKeyBackup"); }); - afterEach(async () => { - fetchMock.mockClear(); - }); - it("Should not query server if no backup", async () => { fetchMock.get("path:/_matrix/client/v3/room_keys/version", { status: 404, @@ -397,7 +388,7 @@ describe("PerSessionKeyBackupDownloader", () => { }); describe("Given Backup state update", () => { - it("After initial sync, when backup become trusted it should request keys for past requests", async () => { + it("After initial sync, when backup becomes trusted it should request keys for past requests", async () => { // there is a backup mockRustBackupManager.requestKeyBackupVersion.mockResolvedValue(TestData.SIGNED_BACKUP_DATA); @@ -453,12 +444,6 @@ describe("PerSessionKeyBackupDownloader", () => { backupVersion: TestData.SIGNED_BACKUP_DATA.version!, decryptionKey: RustSdkCryptoJs.BackupDecryptionKey.fromBase64(TestData.BACKUP_DECRYPTION_KEY_BASE64), } as unknown as RustSdkCryptoJs.BackupKeys); - - jest.useFakeTimers(); - }); - - afterEach(() => { - jest.useRealTimers(); }); it("Should wait on rate limit error", async () => { diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index a056e87ef77..ffbb33afdef 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -25,6 +25,9 @@ import { CryptoEvent } from "../matrix"; import { encodeUri, sleep } from "../utils"; import { BackupDecryptor } from "../common-crypto/CryptoBackend"; +// The minimum time to wait between two retries in case of errors. To avoid hammering the server. +const KEY_BACKUP_BACKOFF = 5000; // ms + /** * Enumerates the different kind of errors that can occurs when downloading and importing a key from backup. */ @@ -110,15 +113,12 @@ export class PerSessionKeyBackupDownloader { * @param olmMachine - The olm machine to use. * @param http - The http instance to use. * @param logger - The logger to use. - * @param backoffDuration - The minimum time to wait between two retries in case of errors. To avoid hammering the server. - * */ public constructor( logger: Logger, private readonly olmMachine: OlmMachine, private readonly http: MatrixHttpApi, private readonly backupManager: RustBackupManager, - private readonly backoffDuration: number, ) { this.logger = logger.getChild("[PerSessionKeyBackupDownloader]"); @@ -206,7 +206,7 @@ export class PerSessionKeyBackupDownloader { if (this.sessionLastCheckAttemptedTime.size > 100) { this.sessionLastCheckAttemptedTime = new Map( Array.from(this.sessionLastCheckAttemptedTime).filter((sid, ts) => { - return Math.max(now - ts, 0) < this.backoffDuration; + return Math.max(now - ts, 0) < KEY_BACKUP_BACKOFF; }), ); } @@ -216,7 +216,7 @@ export class PerSessionKeyBackupDownloader { private wasRequestedRecently(megolmSessionId: string): boolean { const lastCheck = this.sessionLastCheckAttemptedTime.get(megolmSessionId); if (!lastCheck) return false; - return Math.max(Date.now() - lastCheck, 0) < this.backoffDuration; + return Math.max(Date.now() - lastCheck, 0) < KEY_BACKUP_BACKOFF; } private pauseLoop(): void { @@ -233,10 +233,10 @@ export class PerSessionKeyBackupDownloader { /** * Requests a key from the server side backup. + * * @param version - The backup version to use. * @param roomId - The room ID of the room where the error occurred. * @param sessionId - The megolm session ID that is missing. - * */ private async requestRoomKeyFromBackup( version: string, @@ -293,7 +293,7 @@ export class PerSessionKeyBackupDownloader { break; case KeyDownloadErrorCode.NETWORK_ERROR: // We don't want to hammer if there is a problem, so wait a bit. - await sleep(this.backoffDuration); + await sleep(KEY_BACKUP_BACKOFF); break; case KeyDownloadErrorCode.STOPPED: // If the downloader was stopped, we don't want to retry. @@ -355,7 +355,7 @@ export class PerSessionKeyBackupDownloader { throw new KeyDownloadRateLimit(waitTime); } else { // apply the default backoff time - throw new KeyDownloadRateLimit(this.backoffDuration); + throw new KeyDownloadRateLimit(KEY_BACKUP_BACKOFF); } } } @@ -383,7 +383,7 @@ export class PerSessionKeyBackupDownloader { * Gets the current backup configuration or create one if it doesn't exist. * * When a valid configuration is found it is cached and returned for subsequent calls. - * If a check is forced or a check has not yet been done, a new check is done. + * Otherwise, if a check is forced or a check has not yet been done, a new check is done. * * @param forceCheck - If true, force a check of the backup configuration. * diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 44e236cde88..19b8a7af084 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -81,8 +81,6 @@ interface ISignableObject { unsigned?: object; } -const KEY_BACKUP_BACKOFF = 5000; // ms - /** * An implementation of {@link CryptoBackend} using the Rust matrix-sdk-crypto. * @@ -150,7 +148,6 @@ export class RustCrypto extends TypedEventEmitter Date: Thu, 7 Dec 2023 18:09:03 +0100 Subject: [PATCH 17/24] remove forceCheck as not useful --- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index ffbb33afdef..8d8ae6ccec2 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -177,10 +177,10 @@ export class PerSessionKeyBackupDownloader { * This will trigger a check of the backup configuration. */ private onBackupStatusChanged = (): void => { - // we want to check configuration + // we want to force check configuration, so we clear the current one. this.hasConfigurationProblem = false; this.configuration = null; - this.getOrCreateBackupConfiguration(true).then((decryptor) => { + this.getOrCreateBackupConfiguration().then((decryptor) => { if (decryptor) { // restart the download loop if it was stopped this.downloadKeysLoop(); @@ -321,7 +321,7 @@ export class PerSessionKeyBackupDownloader { * @param targetSessionId - ID of the session for which to check backup. */ private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { - const configuration = await this.getOrCreateBackupConfiguration(false); + const configuration = await this.getOrCreateBackupConfiguration(); if (!configuration) { throw new KeyDownloadError(KeyDownloadErrorCode.CONFIGURATION_ERROR); } @@ -364,7 +364,7 @@ export class PerSessionKeyBackupDownloader { } private async decryptAndImport(sessionInfo: SessionInfo, data: KeyBackupSession): Promise { - const configuration = await this.getOrCreateBackupConfiguration(false); + const configuration = await this.getOrCreateBackupConfiguration(); if (!configuration) { throw new Error("Backup: No configuration"); @@ -385,16 +385,16 @@ export class PerSessionKeyBackupDownloader { * When a valid configuration is found it is cached and returned for subsequent calls. * Otherwise, if a check is forced or a check has not yet been done, a new check is done. * - * @param forceCheck - If true, force a check of the backup configuration. - * * @returns The backup configuration to use or null if there is a configuration problem. */ - private async getOrCreateBackupConfiguration(forceCheck: boolean): Promise { + private async getOrCreateBackupConfiguration(): Promise { if (this.configuration) { return this.configuration; } - if (this.hasConfigurationProblem && !forceCheck) { + // We already tried to check the configuration and it failed. + // We don't want to try again immediately, we will retry if a configuration change is detected. + if (this.hasConfigurationProblem) { return null; } From 5418dc0858720696c72c4b8016546031e72fb6ff Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:11:06 +0100 Subject: [PATCH 18/24] bad naming --- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 8d8ae6ccec2..6e43c50ce5e 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -180,8 +180,8 @@ export class PerSessionKeyBackupDownloader { // we want to force check configuration, so we clear the current one. this.hasConfigurationProblem = false; this.configuration = null; - this.getOrCreateBackupConfiguration().then((decryptor) => { - if (decryptor) { + this.getOrCreateBackupConfiguration().then((configuration) => { + if (configuration) { // restart the download loop if it was stopped this.downloadKeysLoop(); } From 9d054760c66dae9ebc7ac8b2e0e0bb4d7da9695f Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:11:34 +0100 Subject: [PATCH 19/24] inline pauseLoop --- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 6e43c50ce5e..5d3b1f3ad72 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -219,10 +219,6 @@ export class PerSessionKeyBackupDownloader { return Math.max(Date.now() - lastCheck, 0) < KEY_BACKUP_BACKOFF; } - private pauseLoop(): void { - this.downloadLoopRunning = false; - } - private async getBackupDecryptionKey(): Promise { try { return await this.olmMachine.getBackupKeys(); @@ -297,11 +293,11 @@ export class PerSessionKeyBackupDownloader { break; case KeyDownloadErrorCode.STOPPED: // If the downloader was stopped, we don't want to retry. - this.pauseLoop(); + this.downloadLoopRunning = false; return; case KeyDownloadErrorCode.CONFIGURATION_ERROR: // Backup is not configured correctly, so stop the loop. - this.pauseLoop(); + this.downloadLoopRunning = false; return; } } else if (err instanceof KeyDownloadRateLimit) { @@ -311,7 +307,7 @@ export class PerSessionKeyBackupDownloader { } } // all pending request have been processed, we can stop the loop. - this.pauseLoop(); + this.downloadLoopRunning = false; } /** From 1545127623cc028d2a5fef034cf40d80f4e0116a Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:13:32 +0100 Subject: [PATCH 20/24] mark as paused in finally --- .../PerSessionKeyBackupDownloader.ts | 93 ++++++++++--------- 1 file changed, 48 insertions(+), 45 deletions(-) diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 5d3b1f3ad72..3eb302376fc 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -258,56 +258,59 @@ export class PerSessionKeyBackupDownloader { this.downloadLoopRunning = true; - while (this.queuedRequests.length > 0) { - // we just peek the first one without removing it, so if a new request for same key comes in while we're - // processing this one, it won't queue another request. - const request = this.queuedRequests[0]; - try { - const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); - - if (this.stopped) { - return; - } - // We got the encrypted key from backup, let's try to decrypt and import it. + try { + while (this.queuedRequests.length > 0) { + // we just peek the first one without removing it, so if a new request for same key comes in while we're + // processing this one, it won't queue another request. + const request = this.queuedRequests[0]; try { - await this.decryptAndImport(request, result); - } catch (e) { - this.logger.error( - `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, - e, - ); - } - // now remove the request from the queue as we've processed it. - this.queuedRequests.shift(); - } catch (err) { - if (err instanceof KeyDownloadError) { - switch (err.code) { - case KeyDownloadErrorCode.MISSING_DECRYPTION_KEY: - this.markAsNotFoundInBackup(request.megolmSessionId); - // continue for next one - this.queuedRequests.shift(); - break; - case KeyDownloadErrorCode.NETWORK_ERROR: - // We don't want to hammer if there is a problem, so wait a bit. - await sleep(KEY_BACKUP_BACKOFF); - break; - case KeyDownloadErrorCode.STOPPED: - // If the downloader was stopped, we don't want to retry. - this.downloadLoopRunning = false; - return; - case KeyDownloadErrorCode.CONFIGURATION_ERROR: - // Backup is not configured correctly, so stop the loop. - this.downloadLoopRunning = false; - return; + const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); + + if (this.stopped) { + return; + } + // We got the encrypted key from backup, let's try to decrypt and import it. + try { + await this.decryptAndImport(request, result); + } catch (e) { + this.logger.error( + `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, + e, + ); + } + // now remove the request from the queue as we've processed it. + this.queuedRequests.shift(); + } catch (err) { + if (err instanceof KeyDownloadError) { + switch (err.code) { + case KeyDownloadErrorCode.MISSING_DECRYPTION_KEY: + this.markAsNotFoundInBackup(request.megolmSessionId); + // continue for next one + this.queuedRequests.shift(); + break; + case KeyDownloadErrorCode.NETWORK_ERROR: + // We don't want to hammer if there is a problem, so wait a bit. + await sleep(KEY_BACKUP_BACKOFF); + break; + case KeyDownloadErrorCode.STOPPED: + // If the downloader was stopped, we don't want to retry. + this.downloadLoopRunning = false; + return; + case KeyDownloadErrorCode.CONFIGURATION_ERROR: + // Backup is not configured correctly, so stop the loop. + this.downloadLoopRunning = false; + return; + } + } else if (err instanceof KeyDownloadRateLimit) { + // we want to retry after the backoff time + await sleep(err.retryMillis); } - } else if (err instanceof KeyDownloadRateLimit) { - // we want to retry after the backoff time - await sleep(err.retryMillis); } } + } finally { + // all pending request have been processed, we can stop the loop. + this.downloadLoopRunning = false; } - // all pending request have been processed, we can stop the loop. - this.downloadLoopRunning = false; } /** From e890727219d2ccdccc83202adc3294ae0abd26a5 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:20:08 +0100 Subject: [PATCH 21/24] code review --- .../PerSessionKeyBackupDownloader.spec.ts | 22 +++++++++---------- .../PerSessionKeyBackupDownloader.ts | 7 +++--- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 42be5eeb4b3..b44c2e3078a 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -47,8 +47,7 @@ describe("PerSessionKeyBackupDownloader", () => { // matches the const in PerSessionKeyBackupDownloader const BACKOFF_TIME = 5000; - const mockEmitter = new TypedEventEmitter() as TypedEventEmitter; - + let mockEmitter; let mockHttp: MatrixHttpApi; let mockRustBackupManager: Mocked; let mockOlmMachine: Mocked; @@ -56,7 +55,7 @@ describe("PerSessionKeyBackupDownloader", () => { let expectedSession: { [roomId: string]: { [sessionId: string]: IDeferred } }; - async function expectSessionImported(roomId: string, sessionId: string) { + function expectSessionImported(roomId: string, sessionId: string) { const deferred = defer(); if (!expectedSession[roomId]) { expectedSession[roomId] = {}; @@ -72,6 +71,8 @@ describe("PerSessionKeyBackupDownloader", () => { } beforeEach(async () => { + mockEmitter = new TypedEventEmitter() as TypedEventEmitter; + mockHttp = new MatrixHttpApi(new TypedEventEmitter(), { baseUrl: "http://server/", prefix: "", @@ -89,7 +90,6 @@ describe("PerSessionKeyBackupDownloader", () => { mockRustBackupManager = { getActiveBackupVersion: jest.fn(), - getBackupDecryptionKey: jest.fn(), requestKeyBackupVersion: jest.fn(), importRoomKeys: jest.fn(), createBackupDecryptor: jest.fn().mockReturnValue(mockBackupDecryptor), @@ -176,7 +176,7 @@ describe("PerSessionKeyBackupDownloader", () => { } }); - // @ts-ignore + // @ts-ignore access to private function const spy = jest.spyOn(downloader, "queryKeyBackup"); // Call 3 times for same key @@ -204,7 +204,7 @@ describe("PerSessionKeyBackupDownloader", () => { }); fetchMock.get(`path:/_matrix/client/v3/room_keys/keys/!roomA/sessionA1`, mockCipherKey); - // @ts-ignore + // @ts-ignore access to private function const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const expectImported = expectSessionImported("!roomA", "sessionA1"); @@ -230,7 +230,7 @@ describe("PerSessionKeyBackupDownloader", () => { }, }); - // @ts-ignore + // @ts-ignore access to private function const spy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); @@ -297,7 +297,7 @@ describe("PerSessionKeyBackupDownloader", () => { mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); mockOlmMachine.getBackupKeys.mockResolvedValue(null); - // @ts-ignore + // @ts-ignore access to private function keyQuerySpy = jest.spyOn(downloader, "queryKeyBackup"); }); @@ -410,7 +410,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomC", "sessionC1"); await jest.runAllTimersAsync(); - // @ts-ignore + // @ts-ignore access to private property expect(downloader.hasConfigurationProblem).toEqual(true); // Now the backup becomes trusted @@ -466,7 +466,7 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore const originalImplementation = downloader.queryKeyBackup.bind(downloader); - // @ts-ignore + // @ts-ignore access to private function const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const rateDeferred = defer(); // @ts-ignore @@ -579,7 +579,7 @@ describe("PerSessionKeyBackupDownloader", () => { overwriteRoutes: true, }); - // @ts-ignore + // @ts-ignore access to private function const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 3eb302376fc..1dea2cf5e28 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -84,7 +84,7 @@ type Configuration = { export class PerSessionKeyBackupDownloader { private stopped = false; - /** The version and decryption key to use with current backup if all setup correctly */ + /** The version and decryption key to use with current backup if all set up correctly */ private configuration: Configuration | null = null; /** We remember when a session was requested and not found in backup to avoid query again too soon. @@ -100,7 +100,7 @@ export class PerSessionKeyBackupDownloader { /** The list of requests that are queued. */ private queuedRequests: SessionInfo[] = []; - // Remembers if we have a configuration problem. + /** Remembers if we have a configuration problem. */ private hasConfigurationProblem = false; /** The current server backup version check promise. To avoid doing a server call if one is in flight. */ @@ -197,8 +197,9 @@ export class PerSessionKeyBackupDownloader { /** * Marks the session as not found in backup, to avoid retrying to soon for a key not in backup + * * @param megolmSessionId - The megolm session ID that is missing. - * */ + */ private markAsNotFoundInBackup(megolmSessionId: string): void { const now = Date.now(); this.sessionLastCheckAttemptedTime.set(megolmSessionId, now); From b244fd0cc1e62aeb1a71588fa07bc186b55b5ac9 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:29:39 +0100 Subject: [PATCH 22/24] post merge fix --- .../PerSessionKeyBackupDownloader.spec.ts | 16 ++++++------- .../PerSessionKeyBackupDownloader.ts | 2 +- src/rust-crypto/backup.ts | 23 +++++++++++++++++++ src/rust-crypto/rust-crypto.ts | 18 +-------------- 4 files changed, 33 insertions(+), 26 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index b44c2e3078a..28590c29f24 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -47,7 +47,7 @@ describe("PerSessionKeyBackupDownloader", () => { // matches the const in PerSessionKeyBackupDownloader const BACKOFF_TIME = 5000; - let mockEmitter; + let mockEmitter: TypedEventEmitter; let mockHttp: MatrixHttpApi; let mockRustBackupManager: Mocked; let mockOlmMachine: Mocked; @@ -91,7 +91,7 @@ describe("PerSessionKeyBackupDownloader", () => { mockRustBackupManager = { getActiveBackupVersion: jest.fn(), requestKeyBackupVersion: jest.fn(), - importRoomKeys: jest.fn(), + importBackedUpRoomKeys: jest.fn(), createBackupDecryptor: jest.fn().mockReturnValue(mockBackupDecryptor), on: jest.fn().mockImplementation((event, listener) => { mockEmitter.on(event, listener); @@ -108,7 +108,7 @@ describe("PerSessionKeyBackupDownloader", () => { downloader = new PerSessionKeyBackupDownloader(logger, mockOlmMachine, mockHttp, mockRustBackupManager); expectedSession = {}; - mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { + mockRustBackupManager.importBackedUpRoomKeys.mockImplementation(async (keys) => { const roomId = keys[0].room_id; const sessionId = keys[0].session_id; const deferred = expectedSession[roomId] && expectedSession[roomId][sessionId]; @@ -148,7 +148,7 @@ describe("PerSessionKeyBackupDownloader", () => { return TestData.CURVE25519_KEY_BACKUP_DATA; }); }); - mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { + mockRustBackupManager.importBackedUpRoomKeys.mockImplementation(async (keys) => { awaitKeyImported.resolve(); }); mockBackupDecryptor.decryptSessions.mockResolvedValue([TestData.MEGOLM_SESSION_DATA]); @@ -170,7 +170,7 @@ describe("PerSessionKeyBackupDownloader", () => { const awaitKey2Imported = defer(); - mockRustBackupManager.importRoomKeys.mockImplementation(async (keys) => { + mockRustBackupManager.importBackedUpRoomKeys.mockImplementation(async (keys) => { if (keys[0].session_id === "sessionId2") { awaitKey2Imported.resolve(); } @@ -283,7 +283,7 @@ describe("PerSessionKeyBackupDownloader", () => { // let the first request complete await jest.runAllTimersAsync(); - expect(mockRustBackupManager.importRoomKeys).not.toHaveBeenCalled(); + expect(mockRustBackupManager.importBackedUpRoomKeys).not.toHaveBeenCalled(); expect( fetchMock.calls(`express:/_matrix/client/v3/room_keys/keys/:roomId/:sessionId`).length, ).toStrictEqual(1); @@ -562,7 +562,7 @@ describe("PerSessionKeyBackupDownloader", () => { it("On Unknown error on import skip the key and continue", async () => { const keyImported = defer(); - mockRustBackupManager.importRoomKeys + mockRustBackupManager.importBackedUpRoomKeys .mockImplementationOnce(async () => { throw new Error("Didn't work"); }) @@ -589,7 +589,7 @@ describe("PerSessionKeyBackupDownloader", () => { await keyImported.promise; expect(keyQuerySpy).toHaveBeenCalledTimes(2); - expect(mockRustBackupManager.importRoomKeys).toHaveBeenCalledTimes(2); + expect(mockRustBackupManager.importBackedUpRoomKeys).toHaveBeenCalledTimes(2); }); }); }); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 1dea2cf5e28..af6c52b1d62 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -376,7 +376,7 @@ export class PerSessionKeyBackupDownloader { for (const k of keys) { k.room_id = sessionInfo.roomId; } - await this.backupManager.importRoomKeys(keys); + await this.backupManager.importBackedUpRoomKeys(keys); } /** diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index f25bda4ab39..1b3f8891040 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -194,6 +194,29 @@ export class RustBackupManager extends TypedEventEmitter { + const keysByRoom: Map> = new Map(); + for (const key of keys) { + const roomId = new RustSdkCryptoJs.RoomId(key.room_id); + if (!keysByRoom.has(roomId)) { + keysByRoom.set(roomId, new Map()); + } + keysByRoom.get(roomId)!.set(key.session_id, key); + } + await this.olmMachine.importBackedUpRoomKeys(keysByRoom, (progress: BigInt, total: BigInt): void => { + const importOpt: ImportRoomKeyProgressData = { + total: Number(total), + successes: Number(progress), + stage: "load_keys", + failures: 0, + }; + opts?.progressCallback?.(importOpt); + }); + } + private keyBackupCheckInProgress: Promise | null = null; /** Helper for `checkKeyBackup` */ diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 19b8a7af084..833de7785dc 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1197,23 +1197,7 @@ export class RustCrypto extends TypedEventEmitter { - const keysByRoom: Map> = new Map(); - for (const key of keys) { - const roomId = new RustSdkCryptoJs.RoomId(key.room_id); - if (!keysByRoom.has(roomId)) { - keysByRoom.set(roomId, new Map()); - } - keysByRoom.get(roomId)!.set(key.session_id, key); - } - await this.olmMachine.importBackedUpRoomKeys(keysByRoom, (progress: BigInt, total: BigInt): void => { - const importOpt: ImportRoomKeyProgressData = { - total: Number(total), - successes: Number(progress), - stage: "load_keys", - failures: 0, - }; - opts?.progressCallback?.(importOpt); - }); + return await this.backupManager.importBackedUpRoomKeys(keys, opts); } /////////////////////////////////////////////////////////////////////////////////////////////////////////////////// From b34f15f8cf223724a54e8fe82424749bb28a104d Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:34:28 +0100 Subject: [PATCH 23/24] rename KeyDownloadRateLimit --- .../rust-crypto/PerSessionKeyBackupDownloader.spec.ts | 2 +- src/rust-crypto/PerSessionKeyBackupDownloader.ts | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 28590c29f24..0833f6d8ac5 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -474,7 +474,7 @@ describe("PerSessionKeyBackupDownloader", () => { try { return await originalImplementation(targetRoomId, targetSessionId); } catch (err: any) { - if (err.name === "KeyDownloadRateLimit") { + if (err.name === "KeyDownloadRateLimitError") { rateDeferred.resolve(); } throw err; diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index af6c52b1d62..1d765a60a61 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -55,10 +55,10 @@ class KeyDownloadError extends Error { } } -class KeyDownloadRateLimit extends Error { +class KeyDownloadRateLimitError extends Error { public constructor(public readonly retryMillis: number) { super(`Failed to get key from backup: rate limited`); - this.name = "KeyDownloadRateLimit"; + this.name = "KeyDownloadRateLimitError"; } } @@ -302,7 +302,7 @@ export class PerSessionKeyBackupDownloader { this.downloadLoopRunning = false; return; } - } else if (err instanceof KeyDownloadRateLimit) { + } else if (err instanceof KeyDownloadRateLimitError) { // we want to retry after the backoff time await sleep(err.retryMillis); } @@ -352,10 +352,10 @@ export class PerSessionKeyBackupDownloader { const waitTime = e.data.retry_after_ms; if (waitTime > 0) { this.logger.info(`Rate limited by server, waiting ${waitTime}ms`); - throw new KeyDownloadRateLimit(waitTime); + throw new KeyDownloadRateLimitError(waitTime); } else { // apply the default backoff time - throw new KeyDownloadRateLimit(KEY_BACKUP_BACKOFF); + throw new KeyDownloadRateLimitError(KEY_BACKUP_BACKOFF); } } } From 336e9f688a5c96d2f81b39d72453c45f888b3b4b Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 7 Dec 2023 18:58:08 +0100 Subject: [PATCH 24/24] use same config in loop and pass along --- .../PerSessionKeyBackupDownloader.spec.ts | 77 ++++++++++--------- .../PerSessionKeyBackupDownloader.ts | 50 ++++++------ 2 files changed, 62 insertions(+), 65 deletions(-) diff --git a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts index 0833f6d8ac5..8b1b0f75c2c 100644 --- a/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts +++ b/spec/unit/rust-crypto/PerSessionKeyBackupDownloader.spec.ts @@ -291,14 +291,14 @@ describe("PerSessionKeyBackupDownloader", () => { }); describe("Given no usable backup available", () => { - let keyQuerySpy: SpyInstance; + let getConfigSpy: SpyInstance; beforeEach(async () => { mockRustBackupManager.getActiveBackupVersion.mockResolvedValue(null); mockOlmMachine.getBackupKeys.mockResolvedValue(null); // @ts-ignore access to private function - keyQuerySpy = jest.spyOn(downloader, "queryKeyBackup"); + getConfigSpy = jest.spyOn(downloader, "getOrCreateBackupConfiguration"); }); it("Should not query server if no backup", async () => { @@ -310,9 +310,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( - "Failed to get key from backup: CONFIGURATION_ERROR", - ); + + expect(getConfigSpy).toHaveBeenCalledTimes(1); + expect(getConfigSpy).toHaveReturnedWith(Promise.resolve(null)); }); it("Should not query server if backup not active", async () => { @@ -325,9 +325,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( - "Failed to get key from backup: CONFIGURATION_ERROR", - ); + + expect(getConfigSpy).toHaveBeenCalledTimes(1); + expect(getConfigSpy).toHaveReturnedWith(Promise.resolve(null)); }); it("Should stop if backup key is not cached", async () => { @@ -341,9 +341,9 @@ describe("PerSessionKeyBackupDownloader", () => { downloader.onDecryptionKeyMissingError("!roomId", "sessionId"); await jest.runAllTimersAsync(); - await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( - "Failed to get key from backup: CONFIGURATION_ERROR", - ); + + expect(getConfigSpy).toHaveBeenCalledTimes(1); + expect(getConfigSpy).toHaveReturnedWith(Promise.resolve(null)); }); it("Should stop if backup key cached as wrong version", async () => { @@ -361,9 +361,8 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); - await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( - "Failed to get key from backup: CONFIGURATION_ERROR", - ); + expect(getConfigSpy).toHaveBeenCalledTimes(1); + expect(getConfigSpy).toHaveReturnedWith(Promise.resolve(null)); }); it("Should stop if backup key version does not match the active one", async () => { @@ -381,9 +380,8 @@ describe("PerSessionKeyBackupDownloader", () => { await jest.runAllTimersAsync(); - await expect(keyQuerySpy.mock.results[0].value).rejects.toThrow( - "Failed to get key from backup: CONFIGURATION_ERROR", - ); + expect(getConfigSpy).toHaveBeenCalledTimes(1); + expect(getConfigSpy).toHaveReturnedWith(Promise.resolve(null)); }); }); @@ -469,17 +467,20 @@ describe("PerSessionKeyBackupDownloader", () => { // @ts-ignore access to private function const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const rateDeferred = defer(); - // @ts-ignore - keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { - try { - return await originalImplementation(targetRoomId, targetSessionId); - } catch (err: any) { - if (err.name === "KeyDownloadRateLimitError") { - rateDeferred.resolve(); + + keyQuerySpy.mockImplementation( + // @ts-ignore + async (targetRoomId: string, targetSessionId: string, configuration: any) => { + try { + return await originalImplementation(targetRoomId, targetSessionId, configuration); + } catch (err: any) { + if (err.name === "KeyDownloadRateLimitError") { + rateDeferred.resolve(); + } + throw err; } - throw err; - } - }); + }, + ); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); await rateDeferred.promise; @@ -521,17 +522,19 @@ describe("PerSessionKeyBackupDownloader", () => { const keyQuerySpy: SpyInstance = jest.spyOn(downloader, "queryKeyBackup"); const errorDeferred = defer(); - // @ts-ignore - keyQuerySpy.mockImplementation(async (targetRoomId: string, targetSessionId: string) => { - try { - return await originalImplementation(targetRoomId, targetSessionId); - } catch (err: any) { - if (err.name === "KeyDownloadError") { - errorDeferred.resolve(); + keyQuerySpy.mockImplementation( + // @ts-ignore + async (targetRoomId: string, targetSessionId: string, configuration: any) => { + try { + return await originalImplementation(targetRoomId, targetSessionId, configuration); + } catch (err: any) { + if (err.name === "KeyDownloadError") { + errorDeferred.resolve(); + } + throw err; } - throw err; - } - }); + }, + ); const keyImported = expectSessionImported("!roomA", "sessionA0"); downloader.onDecryptionKeyMissingError("!roomA", "sessionA0"); diff --git a/src/rust-crypto/PerSessionKeyBackupDownloader.ts b/src/rust-crypto/PerSessionKeyBackupDownloader.ts index 1d765a60a61..c8283c65488 100644 --- a/src/rust-crypto/PerSessionKeyBackupDownloader.ts +++ b/src/rust-crypto/PerSessionKeyBackupDownloader.ts @@ -38,14 +38,6 @@ enum KeyDownloadErrorCode { NETWORK_ERROR = "NETWORK_ERROR", /** The loop has been stopped. */ STOPPED = "STOPPED", - /** The backup is not configured correctly. - * - * Example problems can include: - * * There is no backup - * * Backup is not trusted - * * We don't have the correct key in cache - */ - CONFIGURATION_ERROR = "CONFIGURATION_ERROR", } class KeyDownloadError extends Error { @@ -132,7 +124,7 @@ export class PerSessionKeyBackupDownloader { * * This will try to download the key from the backup if there is a trusted active backup. * In case of success the key will be imported and the onRoomKeysUpdated callback will be called - * internally by the rust-sdk and decrytion will be retried. + * internally by the rust-sdk and decryption will be retried. * * @param roomId - The room ID of the room where the error occurred. * @param megolmSessionId - The megolm session ID that is missing. @@ -265,14 +257,22 @@ export class PerSessionKeyBackupDownloader { // processing this one, it won't queue another request. const request = this.queuedRequests[0]; try { - const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId); + // The backup could have changed between the time we queued the request and now, so we need to check + const configuration = await this.getOrCreateBackupConfiguration(); + if (!configuration) { + // Backup is not configured correctly, so stop the loop. + this.downloadLoopRunning = false; + return; + } + + const result = await this.queryKeyBackup(request.roomId, request.megolmSessionId, configuration); if (this.stopped) { return; } // We got the encrypted key from backup, let's try to decrypt and import it. try { - await this.decryptAndImport(request, result); + await this.decryptAndImport(request, result, configuration); } catch (e) { this.logger.error( `Error while decrypting and importing key backup for session ${request.megolmSessionId}`, @@ -297,10 +297,6 @@ export class PerSessionKeyBackupDownloader { // If the downloader was stopped, we don't want to retry. this.downloadLoopRunning = false; return; - case KeyDownloadErrorCode.CONFIGURATION_ERROR: - // Backup is not configured correctly, so stop the loop. - this.downloadLoopRunning = false; - return; } } else if (err instanceof KeyDownloadRateLimitError) { // we want to retry after the backoff time @@ -319,13 +315,13 @@ export class PerSessionKeyBackupDownloader { * * @param targetRoomId - ID of the room that the session is used in. * @param targetSessionId - ID of the session for which to check backup. + * @param configuration - The backup configuration to use. */ - private async queryKeyBackup(targetRoomId: string, targetSessionId: string): Promise { - const configuration = await this.getOrCreateBackupConfiguration(); - if (!configuration) { - throw new KeyDownloadError(KeyDownloadErrorCode.CONFIGURATION_ERROR); - } - + private async queryKeyBackup( + targetRoomId: string, + targetSessionId: string, + configuration: Configuration, + ): Promise { this.logger.debug(`Checking key backup for session ${targetSessionId}`); if (this.stopped) throw new KeyDownloadError(KeyDownloadErrorCode.STOPPED); try { @@ -363,13 +359,11 @@ export class PerSessionKeyBackupDownloader { } } - private async decryptAndImport(sessionInfo: SessionInfo, data: KeyBackupSession): Promise { - const configuration = await this.getOrCreateBackupConfiguration(); - - if (!configuration) { - throw new Error("Backup: No configuration"); - } - + private async decryptAndImport( + sessionInfo: SessionInfo, + data: KeyBackupSession, + configuration: Configuration, + ): Promise { const sessionsToImport: Record = { [sessionInfo.megolmSessionId]: data }; const keys = await configuration!.decryptor.decryptSessions(sessionsToImport);