Skip to content

Commit d12f8d3

Browse files
committed
SMB: Reuse saved password instead of re-prompting on every connect
Two QA follow-ups, both rooted in credentials Cmdr already had but never used: - Credential entries are now keyed by server identity, not the raw name string. `make_account_name` runs the server through `server_identity::credential_key` (bare instance name), so a password the frontend saves under `Naspolya` is found when the OS-mount upgrade path looks it up by the `statfs` service name `Naspolya._smb._tcp.local`. Before, the two never matched: a saved password was invisible on the next launch, leaving a stale `os_mount` (yellow) dot. - The share-activation gate now tries Cmdr's stored password before showing the login form. The share list often loads via the system Keychain (`smbutil`) without exercising Cmdr's own creds, so the gate kept re-prompting even with a working password saved. Stale stored creds fall through to the existing mount-failure login form.
1 parent 5846d35 commit d12f8d3

6 files changed

Lines changed: 120 additions & 19 deletions

File tree

apps/desktop/src-tauri/src/network/CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ server+share+port already mounted, skipping NetFS entirely.
217217
- **Auth mode is a guess**: `GuestAllowed` means "guest worked, creds might also work." `CredsRequired` means "guest failed, must have creds." Can't detect guest-only vs guest-or-creds without trying both.
218218
- **NetFS error 17 (EEXIST) is success** (macOS): Share already mounted. Return existing mount path, set `already_mounted: true`. Not an error.
219219
- **mDNS service type must include `.local.`**: `mdns-sd` requires full form `"_smb._tcp.local."` (trailing dot). Without it, browse() fails silently.
220-
- **Account name is lowercase**: `make_account_name` lowercases server name for consistency. Prevents duplicate entries for "SERVER" vs "server".
220+
- **Account name is keyed by server identity, not the raw string**: `make_account_name` runs the server through `server_identity::credential_key` (lowercase + strip the mDNS service suffix / `.local` down to the bare instance name), so `Naspolya`, `naspolya.local`, and `Naspolya._smb._tcp.local` all key the same entry. Without this the frontend saved under the mDNS instance name while the OS-mount upgrade path looked up by the `statfs` service name, so a just-saved password was never found on the next connect (the picker kept showing the `os_mount` dot and re-prompted). IP literals have no bare form and pass through unchanged.
221221
- **Linux `gio mount` requires GVFS**: The `gvfs-smb` package must be installed. Standard on Ubuntu/Fedora GNOME desktops. KDE desktops may need it explicitly.
222222
- **`ShareListError` uses internally tagged serde format** (`#[serde(tag = "type")]`) with struct variants. This keeps a flat JSON shape (`{ "type": "protocol_error", "message": "..." }`). The `MissingDependency` variant adds an optional `installCommand` field. When adding new variants, use struct syntax (not tuple).
223223
- **macOS smbutil and NetFSMountURLSync fail with loopback IP + non-standard port**: `//127.0.0.1:10480` gives "Broken pipe", but `//localhost:10480` works. `build_smbutil_url` and `NetworkMountView.svelte` both fall back to hostname when IP is `127.0.0.1` or `::1`. This matters for E2E testing against Docker containers on localhost.

apps/desktop/src-tauri/src/network/keychain.rs

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,17 @@ impl From<SecretStoreError> for KeychainError {
6161

6262
/// Creates the account name used for credential storage.
6363
/// Format: "smb://{server}/{share}" or "smb://{server}" for server-level credentials.
64+
///
65+
/// The server is collapsed to its stable identity via
66+
/// [`crate::network::server_identity::credential_key`] so that every name form of one
67+
/// server (mDNS instance name, `.local` hostname, `statfs` service name) keys the same
68+
/// entry. Without this, a password saved by the frontend under `Naspolya` is invisible
69+
/// to the upgrade path looking up `Naspolya._smb._tcp.local`.
6470
fn make_account_name(server: &str, share: Option<&str>) -> String {
71+
let key = crate::network::server_identity::credential_key(server);
6572
match share {
66-
Some(s) => format!("smb://{}/{}", server.to_lowercase(), s),
67-
None => format!("smb://{}", server.to_lowercase()),
73+
Some(s) => format!("smb://{}/{}", key, s),
74+
None => format!("smb://{}", key),
6875
}
6976
}
7077

@@ -187,6 +194,17 @@ mod tests {
187194
assert_eq!(account1, account2);
188195
}
189196

197+
/// All name forms of one server must produce the same account, so a password saved
198+
/// under the mDNS instance name is found when the upgrade path looks it up by the
199+
/// `statfs` service name or the resolved hostname.
200+
#[test]
201+
fn test_make_account_name_collapses_server_name_forms() {
202+
let saved = make_account_name("Naspolya", None);
203+
assert_eq!(saved, "smb://naspolya");
204+
assert_eq!(make_account_name("Naspolya.local", None), saved);
205+
assert_eq!(make_account_name("Naspolya._smb._tcp.local", None), saved);
206+
}
207+
190208
#[test]
191209
fn test_parse_password_entry() {
192210
let entry = make_password_entry("david", "secret123");

apps/desktop/src-tauri/src/network/server_identity.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,19 @@ fn normalize(s: &str) -> String {
3333
s.trim_end_matches('.').to_lowercase()
3434
}
3535

36+
/// The stable key for a server's stored credentials.
37+
///
38+
/// The same NAS reaches the credential store under different names depending on the
39+
/// caller: the frontend saves under the mDNS instance name (`Naspolya`), while the
40+
/// OS-mount upgrade path looks up by the `statfs` server (`Naspolya._smb._tcp.local`)
41+
/// or the resolved hostname (`naspolya.local`). Keying credentials on the raw string
42+
/// splits one server's password across several entries, so a password saved on mount
43+
/// is never found on the next connect. `credential_key` collapses every name form to
44+
/// the same bare identity (IP literals pass through unchanged — they have no bare form).
45+
pub fn credential_key(server: &str) -> String {
46+
bare_name(&normalize(server))
47+
}
48+
3649
/// Extracts the bare name from any of the forms a server name arrives in:
3750
/// `Naspolya._smb._tcp.local` → `naspolya`, `naspolya.local` → `naspolya`,
3851
/// `naspolya` → `naspolya`. Input must already be normalized.
@@ -156,4 +169,25 @@ mod tests {
156169
assert!(same_server("192.168.1.111", "192.168.1.111", &[]));
157170
assert!(same_server("some-nas", "some-nas", &[]));
158171
}
172+
173+
/// Every name form of one server must produce the SAME credential key, so a password
174+
/// saved by the frontend (mDNS instance name) is found by the upgrade path (`statfs`
175+
/// service name / resolved hostname). This is the keying split-brain that left a
176+
/// just-saved Naspolya password unusable on the next connect.
177+
#[test]
178+
fn test_credential_key_collapses_name_forms() {
179+
assert_eq!(credential_key("Naspolya"), "naspolya");
180+
assert_eq!(credential_key("Naspolya.local"), "naspolya");
181+
assert_eq!(credential_key("Naspolya.local."), "naspolya");
182+
assert_eq!(credential_key("Naspolya._smb._tcp.local"), "naspolya");
183+
assert_eq!(credential_key("Naspolya._smb._tcp.local."), "naspolya");
184+
}
185+
186+
/// IP literals have no bare form; they pass through (lowercased) unchanged so two
187+
/// different hosts can't collide on one credential entry.
188+
#[test]
189+
fn test_credential_key_passes_ip_through() {
190+
assert_eq!(credential_key("192.168.1.111"), "192.168.1.111");
191+
assert_eq!(credential_key("localhost"), "localhost");
192+
}
159193
}

apps/desktop/src/lib/file-explorer/network/CLAUDE.md

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -102,13 +102,15 @@ Rendered after user selects a host. Auth flow on mount:
102102
`authenticatedCredentials` is passed to `onShareSelect` so the caller can mount the share without re-prompting.
103103

104104
**Credential gate on share activation** (`activateShare`): every activation path (Enter, double-click, auto-mount)
105-
checks `authMode === 'creds_required' && !authenticatedCredentials` and shows the login form (with the share name in the
106-
title) instead of firing `onShareSelect` with null credentials. This combination is real: on macOS the share-listing
107-
fallback (`smbutil view -N`) authenticates via the SYSTEM Keychain, which Cmdr can't reuse for mounting, so the list
108-
renders fine while Cmdr holds no credentials — activating a share used to fire a doomed guest mount. The gated share
109-
waits in `pendingMountShare`; after a successful sign-in, `connectWithCredentials` fires
110-
`onShareSelect(pending, authenticatedCredentials)`. Cancel on a gated form returns to the share list (the list behind it
111-
is loaded and fine), not the host list. Pinned by `ShareBrowser.test.ts`.
105+
checks `authMode === 'creds_required' && !authenticatedCredentials`. When it trips, it first tries Cmdr's stored
106+
password (`loadStoredCredentials``getSmbCredentials`) and, if found, mounts with it silently; only if none is saved
107+
does it show the login form (with the share name in the title). This matters because the share list often loads via the
108+
SYSTEM Keychain (`smbutil view -N`) without ever exercising Cmdr's own creds, so `authenticatedCredentials` is null even
109+
when a working password is saved — without the stored-creds attempt the user got re-prompted on every visit. Stale
110+
stored creds aren't validated here; the mount fails and `NetworkMountView` surfaces the login form (see its
111+
mount-failure handler). The gated share waits in `pendingMountShare`; after a successful sign-in,
112+
`connectWithCredentials` fires `onShareSelect(pending, authenticatedCredentials)`. Cancel on a gated form returns to the
113+
share list (the list behind it is loaded and fine), not the host list. Pinned by `ShareBrowser.test.ts`.
112114

113115
When `authenticatedCredentials` is set (stored creds were used), a "Forget saved password" button appears in the header
114116
row. Clicking it calls `forgetCredentials` and clears `authenticatedCredentials`.

apps/desktop/src/lib/file-explorer/network/ShareBrowser.svelte

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@
116116
(s) => s.name.localeCompare(shareName, undefined, { sensitivity: 'base' }) === 0,
117117
)
118118
if (match) {
119-
activateShare(match)
119+
void activateShare(match)
120120
} else {
121121
addToast(`Share '${shareName}' not found on ${host.name}`, { level: 'warn' })
122122
}
@@ -130,8 +130,19 @@
130130
* which Cmdr can't reuse for mounting, so the list renders fine while
131131
* `authenticatedCredentials` stays null.
132132
*/
133-
function activateShare(share: ShareInfo) {
133+
async function activateShare(share: ShareInfo) {
134134
if (authMode === 'creds_required' && !authenticatedCredentials) {
135+
// Try Cmdr's stored password before prompting. The share list often loads
136+
// via the system Keychain (smbutil) without ever exercising our own creds,
137+
// so we may already have a working password saved. If the stored creds turn
138+
// out to be stale, the mount fails and NetworkMountView surfaces the login
139+
// form (see its mount-failure handler), so we don't need to validate here.
140+
const stored = await loadStoredCredentials()
141+
if (stored) {
142+
authenticatedCredentials = stored
143+
onShareSelect?.(share, stored)
144+
return
145+
}
135146
pendingMountShare = share
136147
loginError = undefined
137148
showLoginForm = true
@@ -140,6 +151,16 @@
140151
onShareSelect?.(share, authenticatedCredentials)
141152
}
142153
154+
/** Reads Cmdr's stored credentials for this host, or null if none are saved. */
155+
async function loadStoredCredentials(): Promise<{ username: string; password: string } | null> {
156+
try {
157+
const creds = await getSmbCredentials(host.name, null)
158+
return { username: creds.username, password: creds.password }
159+
} catch {
160+
return null
161+
}
162+
}
163+
143164
/** Sync share list to MCP so agents see the same data as the UI. */
144165
async function syncPaneStateToMcp() {
145166
if (!paneId) return
@@ -385,7 +406,7 @@
385406
// noinspection JSUnusedGlobalSymbols -- used dynamically by NetworkMountView / MCP
386407
export function openCursorItem(): void {
387408
if (cursorIndex >= 0 && cursorIndex < sortedShares.length) {
388-
activateShare(sortedShares[cursorIndex])
409+
void activateShare(sortedShares[cursorIndex])
389410
}
390411
}
391412
@@ -395,7 +416,7 @@
395416
396417
function handleShareDoubleClick(index: number) {
397418
if (index >= 0 && index < sortedShares.length) {
398-
activateShare(sortedShares[index])
419+
void activateShare(sortedShares[index])
399420
}
400421
}
401422
@@ -473,7 +494,7 @@
473494
if (e.key === 'Enter') {
474495
e.preventDefault()
475496
if (cursorIndex >= 0 && cursorIndex < sortedShares.length) {
476-
activateShare(sortedShares[cursorIndex])
497+
void activateShare(sortedShares[cursorIndex])
477498
}
478499
return true
479500
}

apps/desktop/src/lib/file-explorer/network/ShareBrowser.test.ts

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,9 @@ describe('ShareBrowser credential gate', () => {
104104
await waitForShareList(target)
105105

106106
api.openCursorItem()
107-
await tick()
107+
await vi.waitFor(() => {
108+
expect(target.querySelector('.login-title')).toBeTruthy()
109+
})
108110

109111
expect(onShareSelect).not.toHaveBeenCalled()
110112
const title = must(target.querySelector('.login-title'), 'the login form')
@@ -113,6 +115,27 @@ describe('ShareBrowser credential gate', () => {
113115
await unmount(component)
114116
})
115117

118+
it('uses stored credentials silently when creds are required (no prompt)', async () => {
119+
// The listing came back creds_required (it succeeded via the system Keychain), but
120+
// Cmdr has the password saved. Activating the share must reuse it, not re-prompt.
121+
h.fetchShares.mockResolvedValue({ shares: [naspi], authMode: 'creds_required', fromCache: false })
122+
h.getSmbCredentials.mockResolvedValue({ username: 'david', password: 'hunter2' })
123+
const onShareSelect = vi.fn()
124+
const { target, component, api } = mountBrowser(onShareSelect)
125+
await waitForShareList(target)
126+
127+
api.openCursorItem()
128+
await vi.waitFor(() => {
129+
expect(onShareSelect).toHaveBeenCalledWith(expect.objectContaining({ name: 'naspi' }), {
130+
username: 'david',
131+
password: 'hunter2',
132+
})
133+
})
134+
expect(target.querySelector('.login-title'), 'must not prompt when stored creds exist').toBeNull()
135+
136+
await unmount(component)
137+
})
138+
116139
it('selects the pending share with the entered credentials after a successful sign-in', async () => {
117140
h.fetchShares.mockResolvedValue({ shares: [naspi], authMode: 'creds_required', fromCache: false })
118141
h.listSharesWithCredentials.mockResolvedValue({ shares: [naspi], authMode: 'creds_required', fromCache: false })
@@ -121,7 +144,9 @@ describe('ShareBrowser credential gate', () => {
121144
await waitForShareList(target)
122145

123146
api.openCursorItem()
124-
await tick()
147+
await vi.waitFor(() => {
148+
expect(target.querySelector('#username')).toBeTruthy()
149+
})
125150

126151
const usernameInput = must(target.querySelector<HTMLInputElement>('#username'), 'the username input')
127152
const passwordInput = must(target.querySelector<HTMLInputElement>('#password'), 'the password input')
@@ -153,8 +178,9 @@ describe('ShareBrowser credential gate', () => {
153178
await waitForShareList(target)
154179

155180
api.openCursorItem()
156-
await tick()
157-
expect(target.querySelector('.login-title')).toBeTruthy()
181+
await vi.waitFor(() => {
182+
expect(target.querySelector('.login-title')).toBeTruthy()
183+
})
158184

159185
// Cancel: the share list is loaded and fine, so stay on it.
160186
const cancelButton = must(

0 commit comments

Comments
 (0)