Skip to content

Commit 5846d35

Browse files
committed
SMB: Fix wrong-password message and stale connection dot
QA follow-ups on the login-flow fix: - A rejected sign-in now says "Invalid username or password" instead of "Authentication required": the new `classify_authenticated_error` maps auth-class rejections of explicit credentials to `AuthFailed` (`SigningRequired` deliberately stays untouched). - The volume picker dot now flips to green (direct) right after a sign-in upgrades the connection: `register_replacing_predecessor` emits `volumes-changed` after registering, since the after-sign-in and already-mounted upgrade paths have no FSEvents mount event to ride.
1 parent 359919a commit 5846d35

4 files changed

Lines changed: 54 additions & 2 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
@@ -172,7 +172,7 @@ When the user mounts an SMB share, we establish a parallel smb2 connection along
172172

173173
Every `NSWorkspaceDidMountNotification` on an SMB share triggers a fresh `register_smb_volume` cycle, and the user can re-trigger the same path via manual "Connect directly" after an unmount/remount cycle. Without explicit cleanup, the new `SmbVolume` simply overwrites the old one's `Arc` slot in `VolumeManager`. The old volume's watcher would still exit eventually — its `watcher_cancel: Mutex<Option<oneshot::Sender<()>>>` drops with the `Arc`, the `Sender` drops, the watcher's `cancel_rx` resolves to `Err(Closed)` and the `select!` branch fires — but timing is non-deterministic ("whenever the last `Arc` ref goes away") and an in-flight `do_attempt_reconnect` on the displaced volume could install a fresh session into a volume that's no longer in the manager.
174174

175-
`register_replacing_predecessor` (in `smb_upgrade.rs`) closes both gaps: it looks up the predecessor via `manager.get(volume_id)`, calls `on_unmount` on it (which sets the `unmounted` flag, transitions state, pings the watcher cancel, and drops the smb2 session), then `register`s the new volume. Both `register_smb_volume` and `try_smb_upgrade` route through this helper.
175+
`register_replacing_predecessor` (in `smb_upgrade.rs`) closes both gaps: it looks up the predecessor via `manager.get(volume_id)`, calls `on_unmount` on it (which sets the `unmounted` flag, transitions state, pings the watcher cancel, and drops the smb2 session), then `register`s the new volume. Both `register_smb_volume` and `try_smb_upgrade` route through this helper. It also emits `volumes-changed` after registering: the after-sign-in and already-mounted upgrade paths have no FSEvents mount event to ride, so without the explicit broadcast the frontend keeps the stale `os_mount` dot on a volume that's already `direct`.
176176

177177
**Gotcha**: `SmbVolume::on_unmount` uses `blocking_write()` / `blocking_lock()` because the FSEvents-thread call site (`volumes::watcher::handle_volume_unmounted`) is sync. Inside `register_replacing_predecessor` we're in an async context, so calling `on_unmount` directly would panic ("cannot block_on within a runtime"). The helper wraps the call in `tokio::task::spawn_blocking(...).await` so the lock acquisition runs on the blocking-thread pool. Don't switch back to a direct call.
178178

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ use super::smb_smbutil::{list_shares_smbutil, list_shares_smbutil_authenticated_
1818
// argv); only the non-macOS authed fallback (smbclient `-A` authfile) still uses this.
1919
#[cfg(not(target_os = "macos"))]
2020
use super::smb_smbutil::list_shares_smbutil_with_auth;
21+
// Only the macOS arm classifies the raw smb2 failure of an authenticated listing
22+
// (Linux retries via the smbclient authfile fallback instead).
23+
#[cfg(target_os = "macos")]
24+
use super::smb_util::classify_authenticated_error;
2125
use super::smb_util::{classify_error, convert_shares, is_auth_error};
2226

2327
/// Lists shares on a network host.
@@ -168,7 +172,9 @@ async fn list_shares_smb2(
168172
}),
169173
Err(e) => {
170174
debug!("smb2 authenticated list failed: {}", e);
171-
Err(classify_error(&e))
175+
// Authenticated context: a rejected session means
176+
// wrong credentials, not "authentication required".
177+
Err(classify_authenticated_error(&e))
172178
}
173179
};
174180
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,12 @@ async fn register_replacing_predecessor(
8989
let _ = tokio::task::spawn_blocking(move || prev.on_unmount()).await;
9090
}
9191
manager.register(volume_id, new_volume);
92+
93+
// Tell the frontend the volume's connection state changed (os_mount → direct).
94+
// The auto-upgrade paths often coincide with an FSEvents mount event that triggers
95+
// a broadcast anyway, but the after-sign-in and already-mounted paths have no
96+
// mount event at all: without this, the picker keeps the stale os_mount dot.
97+
crate::volume_broadcast::emit_volumes_changed();
9298
}
9399

94100
/// Tries to establish a direct smb2 connection and register as `SmbVolume`.

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,25 @@ pub fn classify_error(err: &smb2::Error) -> ShareListError {
2727
}
2828
}
2929

30+
/// Classifies an error from an *authenticated* listing attempt.
31+
///
32+
/// The caller supplied explicit credentials, so an auth-class rejection
33+
/// (`AuthRequired` / `AccessDenied`, for example `STATUS_LOGON_FAILURE`) means the
34+
/// credentials are wrong (`AuthFailed`), not that authentication is required.
35+
/// `SigningRequired` is deliberately NOT remapped: it's a protocol capability mismatch,
36+
/// not a credential problem. Everything else falls through to [`classify_error`].
37+
// Consumed by the macOS arm of `smb_client` today (Linux's authenticated fallback goes
38+
// through smbclient); the classifier itself is platform-agnostic.
39+
#[cfg_attr(not(target_os = "macos"), allow(dead_code))]
40+
pub fn classify_authenticated_error(err: &smb2::Error) -> ShareListError {
41+
match err.kind() {
42+
ErrorKind::AuthRequired | ErrorKind::AccessDenied => ShareListError::AuthFailed {
43+
message: "Invalid username or password".to_string(),
44+
},
45+
_ => classify_error(err),
46+
}
47+
}
48+
3049
/// Converts smb2 share info to Cmdr's ShareInfo type.
3150
/// smb2's `list_shares()` already filters to disk shares and strips `$` shares.
3251
pub fn convert_shares(shares: Vec<smb2::ShareInfo>) -> Vec<ShareInfo> {
@@ -57,6 +76,27 @@ mod tests {
5776
assert!(!is_auth_error(&smb2::Error::Disconnected));
5877
}
5978

79+
/// When the caller supplied explicit credentials, an auth-class rejection means the
80+
/// credentials are WRONG, not that authentication is required. Without the
81+
/// context-aware classifier, a wrong password surfaced as "Authentication required.
82+
/// Please enter your credentials." in the login form (observed in QA against the
83+
/// Naspolya NAS) instead of "Invalid username or password."
84+
#[test]
85+
fn test_classify_authenticated_error_maps_rejection_to_auth_failed() {
86+
match classify_authenticated_error(&smb2::Error::Auth {
87+
message: "STATUS_LOGON_FAILURE during SessionSetup".to_string(),
88+
}) {
89+
ShareListError::AuthFailed { .. } => {}
90+
e => panic!("Expected AuthFailed for a rejected authenticated session, got {:?}", e),
91+
}
92+
93+
// Non-auth errors keep their regular classification.
94+
match classify_authenticated_error(&smb2::Error::Timeout) {
95+
ShareListError::Timeout { .. } => {}
96+
e => panic!("Expected Timeout, got {:?}", e),
97+
}
98+
}
99+
60100
#[test]
61101
fn test_classify_error_timeout() {
62102
match classify_error(&smb2::Error::Timeout) {

0 commit comments

Comments
 (0)