Added V1 support to Chef::User.update and removed admin field.

5 files changed, 189 insertions, 101 deletions

diff --git a/lib/chef/knife/user_create.rb b/lib/chef/knife/user_create.rb
index 992bbbe3c9..6b06153bf8 100644
--- a/lib/chef/knife/user_create.rb
+++ b/lib/chef/knife/user_create.rb
@@ -35,12 +35,6 @@ class Chef
         :long  => "--file FILE",
         :description => "Write the private key to a file, if returned by the server. A private key will be returned when both --user-key and --no-key are NOT passed. In that case, the server will generate a default key for you and return the private key it creates."
 
-      option :admin,
-        :short => "-a",
-        :long  => "--admin",
-        :description => "Create the user as an admin (only relevant for Open Source Chef Server 11).",
-        :boolean => true
-
       option :user_key,
         :long => "--user-key FILENAME",
         :description => "Public key for newly created user. Path to a public key you provide instead of having the server generate one. If --user-key is not passed, the server will create a 'default' key for you, unless you passed --no-key. Note that --user-key cannot be passed with --no-key."
@@ -84,8 +78,6 @@ class Chef
           exit 1
         end
 
-        user.admin(config[:admin])
-
         unless config[:no_key]
           user.create_key(true)
         end
diff --git a/lib/chef/mixin/api_version_request_handling.rb b/lib/chef/mixin/api_version_request_handling.rb
index ea68afc6af..a818ba2a14 100644
--- a/lib/chef/mixin/api_version_request_handling.rb
+++ b/lib/chef/mixin/api_version_request_handling.rb
@@ -21,17 +21,14 @@ class Chef
     # takes in an http exception, and a min and max supported API version and
     # handles all the versioning cases
     #
-    # it will raise an exception if there was a non-versioning related error
+    # 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 not raise, and you
+    # 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
-      return false unless exception.response.code == "406" || !exception.response["x-ops-server-api-version"]
-
-      exception.response["x-ops-server-api-version"]
+      # only rescue 406 Unacceptable with proper header
+      return false if exception.response.code != "406" || exception.response["x-ops-server-api-version"].nil?
 
       # if the version header doesn't exist, just assume API v0
       if exception.response["x-ops-server-api-version"]
@@ -42,7 +39,7 @@ class Chef
         # 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, it will error out properly, just raise it
+          # if it had x-ops-server-api-version header, return false
           return false
         end
       end
diff --git a/lib/chef/user.rb b/lib/chef/user.rb
index 4e0d12e3b5..651e3684c9 100644
--- a/lib/chef/user.rb
+++ b/lib/chef/user.rb
@@ -47,7 +47,6 @@ class Chef
       @private_key = nil
       @create_key = nil
       @password = nil
-      @admin = false
     end
 
     def chef_rest_v0
@@ -101,11 +100,6 @@ class Chef
                     arg, :kind_of => String)
     end
 
-    def admin(arg=nil)
-      set_or_return(:admin,
-                    arg, :kind_of => [TrueClass, FalseClass])
-    end
-
     def create_key(arg=nil)
       set_or_return(:create_key, arg,
                     :kind_of => [TrueClass, FalseClass])
@@ -128,8 +122,7 @@ class Chef
 
     def to_hash
       result = {
-        "username" => @username,
-        "admin" => @admin
+        "username" => @username
       }
       result["display_name"] = @display_name if @display_name
       result["first_name"] = @first_name if @first_name
@@ -159,8 +152,7 @@ class Chef
           :first_name => @first_name,
           :last_name => @last_name,
           :email => @email,
-          :password => @password,
-          :admin => @admin
+          :password => @password
         }
         payload[:public_key] = @public_key if @public_key
         payload[:create_key] = @create_key if @create_key
@@ -184,8 +176,7 @@ class Chef
           :first_name => @first_name,
           :last_name => @last_name,
           :email => @email,
-          :password => @password,
-          :admin => @admin
+          :password => @password
         }
         payload[:middle_name] = @middle_name if @middle_name
         payload[:public_key] = @public_key if @public_key
@@ -196,10 +187,37 @@ class Chef
     end
 
     def update(new_key=false)
-      payload = {:username => username, :admin => admin}
-      payload[:private_key] = new_key if new_key
-      payload[:password] = password if password
-      updated_user = Chef::REST.new(Chef::Config[:chef_server_url]).put("users/#{username}", payload)
+      begin
+        payload = {:username => username}
+        payload[:display_name] = display_name if display_name
+        payload[:first_name] = first_name if first_name
+        payload[:middle_name] = middle_name if middle_name
+        payload[:last_name] = last_name if last_name
+        payload[:email] = email if email
+        payload[:password] = password if password
+
+        # API V1 will fail if these key fields are defined, and try V0 below if relevant 400 is returned
+        payload[:public_key] = public_key if public_key
+        payload[:private_key] = new_key if new_key
+
+        updated_user = chef_root_rest_v1.put("users/#{username}", payload)
+      rescue Net::HTTPServerException => e
+        if e.response.code == "400"
+          # if a 400 is returned but the error message matches the error related to private / public key fields, try V0
+          # else, raise the 400
+          puts "halp"*100
+          puts e.response.body
+          puts e.response.body.class
+          error = Chef::JSONCompat.from_json(e.response.body)["error"].first
+          error_match = /Since Server API v1, all keys must be updated via the keys endpoint/.match(error)
+          if error_match.nil?
+            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])
+        end
+        updated_user = chef_root_rest_v0.put("users/#{username}", payload)
+      end
       Chef::User.from_hash(self.to_hash.merge(updated_user))
     end
 
@@ -217,7 +235,7 @@ class Chef
 
     def reregister
       r = Chef::REST.new(Chef::Config[:chef_server_url])
-      reregistered_self = r.put("users/#{username}", { :username => username, :admin => admin, :private_key => true })
+      reregistered_self = r.put("users/#{username}", { :username => username, :private_key => true })
       private_key(reregistered_self["private_key"])
       self
     end
@@ -227,8 +245,10 @@ class Chef
     end
 
     def inspect
-      "Chef::User username:'#{username}' admin:'#{admin.inspect}'" +
-      "public_key:'#{public_key}' private_key:#{private_key}"
+      inspect_str = "Chef::User username:'#{username}'"
+      inspect_str = "#{inspect_str} public_key:#{public_key}" if public_key
+      inspect_str = "#{inspect_str} private_key:#{private_key}" if private_key
+      inspect_str
     end
 
     # Class Methods
@@ -236,7 +256,6 @@ class Chef
     def self.from_hash(user_hash)
       user = Chef::User.new
       user.username user_hash['username']
-      user.admin user_hash['admin']
       user.display_name user_hash['display_name'] if user_hash.key?('display_name')
       user.first_name user_hash['first_name'] if user_hash.key?('first_name')
       user.middle_name user_hash['middle_name'] if user_hash.key?('middle_name')
diff --git a/spec/unit/knife/user_create_spec.rb b/spec/unit/knife/user_create_spec.rb
index dc2293396c..629a082d00 100644
--- a/spec/unit/knife/user_create_spec.rb
+++ b/spec/unit/knife/user_create_spec.rb
@@ -149,24 +149,6 @@ describe Chef::Knife::UserCreate do
       end
     end
 
-    context "when --admin is passed" do
-      before do
-        knife.config[:admin] = true
-      end
-
-      it "sets the admin field on the user to true" do
-        knife.run
-        expect(knife.user.admin).to be_truthy
-      end
-    end
-
-    context "when --admin is not passed" do
-      it "does not set the admin field to true" do
-        knife.run
-        expect(knife.user.admin).to be_falsey
-      end
-    end
-
     context "when --no-key is passed" do
       before do
         knife.config[:no_key] = true
diff --git a/spec/unit/user_spec.rb b/spec/unit/user_spec.rb
index 7380484409..67ad44a043 100644
--- a/spec/unit/user_spec.rb
+++ b/spec/unit/user_spec.rb
@@ -42,11 +42,11 @@ describe Chef::User do
   end
 
   shared_examples_for "boolean fields with no constraints" do
-    it "should let you set the admin bit" do
+    it "should let you set the field" do
       expect(@user.send(method, true)).to eq(true)
     end
 
-    it "should return the current admin value" do
+    it "should return the current field value" do
       @user.send(method, true)
       expect(@user.send(method)).to eq(true)
     end
@@ -96,16 +96,6 @@ describe Chef::User do
   end
 
   describe "boolean fields" do
-    describe "admin" do
-      it_should_behave_like "boolean fields with no constraints" do
-        let(:method) { :admin }
-      end
-
-      it "should default to false" do
-        expect(@user.admin).to eq(false)
-      end
-    end
-
     describe "create_key" do
       it_should_behave_like "boolean fields with no constraints" do
         let(:method) { :create_key }
@@ -177,10 +167,6 @@ describe Chef::User do
       expect(@json).to include(%q{"username":"black"})
     end
 
-    it "includes the 'admin' flag" do
-      expect(@json).to include(%q{"admin":false})
-    end
-
     it "includes the display name when present" do
       @user.display_name("get_displayed")
       expect(@user.to_json).to include(%{"display_name":"get_displayed"})
@@ -262,7 +248,6 @@ describe Chef::User do
     before(:each) do
       user = {
         "username" => "mr_spinks",
-        "admin" => true,
         "display_name" => "displayed",
         "first_name" => "char",
         "middle_name" => "man",
@@ -284,10 +269,6 @@ describe Chef::User do
       expect(@user.username).to eq("mr_spinks")
     end
 
-    it "preserves the admin status" do
-      expect(@user.admin).to be_truthy
-    end
-
     it "preserves the display name if present" do
       expect(@user.display_name).to eq("displayed")
     end
@@ -326,12 +307,149 @@ describe Chef::User do
   end
 
   describe "Versioned API Interactions" do
+    let(:response_406) { OpenStruct.new(:code => '406') }
+    let(:exception_406) { Net::HTTPServerException.new("406 Not Acceptable", response_406) }
+
     before (:each) do
       @user = Chef::User.new
       allow(@user).to receive(:chef_root_rest_v0).and_return(double('chef rest root v0 object'))
       allow(@user).to receive(:chef_root_rest_v1).and_return(double('chef rest root v1 object'))
     end
 
+    shared_examples_for "version handling" do
+      before do
+        allow(@user.chef_root_rest_v1).to receive(http_verb).and_raise(exception_406)
+      end
+
+      context "when the server does not support the min or max server API version that Chef::User supports" do
+        before do
+          allow(@user).to receive(:handle_version_http_exception).and_return(false)
+        end
+
+        it "raises the original exception" do
+          expect{ @user.send(method) }.to raise_error(exception_406)
+        end
+      end # when the server does not support the min or max server API version that Chef::User supports
+    end # version handling
+
+    describe "update" do
+      before do
+        # populate all fields that are valid between V0 and V1
+        @user.username "some_username"
+        @user.display_name "some_display_name"
+        @user.first_name "some_first_name"
+        @user.middle_name "some_middle_name"
+        @user.last_name "some_last_name"
+        @user.email "some_email"
+        @user.password "some_password"
+      end
+
+      let(:payload) {
+        {
+          :username => "some_username",
+          :display_name => "some_display_name",
+          :first_name => "some_first_name",
+          :middle_name => "some_middle_name",
+          :last_name => "some_last_name",
+          :email => "some_email",
+          :password => "some_password"
+        }
+      }
+
+      context "when server API V1 is valid on the Chef Server receiving the request" do
+        context "when the user submits valid data" do
+          it "properly updates the user" do
+            expect(@user.chef_root_rest_v1).to receive(:put).with("users/some_username", payload).and_return({})
+            @user.update
+          end
+        end
+      end
+
+      context "when server API V1 is not valid on the Chef Server receiving the request" do
+        let(:payload) {
+          {
+            :username => "some_username",
+            :display_name => "some_display_name",
+            :first_name => "some_first_name",
+            :middle_name => "some_middle_name",
+            :last_name => "some_last_name",
+            :email => "some_email",
+            :password => "some_password",
+            :public_key => "some_public_key"
+          }
+        }
+
+        before do
+          @user.public_key "some_public_key"
+          allow(@user.chef_root_rest_v1).to receive(:put)
+        end
+
+        context "when the server returns a 400" do
+          let(:response_400) { OpenStruct.new(:code => '400') }
+          let(:exception_400) { Net::HTTPServerException.new("400 Bad Request", response_400) }
+
+          context "when the 400 was due to public / private key fields no longer being supported" do
+            let(:response_body_400) { '{"error":["Since Server API v1, all keys must be updated via the keys endpoint. "]}' }
+
+            before do
+              allow(response_400).to receive(:body).and_return(response_body_400)
+              allow(@user.chef_root_rest_v1).to receive(:put).and_raise(exception_400)
+            end
+
+            it "proceeds with the V0 PUT since it can handle public / private key fields" do
+              expect(@user.chef_root_rest_v0).to receive(:put).with("users/some_username", payload).and_return({})
+              @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)
+              allow(@user.chef_root_rest_v0).to receive(:put).and_return({})
+              @user.update
+            end
+          end # when the 400 was due to public / private key fields
+
+          context "when the 400 was NOT due to public / private key fields no longer being supported" do
+            let(:response_body_400) { '{"error":["Some other error. "]}' }
+
+            before do
+              allow(response_400).to receive(:body).and_return(response_body_400)
+              allow(@user.chef_root_rest_v1).to receive(:put).and_raise(exception_400)
+            end
+
+            it "will not proceed with the V0 PUT since the original bad request was not key related" do
+              expect(@user.chef_root_rest_v0).to_not receive(:put).with("users/some_username", payload)
+              expect { @user.update }.to raise_error(exception_400)
+            end
+
+            it "raises the original error" do
+              expect { @user.update }.to raise_error(exception_400)
+            end
+
+          end
+        end # when the server returns a 400
+
+        context "when the server returns a 406" do
+          it_should_behave_like "version handling" do
+            let(:method)    { (:update) }
+            let(:http_verb) { (:put) }
+          end
+
+          context "when the server supports API V0" do
+            before do
+              allow(@user).to receive(:handle_version_http_exception).and_return(true)
+              allow(@user.chef_root_rest_v1).to receive(:put).and_raise(exception_406)
+            end
+
+            it "properly updates the user" do
+              expect(@user.chef_root_rest_v0).to receive(:put).with("users/some_username", payload).and_return({})
+              @user.update
+            end
+          end # when the server supports API V0
+        end # when the server returns a 406
+
+      end # when server API V1 is not valid on the Chef Server receiving the request
+    end # update
+
     describe "create" do
       let(:payload) {
         {
@@ -340,8 +458,7 @@ describe Chef::User do
           :first_name => "some_first_name",
           :last_name => "some_last_name",
           :email => "some_email",
-          :password => "some_password",
-          :admin => true
+          :password => "some_password"
         }
       }
       before do
@@ -351,7 +468,6 @@ describe Chef::User do
         @user.last_name "some_last_name"
         @user.email "some_email"
         @user.password "some_password"
-        @user.admin true
       end
 
       shared_examples_for "create valid user" do
@@ -430,23 +546,11 @@ describe Chef::User do
       end # when server API V1 is valid on the Chef Server receiving the request
 
       context "when server API V1 is not valid on the Chef Server receiving the request" do
-        let(:response_406) { OpenStruct.new(:code => '406') }
-        let(:exception_406) { Net::HTTPServerException.new("406 Not Acceptable", response_406) }
-
-        before do
-          allow(@user.chef_root_rest_v1).to receive(:post).and_raise(exception_406)
+        it_should_behave_like "version handling" do
+          let(:method)    { (:create) }
+          let(:http_verb) { (:post) }
         end
 
-        context "when the server does not support the min or max server API version that Chef::User supports" do
-          before do
-            allow(@user).to receive(:handle_version_http_exception).and_return(false)
-          end
-
-          it "raises the original exception" do
-            expect{ @user.create }.to raise_error(exception_406)
-          end
-        end # when the server does not support the min or max server API version that Chef::User supports
-
         context "when the server supports API V0" do
           before do
             allow(@user).to receive(:handle_version_http_exception).and_return(true)
@@ -460,6 +564,7 @@ describe Chef::User do
         end # when the server supports API V0
       end # when server API V1 is not valid on the Chef Server receiving the request
     end # create
+
   end # Versioned API Interactions
 
   describe "API Interactions" do
@@ -501,23 +606,16 @@ describe Chef::User do
         expect(Chef::User.list(true)).to eq(@osc_inflated_response)
       end
     end
+
     describe "read" do
       it "loads a named user from the API" do
         expect(@http_client).to receive(:get).with("users/foobar").and_return({"username" => "foobar", "admin" => true, "public_key" => "pubkey"})
         user = Chef::User.load("foobar")
         expect(user.username).to eq("foobar")
-        expect(user.admin).to eq(true)
         expect(user.public_key).to eq("pubkey")
       end
     end
 
-    describe "update" do
-      it "updates an existing user on via the API" do
-        expect(@http_client).to receive(:put).with("users/foobar", {:username => "foobar", :admin => false}).and_return({})
-        @user.update
-      end
-    end
-
     describe "destroy" do
       it "deletes the specified user via the API" do
         expect(@http_client).to receive(:delete).with("users/foobar")