Validate section values for adding new custom attributes via the API

Currently validation only occurs for providers, but should be consistent throughout.

https://bugzilla.redhat.com/show_bug.cgi?id=1516762

app/controllers/api/providers_controller.rb

@@ -59,16 +59,6 @@ def delete_resource(type, id = nil, _data = nil)
       end
     end
 
-    def custom_attributes_edit_resource(object, type, id, data = nil)
-      formatted_data = format_provider_custom_attributes(data)
-      super(object, type, id, formatted_data)
-    end
-
-    def custom_attributes_add_resource(object, type, id, data = nil)
-      formatted_data = format_provider_custom_attributes(data)
-      super(object, type, id, formatted_data)
-    end
-
     def import_vm_resource(type, id = nil, data = {})
       raise BadRequestError, "Must specify an id for import of VM to a #{type} resource" unless id
 
@@ -99,19 +89,6 @@ def options
 
     private
 
-    def format_provider_custom_attributes(attribute)
-      if CustomAttribute::ALLOWED_API_VALUE_TYPES.include? attribute["field_type"]
-        attribute["value"] = attribute.delete("field_type").safe_constantize.parse(attribute["value"])
-      end
-      attribute["section"] ||= "metadata" unless @req.action == "edit"
-      if attribute["section"].present? && !CustomAttribute::ALLOWED_API_SECTIONS.include?(attribute["section"])
-        raise "Invalid attribute section specified: #{attribute["section"]}"
-      end
-      attribute
-    rescue => err
-      raise BadRequestError, "Invalid provider custom attributes specified - #{err}"
-    end
-
     def provider_ident(provider)
       "Provider id:#{provider.id} name:'#{provider.name}'"
     end

app/controllers/api/subcollections/custom_attributes.rb

@@ -6,16 +6,17 @@ def custom_attributes_query_resource(object)
       end
 
       def custom_attributes_add_resource(object, _type, _id, data = nil)
-        if object.respond_to?(:custom_attributes)
-          add_custom_attribute(object, data)
-        else
-          raise BadRequestError, "#{object.class.name} does not support management of custom attributes"
-        end
+        raise BadRequestError, "#{object.class.name} does not support management of custom attributes" unless object.respond_to?(:custom_attributes)
+        add_custom_attribute(object, data)
+      rescue => err
+        raise BadRequestError, "Could not add custom attributes - #{err}"
       end
 
       def custom_attributes_edit_resource(object, _type, id = nil, data = nil)
         ca = find_custom_attribute(object, id, data)
         edit_custom_attribute(object, ca, data)
+      rescue => err
+        raise BadRequestError, "Could not edit custom attributes - #{err}"
       end
 
       def custom_attributes_delete_resource(object, _type, id = nil, data = nil)
@@ -44,6 +45,7 @@ def add_custom_attribute(object, data)
 
       def edit_custom_attribute(object, ca, data)
         return if ca.blank?
+        data = format_custom_attributes(data)
         update_custom_attributes(ca, data)
         update_custom_field(object, ca)
         ca
@@ -57,6 +59,7 @@ def delete_custom_attribute(object, ca)
       end
 
       def update_custom_attributes(ca, data)
+        data = format_custom_attributes(data)
         ca.update_attributes(data.slice("name", "value", "section"))
       end
 
@@ -79,12 +82,21 @@ def find_custom_attribute_by_data(object, data)
       end
 
       def new_custom_attribute(data)
-        name = data["name"].to_s.strip
-        raise BadRequestError, "Must specify a name for a custom attribute to be added" if name.blank?
-        CustomAttribute.new(:name    => name,
-                            :value   => data["value"],
-                            :source  => data["source"].blank? ? "EVM" : data["source"],
-                            :section => data["section"])
+        data["section"] ||= "metadata"
+        data["source"] ||= "EVM"
+        raise "Must specify a name for a custom attribute to be added" if data["name"].blank?
+        data = format_custom_attributes(data)
+        CustomAttribute.new(data)
+      end
+
+      def format_custom_attributes(attribute)
+        if CustomAttribute::ALLOWED_API_VALUE_TYPES.include?(attribute["field_type"])
+          attribute["value"] = attribute.delete("field_type").safe_constantize.parse(attribute["value"])
+        end
+        if attribute["section"].present? && !CustomAttribute::ALLOWED_API_SECTIONS.include?(attribute["section"])
+          raise "Invalid attribute section specified: #{attribute["section"]}"
+        end
+        attribute
       end
     end
   end

spec/requests/custom_attributes_spec.rb

@@ -40,4 +40,37 @@
     expect(response).to have_http_status(:ok)
     expect(response.parsed_body['href']).to include(api_provider_custom_attribute_url(nil, provider, custom_attribute))
   end
+
+  it 'returns a bad_request for invalid values of section' do
+    vm = FactoryGirl.create(:vm_vmware)
+    api_basic_authorize subcollection_action_identifier(:vms, :custom_attributes, :add, :post)
+
+    post(api_vm_custom_attributes_url(nil, vm), :params => { :action => :add, :resources => [{:section => "bad_section", :name => "test01", :value => "val01"}] })
+
+    expected = {
+      'error' => a_hash_including(
+        'kind'    => 'bad_request',
+        'message' => a_string_including('Invalid attribute section specified')
+      )
+    }
+    expect(response.parsed_body).to include(expected)
+    expect(response).to have_http_status(:bad_request)
+  end
+
+  it 'does not allow editing of custom attributes with incorrect values' do
+    vm = FactoryGirl.create(:vm_vmware)
+    custom_attribute = FactoryGirl.create(:custom_attribute, :resource => vm, :name => 'foo', :value => 'bar')
+    api_basic_authorize subcollection_action_identifier(:vms, :custom_attributes, :edit, :post)
+
+    post(api_vm_custom_attribute_url(nil, vm, custom_attribute), :params => { :action => :edit, :section => "bad_section", :name => "foo", :value => "bar" })
+
+    expected = {
+      'error' => a_hash_including(
+        'kind'    => 'bad_request',
+        'message' => a_string_including('Invalid attribute section specified')
+      )
+    }
+    expect(response.parsed_body).to include(expected)
+    expect(response).to have_http_status(:bad_request)
+  end
 end

spec/requests/providers_spec.rb

@@ -276,8 +276,7 @@ def have_endpoint_attributes(expected_hash)
       post(provider_ca_url, :params => gen_request(:add, [{"name" => "name3", "value" => "value3",
                                                     "section" => "bad_section"}]))
 
-      expect_bad_request("Invalid provider custom attributes specified - " \
-                         "Invalid attribute section specified: bad_section")
+      expect_bad_request("Could not add custom attributes - Invalid attribute section specified: bad_section")
     end
 
     it "add custom attributes to a provider" do