From a1314b7bc10c9f44550517c9defea313ea39fdfc Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Tue, 11 Apr 2023 16:02:26 +0100 Subject: [PATCH] Delete data for mismatched accounts --- .../CryptoMachine/MXCryptoMachine.swift | 55 +++++++-- MatrixSDK/Crypto/MXCryptoV2.swift | 3 +- .../MXCryptoMachineUnitTests.swift | 104 ++++++++++++++++-- changelog.d/pr-1763.bugfix | 1 + 4 files changed, 138 insertions(+), 25 deletions(-) create mode 100644 changelog.d/pr-1763.bugfix diff --git a/MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift b/MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift index d63bed149d..b92cd55f8a 100644 --- a/MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift +++ b/MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift @@ -36,6 +36,8 @@ class MXCryptoMachine { } private static let kdfRounds: Int32 = 500_000 + // Error type will be moved to rust sdk + private static let MismatchedAccountError = "the account in the store doesn't match the account in the constructor" enum Error: Swift.Error { case invalidEvent @@ -72,17 +74,7 @@ class MXCryptoMachine { ) throws { MXCryptoSDKLogger.shared.log(logLine: "Starting logs") - let url = try MXCryptoMachineStore.createStoreURLIfNecessary(for: userId) - let passphrase = try MXCryptoMachineStore.storePassphrase() - log.debug("Opening crypto store at \(url.path)/matrix-sdk-crypto.sqlite3") // Hardcoding path to db for debugging purpose - - machine = try OlmMachine( - userId: userId, - deviceId: deviceId, - path: url.path, - passphrase: passphrase - ) - + self.machine = try Self.createMachine(userId: userId, deviceId: deviceId, log: log) self.requests = MXCryptoRequests(restClient: restClient) self.getRoomAction = getRoomAction @@ -119,7 +111,46 @@ class MXCryptoMachine { func deleteAllData() throws { let url = try MXCryptoMachineStore.storeURL(for: userId) - try FileManager.default.removeItem(at: url) + if FileManager.default.fileExists(atPath: url.path) { + try FileManager.default.removeItem(at: url) + } + } + + // MARK: - Private + + private static func createMachine(userId: String, deviceId: String, log: MXNamedLog) throws -> OlmMachine { + let url = try MXCryptoMachineStore.createStoreURLIfNecessary(for: userId) + let passphrase = try MXCryptoMachineStore.storePassphrase() + + log.debug("Opening crypto store at \(url.path)/matrix-sdk-crypto.sqlite3") // Hardcoding full path to db for debugging purposes + + do { + return try OlmMachine( + userId: userId, + deviceId: deviceId, + path: url.path, + passphrase: passphrase + ) + } catch { + // If we cannot open machine due to a mismatched account, delete previous data and try again + if case CryptoStoreError.CryptoStore(let message) = error, + message.contains(Self.MismatchedAccountError) { + log.error("Credentials of the account do not match, deleting previous data", context: [ + "error": message + ]) + try FileManager.default.removeItem(at: url) + return try OlmMachine( + userId: userId, + deviceId: deviceId, + path: url.path, + passphrase: passphrase + ) + + // Otherwise re-throw the error + } else { + throw error + } + } } } diff --git a/MatrixSDK/Crypto/MXCryptoV2.swift b/MatrixSDK/Crypto/MXCryptoV2.swift index 98c862eeb0..dacfbd2569 100644 --- a/MatrixSDK/Crypto/MXCryptoV2.swift +++ b/MatrixSDK/Crypto/MXCryptoV2.swift @@ -197,7 +197,8 @@ class MXCryptoV2: NSObject, MXCrypto { } if deleteStore { - if let credentials = session?.credentials { + if let credentials = session?.credentials, + MXRealmCryptoStore.hasData(for: credentials) { MXRealmCryptoStore.delete(with: credentials) } else { log.failure("Missing credentials, cannot delete store") diff --git a/MatrixSDKTests/Crypto/CryptoMachine/MXCryptoMachineUnitTests.swift b/MatrixSDKTests/Crypto/CryptoMachine/MXCryptoMachineUnitTests.swift index b2ab11a862..0ef92ebcf3 100644 --- a/MatrixSDKTests/Crypto/CryptoMachine/MXCryptoMachineUnitTests.swift +++ b/MatrixSDKTests/Crypto/CryptoMachine/MXCryptoMachineUnitTests.swift @@ -38,6 +38,7 @@ class MXCryptoMachineUnitTests: XCTestCase { } var myUserId = "@alice:localhost" + var myDeviceId = "ABCD" var otherUserId = "@bob:localhost" var roomId = "!1234:localhost" var verificationRequestId = "$12345" @@ -46,27 +47,106 @@ class MXCryptoMachineUnitTests: XCTestCase { override func setUp() { restClient = MXRestClientStub() + machine = try! createMachine() + } + + override func tearDown() { + do { + try deleteData(userId: myUserId) + } catch { + XCTFail("Cannot tear down test - \(error)") + } + } + + // MARK: - Helpers + + private func createMachine(userId: String? = nil, deviceId: String? = nil) throws -> MXCryptoMachine { MXKeyProvider.sharedInstance().delegate = KeyProvider() - machine = try! MXCryptoMachine( - userId: myUserId, - deviceId: "ABCD", + let machine = try MXCryptoMachine( + userId: userId ?? myUserId, + deviceId: deviceId ?? myDeviceId, restClient: restClient, getRoomAction: { MXRoom(roomId: $0, andMatrixSession: nil) }) MXKeyProvider.sharedInstance().delegate = nil + return machine } - override func tearDown() { - do { - let url = try MXCryptoMachineStore.storeURL(for: myUserId) - guard FileManager.default.fileExists(atPath: url.path) else { - return - } - try FileManager.default.removeItem(at: url) - } catch { - XCTFail("Cannot tear down test - \(error)") + private func deleteData(userId: String) throws { + let url = try MXCryptoMachineStore.storeURL(for: userId) + guard FileManager.default.fileExists(atPath: url.path) else { + return } + try FileManager.default.removeItem(at: url) + } + + // MARK: - Init + + func test_init_createsMachine() throws { + let machine = try createMachine(userId: myUserId, deviceId: myDeviceId) + + XCTAssertEqual(machine.userId, myUserId) + XCTAssertEqual(machine.deviceId, myDeviceId) + } + + func test_init_createsMachinesWithDifferentDeviceIds() throws { + let machine1 = try createMachine(deviceId: "Device1") + XCTAssertEqual(machine1.userId, myUserId) + XCTAssertEqual(machine1.deviceId, "Device1") + + let machine2 = try createMachine(deviceId: "Device2") + XCTAssertEqual(machine2.userId, myUserId) + XCTAssertEqual(machine2.deviceId, "Device2") + + let machine3 = try createMachine(deviceId: "Device3") + XCTAssertEqual(machine3.userId, myUserId) + XCTAssertEqual(machine3.deviceId, "Device3") + } + + func test_init_loadsExistingData() throws { + let machine1 = try createMachine() + try machine1.setRoomAlgorithm(roomId: roomId, algorithm: .megolmV1AesSha2) + + let machine2 = try createMachine() + let settings = machine2.roomSettings(roomId: roomId) + XCTAssertEqual(settings?.algorithm, .megolmV1AesSha2) + } + + func test_init_differentDeviceDeletesExistingData() throws { + let device1 = "Device1" + let device2 = "Device2" + + // Create a machine with some data + let machine1 = try createMachine(deviceId: device1) + try machine1.setRoomAlgorithm(roomId: roomId, algorithm: .megolmV1AesSha2) + + // Create another machine with different device ID, which will delete existing data + let machine2 = try createMachine(deviceId: device2) + XCTAssertNil(machine2.roomSettings(roomId: roomId)) + + // Now opening the first machine again shows data has been removed + let machine3 = try createMachine(deviceId: device1) + XCTAssertNil(machine3.roomSettings(roomId: roomId)) + } + + func test_init_differentUserPreservesExistingData() throws { + let user1 = "@User1:localhost" + let user2 = "@User2:localhost" + + let machine1 = try createMachine(userId: user1) + try machine1.setRoomAlgorithm(roomId: roomId, algorithm: .megolmV1AesSha2) + + let machine2 = try createMachine(userId: user2) + try machine2.setRoomAlgorithm(roomId: roomId, algorithm: .olmV1Curve25519AesSha2) + + // Loading up machine1 again will have previous data unchanged + let machine3 = try createMachine(userId: user1) + let settings = machine3.roomSettings(roomId: roomId) + XCTAssertEqual(settings?.algorithm, .megolmV1AesSha2) + + try deleteData(userId: user1) + try deleteData(userId: user2) } // MARK: - Sync response diff --git a/changelog.d/pr-1763.bugfix b/changelog.d/pr-1763.bugfix new file mode 100644 index 0000000000..cfda7d074e --- /dev/null +++ b/changelog.d/pr-1763.bugfix @@ -0,0 +1 @@ +Crypto: Delete data for mismatched accounts