Better API version error handling helper code.

Renamed Chef::Mixin::ApiVersionRequestHandling.handle_version_http_exception -> server_client_api_version_intersection and made it do much more useful / sane things. See comments for details.

13 files changed, 191 insertions, 62 deletions

diff --git a/lib/chef/api_client.rb b/lib/chef/api_client.rb
index 3b777b3574..ad31fb7d7b 100644
--- a/lib/chef/api_client.rb
+++ b/lib/chef/api_client.rb
@@ -32,7 +32,7 @@ class Chef
 
     include Chef::Mixin::FromFile
     include Chef::Mixin::ParamsValidate
-    include Chef::ApiVersionRequestHandling
+    include Chef::Mixin::ApiVersionRequestHandling
 
     SUPPORTED_API_VERSIONS = [0,1]
 
@@ -272,7 +272,8 @@ class Chef
         new_client = chef_rest_v1.put("clients/#{name}", payload)
       rescue Net::HTTPServerException => e
         # rescue API V0 if 406 and the server supports V0
-        raise e unless handle_version_http_exception(e, SUPPORTED_API_VERSIONS[0], SUPPORTED_API_VERSIONS[-1])
+        supported_versions = server_client_api_version_intersection(e, SUPPORTED_API_VERSIONS)
+        raise e unless supported_versions && supported_versions.include?(0)
         new_client = chef_rest_v0.put("clients/#{name}", payload)
       end
 
@@ -308,7 +309,8 @@ class Chef
 
       rescue Net::HTTPServerException => e
         # rescue API V0 if 406 and the server supports V0
-        raise e unless handle_version_http_exception(e, SUPPORTED_API_VERSIONS[0], SUPPORTED_API_VERSIONS[-1])
+        supported_versions = server_client_api_version_intersection(e, SUPPORTED_API_VERSIONS)
+        raise e unless supported_versions && supported_versions.include?(0)
 
         # under API V0, a key pair will always be created unless public_key is
         # passed on initial POST
diff --git a/lib/chef/knife/user_create.rb b/lib/chef/knife/user_create.rb
index 5332e863ae..e73f6be8b6 100644
--- a/lib/chef/knife/user_create.rb
+++ b/lib/chef/knife/user_create.rb
@@ -78,11 +78,13 @@ knife user create for Open Source 11 Server is being deprecated.
 Open Source 11 Server user commands now live under the knife osc_user namespace.
 For backwards compatibility, we will forward this request to knife osc_user create.
 If you are using an Open Source 11 Server, please use that command to avoid this warning.
-If you are not using an Open Source Chef 11 Server install, please read knife user create --help for proper usage.
 EOF
       end
 
       def run_osc_11_user_create
+        # run osc_user_create with our input
+        ARGV.delete("user")
+        ARGV.unshift("osc_user")
         Chef::Knife.run(ARGV, Chef::Application::Knife.options)
       end
 
@@ -93,12 +95,7 @@ EOF
         # If only 1 arg is passed, assume OSC 11 case.
         if @name_args.length == 1
           ui.warn(osc_11_warning)
-
-          # run osc_user_create with our input
-          ARGV.delete("user")
-          ARGV.unshift("osc_user")
           run_osc_11_user_create
-
         else # EC / CS 12 user create
 
           test_mandatory_field(@name_args[0], "username")
diff --git a/lib/chef/knife/user_delete.rb b/lib/chef/knife/user_delete.rb
index 3a46e33f84..803be6b90c 100644
--- a/lib/chef/knife/user_delete.rb
+++ b/lib/chef/knife/user_delete.rb
@@ -41,6 +41,9 @@ EOF
       end
 
       def run_osc_11_user_delete
+        # run osc_user_delete with our input
+        ARGV.delete("user")
+        ARGV.unshift("osc_user")
         Chef::Knife.run(ARGV, Chef::Application::Knife.options)
       end
 
@@ -82,12 +85,7 @@ EOF
         # OSC 11 case
         if object.username.nil?
           ui.warn(osc_11_warning)
-
-          # run osc_user_delete with our input
-          ARGV.delete("user")
-          ARGV.unshift("osc_user")
           run_osc_11_user_delete
-
         else # proceed with EC / CS delete
           delete_object(@user_name)
         end
diff --git a/lib/chef/knife/user_edit.rb b/lib/chef/knife/user_edit.rb
index 5bed51a1cc..dd2fc02743 100644
--- a/lib/chef/knife/user_edit.rb
+++ b/lib/chef/knife/user_edit.rb
@@ -41,6 +41,9 @@ EOF
       end
 
       def run_osc_11_user_edit
+        # run osc_user_create with our input
+        ARGV.delete("user")
+        ARGV.unshift("osc_user")
         Chef::Knife.run(ARGV, Chef::Application::Knife.options)
       end
 
@@ -62,12 +65,7 @@ EOF
         # forward to deprecated command
         if original_user["username"].nil?
           ui.warn(osc_11_warning)
-
-          # run osc_user_create with our input
-          ARGV.delete("user")
-          ARGV.unshift("osc_user")
           run_osc_11_user_edit
-
         else # EC / CS 12 user create
           edited_user = edit_data(original_user)
           if original_user != edited_user
diff --git a/lib/chef/knife/user_reregister.rb b/lib/chef/knife/user_reregister.rb
index c2b39761cc..eab2245025 100644
--- a/lib/chef/knife/user_reregister.rb
+++ b/lib/chef/knife/user_reregister.rb
@@ -41,6 +41,9 @@ EOF
       end
 
       def run_osc_11_user_reregister
+        # run osc_user_edit with our input
+        ARGV.delete("user")
+        ARGV.unshift("osc_user")
         Chef::Knife.run(ARGV, Chef::Application::Knife.options)
       end
 
@@ -67,10 +70,6 @@ EOF
         # forward to deprecated command
         if user.username.nil?
           ui.warn(osc_11_warning)
-
-          # run osc_user_edit with our input
-          ARGV.delete("user")
-          ARGV.unshift("osc_user")
           run_osc_11_user_reregister
         else # EC / CS 12 case
           user.reregister
diff --git a/lib/chef/knife/user_show.rb b/lib/chef/knife/user_show.rb
index d0e9f64f53..f5e81e9972 100644
--- a/lib/chef/knife/user_show.rb
+++ b/lib/chef/knife/user_show.rb
@@ -43,6 +43,9 @@ EOF
       end
 
       def run_osc_11_user_show
+        # run osc_user_edit with our input
+        ARGV.delete("user")
+        ARGV.unshift("osc_user")
         Chef::Knife.run(ARGV, Chef::Application::Knife.options)
       end
 
@@ -64,10 +67,6 @@ EOF
         # forward to deprecated command
         if user.username.nil?
           ui.warn(osc_11_warning)
-
-          # run osc_user_edit with our input
-          ARGV.delete("user")
-          ARGV.unshift("osc_user")
           run_osc_11_user_show
         else
           output(format_for_display(user))
diff --git a/lib/chef/mixin/api_version_request_handling.rb b/lib/chef/mixin/api_version_request_handling.rb
index 851bcb4968..20ab3bf452 100644
--- a/lib/chef/mixin/api_version_request_handling.rb
+++ b/lib/chef/mixin/api_version_request_handling.rb
@@ -17,43 +17,50 @@
 #
 
 class Chef
-  module ApiVersionRequestHandling
-    # takes in an http exception, and a min and max supported API version and
-    # handles all the versioning cases
-    #
-    # it will return false if there was a non-versioning related error
-    # or the server and the client are not compatible
-    #
-    # if the server does not support versioning, then it will return true, and you
-    # can assume API v0 is safe to send
-    def handle_version_http_exception(exception, min_client_supported_version, max_client_supported_version)
-      # only rescue 406 Unacceptable with proper header
-      return false if exception.response.code != "406" || exception.response["x-ops-server-api-version"].nil?
+  module Mixin
+    module ApiVersionRequestHandling
+      # Input:
+      # exeception:
+      #   Net::HTTPServerException that may or may not contain the x-ops-server-api-version header
+      # supported_client_versions:
+      #  An array of Integers that represent the API versions the client supports.
+      #
+      # Output:
+      # nil:
+      #  If the execption was not a 406 or the server does not support versioning
+      # Array of length zero:
+      #  If there was no intersection between supported client versions and supported server versions
+      # Arrary of Integers:
+      #  If there was an intersection of supported versions, the array returns will contain that intersection
+      def server_client_api_version_intersection(exception, supported_client_versions)
+        # return empty array unless 406 Unacceptable with proper header
+        return nil if exception.response.code != "406" || exception.response["x-ops-server-api-version"].nil?
+
+        # intersection of versions the server and client support, will be of length zero if no intersection
+        server_supported_client_versions = Array.new
 
-      # if the version header doesn't exist, just assume API v0
-      if exception.response["x-ops-server-api-version"]
         header = Chef::JSONCompat.from_json(exception.response["x-ops-server-api-version"])
         min_server_version = Integer(header["min_version"])
         max_server_version = Integer(header["max_version"])
 
-        # if the min API version the server supports is greater than the min version the client supports
-        # and the max API version the server supports is less than the max version the client supports
-        if min_server_version > min_client_supported_version || max_server_version < max_client_supported_version
-          # if it had x-ops-server-api-version header, return false
-          return false
+        supported_client_versions.each do |version|
+          if version >= min_server_version && version <= max_server_version
+            server_supported_client_versions.push(version)
+          end
         end
+        server_supported_client_versions
       end
-      true
-    end
 
-    def reregister_only_v0_supported_error_msg(max_version, min_version)
-<<-EOH
+      def reregister_only_v0_supported_error_msg(max_version, min_version)
+        <<-EOH
 The reregister command only supports server API version 0.
 The server that received the request supports a min version of #{min_version} and a max version of #{max_version}.
 User keys are now managed via the key rotation commmands.
-Please refer to the documentation on how to manage your keys via the key rotation commands.
+Please refer to the documentation on how to manage your keys via the key rotation commands:
+https://docs.chef.io/server_security.html#key-rotation
 EOH
-    end
+      end
 
+    end
   end
 end
diff --git a/lib/chef/user.rb b/lib/chef/user.rb
index 1b5f454099..717deb63c3 100644
--- a/lib/chef/user.rb
+++ b/lib/chef/user.rb
@@ -36,7 +36,7 @@ class Chef
 
     include Chef::Mixin::FromFile
     include Chef::Mixin::ParamsValidate
-    include Chef::ApiVersionRequestHandling
+    include Chef::Mixin::ApiVersionRequestHandling
 
     SUPPORTED_API_VERSIONS = [0,1]
 
@@ -169,7 +169,8 @@ class Chef
         end
       rescue Net::HTTPServerException => e
         # rescue API V0 if 406 and the server supports V0
-        raise e unless handle_version_http_exception(e, SUPPORTED_API_VERSIONS[0], SUPPORTED_API_VERSIONS[-1])
+        supported_versions = server_client_api_version_intersection(e, SUPPORTED_API_VERSIONS)
+        raise e unless supported_versions && supported_versions.include?(0)
         payload = {
           :username => @username,
           :display_name => @display_name,
@@ -212,7 +213,8 @@ class Chef
             raise e
           end
         else # for other types of errors, test for API versioning errors right away
-          raise e unless handle_version_http_exception(e, SUPPORTED_API_VERSIONS[0], SUPPORTED_API_VERSIONS[-1])
+          supported_versions = server_client_api_version_intersection(e, SUPPORTED_API_VERSIONS)
+          raise e unless supported_versions && supported_versions.include?(0)
         end
         updated_user = chef_root_rest_v0.put("users/#{username}", payload)
       end
diff --git a/spec/support/shared/unit/api_versioning.rb b/spec/support/shared/unit/api_versioning.rb
index 61fdfe7faf..a4f353de60 100644
--- a/spec/support/shared/unit/api_versioning.rb
+++ b/spec/support/shared/unit/api_versioning.rb
@@ -28,7 +28,7 @@ shared_examples_for "version handling" do
 
   context "when the server does not support the min or max server API version that Chef::User supports" do
     before do
-      allow(object).to receive(:handle_version_http_exception).and_return(false)
+      allow(object).to receive(:server_client_api_version_intersection).and_return([])
     end
 
     it "raises the original exception" do
diff --git a/spec/support/shared/unit/user_and_client_shared.rb b/spec/support/shared/unit/user_and_client_shared.rb
index b0f3902922..bc5ffa07c2 100644
--- a/spec/support/shared/unit/user_and_client_shared.rb
+++ b/spec/support/shared/unit/user_and_client_shared.rb
@@ -93,7 +93,7 @@ shared_examples_for "user or client create" do
 
     context "when the server supports API V0" do
       before do
-        allow(object).to receive(:handle_version_http_exception).and_return(true)
+        allow(object).to receive(:server_client_api_version_intersection).and_return([0])
         allow(rest_v1).to receive(:post).and_raise(exception_406)
       end
 
diff --git a/spec/unit/api_client_spec.rb b/spec/unit/api_client_spec.rb
index a95597d40e..ba0eca3284 100644
--- a/spec/unit/api_client_spec.rb
+++ b/spec/unit/api_client_spec.rb
@@ -487,7 +487,7 @@ describe Chef::ApiClient do
 
             before do
               allow(@client.chef_rest_v1).to receive(:put).and_raise(exception_406)
-              allow(@client).to receive(:handle_version_http_exception).and_return(true)
+              allow(@client).to receive(:server_client_api_version_intersection).and_return([0])
             end
 
             it_should_behave_like "client updating" do
diff --git a/spec/unit/mixin/api_version_request_handling_spec.rb b/spec/unit/mixin/api_version_request_handling_spec.rb
new file mode 100644
index 0000000000..cc5340e424
--- /dev/null
+++ b/spec/unit/mixin/api_version_request_handling_spec.rb
@@ -0,0 +1,127 @@
+#
+# Author:: Tyler Cloke (tyler@chef.io)
+# Copyright:: Copyright 2015 Chef Software, Inc.
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+
+describe Chef::Mixin::ApiVersionRequestHandling do
+  let(:dummy_class) { Class.new { include Chef::Mixin::ApiVersionRequestHandling } }
+  let(:object) { dummy_class.new }
+
+  describe ".server_client_api_version_intersection" do
+    let(:default_supported_client_versions) { [0,1,2] }
+
+
+    context "when the response code is not 406" do
+      let(:response) { OpenStruct.new(:code => '405') }
+      let(:exception) { Net::HTTPServerException.new("405 Something Else", response) }
+
+      it "returns nil" do
+        expect(object.server_client_api_version_intersection(exception, default_supported_client_versions)).
+          to be_nil
+      end
+
+    end # when the response code is not 406
+
+    context "when the response code is 406" do
+      let(:response) { OpenStruct.new(:code => '406') }
+      let(:exception) { Net::HTTPServerException.new("406 Not Acceptable", response) }
+
+      context "when x-ops-server-api-version header does not exist" do
+        it "returns nil" do
+          expect(object.server_client_api_version_intersection(exception, default_supported_client_versions)).
+            to be_nil
+        end
+      end # when x-ops-server-api-version header does not exist
+
+      context "when x-ops-server-api-version header exists" do
+        let(:min_server_version) { 2 }
+        let(:max_server_version) { 4 }
+        let(:return_hash) {
+          {
+            "min_version" => min_server_version,
+            "max_version" => max_server_version
+          }
+        }
+
+        before(:each) do
+          allow(response).to receive(:[]).with('x-ops-server-api-version').and_return(Chef::JSONCompat.to_json(return_hash))
+        end
+
+        context "when there is no intersection between client and server versions" do
+          shared_examples_for "no intersection between client and server versions" do
+            it "return an array" do
+              expect(object.server_client_api_version_intersection(exception, supported_client_versions)).
+                to be_a_kind_of(Array)
+            end
+
+            it "returns an empty array" do
+              expect(object.server_client_api_version_intersection(exception, supported_client_versions).length).
+                to eq(0)
+            end
+
+          end
+
+          context "when all the versions are higher than the max" do
+            it_should_behave_like "no intersection between client and server versions" do
+              let(:supported_client_versions) { [5,6,7] }
+            end
+          end
+
+          context "when all the versions are lower than the min" do
+            it_should_behave_like "no intersection between client and server versions" do
+              let(:supported_client_versions) { [0,1] }
+            end
+          end
+
+        end # when there is no intersection between client and server versions
+
+        context "when there is an intersection between client and server versions" do
+          context "when multiple versions intersect" do
+            let(:supported_client_versions) { [1,2,3,4,5] }
+
+            it "includes all of the intersection" do
+              expect(object.server_client_api_version_intersection(exception, supported_client_versions)).
+                to eq([2,3,4])
+            end
+          end # when multiple versions intersect
+
+          context "when only the min client version intersects" do
+            let(:supported_client_versions) { [0,1,2] }
+
+            it "includes the intersection" do
+              expect(object.server_client_api_version_intersection(exception, supported_client_versions)).
+                to eq([2])
+            end
+          end # when only the min client version intersects
+
+          context "when only the max client version intersects" do
+            let(:supported_client_versions) { [4,5,6] }
+
+            it "includes the intersection" do
+              expect(object.server_client_api_version_intersection(exception, supported_client_versions)).
+                to eq([4])
+            end
+          end # when only the max client version intersects
+
+        end # when there is an intersection between client and server versions
+
+      end # when x-ops-server-api-version header exists
+    end # when the response code is 406
+
+  end # .server_client_api_version_intersection
+end # Chef::Mixin::ApiVersionRequestHandling
diff --git a/spec/unit/user_spec.rb b/spec/unit/user_spec.rb
index 15d9591712..57822df7e3 100644
--- a/spec/unit/user_spec.rb
+++ b/spec/unit/user_spec.rb
@@ -390,8 +390,8 @@ describe Chef::User do
               @user.update
             end
 
-            it "does not call handle_version_http_exception, since we know to proceed with V0 in this case" do
-              expect(@user).to_not receive(:handle_version_http_exception)
+            it "does not call server_client_api_version_intersection, since we know to proceed with V0 in this case" do
+              expect(@user).to_not receive(:server_client_api_version_intersection)
               allow(@user.chef_root_rest_v0).to receive(:put).and_return({})
               @user.update
             end
@@ -428,7 +428,7 @@ describe Chef::User do
 
           context "when the server supports API V0" do
             before do
-              allow(@user).to receive(:handle_version_http_exception).and_return(true)
+              allow(@user).to receive(:server_client_api_version_intersection).and_return([0])
               allow(@user.chef_root_rest_v1).to receive(:put).and_raise(exception_406)
             end
 
@@ -491,7 +491,7 @@ describe Chef::User do
 
       context "when handling API V0" do
         before do
-          allow(@user).to receive(:handle_version_http_exception).and_return(true)
+          allow(@user).to receive(:server_client_api_version_intersection).and_return([0])
           allow(@user.chef_root_rest_v1).to receive(:post).and_raise(exception_406)
         end