Page MenuHomePhorge
Files F85630918

D335.1786267651.diff
No OneTemporary
Actions

Size
13 KB
Referenced Files
None
Subscribers
None

D335.1786267651.diff
View Options

diff --git a/src/client/actions/encryption.cpp b/src/client/actions/encryption.cpp
--- a/src/client/actions/encryption.cpp
+++ b/src/client/actions/encryption.cpp
@@ -542,6 +542,19 @@
m.deviceLists.addDevice(userId, deviceId, deviceInfo);
}
if (wantedToFetchAllForUser(userId)) {
+ // We requested all devices for this user, so any
+ // device currently tracked but absent from the
+ // response has been deleted by its owner. Mark it
+ // as deleted rather than removing it, so we can still
+ // verify past decrypted events from that device.
+ auto existingDevices = m.deviceLists.devicesFor(userId);
+ for (auto [deviceId, info] : existingDevices) {
+ if (!deviceMap.count(deviceId)) {
+ kzo.client.dbg() << "Marking deleted device " << userId
+ << "/" << deviceId << std::endl;
+ m.deviceLists.markDeviceAsDeleted(userId, deviceId);
+ }
+ }
m.deviceLists.markUpToDate(userId);
}
}
diff --git a/src/client/client-model.cpp b/src/client/client-model.cpp
--- a/src/client/client-model.cpp
+++ b/src/client/client-model.cpp
@@ -289,7 +289,8 @@
// XXX: preliminary approach
auto shouldSendP = [=](auto deviceInfo, auto /* deviceMap */) {
- return deviceInfo.trustLevel >= trustLevelNeeded;
+ return deviceInfo.trustLevel >= trustLevelNeeded
+ && !deviceInfo.deleted;
};
diff --git a/src/client/device-list-tracker.hpp b/src/client/device-list-tracker.hpp
--- a/src/client/device-list-tracker.hpp
+++ b/src/client/device-list-tracker.hpp
@@ -38,12 +38,17 @@
std::string curve25519Key;
std::optional<std::string> displayName;
DeviceTrustLevel trustLevel{Unseen};
+ /// Whether this device has been deleted by its owner.
+ /// Deleted devices are kept in the tracker for verifying
+ /// past decrypted events, but are excluded from
+ /// devicesToSendKeys().
+ bool deleted{false};
friend bool operator==(const DeviceKeyInfo &a, const DeviceKeyInfo &b) = default;
friend bool operator!=(const DeviceKeyInfo &a, const DeviceKeyInfo &b) = default;
};
template<class Archive>
- void serialize(Archive &ar, DeviceKeyInfo &i, std::uint32_t const /*version*/)
+ void serialize(Archive &ar, DeviceKeyInfo &i, std::uint32_t const version)
{
ar
& i.deviceId
@@ -52,6 +57,11 @@
& i.displayName
& i.trustLevel
;
+ if (version >= 1) {
+ ar & i.deleted;
+ } else {
+ i.deleted = false;
+ }
}
struct DeviceListTracker
@@ -83,6 +93,11 @@
void addVerifiedDeviceKeyInfo(std::string userId, std::string deviceId, DeviceKeyInfo info);
+ /// Mark a device as deleted by its owner. The device is kept
+ /// in the tracker (for verifying past decrypted events) but
+ /// will be excluded from devicesToSendKeys().
+ void markDeviceAsDeleted(std::string userId, std::string deviceId);
+
void markUpToDate(std::string userId);
DeviceMapT devicesFor(std::string userId) const;
@@ -118,5 +133,5 @@
}
}
-BOOST_CLASS_VERSION(Kazv::DeviceKeyInfo, 0)
+BOOST_CLASS_VERSION(Kazv::DeviceKeyInfo, 1)
BOOST_CLASS_VERSION(Kazv::DeviceListTracker, 0)
diff --git a/src/client/device-list-tracker.cpp b/src/client/device-list-tracker.cpp
--- a/src/client/device-list-tracker.cpp
+++ b/src/client/device-list-tracker.cpp
@@ -27,6 +27,8 @@
b.displayName = std::nullopt;
a.trustLevel = Unseen;
b.trustLevel = Unseen;
+ a.deleted = false;
+ b.deleted = false;
return std::move(a) == std::move(b);
}
@@ -130,6 +132,16 @@
});
}
+ void DeviceListTracker::markDeviceAsDeleted(std::string userId, std::string deviceId)
+ {
+ deviceLists = std::move(deviceLists)
+ .update(userId, [=](auto deviceMap) {
+ auto info = deviceMap[deviceId];
+ info.deleted = true;
+ return std::move(deviceMap).set(deviceId, info);
+ });
+ }
+
void DeviceListTracker::markUpToDate(std::string userId)
{
usersToTrackDeviceLists = std::move(usersToTrackDeviceLists).set(userId, false);
diff --git a/src/tests/client/device-list-tracker-test.cpp b/src/tests/client/device-list-tracker-test.cpp
--- a/src/tests/client/device-list-tracker-test.cpp
+++ b/src/tests/client/device-list-tracker-test.cpp
@@ -15,7 +15,7 @@
using namespace Kazv;
using namespace Kazv::Factory;
-TEST_CASE("Bug: DeviceListTracker has no mechanism to remove devices", "[client][device-list-tracker]")
+TEST_CASE("DeviceListTracker marks devices as deleted but keeps them", "[client][device-list-tracker]")
{
DeviceListTracker tracker;
auto info1 = makeDeviceKeyInfo(withDeviceId("device1") | withDeviceTrustLevel(Verified));
@@ -27,18 +27,23 @@
REQUIRE(tracker.devicesFor(userId).size() == 2);
- // The remote user deletes device2. In a correct implementation,
- // device2 should be removed from the tracker. But there is no
- // removeDevice() API on DeviceListTracker, so the deleted device
- // persists.
+ // Mark device2 as deleted
+ tracker.markDeviceAsDeleted(userId, "device2");
- // Expected correct behavior: tracker.devicesFor(userId).size() == 1
- // Actual buggy behavior: tracker.devicesFor(userId).size() == 2
- REQUIRE(tracker.devicesFor(userId).size() == 2); // confirms the bug
- REQUIRE(tracker.get(userId, "device2").has_value()); // deleted device persists
+ // The device is still in the tracker (for verifying past events)
+ REQUIRE(tracker.devicesFor(userId).size() == 2);
+ REQUIRE(tracker.get(userId, "device2").has_value());
+
+ // But it is marked as deleted
+ REQUIRE(tracker.get(userId, "device2").value().deleted);
+ REQUIRE(!tracker.get(userId, "device1").value().deleted);
+
+ // findByCurve25519Key still finds the deleted device
+ REQUIRE(tracker.findByCurve25519Key(userId, info2.curve25519Key).has_value());
+ REQUIRE(tracker.findByCurve25519Key(userId, info2.curve25519Key).value().deleted);
}
-TEST_CASE("Bug: devicesToSendKeys includes deleted devices", "[client][device-list-tracker]")
+TEST_CASE("devicesToSendKeys excludes deleted devices", "[client][device-list-tracker]")
{
auto info1 = makeDeviceKeyInfo(withDeviceId("device1") | withDeviceTrustLevel(Verified));
auto info2 = makeDeviceKeyInfo(withDeviceId("device2") | withDeviceTrustLevel(Verified));
@@ -53,15 +58,13 @@
REQUIRE(client.deviceLists.devicesFor(userId).size() == 2);
REQUIRE(client.devicesToSendKeys(userId).size() == 2);
- // Remote user deletes device2. After processing a full keys/query,
- // device2 should no longer be returned by devicesToSendKeys. But
- // since there is no removal mechanism, the deleted device persists.
+ // Mark device2 as deleted
+ client.deviceLists.markDeviceAsDeleted(userId, "device2");
- // Expected correct behavior: client.devicesToSendKeys(userId).size() == 1
- // Actual buggy behavior: client.devicesToSendKeys(userId).size() == 2
+ // devicesToSendKeys should now exclude the deleted device
auto devices = client.devicesToSendKeys(userId);
- REQUIRE(devices.size() == 2); // confirms the bug: deleted device still sent keys
- REQUIRE(devices[1] == "device2");
+ REQUIRE(devices.size() == 1);
+ REQUIRE(devices[0] == "device1");
}
static json makeSignedDeviceInfo(const ClientModel &client)
@@ -70,7 +73,7 @@
return json::parse(std::get<Bytes>(next.nextJobs[0].requestBody()))["device_keys"];
}
-TEST_CASE("Bug: processResponse(QueryKeysResponse) does not remove deleted devices", "[client][device-list-tracker]")
+TEST_CASE("processResponse(QueryKeysResponse) marks deleted devices on full query", "[client][device-list-tracker]")
{
// Set up two device identity cryptos for the same user
auto device1Crypto = makeCrypto();
@@ -106,6 +109,7 @@
REQUIRE(client.deviceLists.devicesFor("@alice:example.com").size() == 2);
REQUIRE(client.deviceLists.get("@alice:example.com", "device1").has_value());
REQUIRE(client.deviceLists.get("@alice:example.com", "device2").has_value());
+ REQUIRE(!client.deviceLists.get("@alice:example.com", "device2").value().deleted);
// Now Alice deletes device2. We get a new full key query response
// that only contains device1.
@@ -121,16 +125,19 @@
| withResponseDataKV("deviceKeys",
json::object({{"@alice:example.com", json::array()}})))));
- // BUG: device2 should have been removed since we fetched ALL devices
- // and it was not in the response. But processResponse only adds/updates
- // devices; it never removes them.
- REQUIRE(client.deviceLists.devicesFor("@alice:example.com").size() == 2); // confirms the bug
- REQUIRE(client.deviceLists.get("@alice:example.com", "device2").has_value()); // deleted device persists
+ // device2 is kept in the tracker (for verifying past events) but marked deleted
+ REQUIRE(client.deviceLists.devicesFor("@alice:example.com").size() == 2);
+ REQUIRE(client.deviceLists.get("@alice:example.com", "device2").has_value());
+ REQUIRE(client.deviceLists.get("@alice:example.com", "device2").value().deleted);
- // Symptom: devicesToSendKeys still includes the deleted device
+ // device1 is not deleted
+ REQUIRE(!client.deviceLists.get("@alice:example.com", "device1").value().deleted);
+
+ // devicesToSendKeys excludes the deleted device
client.trustLevelNeededToSendKeys = Unseen;
auto devices = client.devicesToSendKeys("@alice:example.com");
- REQUIRE(devices.size() == 2); // confirms the symptom: keys still sent to deleted device
+ REQUIRE(devices.size() == 1);
+ REQUIRE(devices[0] == "device1");
}
static ClientModel makeDeviceClient(std::string userId, std::string deviceId)
@@ -141,7 +148,7 @@
return client;
}
-TEST_CASE("Bug: mixed full and specific queries preserve unqueried devices", "[client][device-list-tracker]")
+TEST_CASE("mixed full and specific queries: full marks deleted, specific preserves unqueried", "[client][device-list-tracker]")
{
// Create crypto identities for all devices of two users
auto aliceDev1 = makeDeviceClient("@alice:example.com", "device1");
@@ -212,32 +219,32 @@
})))));
// For Alice (full query): device3 was deleted and NOT in the response.
- // Since we queried for ALL devices, device3 should be removed.
- // BUG: device3 persists because processResponse never removes devices.
- // Expected correct behavior: size == 2, device3 gone
- // Actual buggy behavior: size == 3, device3 still present
- REQUIRE(client.deviceLists.devicesFor("@alice:example.com").size() == 3); // confirms the bug
- REQUIRE(client.deviceLists.get("@alice:example.com", "device3").has_value()); // deleted device persists
+ // device3 is kept in the tracker (for verifying past events) but marked
+ // as deleted.
+ REQUIRE(client.deviceLists.devicesFor("@alice:example.com").size() == 3);
+ REQUIRE(client.deviceLists.get("@alice:example.com", "device3").has_value());
+ REQUIRE(client.deviceLists.get("@alice:example.com", "device3").value().deleted);
+ REQUIRE(!client.deviceLists.get("@alice:example.com", "device1").value().deleted);
+ REQUIRE(!client.deviceLists.get("@alice:example.com", "device2").value().deleted);
// For Bob (specific query for device1 only): device2 was NOT in the
- // request and NOT in the response. The gatekeeper constraint requires
- // that device2 must NOT be deleted, because we did not query for all
- // of Bob's devices -- the response only contains the specific devices
- // we asked for, so absence from the response does not mean deletion.
- // This assertion verifies the gatekeeper behaviour: unqueried devices
- // are preserved.
- REQUIRE(client.deviceLists.devicesFor("@bob:example.com").size() == 2); // gatekeeper: device2 preserved
- REQUIRE(client.deviceLists.get("@bob:example.com", "device2").has_value()); // device2 NOT deleted
- REQUIRE(client.deviceLists.get("@bob:example.com", "device1").has_value()); // device1 still present
-
- // Symptom for Alice: devicesToSendKeys still includes the deleted device3
+ // request and NOT in the response. Since we did not query for all
+ // of Bob's devices, absence from the response does not mean deletion.
+ // device2 must be preserved and NOT marked as deleted.
+ REQUIRE(client.deviceLists.devicesFor("@bob:example.com").size() == 2);
+ REQUIRE(client.deviceLists.get("@bob:example.com", "device2").has_value());
+ REQUIRE(!client.deviceLists.get("@bob:example.com", "device2").value().deleted);
+ REQUIRE(client.deviceLists.get("@bob:example.com", "device1").has_value());
+ REQUIRE(!client.deviceLists.get("@bob:example.com", "device1").value().deleted);
+
+ // devicesToSendKeys excludes Alice's deleted device3
client.trustLevelNeededToSendKeys = Unseen;
auto aliceDevices = client.devicesToSendKeys("@alice:example.com");
- REQUIRE(aliceDevices.size() == 3); // bug: deleted device3 still sent keys
+ REQUIRE(aliceDevices.size() == 2);
// Bob's devicesToSendKeys correctly includes both devices
auto bobDevices = client.devicesToSendKeys("@bob:example.com");
- REQUIRE(bobDevices.size() == 2); // correct: both devices present
+ REQUIRE(bobDevices.size() == 2);
}
TEST_CASE("DeviceListTracker::findByCurve25519Key")

File Metadata

Mime Type
text/plain
Expires
Sun, Aug 9, 2:27 AM (19 h, 8 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
1724851
Default Alt Text
D335.1786267651.diff (13 KB)

Event Timeline