From ec696a21ab33b3a9b6cb5b17aebf9599e47bc0e8 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Mon, 3 Apr 2017 15:53:42 -0400 Subject: [PATCH 1/2] edit vm https://bugzilla.redhat.com/show_bug.cgi?id=1428250 update custom identifier --- app/controllers/api/vms_controller.rb | 38 +++++++++++++++ config/api.yml | 2 - spec/requests/api/vms_spec.rb | 69 +++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/vms_controller.rb b/app/controllers/api/vms_controller.rb index cfa5dd38481..7a6ba1e607d 100644 --- a/app/controllers/api/vms_controller.rb +++ b/app/controllers/api/vms_controller.rb @@ -8,6 +8,8 @@ class VmsController < BaseController include Subcollections::Software include Subcollections::Snapshots + VALID_EDIT_ATTRS = %w(custom_1 description children parent).freeze + def start_resource(type, id = nil, _data = nil) raise BadRequestError, "Must specify an id for starting a #{type} resource" unless id @@ -86,6 +88,18 @@ def shelve_offload_resource(type, id = nil, _data = nil) end end + def edit_resource(type, id, data) + attrs = validate_edit_data(data) + parent, children = build_parent_children(data) + resource_search(id, type, collection_class(type)).tap do |vm| + vm.update_attributes!(attrs) + vm.replace_children(children) + vm.set_parent(parent) + end + rescue => err + raise BadRequestError, "Cannot edit VM - #{err}" + end + def delete_resource(type, id = nil, _data = nil) raise BadRequestError, "Must specify an id for deleting a #{type} resource" unless id @@ -231,6 +245,30 @@ def request_console_resource(type, id = nil, data = nil) private + def validate_edit_data(data) + invalid_keys = data.keys - VALID_EDIT_ATTRS + raise BadRequestError, "Cannot edit values #{invalid_keys.join(', ')}" if invalid_keys.present? + data.dup.except('parent', 'children') + end + + def build_parent_children(data) + children = if data.key?('children') + data['children'].collect do |child| + child_collection, child_id = parse_href(child['href']) + resource_search(child_id, child_collection, collection_class(child_collection)) + end + else + [] + end + + parent = if data.key?('parent') + parent_collection, parent_id = parse_href(data.fetch('parent', 'href')) + resource_search(parent_id, parent_collection, collection_class(parent_collection)) + end + + [parent, children] + end + def vm_ident(vm) "VM id:#{vm.id} name:'#{vm.name}'" end diff --git a/config/api.yml b/config/api.yml index 98a4392c59b..9c7ea9c8565 100644 --- a/config/api.yml +++ b/config/api.yml @@ -2265,7 +2265,6 @@ :disabled: true - :name: edit :identifier: vm_edit - :disabled: true - :name: add_lifecycle_event :identifier: vm_edit - :name: add_event @@ -2313,7 +2312,6 @@ :post: - :name: edit :identifier: vm_edit - :disabled: true - :name: add_lifecycle_event :identifier: vm_edit - :name: add_event diff --git a/spec/requests/api/vms_spec.rb b/spec/requests/api/vms_spec.rb index be0a19a0396..decbced1dac 100644 --- a/spec/requests/api/vms_spec.rb +++ b/spec/requests/api/vms_spec.rb @@ -28,6 +28,75 @@ def update_raw_power_state(state, *vms) vms.each { |vm| vm.update_attributes!(:raw_power_state => state) } end + context 'Vm edit' do + let(:new_vms) { FactoryGirl.create_list(:vm_openstack, 2) } + + before do + vm.set_child(vm_openstack) + vm.set_parent(vm_openstack1) + end + + it 'cannot edit a VM without an appropriate role' do + api_basic_authorize + + run_post(vms_url(vm.id), :action => 'edit') + + expect(response).to have_http_status(:forbidden) + end + + it 'can edit a VM with an appropriate role' do + api_basic_authorize collection_action_identifier(:vms, :edit) + children = new_vms.collect do |vm| + { 'href' => vms_url(vm.id) } + end + + run_post(vms_url(vm.id), :action => 'edit', + :description => 'bar', + :children => children, + :custom_1 => 'foobar', + :parent => vms_url(vm_openstack2.id)) + + expected = { + 'description' => 'bar' + } + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + expect(vm.reload.children).to match_array(new_vms) + expect(vm.parent).to eq(vm_openstack2) + expect(vm.custom_1).to eq('foobar') + end + + it 'only allows edit of custom_1, description, parent, and children' do + api_basic_authorize collection_action_identifier(:vms, :edit) + + run_post(vms_url(vm.id), :action => 'edit', :name => 'foo', :autostart => true, :power_state => 'off') + + expected = { + 'error' => a_hash_including( + 'kind' => 'bad_request', + 'message' => 'Cannot edit VM - Cannot edit values name, autostart, power_state' + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + + it 'can edit multiple vms' do + api_basic_authorize collection_action_identifier(:vms, :edit) + + run_post(vms_url, :action => 'edit', :resources => [{ :id => vm.id, :description => 'foo' }, { :id => vm_openstack.id, :description => 'bar'}]) + + expected = { + 'results' => [ + a_hash_including('description' => 'foo'), + a_hash_including('description' => 'bar') + ] + } + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + end + end + context "Vm accounts subcollection" do let(:acct1) { FactoryGirl.create(:account, :vm_or_template_id => vm.id, :name => "John") } let(:acct2) { FactoryGirl.create(:account, :vm_or_template_id => vm.id, :name => "Jane") } From 619e2776bd81e082619b05be2b19d57341dd32b4 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 5 Apr 2017 09:08:08 -0400 Subject: [PATCH 2/2] validate collection type --- app/controllers/api/vms_controller.rb | 35 ++++++++++++++++----------- spec/requests/api/vms_spec.rb | 27 +++++++++++++++++---- 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/vms_controller.rb b/app/controllers/api/vms_controller.rb index 7a6ba1e607d..f8f7ed5663a 100644 --- a/app/controllers/api/vms_controller.rb +++ b/app/controllers/api/vms_controller.rb @@ -8,7 +8,8 @@ class VmsController < BaseController include Subcollections::Software include Subcollections::Snapshots - VALID_EDIT_ATTRS = %w(custom_1 description children parent).freeze + VALID_EDIT_ATTRS = %w(description child_resources parent_resource).freeze + RELATIONSHIP_COLLECTIONS = [:vms, :templates].freeze def start_resource(type, id = nil, _data = nil) raise BadRequestError, "Must specify an id for starting a #{type} resource" unless id @@ -230,7 +231,7 @@ def request_console_resource(type, id = nil, data = nil) # protocol = data["protocol"] || "mks" # However, there are different entitlements for the different protocol as per miq_product_feature, # so we may go for different action, i.e. request_console_vnc - #protocol = "mks" + # protocol = "mks" protocol = data["protocol"] || "vnc" api_action(type, id) do |klass| @@ -246,27 +247,33 @@ def request_console_resource(type, id = nil, data = nil) private def validate_edit_data(data) - invalid_keys = data.keys - VALID_EDIT_ATTRS + invalid_keys = data.keys - VALID_EDIT_ATTRS - valid_custom_attrs raise BadRequestError, "Cannot edit values #{invalid_keys.join(', ')}" if invalid_keys.present? - data.dup.except('parent', 'children') + data.except('parent_resource', 'child_resources') end def build_parent_children(data) - children = if data.key?('children') - data['children'].collect do |child| - child_collection, child_id = parse_href(child['href']) - resource_search(child_id, child_collection, collection_class(child_collection)) + children = if data.key?('child_resources') + data['child_resources'].collect do |child| + fetch_relationship(child['href']) end - else - [] end - parent = if data.key?('parent') - parent_collection, parent_id = parse_href(data.fetch('parent', 'href')) - resource_search(parent_id, parent_collection, collection_class(parent_collection)) + parent = if data.key?('parent_resource') + fetch_relationship(data['parent_resource']['href']) end - [parent, children] + [parent, Array(children)] + end + + def fetch_relationship(href) + collection, id = parse_href(href) + raise "Invalid relationship type #{collection}" unless RELATIONSHIP_COLLECTIONS.include?(collection) + resource_search(id, collection, collection_class(collection)) + end + + def valid_custom_attrs + Vm.virtual_attribute_names.select { |name| name =~ /custom_\d/ } end def vm_ident(vm) diff --git a/spec/requests/api/vms_spec.rb b/spec/requests/api/vms_spec.rb index decbced1dac..a5c18365605 100644 --- a/spec/requests/api/vms_spec.rb +++ b/spec/requests/api/vms_spec.rb @@ -50,11 +50,12 @@ def update_raw_power_state(state, *vms) { 'href' => vms_url(vm.id) } end - run_post(vms_url(vm.id), :action => 'edit', - :description => 'bar', - :children => children, - :custom_1 => 'foobar', - :parent => vms_url(vm_openstack2.id)) + run_post(vms_url(vm.id), :action => 'edit', + :description => 'bar', + :child_resources => children, + :custom_1 => 'foobar', + :custom_9 => 'fizzbuzz', + :parent_resource => { :href => vms_url(vm_openstack2.id) }) expected = { 'description' => 'bar' @@ -64,6 +65,7 @@ def update_raw_power_state(state, *vms) expect(vm.reload.children).to match_array(new_vms) expect(vm.parent).to eq(vm_openstack2) expect(vm.custom_1).to eq('foobar') + expect(vm.custom_9).to eq('fizzbuzz') end it 'only allows edit of custom_1, description, parent, and children' do @@ -95,6 +97,21 @@ def update_raw_power_state(state, *vms) expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end + + it 'requires a valid child/parent relationship ' do + api_basic_authorize collection_action_identifier(:vms, :edit) + + run_post(vms_url(vm.id), :action => 'edit', :parent_resource => { :href => users_url(10) }) + + expected = { + 'error' => a_hash_including( + 'kind' => 'bad_request', + 'message' => 'Cannot edit VM - Invalid relationship type users' + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end end context "Vm accounts subcollection" do