A removed MLS device stays removed (GRYT-1555)

src/db/sqlite/connection.ts

@@ -590,6 +590,15 @@ function createSchema(d: DatabaseSync): void {
       PRIMARY KEY (server_user_id, device_id)
     );
 
+    -- Devices their owner removed (GRYT-1555). Kept for good: a phone in a drawer can
+    -- come back after any window we'd pick, and a row is a few dozen bytes.
+    CREATE TABLE IF NOT EXISTS mls_removed_devices (
+      server_user_id TEXT NOT NULL,
+      device_id TEXT NOT NULL,
+      removed_at TEXT NOT NULL,
+      PRIMARY KEY (server_user_id, device_id)
+    );
+
     -- data goes to NULL when a package is handed out. The row stays until the
     -- retention sweep, so a Welcome naming the ref can still find its device.
     CREATE TABLE IF NOT EXISTS mls_key_packages (

src/db/sqlite/mergeGuest.test.ts

@@ -217,6 +217,7 @@ describe("merging a guest into an account that is already a member", () => {
       "users.server_user_id", "roles.server_user_id", "conversation_members.server_user_id",
       "mentions.server_user_id", "refresh_tokens.server_user_id",
       "mls_devices.server_user_id", "mls_key_packages.server_user_id", "mls_welcomes.server_user_id",
+      "mls_removed_devices.server_user_id",
     ]);
     const listed = new Set(AUTHOR_COLUMNS.map(([t, c]) => `${t}.${c}`));
     const tables = db.prepare(`SELECT name FROM sqlite_master WHERE type = 'table'`).all() as { name: string }[];

src/db/sqlite/mls.test.ts

@@ -25,6 +25,7 @@ import {
   MLS_MAX_KEY_PACKAGES,
   mlsKeyPackageOwner,
   oldestMlsSeq,
+  isRemovedMlsDevice,
   removeMlsDevice,
   sweepMls,
   touchMlsDevice,
@@ -80,6 +81,20 @@ describe("devices", () => {
     assert.deepEqual(listMlsDevices([me]), []);
     assert.equal(mlsKeyPackageOwner("ref-removed"), null);
   });
+
+  it("never lets a removed device id back in, and remembers it through a guest merge", async () => {
+    const guest = await upsertUser("mls-removed-guest", "Guest");
+    const account = await upsertUser("mls-removed-account", "Account");
+    touchMlsDevice(guest.server_user_id, "lost-phone");
+    removeMlsDevice(guest.server_user_id, "lost-phone");
+
+    assert.equal(isRemovedMlsDevice(guest.server_user_id, "lost-phone"), true);
+    assert.equal(touchMlsDevice(guest.server_user_id, "lost-phone"), "device_removed");
+    assert.equal(touchMlsDevice(guest.server_user_id, "new-phone"), "ok");
+
+    assert.ok(mergeGuestIntoAccount("mls-removed-guest", "mls-removed-account"));
+    assert.equal(touchMlsDevice(account.server_user_id, "lost-phone"), "device_removed");
+  });
 });
 
 describe("KeyPackages", () => {

src/db/sqlite/mls.ts

@@ -99,14 +99,15 @@ function rowToEntry(r: Record<string, unknown>): MlsLogEntry {
 
 // ── Devices ─────────────────────────────────────────────────────────────
 
-/** A new device past the cap is refused; a known one only has last_seen_at moved. */
+/** A new device past the cap is refused, and so is a removed one; a known one only has last_seen_at moved. */
 export function touchMlsDevice(
   serverUserId: string,
   deviceId: string,
   now = new Date(),
-): "ok" | "too_many_devices" {
+): "ok" | "too_many_devices" | "device_removed" {
   return inTransaction(() => {
     const db = getSqliteDb();
+    if (isRemovedMlsDevice(serverUserId, deviceId)) return "device_removed";
     const at = toIso(now);
     const known = db
       .prepare(`UPDATE mls_devices SET last_seen_at = ? WHERE server_user_id = ? AND device_id = ?`)
@@ -148,10 +149,19 @@ export function listMlsDevices(serverUserIds: string[]): MlsDevice[] {
   }));
 }
 
-/** Its KeyPackages and waiting Welcomes go with it, since nothing can use them now. */
-export function removeMlsDevice(serverUserId: string, deviceId: string): boolean {
+export function isRemovedMlsDevice(serverUserId: string, deviceId: string): boolean {
+  return !!getSqliteDb()
+    .prepare(`SELECT 1 FROM mls_removed_devices WHERE server_user_id = ? AND device_id = ?`)
+    .get(serverUserId, deviceId);
+}
+
+/** Its KeyPackages and waiting Welcomes go with it, and the id can never register again. */
+export function removeMlsDevice(serverUserId: string, deviceId: string, now = new Date()): boolean {
   return inTransaction(() => {
     const db = getSqliteDb();
+    db.prepare(
+      `INSERT OR IGNORE INTO mls_removed_devices (server_user_id, device_id, removed_at) VALUES (?, ?, ?)`,
+    ).run(serverUserId, deviceId, toIso(now));
     const gone = db
       .prepare(`DELETE FROM mls_devices WHERE server_user_id = ? AND device_id = ?`)
       .run(serverUserId, deviceId);
@@ -169,6 +179,8 @@ export function carryMlsDevicesForward(db: DatabaseSync, from: string, to: strin
         AND device_id IN (SELECT device_id FROM mls_devices WHERE server_user_id = ?)`,
   ).run(from, to);
   db.prepare(`UPDATE mls_devices SET server_user_id = ? WHERE server_user_id = ?`).run(to, from);
+  db.prepare(`UPDATE OR IGNORE mls_removed_devices SET server_user_id = ? WHERE server_user_id = ?`).run(to, from);
+  db.prepare(`DELETE FROM mls_removed_devices WHERE server_user_id = ?`).run(from);
   db.prepare(`UPDATE mls_key_packages SET server_user_id = ? WHERE server_user_id = ?`).run(to, from);
   db.prepare(`UPDATE mls_welcomes SET server_user_id = ? WHERE server_user_id = ?`).run(to, from);
 }

src/socket/handlers/mls.test.ts

@@ -437,6 +437,32 @@ describe("KeyPackages", () => {
     assert.equal((await publish(carol, await makeDevice("carol-5", 1))).ok, true, "removing one frees its slot");
   });
 
+  it("won't take a removed device back, though a new one is fine (GRYT-1555)", async () => {
+    const hana = await connect("Hana");
+    const ivar = await connect("Ivar");
+    const conv = (await openDirectConversation(hana.serverUserId, ivar.serverUserId)).conversation_id;
+    const laptop = await makeDevice("hana-laptop", 1);
+    const phone = await makeDevice("hana-phone", 2);
+    const ivarPhone = await makeDevice("ivar-phone", 1);
+    assert.equal((await publish(hana, laptop)).ok, true);
+    assert.equal((await publish(hana, phone)).ok, true);
+    assert.equal((await publish(ivar, ivarPhone)).ok, true);
+
+    assert.equal((await hana.call("mls:device:remove", { deviceId: phone.id })).ok, true);
+    // The phone is still signed in and does what the driver does on reconnect.
+    assert.equal((await hana.call("mls:sync", { deviceId: phone.id })).error, "device_removed");
+    assert.equal((await publish(hana, await makeDevice("hana-phone", 2))).error, "device_removed");
+    const own = await hana.call("mls:keypackages:claim", { conversationId: conv, deviceId: phone.id });
+    assert.equal(own.error, "device_removed");
+
+    const claimed = await ivar.call("mls:keypackages:claim", { conversationId: conv, deviceId: ivarPhone.id });
+    assert.deepEqual((claimed.keyPackages as { deviceId: string }[]).map((k) => k.deviceId), [laptop.id]);
+    const listed = await ivar.call("mls:devices", { conversationId: conv });
+    assert.equal((listed.devices as { deviceId: string }[]).some((d) => d.deviceId === phone.id), false);
+
+    assert.equal((await publish(hana, await makeDevice("hana-phone-again", 1))).ok, true, "setting it up again is a new device");
+  });
+
   it("are handed out once each, then the last-resort one", async () => {
     const eve = await connect("Eve");
     const frank = await connect("Frank");

src/socket/handlers/mls.ts

@@ -20,6 +20,7 @@ import {
   getUserByServerId,
   insertMessage,
   isMlsDevice,
+  isRemovedMlsDevice,
   listConversationsForUser,
   listMlsDevices,
   listMlsGroupsForMember,
@@ -167,6 +168,10 @@ export function registerMlsHandlers(ctx: HandlerContext): EventHandlerMap {
       ack(fail("invalid_device", "deviceId has to be 1 to 64 letters, digits, - or _."));
       return false;
     }
+    if (isRemovedMlsDevice(auth.tokenPayload.serverUserId, deviceId)) {
+      ack(fail("device_removed", "You removed this device from encrypted messages here."));
+      return false;
+    }
     if (mustExist && !isMlsDevice(auth.tokenPayload.serverUserId, deviceId)) {
       ack(fail("unknown_device", "Publish KeyPackages from this device first."));
       return false;
@@ -345,7 +350,12 @@ export function registerMlsHandlers(ctx: HandlerContext): EventHandlerMap {
         }
 
         const isNew = !isMlsDevice(self, payload.deviceId);
-        if (touchMlsDevice(self, payload.deviceId) === "too_many_devices") {
+        const touched = touchMlsDevice(self, payload.deviceId);
+        if (touched === "device_removed") {
+          ack(fail("device_removed", "You removed this device from encrypted messages here."));
+          return;
+        }
+        if (touched === "too_many_devices") {
           ack(fail("too_many_devices", "You have five devices using encrypted messages here. Remove one first."));
           return;
         }