diff --git a/app/fetchers/base_service_list_fetcher.rb b/app/fetchers/base_service_list_fetcher.rb index 2a8c795bc32..72b7d1ad1de 100644 --- a/app/fetchers/base_service_list_fetcher.rb +++ b/app/fetchers/base_service_list_fetcher.rb @@ -63,8 +63,7 @@ def fetch(klass, message, omniscient: false, readable_orgs_query: nil, readable_ end.from_self(alias: klass.table_name) end - # Select DISTINCT entries and eager load associations. - dataset.distinct.eager(eager_loaded_associations) + dataset.eager(eager_loaded_associations) end def readable_by_plan_org(dataset, readable_orgs_query) diff --git a/app/models/services/service_plan_visibility.rb b/app/models/services/service_plan_visibility.rb index 78f3f88e131..ec7d399f510 100644 --- a/app/models/services/service_plan_visibility.rb +++ b/app/models/services/service_plan_visibility.rb @@ -11,6 +11,7 @@ def validate validates_presence :organization validates_unique %i[organization_id service_plan_id] validate_plan_is_not_private + validate_plan_is_not_public end def self.visible_private_plan_ids_for_user(user) @@ -28,5 +29,11 @@ def validate_plan_is_not_private errors.add(:service_plan, 'is from a private broker') end + + def validate_plan_is_not_public + return unless service_plan&.public? + + errors.add(:service_plan, 'is publicly available') + end end end diff --git a/spec/support/fakes/blueprints.rb b/spec/support/fakes/blueprints.rb index 7244bf6cb3f..5b0fba1c7a8 100644 --- a/spec/support/fakes/blueprints.rb +++ b/spec/support/fakes/blueprints.rb @@ -566,7 +566,7 @@ module VCAP::CloudController end ServicePlanVisibility.blueprint do - service_plan { ServicePlan.make } + service_plan { ServicePlan.make(public: false) } organization { Organization.make } end diff --git a/spec/unit/access/service_plan_visibility_access_spec.rb b/spec/unit/access/service_plan_visibility_access_spec.rb index 10b9345276d..83822d8ae0a 100644 --- a/spec/unit/access/service_plan_visibility_access_spec.rb +++ b/spec/unit/access/service_plan_visibility_access_spec.rb @@ -7,7 +7,7 @@ module VCAP::CloudController let(:user) { VCAP::CloudController::User.make } let(:service) { VCAP::CloudController::Service.make } let(:org) { VCAP::CloudController::Organization.make } - let(:service_plan) { VCAP::CloudController::ServicePlan.make(service:) } + let(:service_plan) { VCAP::CloudController::ServicePlan.make(service: service, public: false) } let(:object) { VCAP::CloudController::ServicePlanVisibility.make(organization: org, service_plan: service_plan) } diff --git a/spec/unit/models/services/service_plan_spec.rb b/spec/unit/models/services/service_plan_spec.rb index a0c5b1b79ff..6a0310cffef 100644 --- a/spec/unit/models/services/service_plan_spec.rb +++ b/spec/unit/models/services/service_plan_spec.rb @@ -7,7 +7,7 @@ module VCAP::CloudController describe 'Associations' do it { is_expected.to have_associated :service } it { is_expected.to have_associated :service_instances, class: ManagedServiceInstance } - it { is_expected.to have_associated :service_plan_visibilities } + it { is_expected.to have_associated :service_plan_visibilities, { test_instance: ServicePlan.make(public: false) } } it { is_expected.to have_associated :labels, class: ServicePlanLabelModel } it { is_expected.to have_associated :annotations, class: ServicePlanAnnotationModel } end @@ -153,7 +153,7 @@ module VCAP::CloudController end describe '#destroy' do - let(:service_plan) { ServicePlan.make } + let(:service_plan) { ServicePlan.make(public: false) } it 'destroys associated dependencies' do service_plan_visibility = ServicePlanVisibility.make(service_plan:) diff --git a/spec/unit/models/services/service_plan_visibility_spec.rb b/spec/unit/models/services/service_plan_visibility_spec.rb index 4a6637cac73..f8708027578 100644 --- a/spec/unit/models/services/service_plan_visibility_spec.rb +++ b/spec/unit/models/services/service_plan_visibility_spec.rb @@ -20,13 +20,26 @@ module VCAP::CloudController space = Space.make(organization:) private_broker = ServiceBroker.make(space:) service = Service.make service_broker: private_broker, active: true - plan = ServicePlan.make(service:) + plan = ServicePlan.make(service: service, public: false) expect do ServicePlanVisibility.create service_plan: plan, organization: organization end.to raise_error Sequel::ValidationFailed, 'service_plan is from a private broker' end end + + context 'when the service plan visibility is for a public plan' do + it 'returns a validation error' do + organization = Organization.make + private_broker = ServiceBroker.make + service = Service.make service_broker: private_broker, active: true + plan = ServicePlan.make(service: service, public: true) + + expect do + ServicePlanVisibility.create service_plan: plan, organization: organization + end.to raise_error Sequel::ValidationFailed, 'service_plan is publicly available' + end + end end describe 'Serialization' do diff --git a/spec/unit/models/services/service_spec.rb b/spec/unit/models/services/service_spec.rb index 6d81a0152da..0bfff917c82 100644 --- a/spec/unit/models/services/service_spec.rb +++ b/spec/unit/models/services/service_spec.rb @@ -211,12 +211,12 @@ def records(user) describe '#purge' do let!(:event_repository) { double(Repositories::ServiceUsageEventRepository) } - let!(:service_plan) { ServicePlan.make(service:) } + let!(:service_plan) { ServicePlan.make(service: service, public: false) } let!(:service_plan_visibility) { ServicePlanVisibility.make(service_plan:) } let!(:service_instance) { ManagedServiceInstance.make(service_plan:) } let!(:service_binding) { ServiceBinding.make(service_instance:) } - let!(:service_plan_2) { ServicePlan.make(service:) } + let!(:service_plan_2) { ServicePlan.make(service: service, public: false) } let!(:service_plan_visibility_2) { ServicePlanVisibility.make(service_plan: service_plan_2) } let!(:service_instance_2) { ManagedServiceInstance.make(service_plan: service_plan_2) } let!(:service_binding_2) { ServiceBinding.make(service_instance: service_instance_2) }