From 7dd0212b8c5f4c84772c374bbc55c5345616583c Mon Sep 17 00:00:00 2001 From: Philipp Thun Date: Fri, 10 Oct 2025 10:56:57 +0200 Subject: [PATCH] Tighten the semantics of POST /v3/service_plans/:guid/visibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Allow POST only when type = organization. Otherwise fail with 422 Unprocessable Entity. - Ensure the target plan already has visibility type organization. If the plan’s visibility type is not organization, the request fails with 422 Unprocessable Entity. --- .../v3/service_plan_visibility_update.rb | 13 ++++- .../v3/service_plan_visibility_controller.rb | 4 +- .../service_plan_visibility/_apply.md.erb | 6 +-- .../service_plan_visibility/_update.md.erb | 2 +- spec/request/service_plan_visibility_spec.rb | 35 +++---------- spec/support/lifecycle_tests_helpers.rb | 2 +- .../v3/service_plan_visibility_update_spec.rb | 52 ++++++++++++++++++- 7 files changed, 74 insertions(+), 40 deletions(-) diff --git a/app/actions/v3/service_plan_visibility_update.rb b/app/actions/v3/service_plan_visibility_update.rb index 5d3b94a8fd2..8038be9698b 100644 --- a/app/actions/v3/service_plan_visibility_update.rb +++ b/app/actions/v3/service_plan_visibility_update.rb @@ -11,7 +11,12 @@ def update(service_plan, message, append_organizations: false) type = message.type requested_org_guids = message.organizations&.pluck(:guid) || [] - unprocessable!("cannot update plans with visibility type 'space'") if space?(service_plan) + unprocessable!("cannot update plans with visibility type 'space'") if visibility_type_space?(service_plan) + + if append_organizations # i.e. POST request + unprocessable!("type must be 'organization'") unless org?(type) + unprocessable!("can only append organizations to plans with visibility type 'organization'") unless visibility_type_org?(service_plan) + end service_plan.db.transaction do service_plan.lock! @@ -69,10 +74,14 @@ def unprocessable!(message) raise UnprocessableRequest.new(message) end - def space?(service_plan) + def visibility_type_space?(service_plan) service_plan.visibility_type == VCAP::CloudController::ServicePlanVisibilityTypes::SPACE end + def visibility_type_org?(service_plan) + service_plan.visibility_type == VCAP::CloudController::ServicePlanVisibilityTypes::ORGANIZATION + end + def org?(type) type == VCAP::CloudController::ServicePlanVisibilityTypes::ORGANIZATION end diff --git a/app/controllers/v3/service_plan_visibility_controller.rb b/app/controllers/v3/service_plan_visibility_controller.rb index 940f72ad194..8a077bd0ad8 100644 --- a/app/controllers/v3/service_plan_visibility_controller.rb +++ b/app/controllers/v3/service_plan_visibility_controller.rb @@ -59,7 +59,7 @@ def event_repository VCAP::CloudController::Repositories::ServiceEventRepository::WithUserActor.new(user_audit_info) end - def update_visibility(opts={}) + def update_visibility(append_organizations: false) service_plan = ServicePlanFetcher.fetch(hashed_params[:guid]) service_plan_not_found! unless service_plan.present? && visible_to_current_user?(plan: service_plan) unauthorized! unless current_user_can_write?(service_plan) @@ -67,7 +67,7 @@ def update_visibility(opts={}) message = ServicePlanVisibilityUpdateMessage.new(hashed_params[:body]) bad_request!(message.errors.full_messages) unless message.valid? - updated_service_plan = V3::ServicePlanVisibilityUpdate.new.update(service_plan, message, **opts) + updated_service_plan = V3::ServicePlanVisibilityUpdate.new.update(service_plan, message, append_organizations:) event_repository.record_service_plan_update_visibility_event(service_plan, message.audit_hash) updated_service_plan rescue V3::ServicePlanVisibilityUpdate::UnprocessableRequest => e diff --git a/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb b/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb index f791e816d98..bafd322bb8f 100644 --- a/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb +++ b/docs/v3/source/includes/resources/service_plan_visibility/_apply.md.erb @@ -28,7 +28,7 @@ Content-Type: application/json <%= yield_content :service_plan_visibility_response %> ``` -This endpoint applies a service plan visibility. It behaves similar to the [PATCH service plan visibility endpoint](#update-a-service-plan-visibility) but this endpoint will append to the existing list of organizations when the service plan is `organization` visible. +This endpoint appends to the existing list of organizations. See [PATCH service plan visibility endpoint](#update-a-service-plan-visibility) to replace the existing list or change the visibility `type`. #### Definition `POST /v3/service_plans/:guid/visibility` @@ -37,8 +37,8 @@ This endpoint applies a service plan visibility. It behaves similar to the [PATC Name | Type | Description ---- | ---- | ----------- -**type** | _string_ | Denotes the visibility of the plan; can be `public`, `admin`, `organization`, see [_list of visibility types_](#list-of-visibility-types) -**organizations** | _array of objects_ | Desired list of organizations GUIDs where the plan will be accessible; required if `type` is `organization` +**type** | _string_ | Denotes the visibility of the plan; must be `organization`, see [_list of visibility types_](#list-of-visibility-types) +**organizations** | _array of objects_ | Desired list of organizations GUIDs where the plan will be accessible; required. #### Permitted roles | diff --git a/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb b/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb index de7a9747a19..59f3fb1487e 100644 --- a/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb +++ b/docs/v3/source/includes/resources/service_plan_visibility/_update.md.erb @@ -28,7 +28,7 @@ Content-Type: application/json <%= yield_content :new_org_service_plan_visibility %> ``` -This endpoint updates a service plan visibility. It behaves similar to the [POST service plan visibility endpoint](#apply-a-service-plan-visibility) but this endpoint will replace the existing list of organizations when the service plan is `organization` visible. +This endpoint updates a service plan visibility. When `type` is `organization`, it will **replace** the existing list of organizations. See [POST service plan visibility endpoint](#apply-a-service-plan-visibility) to append to the existing list. #### Definition `PATCH /v3/service_plans/:guid/visibility` diff --git a/spec/request/service_plan_visibility_spec.rb b/spec/request/service_plan_visibility_spec.rb index 8ebd9bc4d26..faa11a93fac 100644 --- a/spec/request/service_plan_visibility_spec.rb +++ b/spec/request/service_plan_visibility_spec.rb @@ -454,21 +454,11 @@ let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: true) } let(:body) { { type: 'organization', organizations: [{ guid: org.guid }] } } - it 'updates the visibility type AND add the orgs' do + it 'returns a 422 bad request' do post api_url, body.to_json, admin_headers - expect(parsed_response['type']).to eq 'organization' - expect(parsed_response).not_to have_key('organizations') - end - - it 'creates an audit event' do - post api_url, body.to_json, admin_headers - event = VCAP::CloudController::Event.find(type: 'audit.service_plan_visibility.update') - expect(event).to be - expect(event.actee).to eq(service_plan.guid) - expect(event.data).to include({ - 'request' => body.with_indifferent_access - }) + expect(last_response).to have_status_code(422) + expect(parsed_response['errors'].first['detail']).to include("can only append organizations to plans with visibility type 'organization'") end end @@ -519,24 +509,11 @@ context 'when request type is not "organization"' do let(:body) { { type: 'public' } } - it 'behaves like a PATCH' do + it 'returns a 422 bad request' do post api_url, body.to_json, admin_headers - expect(last_response).to have_status_code(200) - get api_url, {}, admin_headers - expect(parsed_response).to eq({ 'type' => 'public' }) - visibilities = VCAP::CloudController::ServicePlanVisibility.where(service_plan:).all - expect(visibilities).to be_empty - end - - it 'creates an audit event' do - post api_url, body.to_json, admin_headers - event = VCAP::CloudController::Event.find(type: 'audit.service_plan_visibility.update') - expect(event).to be - expect(event.actee).to eq(service_plan.guid) - expect(event.data).to include({ - 'request' => body.with_indifferent_access - }) + expect(last_response).to have_status_code(422) + expect(parsed_response['errors'].first['detail']).to include("type must be 'organization'") end end diff --git a/spec/support/lifecycle_tests_helpers.rb b/spec/support/lifecycle_tests_helpers.rb index a3d3056b636..98b17a12526 100644 --- a/spec/support/lifecycle_tests_helpers.rb +++ b/spec/support/lifecycle_tests_helpers.rb @@ -88,7 +88,7 @@ def catalog end def make_plan_visible(plan_guid) - post "v3/service_plans/#{plan_guid}/visibility", { type: 'public' }.to_json, admin_headers + patch "v3/service_plans/#{plan_guid}/visibility", { type: 'public' }.to_json, admin_headers expect(last_response).to have_status_code(200) get "v3/service_plans/#{plan_guid}/visibility", nil, admin_headers diff --git a/spec/unit/actions/v3/service_plan_visibility_update_spec.rb b/spec/unit/actions/v3/service_plan_visibility_update_spec.rb index 4bf02fa5df8..a4475ea8343 100644 --- a/spec/unit/actions/v3/service_plan_visibility_update_spec.rb +++ b/spec/unit/actions/v3/service_plan_visibility_update_spec.rb @@ -19,9 +19,17 @@ module V3 updated_plan = subject.update(service_plan, message) expect(updated_plan.reload.visibility_type).to eq 'public' end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") + end + end end - context 'and its being updated do "organization"' do + context 'and its being updated to "organization"' do let(:org_guid) { Organization.make.guid } let(:other_org_guid) { Organization.make.guid } let(:params) do @@ -38,6 +46,14 @@ module V3 expect(visible_org_guids).to contain_exactly(org_guid, other_org_guid) end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "can only append organizations to plans with visibility type 'organization'") + end + end end end @@ -51,9 +67,17 @@ module V3 updated_plan = subject.update(service_plan, message) expect(updated_plan.reload.visibility_type).to eq 'admin' end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") + end + end end - context 'and its being updated do "organization"' do + context 'and its being updated to "organization"' do let(:org_guid) { Organization.make.guid } let(:other_org_guid) { Organization.make.guid } let(:params) do @@ -70,6 +94,14 @@ module V3 expect(visible_org_guids).to contain_exactly(org_guid, other_org_guid) end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "can only append organizations to plans with visibility type 'organization'") + end + end end end @@ -136,6 +168,14 @@ module V3 expect(updated_plan.service_plan_visibilities).to be_empty expect(ServicePlanVisibility.where(service_plan:).all).to be_empty end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") + end + end end context 'and its being updated to "public"' do @@ -147,6 +187,14 @@ module V3 expect(updated_plan.service_plan_visibilities).to be_empty expect(ServicePlanVisibility.where(service_plan:).all).to be_empty end + + context "when 'append_orgs' is set to true" do + it 'raises an error' do + expect do + subject.update(service_plan, message, append_organizations: true) + end.to raise_error(ServicePlanVisibilityUpdate::UnprocessableRequest, "type must be 'organization'") + end + end end end