-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SFTP-based file handling implementation #74
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions.
synapse/cli/files.py
Outdated
console.print(f"[bold red]Failed to remove file:[/bold red] {e}") | ||
return | ||
|
||
console.print(f"[bold green]File removed:[/bold green] [blue]{remote_path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: newline(s)
SCIFI_DEFAULT_SFTP_USER = "scifi-sftp" | ||
SCIFI_DEFAULT_SFTP_PASS = "fluffy" | ||
|
||
def add_user_arguments(parser: argparse.ArgumentParser): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the way to idiomatically test this in the Description of the PR
|
||
console.print(table) | ||
|
||
def get_dir(sftp_conn: paramiko.SFTPClient, remote_path: str, output_path: str, console: Console): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do a check to make sure the passed remote_path doesn't include "../" for path traversal vulns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldnt matter unless we are worried about the client not handling the recursion properly. Since the SFTP user is chroot'd into the data directory, from their perspective /opt/scifi/data
is the root of the file system and there isnt any path ../
b.add_argument("uri", type=str) | ||
b.add_argument("remote_path", type=str, help="Remote path of file to download") | ||
b.add_argument("--output_path", "-o", type=str, default=os.getcwd(), help="Output path for downloaded file(s)") | ||
b.add_argument("--recursive", "-r", action="store_true", help="Download directories recursively") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
~/workspace/sci/synapse-python sage/files
.venv ❯ python3 synapse/cli/main.py file rm 10.40.62.8 "disk_writer/my test file" --password --recursive
Failed to remove file: [Errno 13] Permission denied
Failed to remove file: [Errno 13] Permission denied
Ive implemented a way to interact with a scifi (or other synapse device) running an SFTP server. This exposes commands to list/read/delete files from a device. The main use-case for this is with the outputs of the DiskWriter node, but if other output files are generated in the future, this could serve as a means to retrieve them.
This is basically just a simple SFTP client. LMK what yall think!