Add multi-key identity support to desktop#228
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughHonors KEEP_HOME for default keep path; adds multi-identity support (types, switch/delete, per-identity relay/bunker persistence and UI); centralizes sidebar with SidebarState and lifetime-bound screen views; refactors many screen view APIs to view_content; improves bunker client syncing and frost message preview; tightens password validation and tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant App
participant Storage
participant IdentityStore
User->>UI: Click identity in sidebar / confirm delete
UI->>App: Send SwitchIdentity(pubkey_hex) or ConfirmDeleteIdentity(pubkey_hex)
App->>IdentityStore: save previous identity configs
App->>Storage: save_relay_urls_for / save_bunker_relays_for
App->>IdentityStore: load_relay_urls_for / load_bunker_relays_for
IdentityStore->>Storage: read per-identity config files
Storage-->>IdentityStore: return relay/bunker configs
IdentityStore-->>App: active identity + configs
App->>UI: trigger re-render of sidebar & screens
UI-->>User: show updated active identity or deletion result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-desktop/src/app.rs (1)
1040-1053:⚠️ Potential issue | 🟠 MajorPer-identity relay/bunker configs are not loaded after unlock or auto-selection.
refresh_identitiesauto-selects a single identity (lines 1535-1543) but doesn't load its per-identity relay or bunker relay configs. Similarly,set_share_screen→resolve_active_sharemay restore a persistedactive_share_hexwithout loading its configs.After unlock,
App::newloads only the globalrelays.json(line 394). Per-identity configs (relays-{hex}.json,bunker-relays-{hex}.json) are only loaded insideSwitchIdentity(lines 1568-1569). This means:
- User unlocks vault → global relay config is loaded.
resolve_active_sharerestoresactive_share_hex→ per-identity config not loaded.- User modifies relays → saved to per-identity file (via
save_relay_urls).- User locks + unlocks → back to global config; per-identity changes appear lost.
Consider loading per-identity configs after the active identity is resolved.
Proposed fix sketch in `set_share_screen`
fn set_share_screen(&mut self, shares: Vec<ShareEntry>) { self.resolve_active_share(&shares); self.refresh_identities(); + if let Some(ref hex) = self.active_share_hex { + self.relay_urls = load_relay_urls_for(&self.keep_path, hex); + self.bunker_relays = load_bunker_relays_for(&self.keep_path, hex); + } self.screen = Screen::ShareList(ShareListScreen::new(shares, self.active_share_hex.clone())); }Also applies to: 1532-1544
🤖 Fix all issues with AI agents
In `@keep-core/src/lib.rs`:
- Around line 878-885: The function default_keep_path currently treats an empty
KEEP_HOME as valid because std::env::var("KEEP_HOME").ok().map(PathBuf::from)
turns "" into the current directory; update default_keep_path to reject blank
values by trimming and checking non-empty before converting (e.g., use
std::env::var("KEEP_HOME").ok().filter(|s|
!s.trim().is_empty()).map(PathBuf::from)), so the dirs::home_dir() fallback is
used for empty/blank env values; also add unit tests for default_keep_path that
set KEEP_HOME to a concrete path and to an empty/blank string (use temp_env or
similar) and assert the returned PathBuf matches the env value in the first case
and falls back to ~/.keep in the second case.
In `@keep-desktop/src/app.rs`:
- Around line 1231-1242: The save_bunker_relays method currently no-ops when
active_share_hex is None; change it to mirror save_relay_urls by matching on
self.active_share_hex and calling save_bunker_relays_for(&self.keep_path, hex,
&self.bunker_relays) for Some(hex) and falling back to
save_bunker_relays(&self.keep_path, &self.bunker_relays) for None (or,
alternatively, emit a clear warning via the same logger used elsewhere) so
bunker relay edits are persisted when no identity is active; update the
save_bunker_relays function accordingly to use the same pattern as
save_relay_urls.
In `@keep-desktop/src/screen/import.rs`:
- Around line 105-109: The import screen's npub truncation is inconsistent:
update the truncated computation (the let truncated = ... block) to match
Identity::truncated_npub and ShareEntry::truncated_npub by changing the
condition to truncate only when npub.len() > 20 (not >= 20) and use a
6-character suffix instead of 8 (format!("{}...{}", &npub[..12],
&npub[npub.len() - 6..])). This will ensure the same display logic across Import
(truncated variable), Identity::truncated_npub, and ShareEntry::truncated_npub.
🧹 Nitpick comments (5)
keep-desktop/src/bunker_service.rs (1)
562-604: Duration display uses integer division — values that don't divide evenly get truncated.For seconds in the range
[3600, 86400),s / 3600truncates: e.g. 5400s (1h30m) displays as"1h". Similarly,s / 86400truncates fractional days. This is likely acceptable for a summary label, but worth noting the lossy display.Also a minor label inconsistency: the UI picker label is
"This session"(bunker.rs line 76) whilesync_bunker_clientsrendersPermissionDuration::Sessionas"Session"(line 575). Consider aligning them.keep-desktop/src/message.rs (1)
29-40: Duplicatedtruncated_npublogic — consider a shared helper.
Identity::truncated_npub()here is identical toShareEntry::truncated_npub()inshares.rs(lines 53–61). Additionally,import.rs(line 106) uses a different suffix length ([len-8..]vs[len-6..]), creating an inconsistency in how npubs are displayed.Consider extracting a single
truncate_npub(npub: &str) -> Stringutility function to ensure consistent display across the app.keep-desktop/src/screen/bunker.rs (1)
76-76: Label updated to "This session" — consider renaming the enum variant to match.The
DurationChoice::JustThisTimevariant (line 86) no longer matches its display label. A rename toDurationChoice::Sessionwould improve readability, though this is purely internal.keep-desktop/src/screen/mod.rs (1)
39-47: Share count badge only visible on the ShareList screen.
share_countis only passed forScreen::ShareList; all other arms passNone. This means the sidebar "Shares" nav item won't display its count badge when the user is on Relay, Bunker, Settings, etc. If the badge is meant to give persistent context, consider always computing the share count inapp.rsand passing it through regardless of active screen.keep-desktop/src/app.rs (1)
193-203: Potential config file collision withshort_hextruncation.
short_hextruncates the pubkey hex to 16 characters (8 bytes). While collisions among cryptographic public keys in the first 8 bytes are astronomically unlikely for a small number of identities, using the full hex (or a longer prefix) would eliminate the theoretical risk entirely. This also affects the clean-up inConfirmDeleteIdentity(lines 1691-1696) where config files are removed by this truncated key.
wksantiago
force-pushed
the
Multi-key
branch
from
18f9cc4 to
ab44267
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-desktop/src/app.rs`:
- Around line 247-253: load_bunker_relays_for currently reads only the
per-identity bunker file and falls back straight to default_bunker_relays, which
ignores an existing global bunker-relays.json; change load_bunker_relays_for to
mirror load_relay_urls_for’s chain: attempt per-identity file via
bunker_relay_config_path_for(pubkey_hex), if that is missing or invalid then try
the global bunker file (use the same path function used for the global config),
and finally call default_bunker_relays; keep the same return type Vec<String>
and error handling style (.ok()/and_then(...).unwrap_or_else(...)) so behavior
is consistent with load_relay_urls_for.
🧹 Nitpick comments (3)
keep-desktop/src/screen/shares.rs (1)
53-62: Duplicatedtruncated_npublogic — extract a shared helper.This exact implementation is repeated in
Identity::truncated_npub(message.rs:31-40). Consider extracting a shared free function (e.g.,fn truncate_npub(npub: &str) -> String) and calling it from both sites to avoid divergence.♻️ Example shared helper
// e.g. in a shared util or in message.rs, then re-use: pub fn truncate_npub(npub: &str) -> String { if npub.len() <= 20 { return npub.to_string(); } format!("{}...{}", &npub[..12], &npub[npub.len() - 6..]) }keep-desktop/src/app.rs (2)
1711-1723:refresh_identitiesre-fetches shares unnecessarily.Every caller of
refresh_identities(refresh_sharesat line 1076,set_share_screenat line 1086) already has the shares in hand, yetrefresh_identitiescallsself.current_shares()again (line 1712), acquiring the mutex and querying the share list a second time. Acceptsharesas a parameter to avoid the redundant work.♻️ Proposed fix
- fn refresh_identities(&mut self) { - let shares = self.current_shares(); - self.identities = self.collect_identities(&shares); + fn refresh_identities(&mut self, shares: &[ShareEntry]) { + self.identities = self.collect_identities(shares); if self.active_share_hex.is_none() && self.identities.len() == 1 { let hex = self.identities[0].pubkey_hex.clone(); let guard = lock_keep(&self.keep);Then update call sites:
// In refresh_shares: let shares = self.current_shares(); self.resolve_active_share(&shares); - self.refresh_identities(); + self.refresh_identities(&shares); // In set_share_screen: self.resolve_active_share(&shares); - self.refresh_identities(); + self.refresh_identities(&shares);
206-220: Consider the (negligible) collision risk from 16-char hex truncation in config filenames.
short_hextruncates to 16 hex chars (64 bits) for per-identity config file names. While collision probability is astronomically low for typical identity counts, using the full hex would eliminate it entirely at the cost of longer filenames. This is fine as-is for practical use.
wksantiago
force-pushed
the
Multi-key
branch
from
ab44267 to
065cc5b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-desktop/src/app.rs`:
- Around line 1814-1890: When confirming deletion in the
Message::ConfirmDeleteIdentity branch, if the identity being deleted is the
currently active one you must disconnect the relay and stop the bunker to avoid
running services with stale keys; detect this by comparing the target pubkey_hex
to self.active_share_hex (or using resolve_active_share() result) and call
self.handle_disconnect_relay() and self.stop_bunker() (or the equivalent
methods) before proceeding with deletion (or immediately after determining
result == true but before refresh_shares()/file removal) so running services are
stopped and the app state stays consistent.
🧹 Nitpick comments (2)
keep-desktop/src/app.rs (2)
1719-1731:refresh_identitiesredundantly fetches shares.
current_shares()locks the Keep and reads all shares from disk. This method is always called right afterrefresh_sharesorset_share_screen, which already have the shares available. Consider accepting a&[ShareEntry]parameter to avoid the duplicate I/O and mutex acquisition.Proposed refactor
- fn refresh_identities(&mut self) { - let shares = self.current_shares(); - self.identities = self.collect_identities(&shares); + fn refresh_identities(&mut self, shares: &[ShareEntry]) { + self.identities = self.collect_identities(shares); if self.active_share_hex.is_none() && self.identities.len() == 1 {Then update callers (
refresh_shares,set_share_screen) to pass the shares they already hold.
206-220: Using a 16-char hex prefix for per-identity config filenames.
short_hextruncates to 16 hex characters (8 bytes). While collision probability is negligible for real cryptographic keys on a single desktop, consider documenting this assumption. The safe.get(..16).unwrap_or(pubkey_hex)is a good defensive pattern.
- Multi-key identity switcher with per-identity relay/bunker configs - Nsec key import alongside FROST shares - Fix TOCTOU race in vault deletion using canonicalize - Fix partial FROST share deletion leaving stale UI state - Fix O(n^2) sanitize_message_preview with incremental char counting - Use safe slicing in short_hex and npub truncation - Deduplicate nsec keys in identity collection - Rename "Just this time" to "This session" for accuracy
M1: Use symlink_metadata instead of canonicalize for vault deletion check M2: Wrap k.lock() in catch_unwind for mutex poison recovery M3: Add min password length validation in keep-core (create only) P1: Use full pubkey hex in relay config filenames to prevent collisions P2: Report deleted/total shares count in partial deletion error P3: Pass shares to refresh_identities to avoid redundant fetch P5: Add server-side guard rejecting deletion of active identity P6: Save relay config to global fallback when no identity selected
kwsantiago
force-pushed
the
Multi-key
branch
from
065cc5b to
c84a610
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-core/src/rotation.rs (1)
121-127:⚠️ Potential issue | 🟠 MajorAdd password validation to
rotate_passwordto matchcreate_innerbehavior.
rotate_passwordcallscreate_header_with_keywithout validating the new password againstMIN_PASSWORD_LEN, allowing users to set arbitrarily short passwords. This bypasses the minimum 8-byte requirement enforced inStorage::create_inner. The password validation must happen before key derivation.Add a call to validate the new password at the start of
rotate_password:Suggested fix
pub fn rotate_password(&mut self, old_password: &str, new_password: &str) -> Result<()> { let lock = acquire_rotation_lock(&self.path)?; + crate::storage::validate_new_password(new_password)?; + if !self.is_unlocked() { self.unlock(old_password)?; }Note:
validate_new_passwordis currently private (fn, notpub). Make itpub(crate)instorage.rsto allow access fromrotation.rs.
🤖 Fix all issues with AI agents
In `@keep-desktop/src/screen/layout.rs`:
- Around line 10-25: The identity_color function can panic when slicing
pubkey_hex if i+2 exceeds the string length; change the byte extraction iterator
used to build bytes in identity_color to only attempt the slice when i + 2 <=
pubkey_hex.len() (e.g., add a bounds check in the filter_map closure or compute
a safe upper bound) so u8::from_str_radix is only called on valid two-character
hex substrings, preserving the existing fallback behavior for missing bytes.
🧹 Nitpick comments (4)
keep-core/src/storage.rs (1)
70-77: Consider adding a test for the new minimum password length enforcement.There's no test asserting that
Storage::createrejects passwords shorter than 8 characters. A quick negative test would lock in this invariant.💡 Suggested test
#[test] fn test_storage_rejects_short_password() { let dir = tempdir().unwrap(); let path = dir.path().join("test-short-pw"); let result = Storage::create(&path, "short", Argon2Params::TESTING); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("too short")); }keep-desktop/src/bunker_service.rs (1)
598-638: Duration formatting truncates sub-unit remainders to zero.The early-return refactor is clean. One minor note on the duration display: integer division drops remainders, so e.g.
Seconds(90)shows"1m"instead of"1m 30s", andSeconds(5400)shows"1h"instead of"1h 30m". If the only source ofSecondsvalues isDurationChoice::Minutes(m)(always multiples of 60), this is fine in practice. If arbitrary second values can come from persisted server state or external clients, the display could be misleading for edge cases.keep-desktop/src/screen/import.rs (1)
105-109: Truncation fix looks correct and consistent.The
> 20condition and 6-char suffix now matchIdentity::truncated_npubandShareEntry::truncated_npub.Consider extracting a shared
truncate_npub(npub: &str) -> Stringhelper to avoid maintaining three identical copies of this logic (here,Identity::truncated_npub, andShareEntry::truncated_npub).keep-desktop/src/app.rs (1)
1801-1866:SwitchIdentitycorrectly persists, disconnects, and reloads per-identity state.The flow saves current configs → disconnects relay/stops bunker → loads new identity's configs → updates active key → refreshes screen. The Nsec keyring primary update (lines 1822-1835) is a good addition. One minor note: errors from
set_active_share_key(line 1818) andset_primary(line 1831) are silently ignored withlet _. Consider at least logging these failures, as they indicate the identity switch may not have fully persisted.♻️ Optional: log failures in identity switch persistence
{ let guard = lock_keep(&self.keep); if let Some(keep) = guard.as_ref() { - let _ = keep.set_active_share_key(Some(&pubkey_hex)); + if let Err(e) = keep.set_active_share_key(Some(&pubkey_hex)) { + tracing::warn!("Failed to persist active share key: {e}"); + } } }let mut guard = lock_keep(&self.keep); if let Some(keep) = guard.as_mut() { - let _ = keep.keyring_mut().set_primary(pubkey_bytes); + if let Err(e) = keep.keyring_mut().set_primary(pubkey_bytes) { + tracing::warn!("Failed to set keyring primary: {e}"); + } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-desktop/src/screen/layout.rs`:
- Line 352: The identity list currently uses
scrollable(list).height(Length::Shrink) which allows it to expand and push nav
items off-screen; change the layout by wrapping the scrollable in a container
with a pixel max height and set the scrollable to height(Length::Fill) so it
scrolls within that cap—update the expression that builds identity_list (the
scrollable(list) usage) to be wrapped in a container with .max_height(/* e.g.
200.0 */) and change Length::Shrink to Length::Fill on the scrollable.
2 participants
Summary
KEEP_HOMEenv var override for data directory pathTest plan
Summary by CodeRabbit
New Features
Bug Fixes
UI Changes
Security