From d1115a0e0c02280310d3fdfcf80a24aef9b324fa Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Tue, 7 Mar 2017 11:27:27 -0500 Subject: [PATCH 1/6] authentication create --- config/api.yml | 2 ++ spec/requests/api/authentications_spec.rb | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/config/api.yml b/config/api.yml index 111d76a87d3..ad51885eb4f 100644 --- a/config/api.yml +++ b/config/api.yml @@ -246,6 +246,8 @@ :identifier: embedded_automation_manager_credentials_delete - :name: edit :identifier: embedded_automation_manager_credentials_edit + - :name: create + :identifier: embedded_automation_manager_credentials_add :resource_actions: :get: - :name: read diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index eb88fa82b62..ee940718cfc 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -176,6 +176,21 @@ expect(response).to have_http_status(:forbidden) end + + it 'can create an authentication' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + + expected = { + 'results' => [ + a_hash_including('name' => 'foo') + ] + } + expect do + run_post(authentications_url, :action => 'create', :name => 'foo') + end.to change(Authentication, :count).by(1) + expect(response.parsed_body).to include(expected) + expect(response).to have_http_status(:ok) + end end describe 'PUT /api/authentications/:id' do From ca7a5241af46ce1d8a2991d5612c54180e79a9ee Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 8 Mar 2017 16:30:27 -0500 Subject: [PATCH 2/6] create in provider queue add class method on authentication to return the type --- .../api/authentications_controller.rb | 15 ++++++ .../api/subcollections/authentications.rb | 8 +++ app/models/authentication.rb | 8 +++ config/api.yml | 10 ++++ spec/models/authentication_spec.rb | 20 ++++++++ spec/requests/api/authentications_spec.rb | 47 +++++++++++++++-- .../api/configuration_script_payloads_spec.rb | 50 +++++++++++++++++++ 7 files changed, 154 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/authentications_controller.rb b/app/controllers/api/authentications_controller.rb index 6342961f103..fb8f7b22193 100644 --- a/app/controllers/api/authentications_controller.rb +++ b/app/controllers/api/authentications_controller.rb @@ -9,6 +9,16 @@ def edit_resource(type, id, data) action_result(false, err.to_s) end + def create_resource(_type, _id, data) + validate_auth_attrs(data) + klass = ::Authentication.class_from_request_data(data) + # TODO: Temporary validation - remove + raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) + klass.create_in_provider_queue(parse_id(data['manager'], :providers), data.except('type', 'manager')) + rescue => err + raise BadRequestError, "Cannot create Authentication - #{err}" + end + def delete_resource(type, id, _data = {}) auth = resource_search(id, type, collection_class(:authentications)) raise "Delete not supported for #{authentication_ident(auth)}" unless auth.respond_to?(:delete_in_provider_queue) @@ -33,5 +43,10 @@ def build_additional_fields :credential_types => ::Authentication.build_credential_options } end + + def validate_auth_attrs(data) + raise 'must supply type' unless data['type'] + raise 'must supply a manager' unless data['manager'] + end end end diff --git a/app/controllers/api/subcollections/authentications.rb b/app/controllers/api/subcollections/authentications.rb index 0e81bea7a3e..009abfbe954 100644 --- a/app/controllers/api/subcollections/authentications.rb +++ b/app/controllers/api/subcollections/authentications.rb @@ -4,6 +4,14 @@ module Authentications def authentications_query_resource(object) object.respond_to?(:authentications) ? object.authentications : [] end + + def authentications_create_resource(parent, _type, _id, data) + klass = ::Authentication.class_from_request_data(data) + raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) + klass.create_in_provider_queue(parent.manager_id, data.except('type')) + rescue => err + raise BadRequestError, "Cannot create Authentication - #{err}" + end end end end diff --git a/app/models/authentication.rb b/app/models/authentication.rb index 096f40a45b5..5c54d962ebc 100644 --- a/app/models/authentication.rb +++ b/app/models/authentication.rb @@ -118,6 +118,14 @@ def self.build_credential_options end end + def self.class_from_request_data(data) + return self unless data.key?('type') + data['type'].constantize.tap do |klass| + return self if klass == self + raise _('Must be an Authentication type') unless descendants.include?(klass) + end + end + private def set_credentials_changed_on diff --git a/config/api.yml b/config/api.yml index ad51885eb4f..e1c87e8de1a 100644 --- a/config/api.yml +++ b/config/api.yml @@ -264,6 +264,9 @@ :get: - :name: read :identifier: embedded_automation_manager_credentials_view + :post: + - :name: create + :identifier: embedded_automation_manager_credentials_add :automate: :description: Automate :options: @@ -594,6 +597,13 @@ :get: - :name: read :identifier: embedded_configuration_script_payload_view + :authentications_subcollection_actions: + :get: + - :name: read + :identifier: embedded_automation_manager_credentials_view + :post: + - :name: create + :identifier: embedded_automation_manager_credentials_add :configuration_script_sources: :description: Configuration Script Source :options: diff --git a/spec/models/authentication_spec.rb b/spec/models/authentication_spec.rb index a6639d56af4..71f250da5be 100644 --- a/spec/models/authentication_spec.rb +++ b/spec/models/authentication_spec.rb @@ -45,4 +45,24 @@ expect(described_class.new(:status => 'Error').retryable_status?).to be_truthy end end + + context '.class_from_request_data' do + it 'raises an error if it is not an authentication type' do + expect do + described_class.class_from_request_data('type' => 'ServiceTemplate') + end.to raise_error(RuntimeError, 'Must be an Authentication type') + end + + it 'returns self if no type is specified' do + expect(described_class.class_from_request_data({})).to eq(described_class) + end + + it 'returns the specified type' do + data = { + 'type' => 'ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential' + } + expect(described_class.class_from_request_data(data)) + .to eq(ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential) + end + end end diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index ee940718cfc..f95f9d93018 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -177,17 +177,56 @@ expect(response).to have_http_status(:forbidden) end + it 'requires a type to create an authentication' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + + run_post(authentications_url, :action => 'create', :name => 'foo') + + expected = { + 'error' => a_hash_including( + 'message' => 'Cannot create Authentication - must supply type' + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + + it 'requires a manager when creating an authentication' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + + run_post(authentications_url, :action => 'create', :type => 'Authentication') + + expected = { + 'error' => a_hash_including( + 'message' => 'Cannot create Authentication - must supply a manager' + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + + it 'requires that the type support create_in_provider_queue' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + + run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager => 'blah') + + expected = { + 'error' => a_hash_including( + 'message' => 'Cannot create Authentication - type not currently supported' + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + it 'can create an authentication' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) expected = { 'results' => [ - a_hash_including('name' => 'foo') + a_hash_including('id' => auth.id) ] } - expect do - run_post(authentications_url, :action => 'create', :name => 'foo') - end.to change(Authentication, :count).by(1) expect(response.parsed_body).to include(expected) expect(response).to have_http_status(:ok) end diff --git a/spec/requests/api/configuration_script_payloads_spec.rb b/spec/requests/api/configuration_script_payloads_spec.rb index 35c49003195..016f995020e 100644 --- a/spec/requests/api/configuration_script_payloads_spec.rb +++ b/spec/requests/api/configuration_script_payloads_spec.rb @@ -67,6 +67,56 @@ end end + describe 'POST /api/configuration_script_payloads/:id/authentications' do + it 'requires a type when creating a new authentication' do + ems = FactoryGirl.create(:ext_management_system) + playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) + api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) + + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :name => 'foo') + + expected = { + 'error' => a_hash_including( + 'message' => a_string_including('must supply a type') + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + + it 'requires that the type support create_in_provider_queue' do + ems = FactoryGirl.create(:ext_management_system) + playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) + api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) + + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :type => 'Authentication') + + expected = { + 'error' => a_hash_including( + 'message' => a_string_including('type not currently supported') + ) + } + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) + end + + it 'creates a new authentication' do + ems = FactoryGirl.create(:ext_management_system) + playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) + auth = FactoryGirl.create(:authentication) + expect(Authentication).to receive(:create_in_provider_queue).with(ems.id, 'name' => auth.name).and_return(auth) + api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) + + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :type => 'Authentication', :name => auth.name) + + expected = { + 'results' => [a_hash_including('id' => auth.id)] + } + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + end + end + describe 'GET /api/configuration_script_payloads/:id/authentications/:id' do it 'returns a specific authentication' do authentication = FactoryGirl.create(:authentication) From 9abeafe9b55e939031ca049ac2dc8b464c36c5af Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Mon, 13 Mar 2017 11:31:08 -0400 Subject: [PATCH 3/6] require resource_manager use action_result remove constantize use action_result --- .../api/authentications_controller.rb | 19 ++-- .../api/subcollections/authentications.rb | 5 +- app/models/authentication.rb | 9 +- spec/models/authentication_spec.rb | 4 + spec/requests/api/authentications_spec.rb | 92 ++++++++++++++----- .../api/configuration_script_payloads_spec.rb | 73 ++++++++++----- 6 files changed, 146 insertions(+), 56 deletions(-) diff --git a/app/controllers/api/authentications_controller.rb b/app/controllers/api/authentications_controller.rb index fb8f7b22193..30591b5cde5 100644 --- a/app/controllers/api/authentications_controller.rb +++ b/app/controllers/api/authentications_controller.rb @@ -10,13 +10,14 @@ def edit_resource(type, id, data) end def create_resource(_type, _id, data) - validate_auth_attrs(data) - klass = ::Authentication.class_from_request_data(data) + attrs = validate_auth_attrs(data) + klass = ::Authentication.class_from_request_data(attrs) # TODO: Temporary validation - remove raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) - klass.create_in_provider_queue(parse_id(data['manager'], :providers), data.except('type', 'manager')) + task_id = klass.create_in_provider_queue(attrs['manager_resource'], attrs.except('type', 'manager_resource')) + action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err - raise BadRequestError, "Cannot create Authentication - #{err}" + action_result(false, err.to_s) end def delete_resource(type, id, _data = {}) @@ -45,8 +46,14 @@ def build_additional_fields end def validate_auth_attrs(data) - raise 'must supply type' unless data['type'] - raise 'must supply a manager' unless data['manager'] + raise 'must supply a manager resource' unless data['manager_resource'] + attrs = data.dup + attrs['manager_resource'] = if data['manager_resource'].key?('href') + parse_href(data['manager_resource']['href']).last + elsif data['manager_resource'].key?('id') + data['manager_resource']['id'] + end + attrs end end end diff --git a/app/controllers/api/subcollections/authentications.rb b/app/controllers/api/subcollections/authentications.rb index 009abfbe954..6f59da378f8 100644 --- a/app/controllers/api/subcollections/authentications.rb +++ b/app/controllers/api/subcollections/authentications.rb @@ -8,9 +8,10 @@ def authentications_query_resource(object) def authentications_create_resource(parent, _type, _id, data) klass = ::Authentication.class_from_request_data(data) raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) - klass.create_in_provider_queue(parent.manager_id, data.except('type')) + task_id = klass.create_in_provider_queue(parent.manager_id, data.except('type')) + action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err - raise BadRequestError, "Cannot create Authentication - #{err}" + action_result(false, err.to_s) end end end diff --git a/app/models/authentication.rb b/app/models/authentication.rb index 5c54d962ebc..1efae793dae 100644 --- a/app/models/authentication.rb +++ b/app/models/authentication.rb @@ -119,11 +119,12 @@ def self.build_credential_options end def self.class_from_request_data(data) - return self unless data.key?('type') - data['type'].constantize.tap do |klass| - return self if klass == self - raise _('Must be an Authentication type') unless descendants.include?(klass) + if !data.key?('type') || data['type'] == to_s + return self end + type = descendants.find { |klass| klass.name == data['type'] } + raise _('Must be an Authentication type') unless type + type end private diff --git a/spec/models/authentication_spec.rb b/spec/models/authentication_spec.rb index 71f250da5be..237ef8f194f 100644 --- a/spec/models/authentication_spec.rb +++ b/spec/models/authentication_spec.rb @@ -57,6 +57,10 @@ expect(described_class.class_from_request_data({})).to eq(described_class) end + it 'returns self if Authentication is specified' do + expect(described_class.class_from_request_data('type' => 'Authentication')).to eq(described_class) + end + it 'returns the specified type' do data = { 'type' => 'ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential' diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index f95f9d93018..c0d2152c27c 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -52,6 +52,7 @@ end describe 'POST /api/authentications' do + let(:manager) { provider.managers.first } let(:params) do { :id => auth.id, @@ -177,58 +178,107 @@ expect(response).to have_http_status(:forbidden) end - it 'requires a type to create an authentication' do + let(:create_params) do + { + :action => 'create', + :description => "Description", + :name => "A Credential", + :related => {}, + :type => 'ManageIQ::Providers::AnsibleTower::AutomationManager::Credential', + :manager_resource => { :href => providers_url(manager.id) } + } + end + + it 'requires a manager resource when creating an authentication' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) - run_post(authentications_url, :action => 'create', :name => 'foo') + run_post(authentications_url, :action => 'create', :type => 'Authentication') expected = { - 'error' => a_hash_including( - 'message' => 'Cannot create Authentication - must supply type' - ) + 'results' => [ + { 'success' => false, 'message' => 'must supply a manager resource' } + ] } - expect(response).to have_http_status(:bad_request) + expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end - it 'requires a manager when creating an authentication' do + it 'requires that the type support create_in_provider_queue' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) - run_post(authentications_url, :action => 'create', :type => 'Authentication') + run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager_resource => { }) expected = { - 'error' => a_hash_including( - 'message' => 'Cannot create Authentication - must supply a manager' - ) + 'results' => [ + { 'success' => false, 'message' => 'type not currently supported' } + ] } - expect(response).to have_http_status(:bad_request) + expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end - it 'requires that the type support create_in_provider_queue' do + it 'can create an authentication' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) - run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager => 'blah') + expected = { + 'results' => [a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + )] + } + run_post(authentications_url, create_params) + + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + end + + it 'can create an authentication with a resource_manager id' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + create_params[:manager_resource] = { :id => manager.id } expected = { - 'error' => a_hash_including( - 'message' => 'Cannot create Authentication - type not currently supported' - ) + 'results' => [a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + )] } - expect(response).to have_http_status(:bad_request) + run_post(authentications_url, create_params) + + expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end - it 'can create an authentication' do + it 'can create authentications in bulk' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) expected = { 'results' => [ - a_hash_including('id' => auth.id) + a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + ), + a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + ) ] } - expect(response.parsed_body).to include(expected) + run_post(authentications_url, :resources => [create_params, create_params]) + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + end + + it 'will forbid creation of an authentication without appropriate role' do + api_basic_authorize + + run_post(authentications_url, :action => 'create') + + expect(response).to have_http_status(:forbidden) end end diff --git a/spec/requests/api/configuration_script_payloads_spec.rb b/spec/requests/api/configuration_script_payloads_spec.rb index 016f995020e..3c5adbc18f9 100644 --- a/spec/requests/api/configuration_script_payloads_spec.rb +++ b/spec/requests/api/configuration_script_payloads_spec.rb @@ -68,53 +68,80 @@ end describe 'POST /api/configuration_script_payloads/:id/authentications' do - it 'requires a type when creating a new authentication' do - ems = FactoryGirl.create(:ext_management_system) - playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) + let(:provider) { FactoryGirl.create(:provider_ansible_tower, :with_authentication) } + let(:manager) { provider.managers.first } + let(:playbook) { FactoryGirl.create(:configuration_script_payload, :manager => manager) } + let(:params) do + { + :action => 'create', + :description => "Description", + :name => "A Credential", + :related => {}, + :type => 'ManageIQ::Providers::AnsibleTower::AutomationManager::Credential', + :manager_resource => { :href => providers_url(manager.id) } + } + end + + it 'requires that the type support create_in_provider_queue' do api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) - run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :name => 'foo') + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :type => 'Authentication') expected = { - 'error' => a_hash_including( - 'message' => a_string_including('must supply a type') - ) + 'results' => [ + { 'success' => false, 'message' => 'type not currently supported' } + ] } - expect(response).to have_http_status(:bad_request) + expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end - it 'requires that the type support create_in_provider_queue' do - ems = FactoryGirl.create(:ext_management_system) - playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) + it 'creates a new authentication with an appropriate role' do api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) - run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :type => 'Authentication') + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", params) expected = { - 'error' => a_hash_including( - 'message' => a_string_including('type not currently supported') - ) + 'results' => [a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + )] } - expect(response).to have_http_status(:bad_request) + expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end - it 'creates a new authentication' do - ems = FactoryGirl.create(:ext_management_system) - playbook = FactoryGirl.create(:configuration_script_payload, :manager => ems) - auth = FactoryGirl.create(:authentication) - expect(Authentication).to receive(:create_in_provider_queue).with(ems.id, 'name' => auth.name).and_return(auth) + it 'can create multiple authentications with an appropriate role' do api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create) - run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :type => 'Authentication', :name => auth.name) + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :resources => [params, params]) expected = { - 'results' => [a_hash_including('id' => auth.id)] + 'results' => [ + a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + ), + a_hash_including( + 'success' => true, + 'message' => 'Creating Authentication', + 'task_id' => a_kind_of(Numeric) + ) + ] } expect(response).to have_http_status(:ok) expect(response.parsed_body).to include(expected) end + + it 'cannot create an authentication without appropriate role' do + api_basic_authorize + + run_post("#{configuration_script_payloads_url(playbook.id)}/authentications", :resources => [params]) + + expect(response).to have_http_status(:forbidden) + end end describe 'GET /api/configuration_script_payloads/:id/authentications/:id' do From 24084ba6bb5e4f1a70819a30e284ae6d5c56543e Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Tue, 21 Mar 2017 09:25:08 -0400 Subject: [PATCH 4/6] require href and validate the collection passed to manager_resource --- app/controllers/api/authentications_controller.rb | 8 +++----- app/models/authentication.rb | 2 ++ spec/requests/api/authentications_spec.rb | 11 +++++------ 3 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/authentications_controller.rb b/app/controllers/api/authentications_controller.rb index 30591b5cde5..f732be04f0c 100644 --- a/app/controllers/api/authentications_controller.rb +++ b/app/controllers/api/authentications_controller.rb @@ -48,11 +48,9 @@ def build_additional_fields def validate_auth_attrs(data) raise 'must supply a manager resource' unless data['manager_resource'] attrs = data.dup - attrs['manager_resource'] = if data['manager_resource'].key?('href') - parse_href(data['manager_resource']['href']).last - elsif data['manager_resource'].key?('id') - data['manager_resource']['id'] - end + collection, id = parse_href(data['manager_resource']['href']) + raise "#{collection} is not a valid manager resource" unless ::Authentication::MANAGER_TYPES.include?(collection) + attrs['manager_resource'] = id attrs end end diff --git a/app/models/authentication.rb b/app/models/authentication.rb index 1efae793dae..c7c43e59c1b 100644 --- a/app/models/authentication.rb +++ b/app/models/authentication.rb @@ -55,6 +55,8 @@ def self.new(*args, &block) :embedded_ansible_credential_types => 'ManageIQ::Providers::EmbeddedAutomationManager::Authentication' }.freeze + MANAGER_TYPES = [:providers].freeze + # FIXME: To address problem with url resolution when displayed as a quadicon, # but it's not *really* the db_name. Might be more proper to override `to_partial_path` def self.db_name diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index c0d2152c27c..19e6b9ceb30 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -206,7 +206,7 @@ it 'requires that the type support create_in_provider_queue' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) - run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager_resource => { }) + run_post(authentications_url, :action => 'create', :type => 'Authentication', :manager_resource => { :href => providers_url(manager.id) }) expected = { 'results' => [ @@ -233,15 +233,14 @@ expect(response.parsed_body).to include(expected) end - it 'can create an authentication with a resource_manager id' do + it 'requires a valid manager resource' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) - create_params[:manager_resource] = { :id => manager.id } + create_params[:manager_resource] = { :href => users_url(10) } expected = { 'results' => [a_hash_including( - 'success' => true, - 'message' => 'Creating Authentication', - 'task_id' => a_kind_of(Numeric) + 'success' => false, + 'message' => 'users is not a valid manager resource' )] } run_post(authentications_url, create_params) From e2e1811064a6916052b6311629f20e031527e7ce Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 22 Mar 2017 11:33:08 -0400 Subject: [PATCH 5/6] create authentications service --- .../api/authentications_controller.rb | 19 +++++++++---------- .../api/subcollections/authentications.rb | 4 +--- app/models/authentication.rb | 2 -- lib/services/api/authentication_service.rb | 10 ++++++++++ spec/requests/api/authentications_spec.rb | 16 ---------------- 5 files changed, 20 insertions(+), 31 deletions(-) create mode 100644 lib/services/api/authentication_service.rb diff --git a/app/controllers/api/authentications_controller.rb b/app/controllers/api/authentications_controller.rb index f732be04f0c..6763198aaba 100644 --- a/app/controllers/api/authentications_controller.rb +++ b/app/controllers/api/authentications_controller.rb @@ -10,11 +10,8 @@ def edit_resource(type, id, data) end def create_resource(_type, _id, data) - attrs = validate_auth_attrs(data) - klass = ::Authentication.class_from_request_data(attrs) - # TODO: Temporary validation - remove - raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) - task_id = klass.create_in_provider_queue(attrs['manager_resource'], attrs.except('type', 'manager_resource')) + manager_resource, attrs = validate_auth_attrs(data) + task_id = AuthenticationService.create_authentication(manager_resource, attrs) action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err action_result(false, err.to_s) @@ -47,11 +44,13 @@ def build_additional_fields def validate_auth_attrs(data) raise 'must supply a manager resource' unless data['manager_resource'] - attrs = data.dup - collection, id = parse_href(data['manager_resource']['href']) - raise "#{collection} is not a valid manager resource" unless ::Authentication::MANAGER_TYPES.include?(collection) - attrs['manager_resource'] = id - attrs + attrs = data.dup.except('manager_resource') + manager_resource = if data['manager_resource'].key?('href') + parse_href(data['manager_resource']['href']).last + elsif data['manager_resource'].key?('id') + data['manager_resource']['id'] + end + [manager_resource, attrs] end end end diff --git a/app/controllers/api/subcollections/authentications.rb b/app/controllers/api/subcollections/authentications.rb index 6f59da378f8..027823e612b 100644 --- a/app/controllers/api/subcollections/authentications.rb +++ b/app/controllers/api/subcollections/authentications.rb @@ -6,9 +6,7 @@ def authentications_query_resource(object) end def authentications_create_resource(parent, _type, _id, data) - klass = ::Authentication.class_from_request_data(data) - raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) - task_id = klass.create_in_provider_queue(parent.manager_id, data.except('type')) + task_id = AuthenticationService.create_authentication(parent.manager_id, data) action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err action_result(false, err.to_s) diff --git a/app/models/authentication.rb b/app/models/authentication.rb index c7c43e59c1b..1efae793dae 100644 --- a/app/models/authentication.rb +++ b/app/models/authentication.rb @@ -55,8 +55,6 @@ def self.new(*args, &block) :embedded_ansible_credential_types => 'ManageIQ::Providers::EmbeddedAutomationManager::Authentication' }.freeze - MANAGER_TYPES = [:providers].freeze - # FIXME: To address problem with url resolution when displayed as a quadicon, # but it's not *really* the db_name. Might be more proper to override `to_partial_path` def self.db_name diff --git a/lib/services/api/authentication_service.rb b/lib/services/api/authentication_service.rb new file mode 100644 index 00000000000..72bb1a3b03b --- /dev/null +++ b/lib/services/api/authentication_service.rb @@ -0,0 +1,10 @@ +module Api + class AuthenticationService + def self.create_authentication(manager_resource, attrs) + klass = ::Authentication.class_from_request_data(attrs) + # TODO: Temporary validation - remove + raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) + klass.create_in_provider_queue(manager_resource, attrs) + end + end +end diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index 19e6b9ceb30..6e26d57ea89 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -233,22 +233,6 @@ expect(response.parsed_body).to include(expected) end - it 'requires a valid manager resource' do - api_basic_authorize collection_action_identifier(:authentications, :create, :post) - create_params[:manager_resource] = { :href => users_url(10) } - - expected = { - 'results' => [a_hash_including( - 'success' => false, - 'message' => 'users is not a valid manager resource' - )] - } - run_post(authentications_url, create_params) - - expect(response).to have_http_status(:ok) - expect(response.parsed_body).to include(expected) - end - it 'can create authentications in bulk' do api_basic_authorize collection_action_identifier(:authentications, :create, :post) From eb89cb8b5920cbe28d7f99ff5b4a199545425507 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 22 Mar 2017 17:35:58 -0400 Subject: [PATCH 6/6] remove support for ID create_authentication to create_authentication_task --- .../api/authentications_controller.rb | 10 ++++------ .../api/subcollections/authentications.rb | 2 +- lib/services/api/authentication_service.rb | 4 ++-- spec/requests/api/authentications_spec.rb | 18 ++++++++++++++++++ 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/authentications_controller.rb b/app/controllers/api/authentications_controller.rb index 6763198aaba..9be15e1a527 100644 --- a/app/controllers/api/authentications_controller.rb +++ b/app/controllers/api/authentications_controller.rb @@ -11,7 +11,7 @@ def edit_resource(type, id, data) def create_resource(_type, _id, data) manager_resource, attrs = validate_auth_attrs(data) - task_id = AuthenticationService.create_authentication(manager_resource, attrs) + task_id = AuthenticationService.create_authentication_task(manager_resource, attrs) action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err action_result(false, err.to_s) @@ -45,11 +45,9 @@ def build_additional_fields def validate_auth_attrs(data) raise 'must supply a manager resource' unless data['manager_resource'] attrs = data.dup.except('manager_resource') - manager_resource = if data['manager_resource'].key?('href') - parse_href(data['manager_resource']['href']).last - elsif data['manager_resource'].key?('id') - data['manager_resource']['id'] - end + manager_collection, manager_id = parse_href(data['manager_resource']['href']) + raise 'invalid manger_resource href specified' unless manager_collection && manager_id + manager_resource = resource_search(manager_id, manager_collection, collection_class(manager_collection)) [manager_resource, attrs] end end diff --git a/app/controllers/api/subcollections/authentications.rb b/app/controllers/api/subcollections/authentications.rb index 027823e612b..1adfca23bdf 100644 --- a/app/controllers/api/subcollections/authentications.rb +++ b/app/controllers/api/subcollections/authentications.rb @@ -6,7 +6,7 @@ def authentications_query_resource(object) end def authentications_create_resource(parent, _type, _id, data) - task_id = AuthenticationService.create_authentication(parent.manager_id, data) + task_id = AuthenticationService.create_authentication_task(parent.manager, data) action_result(true, 'Creating Authentication', :task_id => task_id) rescue => err action_result(false, err.to_s) diff --git a/lib/services/api/authentication_service.rb b/lib/services/api/authentication_service.rb index 72bb1a3b03b..b1a16ee41c6 100644 --- a/lib/services/api/authentication_service.rb +++ b/lib/services/api/authentication_service.rb @@ -1,10 +1,10 @@ module Api class AuthenticationService - def self.create_authentication(manager_resource, attrs) + def self.create_authentication_task(manager_resource, attrs) klass = ::Authentication.class_from_request_data(attrs) # TODO: Temporary validation - remove raise 'type not currently supported' unless klass.respond_to?(:create_in_provider_queue) - klass.create_in_provider_queue(manager_resource, attrs) + klass.create_in_provider_queue(manager_resource.id, attrs) end end end diff --git a/spec/requests/api/authentications_spec.rb b/spec/requests/api/authentications_spec.rb index 6e26d57ea89..edb0f6cf7bf 100644 --- a/spec/requests/api/authentications_spec.rb +++ b/spec/requests/api/authentications_spec.rb @@ -256,6 +256,24 @@ expect(response.parsed_body).to include(expected) end + it 'requires a valid manager_resource' do + api_basic_authorize collection_action_identifier(:authentications, :create, :post) + create_params[:manager_resource] = { :href => '1' } + + expected = { + 'results' => [ + a_hash_including( + 'success' => false, + 'message' => 'invalid manger_resource href specified', + ) + ] + } + run_post(authentications_url, :resources => [create_params]) + + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include(expected) + end + it 'will forbid creation of an authentication without appropriate role' do api_basic_authorize