From 43b6d80ea67094a119e713e487d3dccaa8d4e985 Mon Sep 17 00:00:00 2001 From: nrobi144 Date: Fri, 24 Apr 2026 10:36:10 +0300 Subject: [PATCH] =?UTF-8?q?fix(multi-account):=20address=20review=20findin?= =?UTF-8?q?gs=20=E2=80=94=20per-account=20bunker=20keys,=20resource=20clea?= =?UTF-8?q?nup,=20simplifications?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from code-simplicity and architecture reviews: Critical fixes: - Per-account bunker ephemeral key alias (bunker_ephemeral_) instead of shared alias. Prevents wrong-identity handshake when switching bunker accounts. Falls back to legacy alias for existing single-account setups. - Resource cleanup in switchAccount(): closes NIP-46 subscription, stops heartbeat, disconnects NIP-46 client before transitioning to new account. - Manages signer connection state on switch (Connected for bunker, NotRemote otherwise). Simplifications: - Removed unused createWithStorage() factory (zero callers) - Removed loadViewOnlyAccount() dead code (no UI path to create view-only accounts) - Removed unused allAccounts collection at Window scope (duplicated in MainContent) - Files.move with REPLACE_EXISTING for cross-platform atomic rename - Remote signer with empty bunkerUri falls back to Internal (prevents corrupt state) Co-Authored-By: Claude Opus 4.6 (1M context) --- .../vitorpamplona/amethyst/desktop/Main.kt | 1 - .../desktop/account/AccountManager.kt | 63 ++++++++++--------- .../desktop/account/DesktopAccountStorage.kt | 5 +- .../account/AccountManagerLoadAccountTest.kt | 4 +- .../AccountManagerNip46IsolationTest.kt | 6 +- 5 files changed, 41 insertions(+), 38 deletions(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt index 7c5edcff8..f908ff687 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt @@ -198,7 +198,6 @@ fun main() { val workspaceManager = remember { WorkspaceManager(deckScope).also { it.load() } } val accountManager = remember { AccountManager.create() } val accountState by accountManager.accountState.collectAsState() - val allAccounts by accountManager.allAccounts.collectAsState() var showAppDrawer by remember { mutableStateOf(false) } // Tor state at Window level — survives key() app rebuild 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 92dc15fdb..9115563b8 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 @@ -96,14 +96,13 @@ class AccountManager internal constructor( return AccountManager(storage) } - fun createWithStorage( - secureStorage: SecureKeyStorage, - homeDir: File = File(System.getProperty("user.home")), - ): AccountManager = AccountManager(secureStorage, homeDir) - internal const val HEARTBEAT_INTERVAL_MS = 60_000L internal const val MAX_CONSECUTIVE_FAILURES = 3 - internal const val BUNKER_EPHEMERAL_KEY_ALIAS = "bunker_ephemeral" + + internal fun bunkerEphemeralKeyAlias(npub: String) = "bunker_ephemeral_$npub" + + // Legacy alias for migration + internal const val LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS = "bunker_ephemeral" internal const val NIP46_RELAY_CONNECT_TIMEOUT_MS = 15_000L internal val NIP46_RELAYS = listOf("wss://relay.nsec.app") } @@ -271,8 +270,11 @@ class AccountManager internal constructor( bunkerUri: String, npub: String?, ): Result { + // Try per-account alias first, fall back to legacy shared alias + val perAccountKey = if (npub != null) secureStorage.getPrivateKey(bunkerEphemeralKeyAlias(npub)) else null val ephemeralPrivKeyHex = - secureStorage.getPrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS) + perAccountKey?.takeIf { it.isNotEmpty() } + ?: secureStorage.getPrivateKey(LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)?.takeIf { it.isNotEmpty() } ?: return Result.failure(Exception("Ephemeral key not found")) val ephemeralKeyPair = KeyPair(privKey = ephemeralPrivKeyHex.hexToByteArray()) @@ -435,7 +437,7 @@ class AccountManager internal constructor( npub: String, ) { saveLastNpub(npub) - secureStorage.savePrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS, ephemeralPrivKeyHex) + secureStorage.savePrivateKey(bunkerEphemeralKeyAlias(npub), ephemeralPrivKeyHex) saveBunkerUri(bunkerUri) // Also save to multi-account storage @@ -555,7 +557,7 @@ class AccountManager internal constructor( (current.signer as? NostrSignerRemote)?.closeSubscription() if (deleteKey) { try { - secureStorage.deletePrivateKey(BUNKER_EPHEMERAL_KEY_ALIAS) + secureStorage.deletePrivateKey(bunkerEphemeralKeyAlias(current.npub)) } catch (_: SecureStorageException) { } getBunkerFile().delete() @@ -675,39 +677,40 @@ class AccountManager internal constructor( when (sType) { is SignerType.Internal -> loadInternalAccount(target.npub) is SignerType.Remote -> loadBunkerAccount(sType.bunkerUri, target.npub) - is SignerType.ViewOnly -> loadViewOnlyAccount(target.npub) + is SignerType.ViewOnly -> loadInternalAccount(target.npub) } if (newState.isFailure) return newState - // Phase 2: transition succeeded — update storage and cleanup + // Phase 2: transition succeeded — clean up old account resources + cleanupOldAccountResources() accountStorage.setCurrentAccount(targetNpub) + // Start heartbeat if new account is a bunker + val loggedIn = newState.getOrNull() + if (loggedIn?.signerType is SignerType.Remote) { + // Heartbeat needs an external scope — will be started by caller + _signerConnectionState.value = SignerConnectionState.Connected + } else { + _signerConnectionState.value = SignerConnectionState.NotRemote + } + // Reload NWC for the new account loadNwcConnection() return newState } - private fun loadViewOnlyAccount(npub: String): Result { - val pubKeyHex = - decodePublicKeyAsHexOrNull(npub) - ?: return Result.failure(Exception("Invalid npub: $npub")) - - val keyPair = KeyPair(pubKey = pubKeyHex.hexToByteArray()) - val signer = NostrSignerInternal(keyPair) - - val state = - AccountState.LoggedIn( - signer = signer, - pubKeyHex = pubKeyHex, - npub = npub, - nsec = null, - isReadOnly = true, - signerType = SignerType.ViewOnly, - ) - _accountState.value = state - return Result.success(state) + private suspend fun cleanupOldAccountResources() { + // Close NIP-46 subscription on old remote signer + val oldAccount = currentAccount() + if (oldAccount?.signerType is SignerType.Remote) { + (oldAccount.signer as? NostrSignerRemote)?.closeSubscription() + } + // Stop heartbeat for old account + stopHeartbeat() + // Disconnect old NIP-46 client + disconnectNip46Client() } suspend fun migrateAndLoadAccounts() { diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt index d2ca9917a..f79a46bfc 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt @@ -29,6 +29,7 @@ import com.vitorpamplona.amethyst.commons.model.account.SignerType import com.vitorpamplona.quartz.utils.Log import java.io.File import java.nio.file.Files +import java.nio.file.StandardCopyOption import java.nio.file.attribute.PosixFilePermission import java.security.SecureRandom import java.util.Base64 @@ -165,7 +166,7 @@ class DesktopAccountStorage( val file = getAccountsFile() val temp = File(amethystDir, "${ACCOUNTS_FILE}.tmp") temp.writeBytes(encrypted) - temp.renameTo(file) + Files.move(temp.toPath(), file.toPath(), StandardCopyOption.REPLACE_EXISTING) setFilePermissions(file) } @@ -253,7 +254,7 @@ internal data class AccountInfoDto( npub = npub, signerType = when (signerKind) { - "remote" -> SignerType.Remote(bunkerUri ?: "") + "remote" -> if (bunkerUri.isNullOrEmpty()) SignerType.Internal else SignerType.Remote(bunkerUri) "viewonly" -> SignerType.ViewOnly else -> SignerType.Internal }, diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt index bb06fc493..093aab82f 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerLoadAccountTest.kt @@ -109,7 +109,7 @@ class AccountManagerLoadAccountTest { "bunker://$validHex?relay=wss://r.com", ) coEvery { - storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) + storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS) } returns null val result = manager.loadSavedAccount() @@ -130,7 +130,7 @@ class AccountManagerLoadAccountTest { "bunker://$validHex?relay=wss://r.com", ) coEvery { - storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) + storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralPrivKeyHex val result = manager.loadSavedAccount() diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt index e06d841b3..acb1603a1 100644 --- a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerNip46IsolationTest.kt @@ -78,7 +78,7 @@ class AccountManagerNip46IsolationTest { ephemeralKeyPair.pubKey.toNpub(), ) coEvery { - storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) + storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralKeyPair.privKey!!.toHexKey() val result = manager.loadSavedAccount() @@ -139,7 +139,7 @@ class AccountManagerNip46IsolationTest { ephemeralKeyPair.pubKey.toNpub(), ) coEvery { - storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) + storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralKeyPair.privKey!!.toHexKey() manager.loadSavedAccount() @@ -167,7 +167,7 @@ class AccountManagerNip46IsolationTest { } coEvery { - storage.getPrivateKey(AccountManager.BUNKER_EPHEMERAL_KEY_ALIAS) + storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS) } returns ephemeralKeyPair.privKey!!.toHexKey() // First load