Ruby SFTP client

Question

I'm on Ruby for three weeks and now I want to learn more. Please review this SFTP client and suggest any necessary changes.

# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 3 of the License, or
# (at your option) any later version.
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
# GNU General Public License for more details.
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <https://www.gnu.org/licenses/>.
# What is Net::SFTP?
# Net::SFTP is a pure-Ruby implementation of the SFTP protocol. 
# That’s "SFTP" as in "Secure File Transfer Protocol", as defined as an adjuct to the SSH specification. 
# Not "SFTP" as in "Secure FTP" (a completely different beast). 
# Nor is it an implementation of the "Simple File Transfer Protocol" (which is in no way secure).
require 'net/sftp'
require 'ostruct'
require 'optparse'
class Christina
 Script = "Script => (christina.rb)"
 Version = "Version => (0.0.1)"
 def run(arguments)
 parse(arguments)
 connect(arguments)
 end
 private
 def parse(arguments)
 ARGV << "-h" if ARGV.empty?
 @options = OpenStruct.new
 
 args = OptionParser.new do |args|
 args.banner = "Usage: #{__FILE__} [options]"
 
 args.on("-s", "--set-host=HOST", String,
 "The Host To Connect To") do |set_host|
 @options.set_host = set_host
 end
 
 args.on("-u", "--username=USERNAME", String,
 "Authenticate With A Username") do |username|
 @options.username = username
 end
 args.on("-p", "--password=PASSWORD", String,
 "Authenticate With A Password") do |password|
 @options.password = password
 end
 args.on("-w", "--wharf=WHARF", Integer,
 "Specify The Wharf (Port) The Service Is Running") do |wharf|
 @options.wharf = wharf
 end
 args.on("-t", "--transfer=FILE", String,
 "Upload An Entire File On Disk") do |transfer|
 @options.transfer = transfer
 end
 args.on("-d", "--destination=FILE", String,
 "Destination For The Uploaded File") do |destination|
 @options.destination = destination
 end
 args.on("-m", "--mkdir=CREATE DIRECTORY", String,
 "Create A Directory") do |mkdir|
 @options.mkdir = mkdir
 end
 args.on("-r", "--rmdir=REMOVE DIRECTORY", String,
 "Remove A Directory") do |rmdir|
 @options.rmdir = rmdir
 end
 args.on("-q", "--query=FILE", String,
 "Query A File’s Permissions") do |query|
 @options.query = query
 end
 args.on("-e", "--erase=FILE", String,
 "Delete A File") do |erase|
 @options.erase = erase
 end
 args.on("-c", "--change=FILE", String,
 "Change A File’s Permissions") do |change|
 @options.change = change 
 end
 args.on("-a", "--authorization=INTEGER", Integer,
 "Combine With The Above Command To Change A File's Permissions") do |authorization|
 @options.authorization = authorization
 end
 args.on("-b", "--brand=FILE", String,
 "Brand (Rename) A File") do |name|
 @options.name = name
 end
 args.on("-n", "--new=FILE", String,
 "The Name Off The Renamed File") do |new|
 @options.new = new
 end
 args.on("-l", "--list=DIRECTORY", String,
 "Query The Contents Of A Directory") do |list|
 @options.list = list
 end
 args.on("-g", "--grab=FILE", String,
 "Grab Data Off The Remote Host Directly To A Buffer") do |grab|
 @options.grab = grab
 end
 args.on("-f", "--file=FILE", String,
 "Download Directly To A Local File") do |file|
 @options.file = file
 end
 args.on("-o", "--output=FILE", String,
 "Destination Off The Downloaded File") do |output|
 @options.output = output
 end
 args.on("-h", "--help", "Show Help And Exit") do
 puts args
 exit
 end
 args.on("-V", "--version", "Show Version And Exit") do
 puts Script
 puts Version
 exit 
 end
 begin
 args.parse!(arguments)
 
 rescue OptionParser::MissingArgument => error
 puts "[!] => #{error.message}"
 exit
 rescue OptionParser::InvalidOption => error
 puts "[!] => #{error.message}"
 exit
 end
 end
 end
 def connect(arguments)
 output("----------------------------------------------------------")
 output("[*] Starting at => #{Time.now}")
 output("[*] Operating System => #{RUBY_PLATFORM}")
 output("----------------------------------------------------------")
 output("[i] Connecting to Secure SHell")
 output("\t-- Host => #{@options.set_host}")
 output("\t-- Username => #{@options.username}")
 output("\t-- Password => #{@options.password}")
 output("\t-- Wharf => #{@options.wharf}")
 output("----------------------------------------------------------")
 Net::SFTP.start(@options.set_host, @options.username, :password => @options.password, :port => @options.wharf) do |sftp|
 mkdir(sftp) if @options.mkdir
 rmdir(sftp) if @options.rmdir
 remove(sftp) if @options.erase 
 query(sftp) if @options.query
 list(sftp) if @options.list
 grab(sftp) if @options.grab
 rename(sftp) if @options.name || @options.new
 change(sftp) if @options.change || @options.authorization
 upload(sftp) if @options.transfer || @options.destination
 download(sftp) if @options.file || @options.output
 end
 output("----------------------------------------------------------")
 output("[*] Exiting at => #{Time.now}")
 output("----------------------------------------------------------")
 end
 def mkdir(sftp)
 sftp.mkdir!(@options.mkdir)
 output("[i] Creating Directory => #{@options.mkdir}")
 end
 def rmdir(sftp)
 sftp.rmdir!(@options.rmdir) 
 output("[i] Removing Directory => #{@options.rmdir}")
 end
 def remove(sftp)
 sftp.remove!(@options.erase)
 output("[i] Removing File => #{@options.erase}")
 end
 def query(sftp)
 output("[i] Checking Permissions => #{sftp.stat!(@options.query).permissions}")
 end
 def grab(sftp)
 sftp.download!(@options.grab)
 output("[i] Grabing File => #{@options.grab}")
 end
 def rename(sftp)
 sftp.rename!(@options.name, @options.new)
 output("[i] Renaming File => #{@options.name}")
 output("[i] New File => #{@options.new}")
 end
 def change(sftp)
 sftp.setstat!(@options.change, :permissions => @options.authorization)
 output("[i] Setting Permissions To => #{@options.change}")
 output("[i] Permissions Set To => #{@options.authorization}")
 end
 def upload(sftp) 
 sftp.upload!(@options.transfer, @options.destination)
 output("[i] Uploading File To => #{@options.set_host}")
 output("\t-- Local File => #{@options.transfer}")
 output("\t-- File Destination => #{@options.destination}")
 end
 def download(sftp)
 sftp.download!(@options.file, @options.output)
 output("[i] Downloading File From => #{@options.set_host}")
 output("\t-- Remote File => #{@options.file}")
 output("\t-- File Destination => #{@options.output}")
 end
 def list(sftp)
 output("[i] Listing Contents Of => #{@options.list}")
 output("----------------------------------------------------------")
 sftp.dir.foreach(@options.list) do |entry|
 output(entry.longname)
 end
end
 def output(string)
 puts "#{string}"
 end
end
sftp = Christina.new
sftp.run(ARGV)

Answer 1

... under the terms of the GNU General Public License ...

IANAL, TINLA. As copyright holder you chose to license the code in your posting under the terms of CC-BY-SA, which is incompatible with GPLv3 as it does not allow authors to

... legally restrict others from doing anything the [CC] license permits.

It's possible you would find the (more permissive) MIT or 2-clause BSD licenses to your liking.

tl;dr: These lines do not appear to have their intended effect, in the review context.

The CC folks have this to say about porting code between licenses, which apparently would go somewhat smoothly had you tried to do it in the opposite direction:

People may not adapt a GPLv3-licensed work and use BY-SA to license their contributions. This is not allowed by GPLv3. Thus, this compatibility determination only allows movement one way, from BY-SA 4.0 to GPLv3.

---

A concise "RFC913" citation for that third protocol wouldn't hurt.

For the first and second protocols, I think that you are respectively referring to these?

1. https://en.wikipedia.org/wiki/SSH_File_Transfer_Protocol
2. https://en.wikipedia.org/wiki/Secure_FTP_(software)

But documentation shouldn't be a guessing game. Spell it out for us. Cite your references.

---

The "public" run vs "private" implementation is a nice touch. Thank you.

---

 @options.set_host = set_host

I don't understand why the UI doesn't offer a --host option, but whatever. Even if that remains unchanged, consider assigning just @options.host.

 args.on("-w", "--wharf=WHARF", Integer,

This term doesn't appear in section 4 of rfc1192. Recommend that you

1. Explicitly identify the target audience you're addressing, whichever community would customarily use "wharf" as a 16-bit socket identifier, and
2. Add a --port synonym which has the same effect.

nit: Consider Using Fewer Capital Letters In The Help Text.

 args.on("-p", "--password=PASSWORD", String,

This is a tricky one. What you have there is fine, don't change it. But some folks will look for the ability to have a non-echoing interactive prompt. And a more common practice is to pull such credentials from an exported environment variable. There's also config files like ~/.netrc

 args.on("-t", "--transfer=FILE", String,

Honestly, these are starting to sound less like "options" and more like "commands". You have a bunch of them. And I can envision a caller wanting to execute several of them once they've spent some time setting up an SSH connection. If caller specifies -t A.txt -t B.txt it's pretty clear they'll only get the second file. Consider supporting a list of commands? Or reading commands from a file, such as stdin?

typo ("of"):

 "The Name Off The Renamed File") do |new|

---

 rescue OptionParser::MissingArgument => error
 ...
 rescue OptionParser::InvalidOption => error

DRY. Maybe we can combine the puts behavior, here?

Also, think about the diagnostic value of the message. Maybe the person viewing it would benefit from a reminder that --help is available?

---

 output("[*] Operating System => #{RUBY_PLATFORM}")

Consider clarifying that this is the local OS.

 remove(sftp) if @options.erase 
 query(sftp) if @options.query

Hmmm, that seems undesirable. If I was reading the --help output, I might believe that query happens before erase, but it turns out that is not so. And we also see such inconsistencies in "list" and others. There is a design choice to be made, here. Consider adopting either

1. Args validation limits the caller to just a single verb, or
2. Args support a list of verbs to execute in sequence.

 rename(sftp) if @options.name || @options.new
 change(sftp) if @options.change || @options.authorization
 upload(sftp) if @options.transfer || @options.destination
 download(sftp) if @options.file || @options.output

Sorry, I don't find those obvious. Oh, wait, I just viewed the docs. At least some of those require that both args be specified, right? And we've not done any local validation along those lines, yet. IDK, maybe the rename method being called gives an appropriate diagnostic message to the user when only a single arg appeared on the command line?

 output("[*] Exiting at => #{Time.now}")

This is lovely. Maybe displaying the elapsed time is also of interest to caller?

---

 sftp.mkdir!(@options.mkdir)
 output("[i] Creating Directory => #{@options.mkdir}")

In each of these methods, the order seems backwards. Consider telling the user before executing the verb, in case something goes wrong and it blows up.

Alternatively, announce the fait accompli in the past tense, with "Created".

Also, ruby offers a lovely logger. Consider using it, so callers can reliably parse your output.

And, oh yeah. Everything that @papirtiger said!

• Peilonrayz: IANAL TINLA for the first paragraph? IANAL TINLA Your first paragraph doesn't acknowladge the possibility to dual licence, making your point moot. Additionally I'm pretty sure a good lawyer would plead breaking the TOS rather than breaking CC BY-SA. TOS are basically only important if you want to stay on the service. Also we'd also have to ignore the licence compatibility.

Answer 2

It's a decent first attempt but the way you have structured the code leaves a lot to be desired. As @J_H has covered the nitty gitty pretty well I'm just going to go over the broad conceptual issues.

A big problem here is the destribution of responsibilites.

Everything is just tossed in the cryptically named Christina class (why is it called Christina?) which does everything from parsing command line arguments to handling connections and actually manipulating files. Its a textbook god class.

Split this up into classes that have a single defined responisibity.

If you're writing code to be reused in Ruby its done though gems. Gems have a set of conventions such as the Gemspec that contains information about the gem as well as list of dependencies and the binaries provided by a gem.

Gems typically have a main module as their namespace and the rest of the code in logically nested under that module.

You might want to start working towards a structure that is more like:

# lib/christina.rb
require 'ostruct'
require 'optparse'
require 'net/sftp'
require 'zeitwerk' # this gem must be installed since its not in the stdlib (yet)
# This will automatically require all the files in the project for 
# you as long as you follow its conventions
# @see https://github.com/fxn/zeitwerk
loader = Zeitwerk::Loader.for_gem
loader.setup # ready!
# Put the main description of the module here so that documentation tools can
# find it.
module Christina
 VERSION = '1.2.3' # constants should be allcaps unless they are classes or modules
end

# lib/christina/options_parser.rb
module Christina
 # Parses command line options - does't actually execute them
 class OptionsParser
 # What are the arguments?
 # What does this return?
 def self.parse(arguments)
 # ...
 end
 end
end 

# lib/christina/connection.rb
module Christina
 class Connection
 def initialize(host:, password:, **options)
 @host = host
 # ...
 end
 
 # runs the block within the connection 
 def with_connection(&block)
 Net::SFTP.start(connection_options) do |sftp|
 yield
 end
 end
 private 
 def connection_options
 { host: @host, password: @password } # ...
 end
 end
end

# lib/christina/command.rb
module Christina
 # this are the classes that actually perform the operations on the connection
 class Command
 # ...
 end
end

# lib/christina/command.rb
module Christina
 class Download < Command
 # ...
 end
end

While it might initalially feel easy and comforting to just bang your code out in a single file it doesn't result in code thats easy to maintain, test, reason about, or for others to understand.

You need to apply the same logic to your methods. A method should ideally just do a single job - its often hard enough to even get that right.

Document both the classes and your methods with either YARD or RDOC. Remember that you're writing code for future you. And future you will be very thankful.

Some purely stylistic stuff:

• Adding your license to the code files themselves isn't very common in the Ruby community. Instead its done in a separate file in the root directory. One reason is that you would have to modify the entire project if you ever need to change the licence.
• Signing your work with constant is also somewhat odd. We share code though gems and your name / the names of the contributors is in the gemspec.

Some recommendations:

• Start using Rubocop. It will hurt your feelings - but it will also make you write better Ruby.
• Write units tests and think about how to structure your code for testability.