Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use standard create/update/delete features for Authentications #1220

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Use standard create/update/delete features for Authentications #1220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions app/controllers/api/authentications_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,25 @@
module Api
class AuthenticationsController < BaseController
def edit_resource(type, id, data)
api_resource(type, id, "Updating") do |auth|
ensure_respond_to(type, auth, :update, :update_in_provider_queue)
api_resource(type, id, "Updating", :supports => :update) do |auth|
{:task_id => auth.update_in_provider_queue(data.deep_symbolize_keys)}
end
end

def create_resource(_type, _id, data)
def create_resource(type, _id, data)
manager_resource, attrs = validate_auth_attrs(data)
task_id = AuthenticationService.create_authentication_task(manager_resource, attrs)

klass = ::Authentication.descendant_get(attrs['type'])
ensure_supports(type, klass, :create)
Comment on lines +12 to +13
Copy link
Member

@kbrock kbrock Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darn, this is very close to the api_resource model.

almost like we want to define:

def resource_search(id, type)
  ::Authentication.descendant_get(type)
end

def create_resource(type, _id, data)
  api_resource(type, nil, "Creating", :supports => :create) do |klass|
    manager_resource, attrs = validate_auth_attrs(data)
    {:task_id => klass.create_in_provider_queue(manager_resource.id, attrs.deep_symbolize_keys)}
  end
end

But may need to fall back to api_action.

Now that I think about it, I had refactored this method a number of times.
Never got it quite right.

This is a definite improvement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, right

I kept splitting apart the validate_auth_attrs:

def create_resource(_type, _id, data)
  type, id = data_resource(data)
  # maybe attrs from data_resource ?
  api_resource(type, id, "Creating", :supports => :create) do |auth, klass|
    {:task_id => klass.create_in_provider_queue(auth.id, attrs.deep_symbolize_keys)}
  end
end

def data_resource(data)
  href = Href.new(data['manager_resource']['href'])
  raise 'invalid manager_resource href specified' unless href.subject && href.subject_id
  [href.subject, href.subject_id]
end

nvr mind. there is a reason I never created a PR for this.


task_id = klass.create_in_provider_queue(manager_resource.id, attrs.deep_symbolize_keys)
action_result(true, 'Creating Authentication', :task_id => task_id)
rescue => err
action_result(false, err.to_s)
end

def delete_resource_main_action(type, auth, _data)
ensure_respond_to(type, auth, :delete, :delete_in_provider_queue)
ensure_supports(type, auth, :delete)
kbrock marked this conversation as resolved.
Show resolved Hide resolved
{:task_id => auth.delete_in_provider_queue}
end

Expand Down
7 changes: 5 additions & 2 deletions app/controllers/api/subcollections/authentications.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ def authentications_query_resource(object)
object.respond_to?(:authentications) ? object.authentications : []
end

def authentications_create_resource(parent, _type, _id, data)
task_id = AuthenticationService.create_authentication_task(parent.manager, data)
def authentications_create_resource(parent, type, _id, data)
klass = ::Authentication.descendant_get(data['type'])
ensure_supports(type, klass, :create)

task_id = klass.create_in_provider_queue(parent.manager.id, data.deep_symbolize_keys)
action_result(true, 'Creating Authentication', :task_id => task_id)
rescue => err
action_result(false, err.to_s)
Expand Down
9 changes: 0 additions & 9 deletions lib/services/api/authentication_service.rb
View file
Open in desktop

This file was deleted.

23 changes: 20 additions & 3 deletions spec/requests/authentications_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
RSpec.describe 'Authentications API' do
include Spec::Support::SupportsHelper

let(:manager) { FactoryBot.create(:automation_manager_ansible_tower) }
let(:auth) { FactoryBot.create(:ansible_cloud_credential, :resource => manager) }
let(:auth_2) { FactoryBot.create(:ansible_cloud_credential, :resource => manager) }
Expand Down Expand Up @@ -69,6 +71,7 @@

it 'will delete an authentication' do
api_basic_authorize collection_action_identifier(:authentications, :delete, :post)
stub_supports(auth, :delete)

post(api_authentications_url, :params => { :action => 'delete', :resources => [{ 'id' => auth.id }] })

Expand All @@ -86,14 +89,17 @@
it 'verifies that the type is supported' do
api_basic_authorize collection_action_identifier(:authentications, :delete, :post)
auth = FactoryBot.create(:authentication)
stub_supports_not(auth, :delete)

post(api_authentications_url, :params => { :action => 'delete', :resources => [{ 'id' => auth.id }] })

expect_multiple_action_result(1, :success => false, :message => /Delete not supported/)
expect_multiple_action_result(1, :success => false, :message => /Feature not available\/supported/)
end

it 'will delete multiple authentications' do
api_basic_authorize collection_action_identifier(:authentications, :delete, :post)
stub_supports(auth, :delete)
stub_supports(auth_2, :delete)

post(api_authentications_url, :params => {:action => 'delete', :resources => [{'id' => auth.id}, {'id' => auth_2.id}]})
expect_multiple_action_result(2, :task => true, :success => true, :message => 'Deleting Authentication')
Expand All @@ -109,6 +115,7 @@

it 'can update an authentication with an appropriate role' do
api_basic_authorize collection_action_identifier(:authentications, :edit)
stub_supports(auth, :update)

post(api_authentications_url, :params => { :action => 'edit', :resources => [params] })

Expand All @@ -128,6 +135,7 @@
it 'can update an authentication with an appropriate role' do
params2 = params.dup.merge(:id => auth_2.id)
api_basic_authorize collection_action_identifier(:authentications, :edit)
stub_supports(auth_2, :update)

post(api_authentications_url, :params => { :action => 'edit', :resources => [params, params2] })

Expand Down Expand Up @@ -189,7 +197,7 @@

expected = {
'results' => [
{ 'success' => false, 'message' => 'type not currently supported' }
{ 'success' => false, 'message' => 'Create for Authentications: Feature not available/supported' }
]
}
expect(response).to have_http_status(:bad_request)
Expand All @@ -199,6 +207,8 @@
it 'can create an authentication' do
api_basic_authorize collection_action_identifier(:authentications, :create, :post)

stub_supports(create_params[:type].safe_constantize, :create)

expected = {
'results' => [a_hash_including(
'success' => true,
Expand Down Expand Up @@ -298,6 +308,7 @@

it 'can update an authentication with an appropriate role' do
api_basic_authorize collection_action_identifier(:authentications, :edit)
stub_supports(auth, :update)

put(api_authentication_url(nil, auth), :params => { :resource => params })

Expand All @@ -322,6 +333,7 @@

it 'can update an authentication with an appropriate role' do
api_basic_authorize collection_action_identifier(:authentications, :edit)
stub_supports(auth, :update)

patch(api_authentication_url(nil, auth), :params => [params])

Expand All @@ -345,6 +357,7 @@

it 'will delete an authentication' do
api_basic_authorize action_identifier(:authentications, :delete, :resource_actions, :post)
stub_supports(auth, :delete)

post(api_authentication_url(nil, auth), :params => { :action => 'delete' })

Expand All @@ -361,6 +374,7 @@

it 'can update an authentication with an appropriate role' do
api_basic_authorize collection_action_identifier(:authentications, :edit)
stub_supports(auth, :update)

post(api_authentication_url(nil, auth), :params => { :action => 'edit', :resource => params })

Expand All @@ -376,10 +390,11 @@
it 'requires that the type support update_in_provider_queue' do
api_basic_authorize collection_action_identifier(:authentications, :edit)
auth = FactoryBot.create(:authentication)
stub_supports_not(auth, :update)

post(api_authentication_url(nil, auth), :params => { :action => 'edit', :resource => params })

expect_bad_request("Update not supported for Authentication id: #{auth.id} name: '#{auth.name}'")
expect_bad_request("Update for Authentication id: #{auth.id} name: '#{auth.name}': Feature not available/supported")
end

it 'will forbid update to an authentication without appropriate role' do
Expand Down Expand Up @@ -418,6 +433,8 @@
describe 'DELETE /api/authentications/:id' do
it 'will delete an authentication' do
auth = FactoryBot.create(:embedded_ansible_openstack_credential)
stub_supports(auth, :delete)

api_basic_authorize action_identifier(:authentications, :delete, :resource_actions, :delete)

delete(api_authentication_url(nil, auth))
Expand Down
5 changes: 4 additions & 1 deletion spec/requests/configuration_script_payloads_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
RSpec.describe 'Configuration Script Payloads API' do
include Spec::Support::SupportsHelper

describe 'GET /api/configuration_script_payloads' do
it 'lists all the configuration script payloads with an appropriate role' do
script_payload = FactoryBot.create(:configuration_script_payload)
Expand Down Expand Up @@ -227,7 +229,7 @@

expected = {
'results' => [
{ 'success' => false, 'message' => 'type not currently supported' }
{ 'success' => false, 'message' => 'Create for Authentications: Feature not available/supported' }
]
}
expect(response).to have_http_status(:bad_request)
Expand All @@ -236,6 +238,7 @@

it 'creates a new authentication with an appropriate role' do
api_basic_authorize subcollection_action_identifier(:configuration_script_payloads, :authentications, :create)
stub_supports(Authentication, :create)

post(api_configuration_script_payload_authentications_url(nil, playbook), :params => params)

Expand Down