diff --git a/doc/numberless-accounts.md b/doc/numberless-accounts.md index f063786d..fa8c3b47 100644 --- a/doc/numberless-accounts.md +++ b/doc/numberless-accounts.md @@ -48,8 +48,9 @@ account has no phone number, and `Signal.getSelfACI` returns the ACI. Run `./gradlew test installDist` with JDK 25. Regression tests cover the linking request (including ACI authentication and omission of PNI keys), numbered-account -linking, missing group credential salts, persisted account reload, account discovery, -group membership lookup, recipient-number formatting, and JSON account identifiers. +linking, missing group credential salts, persisted account reload, account discovery +(including superseded accounts and legacy indexes), group membership lookup, +recipient-number formatting, and JSON account identifiers. D-Bus transport tests require a session bus. To run them on an isolated bus, use `dbus-run-session -- ./gradlew --no-daemon :test --tests '*NumberlessDbusTest*' --rerun-tasks`. diff --git a/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStorage.java b/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStorage.java index 40107986..ca202b67 100644 --- a/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStorage.java +++ b/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStorage.java @@ -4,5 +4,6 @@ import java.util.List; public record AccountsStorage(List accounts, Integer version) { - public record Account(String path, String environment, String number, String uuid) {} + // Older indexes omit this flag; entries with only a UUID must then be checked against their account state. + public record Account(String path, String environment, String number, String uuid, Boolean numberless) {} } diff --git a/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStore.java b/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStore.java index e9f27384..8d3ade20 100644 --- a/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStore.java +++ b/lib/src/main/java/org/asamk/signal/manager/storage/accounts/AccountsStore.java @@ -63,7 +63,7 @@ public class AccountsStore { public synchronized Set getAllAccounts() throws IOException { return readAccounts().stream() .filter(a -> a.environment() == null || serviceEnvironment.equals(a.environment())) - .filter(a -> a.number() != null || a.uuid() != null) + .filter(a -> a.number() != null || (a.uuid() != null && Boolean.TRUE.equals(a.numberless()))) .collect(Collectors.toSet()); } @@ -98,14 +98,15 @@ public class AccountsStore { return new AccountsStorage.Account(a.path(), serviceEnvironment, number, - aci == null ? null : aci.toString()); + aci == null ? null : aci.toString(), + number == null && aci != null); } if (number != null && number.equals(a.number())) { - return new AccountsStorage.Account(a.path(), a.environment(), null, a.uuid()); + return new AccountsStorage.Account(a.path(), a.environment(), null, a.uuid(), false); } if (aci != null && aci.toString().equals(a.uuid())) { - return new AccountsStorage.Account(a.path(), a.environment(), a.number(), null); + return new AccountsStorage.Account(a.path(), a.environment(), a.number(), null, false); } return a; @@ -117,7 +118,8 @@ public class AccountsStore { final var account = new AccountsStorage.Account(accountPath, serviceEnvironment, number, - aci == null ? null : aci.toString()); + aci == null ? null : aci.toString(), + number == null && aci != null); updateAccounts(accounts -> { final var existingAccounts = accounts.stream().map(a -> { if (a.environment() != null && !serviceEnvironment.equals(a.environment())) { @@ -125,10 +127,10 @@ public class AccountsStore { } if (number != null && number.equals(a.number())) { - return new AccountsStorage.Account(a.path(), a.environment(), null, a.uuid()); + return new AccountsStorage.Account(a.path(), a.environment(), null, a.uuid(), false); } if (aci != null && aci.toString().equals(a.uuid())) { - return new AccountsStorage.Account(a.path(), a.environment(), a.number(), null); + return new AccountsStorage.Account(a.path(), a.environment(), a.number(), null, false); } return a; @@ -159,7 +161,7 @@ public class AccountsStore { private void createInitialAccounts() throws IOException { final var legacyAccountPaths = getLegacyAccountPaths(); final var accountsStorage = new AccountsStorage(legacyAccountPaths.stream() - .map(number -> new AccountsStorage.Account(number, null, number, null)) + .map(number -> new AccountsStorage.Account(number, null, number, null, false)) .toList(), CURRENT_STORAGE_VERSION); IOUtils.createPrivateDirectories(dataPath); @@ -191,7 +193,7 @@ public class AccountsStore { private List readAccounts() throws IOException { final var pair = openFileChannel(getAccountsFile()); try (final var fileChannel = pair.first(); final var lock = pair.second()) { - final var storage = readAccountsLocked(fileChannel); + var storage = readAccountsLocked(fileChannel); var accountsVersion = storage.version() == null ? 1 : storage.version(); if (accountsVersion > CURRENT_STORAGE_VERSION) { @@ -200,12 +202,36 @@ public class AccountsStore { throw new IOException("Accounts file was created by a no longer supported older version: " + accountsVersion); } else if (accountsVersion < CURRENT_STORAGE_VERSION) { - return upgradeAccountsFile(fileChannel, storage, accountsVersion).accounts(); + storage = upgradeAccountsFile(fileChannel, storage, accountsVersion); } - return storage.accounts(); + final var accounts = storage.accounts().stream().map(this::resolveNumberlessAccount).toList(); + if (!accounts.equals(storage.accounts())) { + saveAccountsLocked(fileChannel, new AccountsStorage(accounts, storage.version())); + } + return accounts; } } + private AccountsStorage.Account resolveNumberlessAccount(final AccountsStorage.Account entry) { + if (entry.numberless() != null) { + return entry; + } + + var numberless = false; + if (entry.number() == null && entry.uuid() != null) { + // Older entries also lose their number when another account takes it over. + try (final var account = accountLoader.loadAccountOrNull(entry.path())) { + if (account == null) { + // Keep the entry unresolved so a locked or unavailable account can be checked later. + return entry; + } + numberless = account.getNumber() == null && account.getAci() != null && entry.uuid() + .equals(account.getAci().toString()); + } + } + return new AccountsStorage.Account(entry.path(), entry.environment(), entry.number(), entry.uuid(), numberless); + } + private AccountsStorage upgradeAccountsFile( final FileChannel fileChannel, final AccountsStorage storage, @@ -226,7 +252,8 @@ public class AccountsStore { return new AccountsStorage.Account(a.path(), getServiceEnvironmentString(account.getServiceEnvironment()), a.number(), - a.uuid()); + a.uuid(), + a.numberless()); } }).toList(); } diff --git a/lib/src/test/java/org/asamk/signal/manager/storage/accounts/AccountsStoreTest.java b/lib/src/test/java/org/asamk/signal/manager/storage/accounts/AccountsStoreTest.java index 2ed5eb31..5f30dd64 100644 --- a/lib/src/test/java/org/asamk/signal/manager/storage/accounts/AccountsStoreTest.java +++ b/lib/src/test/java/org/asamk/signal/manager/storage/accounts/AccountsStoreTest.java @@ -1,13 +1,22 @@ package org.asamk.signal.manager.storage.accounts; +import org.asamk.signal.manager.Settings; import org.asamk.signal.manager.api.ServiceEnvironment; +import org.asamk.signal.manager.storage.SignalAccount; +import org.asamk.signal.manager.storage.Utils; +import org.asamk.signal.manager.util.KeyUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.signal.core.models.ServiceId.ACI; +import java.io.IOException; +import java.nio.channels.OverlappingFileLockException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Set; +import java.util.stream.Collectors; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -15,6 +24,9 @@ import static org.junit.jupiter.api.Assertions.assertTrue; class AccountsStoreTest { + private static final ACI OLD_ACI = ACI.parseOrThrow("11111111-1111-4111-8111-111111111111"); + private static final ACI NEW_ACI = ACI.parseOrThrow("22222222-2222-4222-8222-222222222222"); + @TempDir Path directory; @@ -46,4 +58,151 @@ class AccountsStoreTest { assertEquals(newPath, store.getPathByAci(aci)); assertEquals(1, store.getAllAccounts().size()); } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void replacingANumberDoesNotRediscoverTheOldAccountAsNumberless(final boolean update) throws Exception { + final var store = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, path -> null); + final var oldPath = store.addAccount("+12025550123", OLD_ACI); + Files.createFile(directory.resolve(oldPath)); + final String newPath; + if (update) { + newPath = store.addAccount("+12025550124", NEW_ACI); + store.updateAccount(newPath, "+12025550123", NEW_ACI); + } else { + newPath = store.addAccount("+12025550123", NEW_ACI); + } + + final var reopened = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, path -> { + throw new AssertionError("New entries should not require loading account state for discovery"); + }); + assertEquals(Set.of(newPath), getAccountPaths(reopened)); + assertEquals(newPath, reopened.getPathByNumber("+12025550123")); + assertEquals(oldPath, reopened.getPathByAci(OLD_ACI)); + + // Only an explicit account update should turn the old entry into a numberless account. + reopened.updateAccount(oldPath, null, OLD_ACI); + assertEquals(Set.of(oldPath, newPath), getAccountPaths(reopened)); + } + + @ParameterizedTest + @ValueSource(ints = {1, 2}) + void legacyIndexesDistinguishNumberlessAccountsFromSupersededNumberedAccounts(final int version) throws Exception { + createSavedAccount("old", "+12025550123", OLD_ACI); + createSavedAccount("numberless", null, NEW_ACI); + final var legacyIndex = """ + { + "version": %d, + "accounts": [ + {"path": "old", "environment": %s, "number": null, "uuid": "%s"}, + {"path": "numberless", "environment": %s, "number": null, "uuid": "%s"}, + {"path": "current", "environment": "STAGING", "number": "+12025550123", "uuid": null} + ] + } + """.formatted(version, + version == 1 ? "null" : "\"STAGING\"", + OLD_ACI, + version == 1 ? "null" : "\"STAGING\"", + NEW_ACI); + Files.writeString(directory.resolve("accounts.json"), legacyIndex); + + final var store = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, this::loadAccountOrNull); + assertEquals(Set.of("numberless", "current"), getAccountPaths(store)); + assertEquals("old", store.getPathByAci(OLD_ACI)); + assertEquals("numberless", store.getPathByAci(NEW_ACI)); + assertEquals(Set.of("+12025550123"), store.getAllNumbers()); + + final var reopened = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, path -> { + throw new AssertionError("Resolved entries should not require loading account state again"); + }); + assertEquals(Set.of("numberless", "current"), getAccountPaths(reopened)); + assertTrue(new AccountsStore(directory.toFile(), + ServiceEnvironment.LIVE, + this::loadAccountOrNull).getAllAccounts().isEmpty()); + } + + @Test + void retriesLockedLegacyAccountsAndPersistsSuccessfulClassification() throws Exception { + createSavedAccount("numberless", null, NEW_ACI); + writeLegacyNumberlessEntry(NEW_ACI); + final var store = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, this::loadAccountOrNull); + + try (final var account = SignalAccount.load(directory.toFile(), "numberless", false, Settings.DEFAULT)) { + assertTrue(store.getAllAccounts().isEmpty()); + assertEquals("numberless", store.getPathByAci(NEW_ACI)); + final var storage = Utils.createStorageObjectMapper() + .readValue(directory.resolve("accounts.json").toFile(), AccountsStorage.class); + assertNull(storage.accounts().getFirst().numberless()); + } + + assertEquals(Set.of("numberless"), getAccountPaths(store)); + try (final var account = SignalAccount.load(directory.toFile(), "numberless", false, Settings.DEFAULT)) { + final var reopened = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, path -> { + throw new AssertionError("A classified account should remain discoverable while locked"); + }); + assertEquals(Set.of("numberless"), getAccountPaths(reopened)); + } + } + + @Test + void retriesLegacyAccountsAfterMissingStateIsRestored() throws Exception { + writeLegacyNumberlessEntry(NEW_ACI); + final var store = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, this::loadAccountOrNull); + assertTrue(store.getAllAccounts().isEmpty()); + assertEquals("numberless", store.getPathByAci(NEW_ACI)); + + createSavedAccount("numberless", null, NEW_ACI); + assertEquals(Set.of("numberless"), getAccountPaths(store)); + } + + @Test + void doesNotDiscoverLegacyEntriesWithAMismatchedAci() throws Exception { + createSavedAccount("numberless", null, NEW_ACI); + writeLegacyNumberlessEntry(OLD_ACI); + final var store = new AccountsStore(directory.toFile(), ServiceEnvironment.STAGING, this::loadAccountOrNull); + assertTrue(store.getAllAccounts().isEmpty()); + assertEquals("numberless", store.getPathByAci(OLD_ACI)); + } + + private Set getAccountPaths(final AccountsStore store) throws IOException { + return store.getAllAccounts().stream().map(AccountsStorage.Account::path).collect(Collectors.toSet()); + } + + private void writeLegacyNumberlessEntry(final ACI aci) throws IOException { + final var legacyIndex = """ + { + "version": 2, + "accounts": [ + {"path": "numberless", "environment": "STAGING", "number": null, "uuid": "%s"} + ] + } + """.formatted(aci); + Files.writeString(directory.resolve("accounts.json"), legacyIndex); + } + + private void createSavedAccount(final String path, final String number, final ACI aci) throws IOException { + // Account creation needs a number until the ACI has been assigned. + try (final var account = SignalAccount.create(directory.toFile(), + path, + "+12025550123", + ServiceEnvironment.STAGING, + KeyUtils.generateIdentityKeyPair(), + KeyUtils.generateIdentityKeyPair(), + KeyUtils.createProfileKey(), + Settings.DEFAULT)) { + account.setAci(aci); + account.setNumber(number); + } + } + + private SignalAccount loadAccountOrNull(final String path) { + if (!SignalAccount.accountFileExists(directory.toFile(), path)) { + return null; + } + try { + return SignalAccount.load(directory.toFile(), path, false, Settings.DEFAULT); + } catch (IOException | OverlappingFileLockException e) { + return null; + } + } } diff --git a/src/main/resources/META-INF/native-image/org.asamk/signal-cli/reachability-metadata.json b/src/main/resources/META-INF/native-image/org.asamk/signal-cli/reachability-metadata.json index 60ff4c40..2c4f242d 100644 --- a/src/main/resources/META-INF/native-image/org.asamk/signal-cli/reachability-metadata.json +++ b/src/main/resources/META-INF/native-image/org.asamk/signal-cli/reachability-metadata.json @@ -4012,7 +4012,8 @@ "java.lang.String", "java.lang.String", "java.lang.String", - "java.lang.String" + "java.lang.String", + "java.lang.Boolean" ] }, { @@ -4023,6 +4024,10 @@ "name": "number", "parameterTypes": [] }, + { + "name": "numberless", + "parameterTypes": [] + }, { "name": "path", "parameterTypes": []