Merge branch 'us2600_followup' into stage_us2600_followup

features/lib/rhc_helper/sshkey.rb

@@ -25,9 +25,5 @@ def self.add(*args)
     def self.remove(*args)
       rhc_sshkey_remove args
     end
-
-    def self.update(*args)
-      rhc_sshkey_update args
-    end
   end
 end

features/sshkey.feature

@@ -27,6 +27,12 @@ As an OpenShift user, I want to manage SSH keys with 'rhc sshkey' commands.
     Then the key "key1" should exist
     And the command exits with status code 0
 
+  @sshkey_add
+  Scenario: invalid SSH key is added
+    Given the SSH key "key1" does not exist
+    When a new SSH key "features/support/key3.pub" is added as "key3"
+    Then the command exits with status code 128
+
   @sshkey_add
   Scenario: SSH key with the same name already exists
     Given the SSH key "key1" already exists
@@ -40,12 +46,6 @@ As an OpenShift user, I want to manage SSH keys with 'rhc sshkey' commands.
     When 'rhc sshkey add "key1" "features/support/key1.pub"' is run
     Then the command exits with status code 128
 
-  @sshkey_update
-  Scenario: 'update' subcommand is invoked
-    When 'rhc sshkey update' is run
-    Then the command exits with status code 1
-    And the output includes deprecation warning
-
   @sshkey_remove
   Scenario: SSH key is deleted successfully
     When 'rhc sshkey remove "key1"' is run

features/step_definitions/sshkey_steps.rb

@@ -35,10 +35,6 @@
   @sshkey_output.should match /Name: #{key}/
 end
 
-Then /^the output includes deprecation warning$/ do
-  @sshkey_output.should match /deprecated/
-end
-
 Then /^the key "(.*?)" should exist$/ do |key|
   Sshkey.show "#{key}"
   Sshkey.sshkey_output.should =~ /Name: #{key}/

features/support/key3.pub

@@ -0,0 +1 @@
+ssh-rsa AAAAQDuwb0v02Eu8TDI1TzGV4dFK7qUgwegENLbR+amPS1tP38Ulv3p4CeqmFY+hAGacjFnZWK2LEF/iJBjIbFg1MeUmQHOaGhexNTtomkWzuDt5rINeGhmcCbIu822yCMBjF4usdZVMgwJiyc5HcawmVNUAP9mgjyJ0TSjU51CVU8jh+1EeGv5VINixzOj1wYQuqJ2n5Xu72MRqOOMchOagZHEFfBgvZHJhg9T5fmUqwjZpY93TZNOVCESTkMUuS9UCGPtO1jZmRKNggMymcx7P60dnzH80Y9ZeWaC7GwyizG9wuNJavHfiSzIx+qnI07FejrEbwln/WriD2VDMQTvmyRz asari@strokkur.local

lib/rhc/command_runner.rb

@@ -95,10 +95,11 @@ def create_default_commands
   class CommandHelpBindings
     def initialize(command, instance_commands, global_options)
       @command = command
-      @actions = instance_commands.collect do |ic|
-        m = /^#{command.name} ([^ ]+)/.match(ic[0])
+      @actions = instance_commands.collect do |command_name, command_class|
+        next if command_class.summary.nil?
+        m = /^#{command.name} ([^ ]+)/.match(command_name)
         # if we have a match and it is not an alias then we can use it
-        m and ic[0] == ic[1].name ? {:name => m[1], :summary => ic[1].summary || ""} : nil
+        m and command_name == command_class.name ? {:name => m[1], :summary => command_class.summary || ""} : nil
       end
       @actions.compact!
       @global_options = global_options

lib/rhc/commands/sshkey.rb

@@ -53,20 +53,20 @@ def add(name, key)
         raise ::RHC::KeyFileAccessDeniedException.new("Access denied to '#{key}'.")
       end
       type, content, comment = file.gets.chomp.split
+
+      # validate the user input before sending it to the server
+      begin
+        Net::SSH::KeyFactory.load_data_public_key "#{type} #{content}"
+      rescue NotImplementedError, Net::SSH::Exception => e
+        raise ::RHC::KeyDataInvalidException.new("File '#{key}' contains invalid data")
+      end
+
       rest_client.add_key(name, content, type)
       results { say "SSH key #{key} has been added as '#{name}'" }
 
       0
     end
 
-    summary 'Deprecated. "remove" and "add" instead.'
-    syntax ''
-    def update
-      warn 'Update command is deprecated. Please delete and re-add the key with the same name.'
-
-      1
-    end
-
     summary 'Remove SSH key from the user account'
     syntax '<name>'
     alias_action :delete

lib/rhc/exceptions.rb

@@ -39,6 +39,12 @@ def initialize(message = "Insufficient acces to SSH Key file")
     end
   end
 
+  class KeyDataInvalidException < Exception
+    def initialize(message = "SSH Key file contains invalid data")
+      super message, 128
+    end
+  end
+
   class ScaledApplicationsNotSupportedException < Exception
     def initialize(message="Scaled applications not supported")
       super message, 128

man/rhc-sshkey.1

@@ -18,8 +18,6 @@ Display all the SSH keys for the user account
 Display the SSH key with the given name
 .IP add
 Add SSH key to the user account 
-.IP update
-Deprecated. 'remove' and 'add' instead.
 .IP remove
 Remove SSH key from the user account
 
@@ -50,12 +48,6 @@ Add the SSH public key specified by
 as
 .B "<name>"
 The command fails if a key with the same name or a key with identical content already exists on this account.
-.SS "update"
-This command is deprecated. Please
-.B remove
-and
-.B add
-the key instead.
 .SS "remove <name>"
 Removes the SSH key from the account.
 

spec/rhc/commands/sshkey_spec.rb

@@ -59,6 +59,26 @@
       end
     end
 
+    context "when adding an invalid key" do
+      let(:arguments) { %w[sshkey add --noprompt --config test.conf -l test@test.foo -p password foobar id_rsa.pub] }
+
+      before :each do
+        @rc = MockRestClient.new
+      end
+
+      it "fails to add the key" do
+        FakeFS do
+          keys = @rc.sshkeys
+          num_keys = keys.length
+          File.open('id_rsa.pub', 'w') do |f|
+            f << 'ssh-rsa AADAQABAAABAQCnCOqK7/mmvZ9AtCAerxjAasJ1rSpfuWT4vNm1+O/Fh0Di3chTWjY9a0M2hEnqkqnVG589L9CqCUeT0kdc3Vgw3JEcacSUr1z7tLr9kO+p/D5lSdQYzDGGRFOZ0H6lc/y8iNxWV1VO/sJvKx6cr5zvKIn8Q6GvhVNOxlai0IOb9FJxLGK95GLpZ+elzh8Tc9giy7KfwheAwhV2JoF9uRltE5JP/CNs7w/E29i1Z+jlueuu8RVotLmhSVNJm91Ey7OCtoI1iBE0Wv/SucFe32Qi08RWTM/MaGGz93KQNOVRGjNkosJjPmP1qU6WGBfliDkJAZXB0b6sEcnx1fbVikwZ'
+          end
+          expect { run }.should exit_with_code(128)
+          @rc.sshkeys.length.should == num_keys
+        end
+      end
+    end
+
     context "when adding a nonexistent key" do
       let(:arguments) { %w[sshkey add --noprompt --config test.conf -l test@test.foo -p password foobar id_rsa.pub] }
 
@@ -118,20 +138,4 @@
       end
     end
   end
-
-  describe "update" do
-    context "when invoked" do
-      let (:arguments) { %w[sshkey update --noprompt] }
-
-      before :each do
-        @rc = MockRestClient.new
-      end
-
-      # RHC::Commands::Sshkey#update throws RuntimeError, but it is swallowed
-      # up by the wrapper, so we only see status code 1.
-      it "exits with status code 1" do
-        expect {run}.should exit_with_code(1)
-      end
-    end
-  end
 end