mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 16:14:40 +00:00
fix(account): don't let adding a read-only npub clobber an existing signing account
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
2ee0d91b71
commit
0d0aba90c9
@@ -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<AccountInfo> = savedAccounts()
|
||||
|
||||
+4
-2
@@ -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(
|
||||
|
||||
+12
@@ -508,6 +508,18 @@ class AccountManager internal constructor(
|
||||
suspend fun saveCurrentAccount(): Result<Unit> {
|
||||
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
|
||||
|
||||
+41
@@ -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<AccountState.LoggedIn>(finalState)
|
||||
assertFalse(finalState.isReadOnly)
|
||||
assertEquals(SignerType.Internal, finalState.signerType)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user