Tighten the semantics of POST /v3/service_plans/:guid/visibility

- 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.

Author: philippthun

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

app/controllers/v3/service_plan_visibility_controller.rb

@@ -59,15 +59,15 @@ 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)
 
     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

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
  |

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`

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
 

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

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