From 0d0aba90c9502402fd5e8fa2a1740ebecf516566 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Fri, 26 Jun 2026 19:00:51 -0400 Subject: [PATCH] fix(account): don't let adding a read-only npub clobber an existing signing account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accounts dedup by npub (= pubkey), so adding/scanning your own read-only npub for a pubkey you already hold the nsec for would overwrite the signing account: - Android: setDefaultAccount rewrote the per-npub file from fresh read-only settings — wiping the account's cached follow/relay/mute lists and flipping hasPrivKey off, which silently disables its push notifications (every notification path early-returns on !hasPrivKey). The account looked lost ("can't post anymore") even though the nsec survived on disk. - Desktop: saveCurrentAccount overwrote signerType to ViewOnly, orphaning the stored key and routing every later switch through loadReadOnlyAccount. Guard the downgrade at the single persistence point on each platform: when the account being made current is read-only and a SIGNING account already exists for the same pubkey, keep the signing account and switch to it instead. A signing account already subsumes a read-only one, so this loses nothing. - Android LocalPreferences.setDefaultAccount now returns the settings that actually became current; AccountSessionManager.loginAndStartUI shows that. - Desktop AccountManager.saveCurrentAccount reuses switchAccount() to reload the signing account. Covered by a regression test (verified failing without the guard). This replaces the closed proposal e05208d9, which solved the same underlying bug with a much heavier accountId rework (separate npub/nsec switcher entries — a niche feature) that itself shipped a logout(deleteKey=true) data-loss bug. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../amethyst/LocalPreferences.kt | 24 ++++++++++- .../ui/screen/AccountSessionManager.kt | 6 ++- .../desktop/account/AccountManager.kt | 12 ++++++ .../account/AccountManagerKeyLoginTest.kt | 41 +++++++++++++++++++ 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt index 09864df91f..9b6a2abe3f 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/LocalPreferences.kt @@ -355,7 +355,27 @@ object LocalPreferences { } } - suspend fun setDefaultAccount(accountSettings: AccountSettings) { + /** + * Make [accountSettings] the current account, persisting + caching it. Returns the settings that + * actually became current — normally [accountSettings] itself, but see the downgrade guard below. + */ + suspend fun setDefaultAccount(accountSettings: AccountSettings): AccountSettings { + val npub = accountSettings.keyPair.pubKey.toNpub() + + // Downgrade guard: adding a read-only npub for a pubkey we already hold a SIGNING account for + // must not clobber that account. Accounts dedup by npub, so saving fresh read-only settings + // here would overwrite the signing account's per-npub file — wiping its cached follow/relay/ + // mute lists and flipping hasPrivKey off, which silently disables its push notifications. A + // signing account already does everything the read-only one would, so keep it and just make + // it current instead of degrading it. + if (!accountSettings.isWriteable()) { + val existing = loadAccountConfigFromEncryptedStorage(npub) + if (existing != null && existing.isWriteable()) { + setCurrentAccount(existing) + return existing + } + } + // Save the per-npub file before emitting onto the savedAccounts flow. // Otherwise a collector (e.g. AlwaysOnNotificationServiceManager) can race in // and call loadAccountConfigFromEncryptedStorage(npub) before NOSTR_PUBKEY is @@ -363,9 +383,9 @@ object LocalPreferences { // rest of the session — making every later switch to this account land on // LoggedOff instead of LoggedIn. saveToEncryptedStorage(accountSettings) - val npub = accountSettings.keyPair.pubKey.toNpub() mutex.withLock { cachedAccounts.put(npub, accountSettings) } setCurrentAccount(accountSettings) + return accountSettings } suspend fun allSavedAccounts(): List = savedAccounts() diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/AccountSessionManager.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/AccountSessionManager.kt index 0c85dc7081..bf63ace422 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/AccountSessionManager.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/AccountSessionManager.kt @@ -175,9 +175,11 @@ class AccountSessionManager( } } - localPreferences.setDefaultAccount(accountSettings) + // setDefaultAccount may keep an existing signing account instead of this one when a + // read-only npub is added for a pubkey we already sign for — show whichever became current. + val current = localPreferences.setDefaultAccount(accountSettings) - startUI(accountSettings) + startUI(current) } fun startUI( diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index 114a4cc885..b5c4e5598c 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -508,6 +508,18 @@ class AccountManager internal constructor( suspend fun saveCurrentAccount(): Result { val current = currentAccount() ?: return Result.failure(Exception("No account logged in")) + // Downgrade guard (mirrors Android LocalPreferences.setDefaultAccount): never persist a + // read-only account over an existing SIGNING account for the same pubkey. Accounts key by + // npub, so this would overwrite signerType to ViewOnly and orphan the stored key, routing + // every later switch through read-only. A signing account already subsumes a read-only one, + // so keep it and switch to it instead of degrading it. + if (current.isReadOnly) { + val existing = accountStorage.loadAccounts().firstOrNull { it.npub == current.npub } + if (existing != null && existing.signerType !is SignerType.ViewOnly) { + return switchAccount(current.npub).map { } + } + } + // Bunker accounts: private key saved during loginWithBunker if (current.signerType is SignerType.Remote) { // Still ensure multi-account storage is updated diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt index 05129a4c26..4d511c57f7 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerKeyLoginTest.kt @@ -159,4 +159,45 @@ class AccountManagerKeyLoginTest { val accounts = manager.accountStorage.loadAccounts() assertTrue(accounts.any { it.npub == keyPair.pubKey.toNpub() }) } + + /** + * Regression: adding the read-only npub for a pubkey we already hold a SIGNING account for must + * NOT downgrade it. Accounts key by npub, so persisting the ViewOnly entry would overwrite the + * Internal signerType and orphan the stored key, routing every later switch through read-only. + * The signing account must survive and become current instead. + */ + @Test + fun addingReadOnlyNpubDoesNotDowngradeExistingSigningAccount() = + runTest { + val keyPair = KeyPair() + val npub = keyPair.pubKey.toNpub() + + // 1. Sign in with the nsec and persist it as a full signing account. + manager.loginWithKey(keyPair.privKey!!.toNsec()) + manager.saveCurrentAccount() + assertEquals( + SignerType.Internal, + manager.accountStorage + .loadAccounts() + .first { it.npub == npub } + .signerType, + ) + assertTrue(keyStore.containsKey(npub)) + + // 2. Add the read-only npub for the SAME pubkey and try to save it. + manager.loginWithKey(npub) + assertTrue((manager.accountState.value as AccountState.LoggedIn).isReadOnly) + val result = manager.saveCurrentAccount() + assertTrue(result.isSuccess) + + // 3. The signing account survives: storage stays Internal, the key is kept, + // and the current account was switched back to signing (not downgraded). + val stored = manager.accountStorage.loadAccounts().first { it.npub == npub } + assertEquals(SignerType.Internal, stored.signerType) + assertTrue(keyStore.containsKey(npub)) + val finalState = manager.accountState.value + assertIs(finalState) + assertFalse(finalState.isReadOnly) + assertEquals(SignerType.Internal, finalState.signerType) + } }