Keeper: Nested Shared Folder support and add-account toggles#682
Keeper: Nested Shared Folder support and add-account toggles#682pkamble-ks wants to merge 11 commits into
Conversation
Keeper: Nested Share Subfolder support and add-account toggles
List classic vault and Nested Shared Folder records together, route add/edit/delete to the correct Commander verbs, and expose optional checkboxes on the Add Account panel. Also improve login validation and connectivity error messages.
Keeper: add Nested Shared Folder support and add-account toggles.
pkamble-ks
changed the title
gnachman
left a comment
gnachman
left a comment
There was a problem hiding this comment.
Thanks for this — nested-folder support is a nice addition. A few things to look at, mostly on the iTerm2 host-side integration (the password-manager UI and credential plumbing shared with the other adapters). Inline comments below.
| <windowStyleMask key="styleMask" titled="YES"/> | ||
| <windowPositionMask key="initialPositionMask" leftStrut="YES" rightStrut="YES" topStrut="YES" bottomStrut="YES"/> | ||
| <rect key="contentRect" x="344" y="373" width="330" height="136"/> | ||
| <rect key="contentRect" x="344" y="373" width="330" height="196"/> |
There was a problem hiding this comment.
The New Account panel height is bumped from 136 to 196 for all password managers, but configureFirstAddAccountToggle only toggles hidden on the checkbox/label — nothing shrinks the panel or repositions the OK/Cancel buttons when there's no toggle. Every adapter that doesn't declare an add-account toggle (Keychain, 1Password, Bitwarden, LastPass, KeePassXC) will now show ~60px of empty space between the Password field and the buttons. Could the panel resize (or the controls collapse) when firstAddAccountToggleDescription is nil so the non-toggle case looks the same as before?
There was a problem hiding this comment.
Fixed. Reverted the xib default to 136px and only expand the panel to 196px in configureFirstAddAccountToggle when the adapter declares a toggle. Adapters without a toggle (Bitwarden, KeePassXC, etc.) should look the same as before.
| guard let field = fields.first(where: { $0.key == key }) else { return nil } | ||
| if field.persistInKeychain { | ||
| return try? SSKeychain.password(forService: keychainCredentialServiceName, account: key) | ||
| if keychainSettingsCacheCheckedKeys.contains(key) { |
There was a problem hiding this comment.
Two concerns with this keychain cache:
- It's never invalidated across instances.
PasswordManagerDataSourceProvider.forTerminaland.forBrowsereach hold their own process-lifetime_keeper, but they share one keychain entry.invalidateKeychainSettingsCacheonly runs on the instance that did the write (viastoreSettingsValue), so if the API key is updated on one side, the other keeps serving the stale value for the rest of the session. - Misses are cached too (the key goes into
keychainSettingsCacheCheckedKeyseven when the value is nil), so a key read before it's populated is pinned to nil on that instance.
Net effect is a stale/empty credential that survives until the app restarts. Was there a measured cost to the uncached SSKeychain.password reads that motivated the cache? If so it may be worth keying invalidation off the actual keychain rather than only local writes.
There was a problem hiding this comment.
Removed the keychain settings cache entirely. storedSettingsValue reads from the keychain directly again, so terminal/browser instances and keychain updates no longer go stale for the rest of the session.
| masterPassword = loadPersistedCredentials() | ||
| guard masterPassword == nil else { return } | ||
|
|
||
| if let secretFromSettings = persistedSecretSettingsValue(), |
There was a problem hiding this comment.
This changes credential precedence: persistedSecretSettingsValue() now wins over loadPersistedCredentials(). They read different keychain entries — the secret settings field is stored under account: <fieldKey> (e.g. apiKey), whereas a successful login persists the working credential under account: identifier. So if the value typed into the settings field ever diverges from the credential that actually logged in, the (possibly stale) settings value now shadows the known-good one and auto-login can fail where it previously succeeded. Is the precedence intentional? Falling back to loadPersistedCredentials() first (or only using the settings value when there's no persisted credential) would preserve the old behavior.
There was a problem hiding this comment.
Unintentional regression — fixed.
loadPersistedCredentials() (login credential under identifier) is tried first; the settings keychain value is only used as a fallback when there is no persisted login credential.
| KeeperAdapterLog.write("setPassword: success uid=\(uid) via \(verb)") | ||
| return | ||
| } | ||
| lastResponse = result.raw |
There was a problem hiding this comment.
In the .none source case (any accountID without a classic:/nested: prefix — i.e. records already stored by existing users before this change), setPassword/deleteRecord try the first verb then the second, and lastResponse is overwritten unconditionally. So when the first verb fails for a real reason (e.g. a genuine permission error on a classic record), the code then runs the nsf verb on the same UID and the user is shown the nsf error ("not found in any nested shared folder") instead of the real one. Consider preserving the first meaningful error, or only falling through to the second verb when the first failure looks like a wrong-record-type/not-found signal.
There was a problem hiding this comment.
Fixed. For the .none source case, the second verb is only tried when the first failure looks like not-found / wrong record type (keeperCommandFailureLooksLikeWrongRecordType). Otherwise the first error is surfaced to the user.
| let response = try JSONDecoder().decode(RecordAddResponse.self, from: data) | ||
| guard response.status == "success", let uid = response.data?.record_uid, !uid.isEmpty else { | ||
| let extractedUid: String? = response.data?.effectiveUid | ||
| ?? response.message?.trimmingCharacters(in: .whitespacesAndNewlines) |
There was a problem hiding this comment.
addRecord falls back to using the response message field as the record UID, and unlike the get/set/delete paths it doesn't run the result through validatedRecordUID. If a Commander response ever puts something other than a bare UID in message, the returned identifier becomes nested:<that text> and later get/set/delete on it will fail validation (the add appears to succeed but the record can't be operated on). Could this parse the UID only from a structured field and validate it before returning?
There was a problem hiding this comment.
Fixed. The returned UID must pass validatedRecordUID. Structured data.record_uid / data.uid is preferred; message is only used as a fallback when it validates as a bare UID.
| let uid = try validatedRecordUID(parsed.uid) | ||
|
|
||
| func attempt(_ verb: String) throws -> (success: Bool, raw: Data) { | ||
| let cmd = "\(verb) --force -r \(uid) \(passwordCommandFragment(password: newPassword))" |
There was a problem hiding this comment.
Just confirming --force is intentional/accepted on record-update/nsf-record-update (and record-add/nsf-record-add at line 791)? The test mock matches commands by prefix, so the flag isn't actually exercised — worth a note in the PR that it's been confirmed against a live Commander.
There was a problem hiding this comment.
Yes, intentional. Confirmed against live Commander Service Mode: classic record-add failed without --force under password policy, and updates needed it as well. The integration mock matches commands by prefix so it does not assert the flag explicitly.
| }() | ||
|
|
||
| static let isEnabled: Bool = { | ||
| ProcessInfo.processInfo.environment["ITERM2_KEEPER_ADAPTER_LOG_DISABLED"] == nil |
There was a problem hiding this comment.
This logger is enabled by default and writes to a persistent file at ~/Library/Logs/iterm2-keeper-adapter.log. Passwords are redacted, but account titles and usernames are written in plaintext. For a password-manager integration it'd be safer to default this off (opt-in via the env var) rather than opt-out.
There was a problem hiding this comment.
Changed to opt-in: logging is off by default and enabled with ITERM2_KEEPER_ADAPTER_LOG_ENABLED=1.
| optional toggles in their handshake that the | ||
| Add Account panel renders as checkboxes. The | ||
| Keeper adapter uses this to offer a “Use | ||
| classic permission model” choice — leaving |
There was a problem hiding this comment.
Minor: this line uses an em dash, which the project style avoids in user-facing text. Could swap it for " - " or reword.
There was a problem hiding this comment.
Fixed — replaced the em dash with a period per project style.
Address PR review: panel sizing, credentials, fallback errors, opt-in logging
|
@gnachman Thanks for the review, pushed fixes addressing all inline comments (commit ae7e41d, now on this branch via fork merge). Summary:
Tests still 21/21. Happy to adjust anything else. |
Review notesReviewed the diff plus the PR head blob ( Correctness
Cleanup / efficiency
Conventions
ClearedRelease-notes line widths (all under 50), no |
Prefer nsf-list on duplicate UIDs, fix folder filtering and NSF add UID parsing, dedupe mutation paths, drop auto sync-down on list, invalidate login credentials on API key change, and fix Add Account panel layout. Live Commander validation completed.
Address Keeper adapter and Add Account panel review feedback
|
@gnachman Thanks for the detailed second pass — pushed fixes in b149796 (merged to master via PR #4). Summary by item: Correctness
Cleanup / efficiency
Conventions
Add Account panel (from live testing)
Tests & live validation
Happy to adjust anything else. |
|
Thanks for the substantial work here. I went through it in depth. Most of my feedback is architectural, with a batch of concrete bugs at the end. The recurring theme is Keeper-specific concepts leaking into shared password-manager infrastructure, and I would like to see those pulled back before this is ready for line-by-line review. 1. The API key is modeled as both a master password and a settings fieldThe handshake sets
If a future adapter genuinely needs a login-only settings secret, add an explicit 2. Do not bake the (Classic)/(Nested) label into accountName
Please model record source as a first-class field on the account and let the host format the label. Keep 3. Keep classic:/nested: routing internal to the adapterThe 4. Add-account toggles and flags-add belong on the base data source, not AdapterCapabilitiesRight now the flags-carrying Please move 5. Correctness bugs
6. Question: cache invalidation after set-password
7. Cleanup
- (void)configureNewAccountPanelFieldLayoutShowingToggle:(BOOL)showingToggle {
const CGFloat fieldHeight = 27;
const CGFloat labelOffset = 3;
const CGFloat topMargin = 34;
const CGFloat toggleGap = 5;
const CGFloat panelHeight = showingToggle ? kNewAccountPanelHeightWithToggle
: kNewAccountPanelHeightWithoutToggle;
CGFloat y = panelHeight - topMargin;
iTermFixViewY(_newAccount, y);
iTermFixViewY(_newAccountLabel, y + labelOffset);
y -= fieldHeight;
iTermFixViewY(_newUserName, y);
iTermFixViewY(_newUserNameLabel, y + labelOffset);
y -= fieldHeight;
iTermFixViewY(_newAccountPassword, y);
iTermFixViewY(_newPasswordLabel, y + labelOffset);
iTermFixViewY(_generatePasswordButton, y - labelOffset);
if (!showingToggle) {
return;
}
y -= fieldHeight + toggleGap;
iTermFixViewY(_addAccountToggleCheckbox, y);
y -= fieldHeight;
iTermFixViewY(_addAccountToggleLabel, y);
} |
Drop Settings apiKey, expose sourceLabel for host formatting, move add-account toggles onto PasswordManagerDataSource, and fix list merge/timeout/fallback/logging issues from review.
Keeper: NSF support with adapter-local routing and host sourceLabel
|
@gnachman Thanks for the thorough architectural review — addressed in 40885a1 (on this PR via cec0504). 1. API key as master password onlyDropped the Settings 2. Stable
|
2 participants
Summary
Extends the Keeper Security password manager integration to support Nested Shared Folders (NSF) alongside the classic vault.
list --format=json) with NSF records (nsf-list --records --format=json). Entries are labeled (Classic) or (Nested).record-*/rmvsnsf-record-*/nsf-rm). Account IDs use internalclassic:/nested:prefixes so routing survives the host protocol; Commander still receives plain UIDs.addAccountTogglesin the handshake; the Password Manager Add Account panel renders them as checkboxes and passesflagson add.sync-down. Connectivity and HTML proxy errors surface actionable messages.Commander Service Mode must allow:
list,get,sync-down,record-add,record-update,rm,nsf-list,nsf-record-add,nsf-record-update,nsf-rm.Test plan
cd pwmplugin && swift test --filter KeeperIntegrationTests— 21/21 passed