Prevent deletion of service keys by space supporters

cloudfoundry · Sep 11, 2023 · fc10268 · fc10268
1 parent 56a9486
commit fc10268
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 38 deletions.
diff --git 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
@@ -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
@@ -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,