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()) + } }