Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete data for mismatched accounts #1763

Merged
merged 1 commit into from
Apr 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions MatrixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ class MXCryptoMachine {
}

private static let kdfRounds: Int32 = 500_000
// Error type will be moved to rust sdk
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add this as a FIXME/TODO for visibility ?

Copy link
Contributor Author

@Anderas Anderas Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan of adding TODOs to code, but it is tracked in a draft issue on a sprint board so to be resolved asap. Here its meant to be an explanatory comment rather than a TODO

private static let MismatchedAccountError = "the account in the store doesn't match the account in the constructor"

enum Error: Swift.Error {
case invalidEvent
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
}
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion MatrixSDK/Crypto/MXCryptoV2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
104 changes: 92 additions & 12 deletions MatrixSDKTests/Crypto/CryptoMachine/MXCryptoMachineUnitTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions changelog.d/pr-1763.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Crypto: Delete data for mismatched accounts