From 04ef41281aa38356d4f60690d679ee5d1601bfde Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 22:53:59 +0000 Subject: [PATCH] refactor: move the location-chat identity into the encrypted DataStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GeohashChatIdentityState was the last thing in the app still reading and writing EncryptedSharedPreferences outside the four deliberate mirrors. It kept two keys — the seed its per-geohash throwaway keys derive from, and the handle the user posts under — and neither was covered by the 112-key migration, because neither is in PrefKeys: they are private constants in the state object, so LegacyKeyCoverageTest, which reflects over PrefKeys, structurally could not see them. They were also in a file nothing else uses. The writer passed `signer.pubKey`, which is hex, where every other caller passes an npub, so the identity lived in `secret_keeper_` while the account's own secrets live in `secret_keeper_`. That makes it an orphan rather than a hazard: LegacyPreferenceCleanup enumerates and deletes the npub file, so it never saw these keys, and deleting that file could not have lost them. It also means nothing will ever clean the hex file up on its own. So: read the hex file once, copy into the account's npub-keyed encrypted DataStore, prefer the new store on read, and keep mirroring the legacy write until the legacy writes are retired app-wide — the same terms as AccountSecrets. GeohashIdentitySecrets is its own group with its own migrated marker, and that is load-bearing rather than tidy. Every account save mirrors a whole AccountSecrets built field by field from AccountSettings, which does not hold these — they belong to the state object. Folded into that group, each save would write null over them, and the group save uses putOrRemove, so null removes: the seed would disappear on the next unrelated save and every geohash identity the user has would silently change. Its own marker for the same reason the group is separate — the two migrate out of different files, so neither marker can speak for the other. The keys are deliberately NOT added to LegacyAccountSecretNames.all. That set is what the cleanup gate treats as claimed in the npub file, and these never appear in it; listing them would be inert and would suggest the gate handles them. Shape changes this forced: nickname() and keyPair() are suspend (every call site was already inside withContext(Dispatchers.IO)); setNickname stays fire-and-forget on the account scope, as the SharedPreferences edit {} it replaces already was; and seed creation moved from synchronized to a Mutex, because the store reads it guards are suspending and two racers minting different seeds would strand one caller's identities. Tests: seven, covering the round trip, a seed with no handle, an empty identity still counting as migrated, the two markers staying independent in both directions, and anAccountSaveLeavesTheGeohashIdentityAlone — which pins the wipe this design exists to prevent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AXvKXakvup4inNFfAhhr4L --- .../amethyst/AccountSecretsStore.kt | 41 ++++++ .../vitorpamplona/amethyst/model/Account.kt | 2 +- .../model/GeohashChatIdentityState.kt | 123 ++++++++++++------ .../model/preferences/AccountSecrets.kt | 65 +++++++++ .../AccountSecretsEncryptedStores.kt | 33 +++++ .../preferences/AccountSecretsStoreTest.kt | 90 +++++++++++++ 6 files changed, 311 insertions(+), 43 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/AccountSecretsStore.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/AccountSecretsStore.kt index e60aaa48cf..ee32dde855 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/AccountSecretsStore.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/AccountSecretsStore.kt @@ -22,6 +22,7 @@ package com.vitorpamplona.amethyst import com.vitorpamplona.amethyst.commons.model.preferences.AccountSecrets import com.vitorpamplona.amethyst.commons.model.preferences.AccountSecretsEncryptedStores +import com.vitorpamplona.amethyst.commons.model.preferences.GeohashIdentitySecrets import com.vitorpamplona.quartz.utils.Log import okio.Path.Companion.toOkioPath @@ -98,6 +99,46 @@ class AccountSecretsStore( */ suspend fun stored(npub: String): AccountSecrets? = stores.loadSecrets(npub) + // ── the location-chat identity ──────────────────────────────────── + + /** + * The account's location-chat identity, migrating out of the legacy file on + * first use, on the same terms as [read]. + * + * @param legacy what `secret_keeper_` holds. Note the *hex*: this + * group's legacy file is keyed by the signer's pubkey rather than the npub + * every other group uses, so the caller opens a different file for it. + */ + suspend fun readGeohashIdentity( + npub: String, + legacy: GeohashIdentitySecrets, + ): GeohashIdentitySecrets { + val stored = + try { + stores.loadGeohashIdentity(npub) + } catch (e: Exception) { + Log.w(TAG, "Could not read the location-chat identity for $npub; using the legacy file", e) + return legacy + } + + if (stored != null) return stored + + mirrorGeohashIdentity(npub, legacy) + return legacy + } + + /** Mirrors a save into the new store. The legacy write stays where it is. */ + suspend fun mirrorGeohashIdentity( + npub: String, + value: GeohashIdentitySecrets, + ) { + try { + stores.saveGeohashIdentity(npub, value) + } catch (e: Exception) { + Log.w(TAG, "Could not write the location-chat identity for $npub to the current store", e) + } + } + suspend fun delete(npub: String) { try { stores.removeAccount(npub) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index f99574df02..385a1e6389 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -759,7 +759,7 @@ class Account( val geohashList = GeohashListState(signer, cache, geohashListDecryptionCache, scope, settings) // Anonymous, per-geohash throwaway identities for Bitchat-interoperable location chats. - val geohashIdentity = GeohashChatIdentityState(signer) + val geohashIdentity = GeohashChatIdentityState(signer, scope) val muteListDecryptionCache = MuteListDecryptionCache(signer) val muteList = MuteListState(signer, cache, muteListDecryptionCache, scope, settings) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GeohashChatIdentityState.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GeohashChatIdentityState.kt index 9517a3b62b..699fbb9f37 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GeohashChatIdentityState.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/GeohashChatIdentityState.kt @@ -22,13 +22,23 @@ package com.vitorpamplona.amethyst.model import androidx.core.content.edit import com.vitorpamplona.amethyst.Amethyst +import com.vitorpamplona.amethyst.LegacySharedPreferences +import com.vitorpamplona.amethyst.accountSecretsStore +import com.vitorpamplona.amethyst.commons.model.preferences.GeohashIdentitySecrets +import com.vitorpamplona.amethyst.commons.model.preferences.readLegacyGeohashIdentity import com.vitorpamplona.quartz.experimental.bitchat.identity.GeohashKeyDerivation import com.vitorpamplona.quartz.nip01Core.core.hexToByteArray import com.vitorpamplona.quartz.nip01Core.core.toHexKey import com.vitorpamplona.quartz.nip01Core.crypto.KeyPair import com.vitorpamplona.quartz.nip01Core.signers.NostrSigner import com.vitorpamplona.quartz.nip01Core.signers.NostrSignerInternal +import com.vitorpamplona.quartz.nip19Bech32.toNpub import com.vitorpamplona.quartz.utils.RandomInstance +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.launch +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import java.util.concurrent.ConcurrentHashMap /** * The account's anonymous, per-geohash chat identities. @@ -52,68 +62,97 @@ import com.vitorpamplona.quartz.utils.RandomInstance */ class GeohashChatIdentityState( private val signer: NostrSigner, + private val scope: CoroutineScope, ) { - private val lock = Any() - private val cache = HashMap() + /** + * Guards seed creation as well as the key cache: two callers racing into + * [deviceSeed] must not mint two different seeds, or the loser's cells get + * identities the next launch cannot reproduce. A [Mutex] rather than + * `synchronized`, because the store reads it protects are suspending. + */ + private val mutex = Mutex() + private val cache = ConcurrentHashMap() - @Volatile private var cachedDeviceSeed: ByteArray? = null + /** + * The npub the current store is keyed by. + * + * The legacy file is keyed by the pubkey *hex* — the old code passed + * `signer.pubKey` where every other caller passes an npub, so the identity + * lived in `secret_keeper_`, a different file from the account's own + * `secret_keeper_`. The copy below reads that file and writes the + * npub-keyed store, which is what folds this orphan back in with the rest. + */ + private val npub by lazy { signer.pubKey.hexToByteArray().toNpub() } - @Volatile private var cachedNickname: String? = null + @Volatile private var loaded: GeohashIdentitySecrets? = null + + /** What `secret_keeper_` holds. Touches disk; callers are off the main thread. */ + private fun legacy(): GeohashIdentitySecrets = readLegacyGeohashIdentity(LegacySharedPreferences(Amethyst.instance.encryptedStorage(signer.pubKey))) + + /** The stored identity, copying it out of the legacy file the first time. */ + private suspend fun current(): GeohashIdentitySecrets { + loaded?.let { return it } + return accountSecretsStore.readGeohashIdentity(npub, legacy()).also { loaded = it } + } + + private suspend fun persist(value: GeohashIdentitySecrets) { + loaded = value + accountSecretsStore.mirrorGeohashIdentity(npub, value) + } /** * The user's display handle for location chats: a single global nickname, persisted per account. * Bitchat carries this as the per-message `["n", …]` tag rather than a kind-0 profile, and kind-20000 * messages are ephemeral (relays needn't store them), so the only durable home for it is the device. - * Kept in this account's encrypted storage, so it survives restarts and switches with the account. - * Empty string means "no nickname set". Reads touch disk on first call — invoke off the main thread. + * Empty string means "no nickname set". */ - fun nickname(): String { - cachedNickname?.let { return it } - synchronized(lock) { - cachedNickname?.let { return it } - val value = Amethyst.instance.encryptedStorage(signer.pubKey).getString(PREF_NICKNAME, "") ?: "" - cachedNickname = value - return value - } - } + suspend fun nickname(): String = current().nickname ?: "" - /** Persists the global location-chat nickname (trimmed) for this account. */ + /** + * Persists the global location-chat nickname (trimmed) for this account. + * + * Fire-and-forget on the account scope, which is what the SharedPreferences + * `edit {}` this replaced already did — the caller is a click handler on the + * main thread and the write is not something it waits for. + */ fun setNickname(value: String) { val trimmed = value.trim() - synchronized(lock) { - cachedNickname = trimmed + scope.launch { + persist(current().copy(nickname = trimmed)) + // Mirrored, not moved: the legacy file stays readable until the + // legacy writes are retired app-wide, so a rollback keeps the handle. Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_NICKNAME, trimmed) } } } - /** The Nostr key pair to use inside [geohash]. Derivation is cheap but cached; call off the main thread. */ - fun keyPair(geohash: String): KeyPair = - synchronized(lock) { - cache.getOrPut(geohash) { GeohashKeyDerivation.deriveKeyPair(seed(), geohash) } - } + /** The Nostr key pair to use inside [geohash]. Derivation is cheap but cached. */ + suspend fun keyPair(geohash: String): KeyPair { + cache[geohash]?.let { return it } - private fun seed(): ByteArray = accountPrivKey()?.let { GeohashKeyDerivation.accountSeed(it) } ?: deviceSeed() + return mutex.withLock { + cache[geohash] ?: GeohashKeyDerivation.deriveKeyPair(seed(), geohash).also { cache[geohash] = it } + } + } + + /** Call under [mutex]. */ + private suspend fun seed(): ByteArray = accountPrivKey()?.let { GeohashKeyDerivation.accountSeed(it) } ?: deviceSeed() private fun accountPrivKey(): ByteArray? = (signer as? NostrSignerInternal)?.keyPair?.privKey - /** Random per-account seed, used only when the account key is unreachable (bunker / external signer). */ - private fun deviceSeed(): ByteArray { - cachedDeviceSeed?.let { return it } - synchronized(lock) { - cachedDeviceSeed?.let { return it } - val prefs = Amethyst.instance.encryptedStorage(signer.pubKey) - val existing = prefs.getString(PREF_KEY, null) - val seed = - if (existing != null && existing.length == GeohashKeyDerivation.SEED_SIZE * 2) { - existing.hexToByteArray() - } else { - val fresh = RandomInstance.bytes(GeohashKeyDerivation.SEED_SIZE) - prefs.edit { putString(PREF_KEY, fresh.toHexKey()) } - fresh - } - cachedDeviceSeed = seed - return seed - } + /** + * Random per-account seed, used only when the account key is unreachable (bunker / external signer). + * + * Call under [mutex]: minting a second seed for an account that already has + * one would change every throwaway identity it has ever used. + */ + private suspend fun deviceSeed(): ByteArray { + val stored = current().deviceSeed + if (stored != null && stored.length == GeohashKeyDerivation.SEED_SIZE * 2) return stored.hexToByteArray() + + val fresh = RandomInstance.bytes(GeohashKeyDerivation.SEED_SIZE) + persist(current().copy(deviceSeed = fresh.toHexKey())) + Amethyst.instance.encryptedStorage(signer.pubKey).edit { putString(PREF_KEY, fresh.toHexKey()) } + return fresh } companion object { diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecrets.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecrets.kt index 7a18508ff1..46769ead3d 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecrets.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecrets.kt @@ -102,6 +102,22 @@ object LegacyAccountSecretNames { /** The private key, which lives in its own store rather than in [AccountSecrets]. */ const val NOSTR_PRIVKEY = "nostr_privkey" + /** + * The location-chat identity, which is [GeohashIdentitySecrets] rather than + * part of [AccountSecrets] — see that class for why it is its own group. + * + * These two sit in `secret_keeper_`, not `secret_keeper_`: + * the writer passed `signer.pubKey`, which is hex, where every other caller + * passes an npub. They are therefore in a *different file* from everything + * else named here, which is why they are deliberately **not** in [all]: + * [all] is what `LegacyPreferenceCleanup` treats as claimed in the npub + * file, and these never appear in it. That file's deletion cannot lose + * them, and cannot clean them up either — retiring the hex file is its own + * job, once these writes stop. + */ + const val GEOHASH_DEVICE_SEED = "geohash_chat_device_seed" + const val GEOHASH_NICKNAME = "geohash_chat_nickname" + val all = setOf( NIP46_SIGNER_ENABLED, @@ -135,3 +151,52 @@ fun readLegacyAccountSecrets(source: LegacyPreferenceSource) = legacyDefaultNwcWalletId = source.getString(LegacyAccountSecretNames.DEFAULT_NWC_WALLET_ID), legacyZapPaymentRequestServer = source.getString(LegacyAccountSecretNames.ZAP_PAYMENT_REQUEST_SERVER), ) + +/** + * The account's location-chat identity: the seed its per-geohash throwaway keys + * come from, and the handle it posts under. + * + * # Why this is not two more fields on [AccountSecrets] + * + * Every account save mirrors a whole [AccountSecrets], built field by field from + * `AccountSettings` — which does not hold these, because they are owned by + * `GeohashChatIdentityState` rather than by the settings object. Folding them in + * would make each save write null over them, and the group save uses + * `putOrRemove`, so null *deletes*. The seed would vanish on the next unrelated + * save and every geohash identity the user has would silently change. A separate + * group with its own save path cannot be wiped by a save that does not know + * about it. + * + * # Why encrypted + * + * The whole point of the seed is that the identities derived from it are + * unlinkable to the npub. Anyone who can read it can link every cell the user + * has ever posted in, to each other and to the device, which is exactly what the + * feature exists to prevent. It was in an encrypted file before; it stays in one. + */ +data class GeohashIdentitySecrets( + val deviceSeed: String? = null, + val nickname: String? = null, +) + +/** Keys for [GeohashIdentitySecrets] inside an [EncryptedDataStore]. */ +internal object GeohashIdentityKeys { + val deviceSeed = stringPreferencesKey(LegacyAccountSecretNames.GEOHASH_DEVICE_SEED) + val nickname = stringPreferencesKey(LegacyAccountSecretNames.GEOHASH_NICKNAME) + + /** Records that the one-off copy out of the legacy file has run for this account. */ + val migrated = stringPreferencesKey("migrated.geohashIdentity") +} + +/** + * The location-chat identity as the legacy file holds it. + * + * Both absent is a real answer — an account that never opened a location chat — + * and is why the caller compares against [GeohashIdentitySecrets] rather than + * treating null as "not migrated". + */ +fun readLegacyGeohashIdentity(source: LegacyPreferenceSource) = + GeohashIdentitySecrets( + deviceSeed = source.getString(LegacyAccountSecretNames.GEOHASH_DEVICE_SEED), + nickname = source.getString(LegacyAccountSecretNames.GEOHASH_NICKNAME), + ) 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 082c14fd06..90d4a323e3 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 @@ -182,6 +182,39 @@ class AccountSecretsEncryptedStores( } } + // ── the location-chat identity ──────────────────────────────────── + + /** + * Reads [GeohashIdentitySecrets], or null when this account has not been + * copied out of the legacy encrypted file yet. + * + * Its own marker, not [AccountSecretKeys.migrated]: the two groups migrate + * from *different files* (this one from `secret_keeper_`, the + * secrets from `secret_keeper_`), so one marker cannot speak for both. + */ + suspend fun loadGeohashIdentity(npub: String): GeohashIdentitySecrets? { + val stored = getDataStore(npub).snapshot() + if (stored[GeohashIdentityKeys.migrated] == null) return null + + return GeohashIdentitySecrets( + deviceSeed = stored[GeohashIdentityKeys.deviceSeed], + nickname = stored[GeohashIdentityKeys.nickname], + ) + } + + /** Writes the group and its marker as one edit, for the reasons [saveSecrets] gives. */ + suspend fun saveGeohashIdentity( + npub: String, + value: GeohashIdentitySecrets, + ) { + getDataStore(npub).edit { + putOrRemove(GeohashIdentityKeys.deviceSeed, value.deviceSeed) + putOrRemove(GeohashIdentityKeys.nickname, value.nickname) + + put(GeohashIdentityKeys.migrated, "true") + } + } + private fun decodeSet(raw: String?): Set = raw ?.split(AccountSecretKeys.SET_SEPARATOR) diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsStoreTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsStoreTest.kt index 32962ebd25..fc22b515a1 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsStoreTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/model/preferences/AccountSecretsStoreTest.kt @@ -206,4 +206,94 @@ class AccountSecretsStoreTest { assertEquals(filled, subject.loadSecrets(npub)) } + // ── the location-chat identity ──────────────────────────────────── + + @Test + fun anUnmigratedGeohashIdentityReadsAsNull() = + runTest { + assertNull(stores().loadGeohashIdentity(npub)) + } + + @Test + fun theGeohashIdentityRoundTrips() = + runTest { + val subject = stores() + val value = GeohashIdentitySecrets(deviceSeed = "a".repeat(64), nickname = "vitor") + + subject.saveGeohashIdentity(npub, value) + + assertEquals(value, subject.loadGeohashIdentity(npub)) + } + + /** + * An account that opened a location chat under a bunker signer has a seed but + * never set a handle. Absent must come back absent rather than as "". + */ + @Test + fun aSeedWithNoNicknameRoundTrips() = + runTest { + val subject = stores() + val value = GeohashIdentitySecrets(deviceSeed = "b".repeat(64), nickname = null) + + subject.saveGeohashIdentity(npub, value) + + assertEquals(value, subject.loadGeohashIdentity(npub)) + } + + /** An account that holds neither key still counts as migrated, or the copy runs forever. */ + @Test + fun anEmptyGeohashIdentityStillCountsAsMigrated() = + runTest { + val subject = stores() + + subject.saveGeohashIdentity(npub, GeohashIdentitySecrets()) + + assertEquals(GeohashIdentitySecrets(), subject.loadGeohashIdentity(npub)) + } + + /** + * The two groups migrate out of *different* legacy files — the secrets from + * `secret_keeper_`, the identity from `secret_keeper_` — + * so neither marker may stand in for the other. Saving one must leave the + * other reading as not-yet-copied. + */ + @Test + fun theTwoGroupsMigrateIndependently() = + runTest { + val subject = stores() + + subject.saveSecrets(npub, filled) + + assertNull("saving the secrets must not mark the identity migrated", subject.loadGeohashIdentity(npub)) + } + + @Test + fun savingTheIdentityDoesNotMarkTheSecretsMigrated() = + runTest { + val subject = stores() + + subject.saveGeohashIdentity(npub, GeohashIdentitySecrets(deviceSeed = "c".repeat(64))) + + assertNull(subject.loadSecrets(npub)) + } + + /** + * The seed must survive an unrelated account save. This is the whole reason + * the identity is its own group: every save mirrors a full AccountSecrets + * built from AccountSettings, which does not hold the seed, and the group + * save removes keys whose value is null. Folded into that group, the seed + * would be deleted here — and every geohash identity the user has would + * silently change. + */ + @Test + fun anAccountSaveLeavesTheGeohashIdentityAlone() = + runTest { + val subject = stores() + val identity = GeohashIdentitySecrets(deviceSeed = "d".repeat(64), nickname = "vitor") + subject.saveGeohashIdentity(npub, identity) + + subject.saveSecrets(npub, filled) + + assertEquals(identity, subject.loadGeohashIdentity(npub)) + } }