From e8af62717ebafa28a44c4a1ad14eae0358d18556 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 23 Sep 2026 23:12:12 +0000 Subject: [PATCH] fix: correctness and cost problems found reviewing the migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review pass over the full range turned up eleven things. The two that lose user data: **A deleted account came back.** deleteAccount cleared the legacy file, the key store and the secrets store, but never the account's plain DataStore — deleteUserPreferenceFile sweeps shared_prefs/ and that store lives in filesDir/datastore/. AccountPreferenceStores.removeAccount existed and had no caller. Before this branch the orphan was merely litter, because nothing read it; now it holds nostr_pubkey, so re-adding the same npub found a live identity, setDefaultAccount's downgrade guard saw a writeable account and restored the one the user had just deleted. Everything else in there — cached contact and mute lists, 31 follow-list filters, dismissed polls — also stayed on disk unencrypted for good. **The notifications Global -> Selected migration stamped itself done and threw the result away.** It wrote the corrected filter to the legacy DEFAULT_NOTIFICATION_FOLLOW_LIST, which nothing reads once the follow-list copy marker is set, while the stamp went somewhere that persists. End a session with no save and the next launch skips the migration and reads Global back out of the DataStore — the account stays on raw Global notifications permanently. It now writes through followListStore and stamps only after that write succeeds. Two more that would have bitten later: - The SAVED_ACCOUNTS upgrade branch wrote ALL_ACCOUNT_INFO to the legacy file without mirroring it into the roster store, whose own copy had already run against a key that did not exist yet and whose marker was already set. Those installs would open as a fresh install the moment the legacy write goes — the failure AccountRoster's KDoc calls the most consequential to get wrong. - deletePrivateKey gated its removal on a decrypting read, so a rotated or wiped keystore — exactly when the value is unreadable — skipped the delete and left a deleted account's key on disk. It now tests presence without decrypting, via a new EncryptedDataStore.contains. Two crashes from DataStore's one-store-per-path registry, which only releases on scope cancellation: desktopChessDismissedGamesStore() and Android's SecureKeyStorage both built a store per call over a fixed path. The chess one is reachable today — the view model builds one in its constructor under a remember(account), so reopening that screen threw from an unhandled scope.launch and took the screen's scope down with it. Both are now one store per process. EncryptedSharedPreferences.create was idempotent, which is why neither needed this before. Chess dismissals were fire-and-forget saves of a read-modify-write snapshot, so two in quick succession could land out of order and drop one, and a dismissal racing the async seed wrote a snapshot missing everything already stored. They now go through a single conflated channel with one consumer that writes the current set, and the seed asks for a write when it finds it unioned into a set someone had already persisted. Cost, on paths that run constantly: - saveSecrets did ten separate encrypted-file rewrites per account save, none of which DataStore could skip since AES-GCM re-randomises the IV so the ciphertext differs even when the value does not. One edit now — which also makes the marker mean what LegacyPreferenceCleanup reads it as, since it can no longer exist without the values beside it. loadSecrets likewise reads one snapshot instead of ten flow collections. - SecretEncryption was default-constructed per store: eight AndroidKeyStore loads and eight per-thread Cipher caches for one key alias. One shared instance; both actuals document concurrent use. - updateSavedAccounts compared a MutableStateFlow to a List, so the guard was unconditionally true and every call rewrote both stores. Pre-existing, but this branch put an encrypt and an encrypted-store write behind it. - The cleanup ran inside the mutex that serialises every account load, so once enabled each account on a multi-account cold start would wait for the previous one's full pass. It now runs outside the lock, once, on the call that did the loading. And one design flaw in the new cleanup itself: it compared the stored secrets against the legacy file, justified by their being dual-written — but the check only runs once LEGACY_WRITES_RETIRED turns that off, from which point the legacy copy is frozen. Any account that re-paired a bunker after upgrading would have differed forever and never had its file deleted. Secrets are now gated on the migration marker, like the plain groups. The private-key comparison stays: an npub is derived from its key, so that one cannot legitimately change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AXvKXakvup4inNFfAhhr4L --- .../amethyst/LegacyPreferenceCleanup.kt | 42 +++---- .../amethyst/LocalPreferences.kt | 116 +++++++++++------- .../amethyst/LegacyPreferenceCleanupTest.kt | 43 +++---- .../commons/keystorage/SecureKeyStorage.kt | 44 +++++-- .../commons/nip64Chess/ChessLobbyLogic.kt | 48 +++++--- .../AccountSecretsEncryptedStores.kt | 68 +++++----- .../model/preferences/EncryptedDataStore.kt | 86 ++++++++++++- .../nip64Chess/ChessDismissedGamesStoreJvm.kt | 18 ++- .../preferences/EncryptedDataStoreTest.kt | 84 +++++++++++++ .../ChessDismissedGamesStoreTest.kt | 15 +++ 10 files changed, 405 insertions(+), 159 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanup.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanup.kt index fc7973abe6..9872ab3ac8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanup.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanup.kt @@ -33,7 +33,6 @@ import com.vitorpamplona.amethyst.commons.model.preferences.NotificationPrefsSto import com.vitorpamplona.amethyst.commons.model.preferences.RelayAuthStore import com.vitorpamplona.amethyst.commons.model.preferences.TopNavFollowListStore import com.vitorpamplona.amethyst.commons.model.preferences.UploadSettingsStore -import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyAccountSecrets import com.vitorpamplona.quartz.utils.Log /** @@ -133,9 +132,19 @@ interface MigratedSecrets { * `CopyOnceMigration` writes the values and its marker as a single * `Preferences`, committed atomically, so the marker cannot be set without them. * - * The secrets and the private key are still written to *both* stores on every - * save, so for those the stronger question is available and is asked: read both - * back and require them to agree. + * The private key takes the strongest form: read it back and require it to + * equal the legacy one. That comparison stays valid forever, because an npub is + * derived from its private key, so the key for a given npub can never change. + * + * The secrets cannot be compared, and the reason is worth stating because the + * obvious reading is wrong. They *are* dual-written today — but this whole + * check only runs once [legacyWritesRetired] is true, and from that release on + * the legacy copy is frozen while the live one keeps moving. An account that + * re-pairs a bunker or adds a wallet after upgrading would then differ from the + * file forever and never have it deleted. So they are gated the same way as the + * plain groups: on the copy having run, which + * [AccountSecretsEncryptedStores.loadSecrets] reports by returning non-null + * only once its marker is set, and it writes that marker last. * * # Why an unrecognised key blocks * @@ -223,17 +232,10 @@ class LegacyPreferenceCleanup( npub: String, legacy: LegacyPreferenceSource, ): List { - val expected = readLegacyAccountSecrets(legacy) val reasons = mutableListOf() try { - val stored = secrets.secrets(npub) - when { - stored == null -> reasons += "the secrets have not been copied across" - // Field names only. These values are bunker secrets and wallet - // connection strings; a log line is the last place for them. - stored != expected -> reasons += "the stored secrets differ from the legacy file: ${differingFields(expected, stored)}" - } + if (secrets.secrets(npub) == null) reasons += "the secrets have not been copied across" } catch (e: Exception) { Log.w(TAG, "Could not read the secrets store for $npub", e) reasons += "the secrets store could not be read" @@ -256,22 +258,6 @@ class LegacyPreferenceCleanup( return reasons } - private fun differingFields( - expected: AccountSecrets, - stored: AccountSecrets, - ): String = - listOfNotNull( - "nip46SignerEnabled".takeIf { expected.nip46SignerEnabled != stored.nip46SignerEnabled }, - "nip46BunkerSecret".takeIf { expected.nip46BunkerSecret != stored.nip46BunkerSecret }, - "nip46TransportKey".takeIf { expected.nip46TransportKey != stored.nip46TransportKey }, - "nip46SeenRequestIds".takeIf { expected.nip46SeenRequestIds != stored.nip46SeenRequestIds }, - "nwcWallets".takeIf { expected.nwcWalletsJson != stored.nwcWalletsJson }, - "clinkDebitWallets".takeIf { expected.clinkDebitWalletsJson != stored.clinkDebitWalletsJson }, - "defaultPaymentSourceId".takeIf { expected.defaultPaymentSourceId != stored.defaultPaymentSourceId }, - "defaultNwcWalletId".takeIf { expected.legacyDefaultNwcWalletId != stored.legacyDefaultNwcWalletId }, - "zapPaymentServer".takeIf { expected.legacyZapPaymentRequestServer != stored.legacyZapPaymentRequestServer }, - ).joinToString() - /** * Deletes the account's legacy file if — and only if — [verify] comes back * empty and the app has stopped writing to it. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt index 9df3f2a2f5..8c514f3774 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt @@ -390,10 +390,19 @@ object LocalPreferences { private suspend fun loadAccountStores( npub: String, - legacyIdentity: () -> AccountIdentity, + legacy: SharedPreferences, ) = AccountStoreData( - identity = identityStore(npub).load().orIfUnusable(legacyIdentity), - followLists = followListStore(npub).load(), + identity = + identityStore(npub).load().orIfUnusable { + AccountIdentity( + pubKeyHex = legacy.getString(PrefKeys.NOSTR_PUBKEY, null), + loginWithExternalSigner = legacy.getBoolean(PrefKeys.LOGIN_WITH_EXTERNAL_SIGNER, false), + externalSignerPackageName = legacy.getString(PrefKeys.SIGNER_PACKAGE_NAME, null), + localRelayServers = legacy.getStringSet(PrefKeys.LOCAL_RELAY_SERVERS, null) ?: setOf(), + openBackupConflictsJson = legacy.getString(PrefKeys.OPEN_BACKUP_CONFLICTS, null), + ) + }, + followLists = migrateNotificationFilter(npub, legacy, followListStore(npub).load()), latestEvents = latestEventStore(npub).load(), uploadSettings = uploadSettingsStore(npub).load(), dialogDismissal = dialogDismissalStore(npub).load(), @@ -522,9 +531,17 @@ object LocalPreferences { ) } + val json = JsonMapper.toJson(migrated) edit { - putString(PrefKeys.ALL_ACCOUNT_INFO, JsonMapper.toJson(migrated)) + putString(PrefKeys.ALL_ACCOUNT_INFO, json) } + // Mirrored as well, exactly as updateSavedAccounts does. The + // roster's own copy has already run by this point, against an + // ALL_ACCOUNT_INFO that did not exist yet, and its marker is + // set — so without this the roster store stays permanently + // empty for these installs and they open as a fresh install + // the moment the legacy write goes. + accountRoster.mirrorAllAccountInfoJson(json) migrated } @@ -535,7 +552,10 @@ object LocalPreferences { private suspend fun updateSavedAccounts(accounts: List) = withContext(Dispatchers.IO) { - if (savedAccounts != accounts) { + // .value, not the flow: StateFlow does not override equals, so + // comparing the holder to a List was unconditionally true and every + // call rewrote both stores. + if (savedAccounts.value != accounts) { savedAccounts.emit(accounts) val json = JsonMapper.toJson(accounts.filter { !it.isTransient }) @@ -614,6 +634,12 @@ object LocalPreferences { encryptedPreferences(accountInfo.npub).edit(commit = true) { clear() } accountKeyStore.delete(accountInfo.npub) accountSecretsStore.delete(accountInfo.npub) + // The account's plain DataStore, which deleteUserPreferenceFile cannot + // reach: that sweeps shared_prefs/, this lives in filesDir/datastore/. + // Left behind it would keep the deleted account's pubkey, signer and + // cached events on disk — and re-adding the same npub would find a + // live identity there and resurrect the account that was just deleted. + accountStores.removeAccount(accountInfo.npub) removeAccount(accountInfo) deleteUserPreferenceFile(accountInfo.npub) @@ -981,26 +1007,33 @@ object LocalPreferences { cachedAccounts[npub]?.let { return it } return withContext(Dispatchers.IO) { - mutex.withLock { - cachedAccounts[npub]?.let { return@withContext it } + var loadedHere = false - val accountSettings = innerLoadCurrentAccountFromEncryptedStorage(npub) + val accountSettings = + mutex.withLock { + cachedAccounts[npub]?.let { return@withLock it } - // Only cache successful loads. Caching null would leave the account - // permanently unreachable for the rest of the session if a reader - // raced in before the per-npub file finished being written. - if (accountSettings != null) { - cachedAccounts.put(npub, accountSettings) + val loaded = innerLoadCurrentAccountFromEncryptedStorage(npub) - // Everything this account has is now migrated and just been - // read back, which is the only moment the legacy file can be - // shown to be redundant. It will not be, yet — see - // [LEGACY_WRITES_RETIRED]. - legacyCleanup.deleteIfVerified(npub) + // Only cache successful loads. Caching null would leave the account + // permanently unreachable for the rest of the session if a reader + // raced in before the per-npub file finished being written. + if (loaded != null) { + cachedAccounts.put(npub, loaded) + loadedHere = true + } + + loaded } - return@withContext accountSettings - } + // Outside the lock, and only for the call that did the loading. + // Verifying decrypts the whole legacy file and reads three stores, + // while `mutex` serialises every account load — under the lock, each + // account on a multi-account cold start would wait for the previous + // one's full cleanup pass. Nothing here feeds the load. + if (loadedHere) legacyCleanup.deleteIfVerified(npub) + + accountSettings } } @@ -1021,16 +1054,7 @@ object LocalPreferences { // identity falls back to that file when its store cannot // produce a pubkey: an account without one vanishes from the // app entirely, private key intact. - val stores = - loadAccountStores(npub) { - AccountIdentity( - pubKeyHex = getString(PrefKeys.NOSTR_PUBKEY, null), - loginWithExternalSigner = getBoolean(PrefKeys.LOGIN_WITH_EXTERNAL_SIGNER, false), - externalSignerPackageName = getString(PrefKeys.SIGNER_PACKAGE_NAME, null), - localRelayServers = getStringSet(PrefKeys.LOCAL_RELAY_SERVERS, null) ?: setOf(), - openBackupConflictsJson = getString(PrefKeys.OPEN_BACKUP_CONFLICTS, null), - ) - } + val stores = loadAccountStores(npub, this) val identity = stores.identity val pubKey = identity.pubKeyHex ?: return@with null val privKey = @@ -1427,17 +1451,27 @@ object LocalPreferences { * deliberate raw-Global choice is never reverted. Accounts created after the * split are stamped at save time, so they are never touched here. */ - private fun SharedPreferences.migrateNotificationFilter(current: TopFilter): TopFilter { - if (getBoolean(PrefKeys.NOTIF_GLOBAL_TO_CURATED_MIGRATED, false)) return current + private suspend fun migrateNotificationFilter( + npub: String, + legacy: SharedPreferences, + filters: Map, + ): Map { + if (legacy.getBoolean(PrefKeys.NOTIF_GLOBAL_TO_CURATED_MIGRATED, false)) return filters + val current = filters.getValue(FollowListSlot.NOTIFICATION) val migrated = if (current is TopFilter.Global) TopFilter.Selected else current - edit { - if (migrated !== current) { - putString(PrefKeys.DEFAULT_NOTIFICATION_FOLLOW_LIST, JsonMapper.toJson(migrated)) - } - putBoolean(PrefKeys.NOTIF_GLOBAL_TO_CURATED_MIGRATED, true) - } - return migrated + + // Into the store the loader reads, not the legacy key it no longer does. + // Writing it to the legacy file and stamping anyway left the account on + // raw Global for good: the stamp survives, the corrected value does not, + // and the next launch reads Global back out of the DataStore. + if (migrated !== current) followListStore(npub).save(FollowListSlot.NOTIFICATION, migrated) + + // Stamped only once the value is actually stored, so a failed write + // means the migration runs again rather than being lost. + legacy.edit { putBoolean(PrefKeys.NOTIF_GLOBAL_TO_CURATED_MIGRATED, true) } + + return if (migrated === current) filters else filters + (FollowListSlot.NOTIFICATION to migrated) } /** @@ -1446,11 +1480,11 @@ object LocalPreferences { * returns every slot, so a missing one is a bug in this mapping rather * than a user with no saved filter, and should fail loudly. */ - private fun SharedPreferences.toFollowListPrefs(filters: Map): FollowListPrefs = + private fun toFollowListPrefs(filters: Map): FollowListPrefs = FollowListPrefs( home = filters.getValue(FollowListSlot.HOME), stories = filters.getValue(FollowListSlot.STORIES), - notification = migrateNotificationFilter(filters.getValue(FollowListSlot.NOTIFICATION)), + notification = filters.getValue(FollowListSlot.NOTIFICATION), discovery = filters.getValue(FollowListSlot.DISCOVERY), polls = filters.getValue(FollowListSlot.POLLS), pictures = filters.getValue(FollowListSlot.PICTURES), diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanupTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanupTest.kt index c79704c29a..18bad7f959 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanupTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/LegacyPreferenceCleanupTest.kt @@ -180,39 +180,28 @@ class LegacyPreferenceCleanupTest { assertTrue(!files.deleted) } - /** Both stores are still written, so a disagreement means a write was lost. */ + /** + * A migrated secrets group that has since moved on from the legacy file + * must not block deletion. + * + * This check only ever runs in the release that stopped writing the legacy + * file, so from then on that copy is frozen while the live one keeps + * changing. Comparing the two would mean any account that re-pairs a bunker + * or adds a wallet after upgrading never gets its file deleted. The gate is + * the migration marker, which is what a non-null read reports. + */ @Test - fun secretsThatDisagreeStopTheDeletion() = + fun secretsThatHaveMovedOnSinceTheCopyDoNotBlock() = runTest { val (files, subject) = cleanup( - mapOf("legacy_flag" to true, "nip46BunkerSecret" to "from-the-file"), - secrets = FakeSecrets(stored = AccountSecrets(nip46BunkerSecret = "stale")), + mapOf("legacy_flag" to true, "nip46BunkerSecret" to "what-the-file-still-says"), + secrets = FakeSecrets(stored = AccountSecrets(nip46BunkerSecret = "re-paired since")), ) - val result = subject.deleteIfVerified(NPUB) - - assertEquals( - LegacyCleanupResult.Kept(listOf("the stored secrets differ from the legacy file: nip46BunkerSecret")), - result, - ) - assertTrue(!files.deleted) - } - - /** The values themselves are bunker secrets and wallet strings. */ - @Test - fun aSecretsMismatchNamesTheFieldAndNotTheValue() = - runTest { - val (_, subject) = - cleanup( - mapOf("nwcWallets" to "nostr+walletconnect://deadbeef?secret=hunter2"), - secrets = FakeSecrets(stored = AccountSecrets()), - ) - - val reason = subject.verify(NPUB).single() - - assertTrue(reason, !reason.contains("hunter2")) - assertTrue(reason, reason.contains("nwcWallets")) + assertEquals(emptyList(), subject.verify(NPUB)) + assertEquals(LegacyCleanupResult.Deleted, subject.deleteIfVerified(NPUB)) + assertTrue(files.deleted) } @Test diff --git a/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index f3b08ae257..65ce93363e 100644 --- a/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/androidMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -68,21 +68,33 @@ actual class SecureKeyStorage private actual constructor() { appContext = context.applicationContext return SecureKeyStorage() } - } - private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + /** + * One scope and one store for the whole process, not one per instance. + * + * [create] hands out a new [SecureKeyStorage] on every call — harmless + * when the store was `EncryptedSharedPreferences.create`, which is + * idempotent, but DataStore keeps a process-wide registry keyed by file + * path and only releases an entry when the owning scope ends. A + * per-instance store over a fixed path meant the second instance threw + * "multiple DataStores active for the same file" on its first read — + * which, for this store, reads as the account having no private key. + */ + private val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) - private val store by lazy { - EncryptedDataStore( - PreferenceDataStoreFactory.createWithPath( + private val sharedStore by lazy { + EncryptedDataStore( + PreferenceDataStoreFactory.createWithPath( + scope = scope, + produceFile = { File(appContext.filesDir, STORE_FILE).toOkioPath() }, + ), scope = scope, - produceFile = { File(appContext.filesDir, STORE_FILE).toOkioPath() }, - ), - SecretEncryption(), - scope = scope, - ) + ) + } } + private val store get() = sharedStore + private fun keyFor(npub: String) = stringPreferencesKey(KEY_PREFIX + npub) actual suspend fun savePrivateKey( @@ -117,10 +129,18 @@ actual class SecureKeyStorage private actual constructor() { throw SecureStorageException("Failed to retrieve private key", e) } + /** + * Removes the key unconditionally, and reports whether one was there. + * + * The presence test deliberately does not decrypt. Gating the removal on a + * successful decrypting read meant a rotated or wiped AndroidKeyStore — + * exactly when the value is unreadable — skipped the delete, leaving the + * private key of a deleted account on disk. + */ actual suspend fun deletePrivateKey(npub: String): Boolean = try { - val existed = store.get(keyFor(npub)) != null - if (existed) store.remove(keyFor(npub)) + val existed = store.contains(keyFor(npub)) + store.remove(keyFor(npub)) existed } catch (e: Exception) { throw SecureStorageException("Failed to delete private key", e) diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessLobbyLogic.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessLobbyLogic.kt index d799e48997..eda246f63e 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessLobbyLogic.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessLobbyLogic.kt @@ -35,6 +35,7 @@ import com.vitorpamplona.quartz.utils.TimeUtils import com.vitorpamplona.quartz.utils.cache.LargeCache import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.delay import kotlinx.coroutines.launch @@ -141,16 +142,43 @@ class ChessLobbyLogic( * or two. * * The seed unions rather than replaces, so a dismissal the user makes - * before the read lands is not overwritten by it. + * before the read lands is not overwritten by it — and it asks for a write + * when it did union something in, because that earlier dismissal already + * persisted a snapshot that did not have the stored ids in it. */ private val dismissedGameIds: MutableSet = mutableSetOf() + /** + * Serialises persistence, so a save cannot land out of order. + * + * Each dismissal used to launch its own `storage.save(snapshot)`. Two of + * them are unordered on the same dispatcher, so the first dismissal's + * smaller snapshot could be written *after* the second's and drop it — the + * game came back on the next launch. One consumer, writing whatever the set + * currently holds, cannot reorder; conflation is safe for the same reason, + * since a dropped signal is one whose contents the next write includes. + */ + private val persistRequests = Channel(Channel.CONFLATED) + init { dismissedStorage?.let { storage -> + scope.launch { + for (unused in persistRequests) { + storage.save(userPubkey, dismissedGameIdsLock.withLock { dismissedGameIds.toSet() }) + } + } scope.launch { val stored = storage.load(userPubkey) if (stored.isNotEmpty()) { - dismissedGameIdsLock.withLock { dismissedGameIds.addAll(stored) } + val union = + dismissedGameIdsLock.withLock { + dismissedGameIds.addAll(stored) + dismissedGameIds.size + } + // Larger than what was on disk means a dismissal beat this + // read, and the snapshot it wrote is missing everything that + // was already stored. Write the union back. + if (union > stored.size) persistRequests.trySend(Unit) } } } @@ -984,23 +1012,15 @@ class ChessLobbyLogic( fun dismissCompletedGame(gameId: String) { state.removeCompletedGame(gameId) - val snapshot = - dismissedGameIdsLock.withLock { - dismissedGameIds.add(gameId) - dismissedGameIds.toSet() - } - dismissedStorage?.let { storage -> scope.launch { storage.save(userPubkey, snapshot) } } + dismissedGameIdsLock.withLock { dismissedGameIds.add(gameId) } + persistRequests.trySend(Unit) } fun dismissAllCompletedGames() { val allIds = state.completedGames.value.map { it.gameId } state.clearCompletedGames() - val snapshot = - dismissedGameIdsLock.withLock { - dismissedGameIds.addAll(allIds) - dismissedGameIds.toSet() - } - dismissedStorage?.let { storage -> scope.launch { storage.save(userPubkey, snapshot) } } + dismissedGameIdsLock.withLock { dismissedGameIds.addAll(allIds) } + persistRequests.trySend(Unit) } /** diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsEncryptedStores.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsEncryptedStores.kt index c56bf6b9bd..a359fbcfd2 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsEncryptedStores.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsEncryptedStores.kt @@ -126,53 +126,53 @@ class AccountSecretsEncryptedStores( * genuinely holds no secrets must not trigger it forever. */ suspend fun loadSecrets(npub: String): AccountSecrets? { - val store = getDataStore(npub) - if (store.get(AccountSecretKeys.migrated) == null) return null + // One snapshot for the whole group rather than ten flow collections. + val stored = getDataStore(npub).snapshot() + if (stored[AccountSecretKeys.migrated] == null) return null return AccountSecrets( - nip46SignerEnabled = store.get(AccountSecretKeys.nip46SignerEnabled).toBoolean(), - nip46BunkerSecret = store.get(AccountSecretKeys.nip46BunkerSecret) ?: "", - nip46TransportKey = store.get(AccountSecretKeys.nip46TransportKey) ?: "", - nip46SeenRequestIds = decodeSet(store.get(AccountSecretKeys.nip46SeenRequestIds)), - nwcWalletsJson = store.get(AccountSecretKeys.nwcWallets), - clinkDebitWalletsJson = store.get(AccountSecretKeys.clinkDebitWallets), - defaultPaymentSourceId = store.get(AccountSecretKeys.defaultPaymentSourceId), - legacyDefaultNwcWalletId = store.get(AccountSecretKeys.legacyDefaultNwcWalletId), - legacyZapPaymentRequestServer = store.get(AccountSecretKeys.legacyZapPaymentRequestServer), + nip46SignerEnabled = stored[AccountSecretKeys.nip46SignerEnabled].toBoolean(), + nip46BunkerSecret = stored[AccountSecretKeys.nip46BunkerSecret] ?: "", + nip46TransportKey = stored[AccountSecretKeys.nip46TransportKey] ?: "", + nip46SeenRequestIds = decodeSet(stored[AccountSecretKeys.nip46SeenRequestIds]), + nwcWalletsJson = stored[AccountSecretKeys.nwcWallets], + clinkDebitWalletsJson = stored[AccountSecretKeys.clinkDebitWallets], + defaultPaymentSourceId = stored[AccountSecretKeys.defaultPaymentSourceId], + legacyDefaultNwcWalletId = stored[AccountSecretKeys.legacyDefaultNwcWalletId], + legacyZapPaymentRequestServer = stored[AccountSecretKeys.legacyZapPaymentRequestServer], ) } /** - * Writes the group, then the marker. + * Writes the group and its marker as one edit. * - * Marker last on purpose: a crash midway leaves the account looking - * unmigrated, so the next load copies from the legacy file again rather - * than reading a half-written set of secrets as complete. + * One edit, not ten. Every account save runs this, and a key at a time cost + * ten encrypted-file rewrites — none of which DataStore could skip, because + * AES-GCM re-randomises the IV so the ciphertext differs even when the value + * does not. + * + * It also makes the marker meaningful. Written in its own transaction after + * the others it merely *tended* to be last; in the same one it cannot exist + * without them, so a marker found on disk proves a complete group — which is + * what `LegacyPreferenceCleanup` reads it as before deleting the legacy file. */ suspend fun saveSecrets( npub: String, value: AccountSecrets, ) { - val store = getDataStore(npub) + getDataStore(npub).edit { + put(AccountSecretKeys.nip46SignerEnabled, value.nip46SignerEnabled.toString()) + put(AccountSecretKeys.nip46BunkerSecret, value.nip46BunkerSecret) + put(AccountSecretKeys.nip46TransportKey, value.nip46TransportKey) + put(AccountSecretKeys.nip46SeenRequestIds, value.nip46SeenRequestIds.joinToString(AccountSecretKeys.SET_SEPARATOR)) + putOrRemove(AccountSecretKeys.nwcWallets, value.nwcWalletsJson) + putOrRemove(AccountSecretKeys.clinkDebitWallets, value.clinkDebitWalletsJson) + putOrRemove(AccountSecretKeys.defaultPaymentSourceId, value.defaultPaymentSourceId) + putOrRemove(AccountSecretKeys.legacyDefaultNwcWalletId, value.legacyDefaultNwcWalletId) + putOrRemove(AccountSecretKeys.legacyZapPaymentRequestServer, value.legacyZapPaymentRequestServer) - store.save(AccountSecretKeys.nip46SignerEnabled, value.nip46SignerEnabled.toString()) - store.save(AccountSecretKeys.nip46BunkerSecret, value.nip46BunkerSecret) - store.save(AccountSecretKeys.nip46TransportKey, value.nip46TransportKey) - store.save(AccountSecretKeys.nip46SeenRequestIds, value.nip46SeenRequestIds.joinToString(AccountSecretKeys.SET_SEPARATOR)) - store.putOrRemove(AccountSecretKeys.nwcWallets, value.nwcWalletsJson) - store.putOrRemove(AccountSecretKeys.clinkDebitWallets, value.clinkDebitWalletsJson) - store.putOrRemove(AccountSecretKeys.defaultPaymentSourceId, value.defaultPaymentSourceId) - store.putOrRemove(AccountSecretKeys.legacyDefaultNwcWalletId, value.legacyDefaultNwcWalletId) - store.putOrRemove(AccountSecretKeys.legacyZapPaymentRequestServer, value.legacyZapPaymentRequestServer) - - store.save(AccountSecretKeys.migrated, "true") - } - - private suspend fun EncryptedDataStore.putOrRemove( - key: androidx.datastore.preferences.core.Preferences.Key, - value: String?, - ) { - if (value != null) save(key, value) else remove(key) + put(AccountSecretKeys.migrated, "true") + } } private fun decodeSet(raw: String?): Set = diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStore.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStore.kt index 01b26082bb..477e50fe0b 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStore.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStore.kt @@ -21,6 +21,7 @@ package com.vitorpamplona.amethyst.commons.model.preferences import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.MutablePreferences import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.emptyPreferences @@ -41,7 +42,7 @@ import kotlin.io.encoding.Base64 */ class EncryptedDataStore( private val store: DataStore, - private val encryption: SecretEncryption = SecretEncryption(), + private val encryption: SecretEncryption = sharedSecretEncryption, private val scope: CoroutineScope, ) { private fun encrypt(value: String): String = Base64.encode(encryption.encrypt(value.encodeToByteArray())) @@ -74,6 +75,19 @@ class EncryptedDataStore( ?.get(key) ?.let { decrypt(it) } + /** + * Whether the key is present, without decrypting it. + * + * For callers that only need presence — deleting, say. [get] would report a + * value it cannot decrypt as absent, which is the wrong answer when the + * decision being made is whether to remove it. + */ + suspend fun contains(key: Preferences.Key): Boolean = + store.data + .catch { e -> if (e is IOException) emit(emptyPreferences()) else throw e } + .firstOrNull() + ?.contains(key) == true + /** * The value, or null only when the key is genuinely absent. * @@ -84,6 +98,66 @@ class EncryptedDataStore( */ suspend fun getOrThrow(key: Preferences.Key): String? = store.data.first()[key]?.let { decrypt(it) } + /** + * Reads or writes several keys against one snapshot of the store. + * + * A key at a time costs a full DataStore round trip each — a transform, a + * serialize, a temp-file write, an fsync and a rename to save; a fresh flow + * collection to read. Worse for writes, AES-GCM re-randomises the IV, so the + * ciphertext differs every time and DataStore's "value unchanged, skip the + * write" shortcut never fires: all of them always reach disk. + * + * One [edit] is also a single transaction, which is what lets a group be + * written with its own migration marker and never be seen half-applied. + */ + suspend fun edit(block: Editor.() -> Unit) { + store.edit { prefs -> Editor(prefs, ::encrypt).block() } + } + + class Editor internal constructor( + private val prefs: MutablePreferences, + private val encrypt: (String) -> String, + ) { + fun put( + key: Preferences.Key, + value: String, + ) { + prefs[key] = encrypt(value) + } + + fun remove(key: Preferences.Key) { + prefs.remove(key) + } + + fun putOrRemove( + key: Preferences.Key, + value: String?, + ) { + if (value != null) put(key, value) else remove(key) + } + } + + /** + * One snapshot of the store, decrypting on access. + * + * Reads every key of a group against the same collection, and — since the + * values are one atomic write — against the same version of it. + */ + suspend fun snapshot(): Snapshot = + Snapshot( + store.data + .catch { e -> if (e is IOException) emit(emptyPreferences()) else throw e } + .firstOrNull() ?: emptyPreferences(), + ::decrypt, + ) + + class Snapshot internal constructor( + private val prefs: Preferences, + private val decrypt: (String) -> String?, + ) { + operator fun get(key: Preferences.Key): String? = prefs[key]?.let(decrypt) + } + fun getProperty( key: Preferences.Key, parser: (String) -> T, @@ -108,3 +182,13 @@ class EncryptedDataStore( scope = scope, ) } + +/** + * The one [SecretEncryption] every encrypted store shares. + * + * Its constructor loads the AndroidKeyStore and its first use probes the key's + * security level, and each instance keeps its own per-thread Cipher cache — all + * for a single key alias. There were eight instances doing that independently. + * Both actuals are documented as safe for concurrent use, so one will do. + */ +internal val sharedSecretEncryption: SecretEncryption by lazy { SecretEncryption() } diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreJvm.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreJvm.kt index 5d0153c51d..436f56a2a9 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreJvm.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreJvm.kt @@ -33,8 +33,22 @@ import java.io.File * carrying it over — the dismissed list is a convenience, and chess has few * enough users that a migration is not worth the code. */ -fun desktopChessDismissedGamesStore(): ChessDismissedGamesStore { +fun desktopChessDismissedGamesStore(): ChessDismissedGamesStore = sharedStore + +/** + * One store for the process. + * + * DataStore keeps a process-wide registry keyed by file path and only releases + * an entry when the owning scope ends; the factory's own scope never does. So + * building a fresh store per call — and the chess view model builds one in its + * constructor, under a `remember(account)` — made the second one throw + * "multiple DataStores active for the same file" on its first read. That + * surfaced from a `scope.launch` with no handler, taking the screen's whole + * scope down with it. The `java.util.prefs` node this replaced was safe to + * construct repeatedly, so nothing here used to need a singleton. + */ +private val sharedStore: ChessDismissedGamesStore by lazy { val file = File(appDataDir, "chess_dismissed_games.preferences_pb") file.parentFile?.mkdirs() - return ChessDismissedGamesStore(PreferenceDataStoreFactory.createWithPath(produceFile = { file.toOkioPath() })) + ChessDismissedGamesStore(PreferenceDataStoreFactory.createWithPath(produceFile = { file.toOkioPath() })) } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStoreTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStoreTest.kt index 6354f5e924..97c2b6ec3a 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStoreTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/EncryptedDataStoreTest.kt @@ -21,6 +21,7 @@ package com.vitorpamplona.amethyst.commons.model.preferences import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -211,4 +212,87 @@ class EncryptedDataStoreTest { ) scope.cancel() } + + @Test + fun editWritesEveryKeyInOneGo() = + runTest { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val subject = store(scope) + val other = stringPreferencesKey("bunker") + + subject.edit { + put(key, "wallet") + put(other, "secret") + } + + val snapshot = subject.snapshot() + assertEquals("wallet", snapshot[key]) + assertEquals("secret", snapshot[other]) + } + + /** + * The whole point of writing a group in one edit: a marker written beside + * its values cannot be found on disk without them. + */ + @Test + fun editIsOneTransaction() = + runTest { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val subject = store(scope) + val marker = stringPreferencesKey("migrated") + + runCatching { + subject.edit { + put(key, "wallet") + put(marker, "true") + throw IllegalStateException("crash midway") + } + } + + val snapshot = subject.snapshot() + assertNull(snapshot[marker]) + assertNull(snapshot[key]) + } + + @Test + fun putOrRemoveClearsANullValue() = + runTest { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val subject = store(scope) + + subject.edit { put(key, "wallet") } + subject.edit { putOrRemove(key, null) } + + assertNull(subject.snapshot()[key]) + } + + /** + * `contains` must not decrypt. + * + * A value the current key cannot decrypt — a rotated or wiped keystore — is + * still a value that is there, and deleting it has to happen anyway. + * `deletePrivateKey` gated its removal on a decrypting read and so skipped + * exactly the case that needed it, leaving a deleted account's private key + * on disk. + */ + @Test + fun containsSeesAValueThatCannotBeDecrypted() = + runTest { + val scope = CoroutineScope(Dispatchers.IO + SupervisorJob()) + val n = seq++ + val dataFile = File(folder.root, "secrets_$n.preferences_pb") + val raw = + PreferenceDataStoreFactory.createWithPath(scope = scope, produceFile = { dataFile.toOkioPath() }) + // Ciphertext this store's key was never used to produce. + raw.edit { prefs -> prefs[key] = "bm90LWFjdHVhbGx5LWNpcGhlcnRleHQ=" } + + val subject = + EncryptedDataStore(raw, SecretEncryption(File(folder.root, "secret_$n.key")), scope = scope) + + assertTrue(subject.contains(key)) + assertNull(runCatching { subject.get(key) }.getOrNull()) + + subject.remove(key) + assertTrue(!subject.contains(key)) + } } diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreTest.kt index 0d4efd334f..9e28130961 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/nip64Chess/ChessDismissedGamesStoreTest.kt @@ -31,6 +31,7 @@ import kotlinx.coroutines.test.runTest import okio.Path.Companion.toOkioPath import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertSame import org.junit.Assert.assertTrue import org.junit.Rule import org.junit.Test @@ -104,4 +105,18 @@ class ChessDismissedGamesStoreTest { assertEquals(setOf("c"), store.load("npub1")) } + + /** + * The desktop factory has to hand back one store, not a new one per call. + * + * DataStore registers a live store per file path and only releases it when + * the owning scope ends — and the factory's own scope never does. Building + * a fresh one per call meant the second `IllegalStateException: multiple + * DataStores active for the same file`, thrown from the chess view model's + * constructor the second time that screen opened. + */ + @Test + fun theDesktopFactoryReturnsOneStore() { + assertSame(desktopChessDismissedGamesStore(), desktopChessDismissedGamesStore()) + } }