feat(auth): use default ssh key path if field is not filled - #103
Conversation
21fc5ff to
afc2b6d
Compare
There was a problem hiding this comment.
Pull request overview
Adds automatic SSH private-key discovery when no path is provided, supporting SSH and SFTP connections.
Changes:
- Allows empty public-key paths in connection flows.
- Resolves
~/.ssh/id_rsaorid_ed25519automatically. - Centralizes cross-platform tilde expansion.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/App.tsx |
Relaxes selected credential checks. |
src/components/connection-dialog.tsx |
Removes mandatory key-path validation. |
src-tauri/src/commands.rs |
Resolves default keys for SSH/SFTP. |
src-tauri/src/lib.rs |
Registers the key-path module. |
src-tauri/src/os_keypath.rs |
Implements key discovery and path expansion. |
src-tauri/src/sftp_client.rs |
Uses shared tilde expansion. |
src-tauri/src/ssh/mod.rs |
Uses shared tilde expansion. |
Suppressed comments (1)
src/App.tsx:590
- The same catch-all causes saved SSH connections using the advertised
keyboard-interactivemethod to bypass the credential dialog and call a backend that rejects that method. Onlypublickeyshould be considered credential-complete without a stored value here.
: (connectionData.authMethod === 'password'
? !!connectionData.password
: true);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| : (connectionData.authMethod === 'password' | ||
| ? !!connectionData.password | ||
| : !!connectionData.privateKeyPath); | ||
| : true) |
| : connectionData.authMethod === 'password' | ||
| ? !!connectionData.password | ||
| : (connectionData.authMethod === 'anonymous' ? true : !!connectionData.privateKeyPath); | ||
| : true; // Public-key auth can fallback to the default SSH key |
| /// Resolves the configured SSH private key path or falls back to the user's default key. | ||
| /// | ||
| /// If no key path is provided, checks for `id_rsa` first, then `id_ed25519`. | ||
| pub fn resolve_private_key_path(key_path: Option<&str>) -> Result<String, String> { |
afc2b6d to
49d59e8
Compare
|
@GOODBOY008 i added tests for os_keypath and move check credentials to connection-storage.ts |
49d59e8 to
3c87eed
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Default-key selection can choose an unusable identity without trying another valid key, and explicit paths unnecessarily require a resolvable home directory.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Balanced
| pub fn resolve_private_key_path(key_path: Option<&str>) -> Result<String, String> { | ||
| let home = dirs::home_dir().ok_or_else(|| "Could not determine home directory".to_string())?; | ||
| resolve_private_key_path_with_home(key_path, &home) | ||
| } |
There was a problem hiding this comment.
Ok, Copilot is right. Fixed this one
| for filename in ["id_rsa", "id_ed25519"] { | ||
| let candidate = ssh_dir.join(filename); | ||
| if candidate.is_file() { | ||
| return Ok(candidate.to_string_lossy().into_owned()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think existence doesn't imply authorization. I'm keeping the single attempt purposely: the fallback targets the common one-default-key setup, the failure is deterministic and names the exact key tried, and hosts with several keys can specify the path explicitly
There was a problem hiding this comment.
@supercute I add the failed key path to error msg and this will be useful to users.
|
@supercute I will resolve this conflicts. |
7e38630 to
d0d99b0
Compare
1. Added connection-storage.ts for centralize credentical checks. 2. Added tests for os_keypath.rs and for connection-storage.ts feat(auth): use default ssh key path if field is not filled
d0d99b0 to
afdaf2c
Compare
GOODBOY008
left a comment
GOODBOY008
left a comment
There was a problem hiding this comment.
@supercute Thanks for your contributions, LGTM~
Closes #102