Krille/refactor user device keys#2414
Conversation
PR SummaryHigh Risk Overview Trust and encryption eligibility move to async APIs on keys ( Encryption paths (megolm session setup, olm, SSSS, cross-signing, verification, VoIP to-device) and Reviewed by Cursor Bugbot for commit 6e6561d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Trust state lost on refresh
- The outdated cached lists collected in oldDeviceKeys are now used as the base (via oldDeviceKeys.remove) when merging fresh server keys, and any user omitted by the server is retained, preserving verification/blocked/TOFU/signature state.
- ✅ Fixed: Verification skips key refetch
- The redundant nested checks in the Request/Ready/Start branches now mark the user outdated and refetch device keys from the server before cancelling, restoring the pre-refactor refetch behavior.
Or push these changes by commenting:
@cursor push e86c84d55b
Preview (e86c84d55b)
diff --git a/lib/encryption/utils/key_verification.dart b/lib/encryption/utils/key_verification.dart
--- a/lib/encryption/utils/key_verification.dart
+++ b/lib/encryption/utils/key_verification.dart
@@ -367,7 +367,7 @@
Logs().i('[Key Verification] Received type $type: $payload');
// Fetch once upfront to avoid multiple DB round-trips within the switch.
final knownMethods = await knownVerificationMethods;
- final userKeys = await client.fetchUserDeviceKeysLists({userId});
+ var userKeys = await client.fetchUserDeviceKeysLists({userId});
try {
var thisLastStep = lastStep;
switch (type) {
@@ -392,6 +392,10 @@
// ensure we have the other sides keys
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
+ // The device might be new and not yet in our cache, so force a
+ // refresh from the server before giving up.
+ await client.database.storeUserDeviceKeysInfo(userId, true);
+ userKeys = await client.fetchUserDeviceKeysLists({userId});
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
await cancel('im.fluffychat.unknown_device');
return;
@@ -438,6 +442,10 @@
// ensure we have the other sides keys
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
+ // The device might be new and not yet in our cache, so force a
+ // refresh from the server before giving up.
+ await client.database.storeUserDeviceKeysInfo(userId, true);
+ userKeys = await client.fetchUserDeviceKeysLists({userId});
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
await cancel('im.fluffychat.unknown_device');
return;
@@ -524,6 +532,10 @@
// ensure we have the other sides keys
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
+ // The device might be new and not yet in our cache, so force a
+ // refresh from the server before giving up.
+ await client.database.storeUserDeviceKeysInfo(userId, true);
+ userKeys = await client.fetchUserDeviceKeysLists({userId});
if (userKeys[userId]?.deviceKeys[deviceId!] == null) {
await cancel('im.fluffychat.unknown_device');
return;
diff --git a/lib/src/client.dart b/lib/src/client.dart
--- a/lib/src/client.dart
+++ b/lib/src/client.dart
@@ -3311,10 +3311,8 @@
if (deviceKeys != null) {
for (final rawDeviceKeyListEntry in deviceKeys.entries) {
final userId = rawDeviceKeyListEntry.key;
- final userKeys = userDeviceKeys[userId] ??= DeviceKeysList(
- userId,
- this,
- );
+ final userKeys = userDeviceKeys[userId] ??=
+ oldDeviceKeys.remove(userId) ?? DeviceKeysList(userId, this);
final oldKeys = Map<String, DeviceKeys>.from(userKeys.deviceKeys);
userKeys.deviceKeys.clear();
for (final rawDeviceKeyEntry in rawDeviceKeyListEntry.value.entries) {
@@ -3433,10 +3431,8 @@
}
for (final crossSigningKeyListEntry in keys.entries) {
final userId = crossSigningKeyListEntry.key;
- final userKeys = userDeviceKeys[userId] ??= DeviceKeysList(
- userId,
- this,
- );
+ final userKeys = userDeviceKeys[userId] ??=
+ oldDeviceKeys.remove(userId) ?? DeviceKeysList(userId, this);
final oldKeys = Map<String, CrossSigningKey>.from(
userKeys.crossSigningKeys,
);
@@ -3496,6 +3492,12 @@
}
}
+ // Keep the previously cached (still outdated) lists for any user the
+ // server failed to return, so we don't lose their trust state.
+ for (final oldEntry in oldDeviceKeys.entries) {
+ userDeviceKeys.putIfAbsent(oldEntry.key, () => oldEntry.value);
+ }
+
// now process all the failures
if (response.failures != null) {
for (final failureDomain in response.failures?.keys ?? <String>[]) {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 6e6561d. Configure here.
6e6561d to
630ca76
Compare
td-famedly
left a comment
td-famedly
left a comment
There was a problem hiding this comment.
i will block this for my review. Please ping me once it's ready
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2414 +/- ##
==========================================
+ Coverage 59.66% 59.74% +0.08%
==========================================
Files 161 161
Lines 20329 20398 +69
==========================================
+ Hits 12129 12187 +58
- Misses 8200 8211 +11
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
541dd05 to
8e9b093
Compare
6e4a21f to
5b113af
Compare
2 participants
closes https://famedly.atlassian.net/browse/FM-758
What does this change?
We no longer load all user device keys on app start and we no longer update them on every sync. Instead we load and update them on demand. Means
Client.userDeviceKeys[userId]has been replaced by the asyncClient.fetchUserDeviceKeysList(userId). This needed a very huge refactoring of the SDK as we no longer hold keys in memory but have them all database centric.This should make the whole key handling more stable as we have no more in memory cache which can get outdated. It should also improve the performance as we no longer need to load all keys on app start. It should also speed up sync as we no longer update outdated keys on every sync.
Instead it could slow down sending the first encrypted message in a room for a few milliseconds. But this should actually be the better deal.
TODOS: