fix(multi-account): address review findings — per-account bunker keys, resource cleanup, simplifications

Fixes from code-simplicity and architecture reviews:

Critical fixes:
- Per-account bunker ephemeral key alias (bunker_ephemeral_<npub>) 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) <noreply@anthropic.com>
This commit is contained in:
nrobi144
2026-04-24 10:36:10 +03:00
parent 0a378c7c4a
commit 43b6d80ea6
5 changed files with 41 additions and 38 deletions
@@ -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
@@ -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<AccountState.LoggedIn> {
// 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<AccountState.LoggedIn> {
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() {
@@ -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
},
@@ -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()
@@ -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