From fc1026873ccdc74f49afb62cfb952f8994744a9d Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Mon, 11 Sep 2023 09:29:38 +0200 Subject: [PATCH] Prevent deletion of service keys by space supporters --- .../service_credential_bindings_controller.rb | 6 +- .../_delete.md.erb | 6 +- .../service_credential_bindings_spec.rb | 106 ++++++++++++------ 3 files changed, 80 insertions(+), 38 deletions(-) diff --git a/app/controllers/v3/service_credential_bindings_controller.rb b/app/controllers/v3/service_credential_bindings_controller.rb index 1d50fe01b88..3c4d59edde1 100644 --- a/app/controllers/v3/service_credential_bindings_controller.rb +++ b/app/controllers/v3/service_credential_bindings_controller.rb @@ -103,7 +103,11 @@ def update def destroy not_found! unless service_credential_binding.present? - unauthorized! unless can_bind_in_active_space?(binding_space) + if service_credential_binding.is_a?(ServiceKey) + unauthorized! unless can_write_to_active_space?(binding_space) + else + unauthorized! unless can_bind_in_active_space?(binding_space) + end suspended! unless is_space_active?(binding_space) type = service_credential_binding.is_a?(ServiceKey) ? :key : :credential diff --git a/docs/v3/source/includes/resources/service_credential_bindings/_delete.md.erb b/docs/v3/source/includes/resources/service_credential_bindings/_delete.md.erb index 0898ddccc1b..2f29162ca1a 100644 --- a/docs/v3/source/includes/resources/service_credential_bindings/_delete.md.erb +++ b/docs/v3/source/includes/resources/service_credential_bindings/_delete.md.erb @@ -36,8 +36,8 @@ respond synchronously to the delete request. `DELETE /v3/service_credential_bindings/:guid` #### Permitted roles - | ---- | +Role | Notes +--- | --- Admin | Space Developer | -Space Supporter | +Space Supporter | Only allowed to delete bindings of type `app`. diff --git a/spec/request/service_credential_bindings_spec.rb b/spec/request/service_credential_bindings_spec.rb index e9ad2e6acb9..72074d55e35 100644 --- a/spec/request/service_credential_bindings_spec.rb +++ b/spec/request/service_credential_bindings_spec.rb @@ -2060,9 +2060,10 @@ def check_filtered_bindings(*bindings) } } let(:binding) { VCAP::CloudController::ServiceBinding.make(**binding_details) } - let(:service_instance) { VCAP::CloudController::UserProvidedServiceInstance.make(**service_instance_details) } context 'user provided services' do + let(:service_instance) { VCAP::CloudController::UserProvidedServiceInstance.make(**service_instance_details) } + context 'permissions' do let(:db_check) { lambda { @@ -2124,50 +2125,49 @@ def check_filtered_bindings(*bindings) accepts_incomplete: true, } end - - context 'permissions' do - let(:db_check) { - lambda { - expect(last_response.headers['Location']).to match(%r(http.+/v3/jobs/[a-fA-F0-9-]+)) - } + let(:db_check) { + lambda { + expect(last_response.headers['Location']).to match(%r(http.+/v3/jobs/[a-fA-F0-9-]+)) } + } - context 'users in the originating service instance space' do - it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS do - let(:expected_codes_and_responses) do - responses_for_space_restricted_async_delete_endpoint( - permitted_roles: SpaceRestrictedResponseGenerators.default_write_permitted_roles + ['space_supporter'] - ) + context 'app binding' do + context 'permissions' do + context 'users in the originating service instance space' do + it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS do + let(:expected_codes_and_responses) do + responses_for_space_restricted_async_delete_endpoint( + permitted_roles: SpaceRestrictedResponseGenerators.default_write_permitted_roles + ['space_supporter'] + ) + end end - end - it_behaves_like 'permissions for delete endpoint when organization is suspended', 202, - SpaceRestrictedResponseGenerators.default_suspended_roles + ['space_supporter'] - end + it_behaves_like 'permissions for delete endpoint when organization is suspended', 202, + SpaceRestrictedResponseGenerators.default_suspended_roles + ['space_supporter'] + end - context 'users in the space where the SI has been shared to' do - let(:original_org) { VCAP::CloudController::Organization.make } - let(:original_space) { VCAP::CloudController::Space.make(organization: original_org) } - let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: original_space) } + context 'users in the space where the SI has been shared to' do + let(:original_org) { VCAP::CloudController::Organization.make } + let(:original_space) { VCAP::CloudController::Space.make(organization: original_org) } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: original_space) } - before do - service_instance.add_shared_space(space) - end + before do + service_instance.add_shared_space(space) + end - it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS do - let(:expected_codes_and_responses) do - responses_for_space_restricted_async_delete_endpoint( - permitted_roles: SpaceRestrictedResponseGenerators.default_write_permitted_roles + ['space_supporter'] - ) + it_behaves_like 'permissions for delete endpoint', ['space_supporter'] do + let(:expected_codes_and_responses) do + responses_for_space_restricted_async_delete_endpoint( + permitted_roles: SpaceRestrictedResponseGenerators.default_write_permitted_roles + ['space_supporter'] + ) + end end - end - it_behaves_like 'permissions for delete endpoint when organization is suspended', 202, - SpaceRestrictedResponseGenerators.default_suspended_roles + ['space_supporter'] + it_behaves_like 'permissions for delete endpoint when organization is suspended', 202, + SpaceRestrictedResponseGenerators.default_suspended_roles + ['space_supporter'] + end end - end - context 'app binding' do it_behaves_like 'service credential binding delete endpoint', 'service_binding', VCAP::CloudController::ServiceBinding, @@ -2178,6 +2178,44 @@ def check_filtered_bindings(*bindings) context 'key bindings' do let(:bound_app) { nil } let(:binding) { VCAP::CloudController::ServiceKey.make(service_instance: service_instance) } + + context 'permissions' do + context 'users in the originating service instance space' do + it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS do + let(:expected_codes_and_responses) { responses_for_space_restricted_async_delete_endpoint } + end + + it_behaves_like 'permissions for delete endpoint when organization is suspended', 202 + end + + context 'users in the space where the SI has been shared to' do + let(:original_org) { VCAP::CloudController::Organization.make } + let(:original_space) { VCAP::CloudController::Space.make(organization: original_org) } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(space: original_space) } + + before do + service_instance.add_shared_space(space) + end + + let(:expected_codes_and_responses) do + h = Hash.new(code: 404) + h['admin'] = { code: 202 } + h['admin_read_only'] = h['global_auditor'] = { code: 403 } + h + end + + it_behaves_like 'permissions for delete endpoint', ALL_PERMISSIONS + + context 'when organization is suspended' do + before do + org.update(status: VCAP::CloudController::Organization::SUSPENDED) + end + + it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS + end + end + end + it_behaves_like 'service credential binding delete endpoint', 'service_key', VCAP::CloudController::ServiceKey,