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

Fix load balancers access in API #13866

Merged
merged 4 commits into from
Feb 13, 2017
Merged

Fix load balancers access in API #13866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6415748
Add some failing tests for load balancer subcollection access
imtayadeway Feb 10, 2017
caf8d04
Use the correct action name for GETs
imtayadeway Feb 10, 2017
196e069
Fix load balancers subcollection show access
imtayadeway Feb 10, 2017
91535c5
Load balancers subcollection config can be generic
imtayadeway Feb 10, 2017
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
16 changes: 8 additions & 8 deletions config/api.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,10 +830,6 @@
:identifier: instance_guest_restart
- :name: reset
:identifier: instance_reset
:load_balancers_subcollection_actions:
:get:
- :name: show
:identifier: load_balancer_show
:snapshots_subcollection_actions:
:get:
- :name: read
Expand Down Expand Up @@ -871,6 +867,14 @@
:get:
- :name: read
:identifier: load_balancer_show
:subcollection_actions:
:get:
- :name: read
:identifier: load_balancer_show
:subresource_actions:
:get:
- :name: read
:identifier: load_balancer_show
:measures:
:description: Measures
:identifier: measure
Expand Down Expand Up @@ -1124,10 +1128,6 @@
:get:
- :name: show
- :identifier: miq_cloud_networks_view
Copy link
Member

Choose a reason for hiding this comment

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

why moving these out of here ? keeping them here gives us finer granularity on sub*/roles based on collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need that granularity right now - the identifiers are the same and based on load balancers, not the parent collection. If that changes they can always be broken out again - but I saw an opportunity to consolidate and simplify here.

Copy link
Member

Choose a reason for hiding this comment

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

ok, so I take it the portion of the PR fixing the bug was adding the subresource_actions get/read role ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abellotti yep, I added the refactoring as a separate commit last in 91535c5

:load_balancers_subcollection_actions:
:get:
- :name: show
:identifier: load_balancer_show
:provision_dialogs:
:description: Provisioning Dialogs
:identifier: miq_ae_customization_explorer
Expand Down
20 changes: 18 additions & 2 deletions spec/requests/api/instances_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ def update_raw_power_state(state, *instances)
end

it 'queries all load balancers on an instance' do
api_basic_authorize subcollection_action_identifier(:instances, :load_balancers, :show, :get)
api_basic_authorize subcollection_action_identifier(:instances, :load_balancers, :read, :get)
expected = {
'name' => 'load_balancers',
'resources' => [
Expand All @@ -469,12 +469,28 @@ def update_raw_power_state(state, *instances)
expect(response.parsed_body).to include(expected)
end

it "will not show an instance's load balancers without the appropriate role" do
api_basic_authorize

run_get("#{instances_url(@vm.id)}/load_balancers")

expect(response).to have_http_status(:forbidden)
end

it 'queries a single load balancer on an instance' do
api_basic_authorize subcollection_action_identifier(:instances, :load_balancers, :show, :get)
api_basic_authorize subcollection_action_identifier(:instances, :load_balancers, :read, :get)
run_get("#{instances_url(@vm.id)}/load_balancers/#{@load_balancer.id}")

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include('id' => @load_balancer.id)
end

it "will not show an instance's load balancer without the appropriate role" do
api_basic_authorize

run_get("#{instances_url(@vm.id)}/load_balancers/#{@load_balancer.id}")

expect(response).to have_http_status(:forbidden)
end
end
end
20 changes: 18 additions & 2 deletions spec/requests/api/providers_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,7 +832,7 @@ def gen_import_request
end

it 'queries all load balancers' do
api_basic_authorize subcollection_action_identifier(:providers, :load_balancers, :show, :get)
api_basic_authorize subcollection_action_identifier(:providers, :load_balancers, :read, :get)
expected = {
'resources' => [
{ 'href' => a_string_matching("#{providers_url(@provider.id)}/load_balancers/#{@load_balancer.id}") }
Expand All @@ -845,14 +845,30 @@ def gen_import_request
expect(response.parsed_body).to include(expected)
end

it "will not show a provider's load balancers without the appropriate role" do
api_basic_authorize

run_get("#{providers_url(@provider.id)}/load_balancers")

expect(response).to have_http_status(:forbidden)
end

it 'queries a single load balancer' do
api_basic_authorize subcollection_action_identifier(:providers, :load_balancers, :show, :get)
api_basic_authorize subcollection_action_identifier(:providers, :load_balancers, :read, :get)

run_get("#{providers_url(@provider.id)}/load_balancers/#{@load_balancer.id}")

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to include('id' => @load_balancer.id)
end

it "will not show a provider's load balancer without the appropriate role" do
api_basic_authorize

run_get("#{providers_url(@provider.id)}/load_balancers/#{@load_balancer.id}")

expect(response).to have_http_status(:forbidden)
end
end

describe 'edit custom_attributes on providers' do
Expand Down