From 84649866048ce97748f7ec20a7adb59d4d6c4470 Mon Sep 17 00:00:00 2001 From: Denise Yu Date: Wed, 15 Nov 2017 16:16:00 +0000 Subject: [PATCH 1/2] Prevent sharing into a space where access to a service is disabled - Return a 422 when the service access is not enabled in the target space [#150801606] Signed-off-by: Alex Blease --- .../v3/service_instances_controller.rb | 9 +++++++ .../v3/service_instance_controller_spec.rb | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index a3dd03dc0e9..1cfe81e0546 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -39,6 +39,7 @@ def share_service_instance spaces = Space.where(guid: message.guids) check_spaces_exist_and_are_readable!(message.guids, spaces) check_spaces_are_writeable!(spaces) + check_spaces_have_service_access_enabled!(spaces, service_instance) share = ServiceInstanceShare.new share.create(service_instance, spaces, user_audit_info) @@ -70,6 +71,14 @@ def unshare_service_instance private + def check_spaces_have_service_access_enabled!(spaces, service_instance) + spaces.each do |space| + visible_plans = ServicePlan.organization_visible(space.organization) + error_msg = "Access to service #{service_instance.service.label} and plan #{service_instance.service_plan.name} is not enabled in #{space.organization.name}/#{space.name}" + unprocessable!(error_msg) unless visible_plans.include?(service_instance.service_plan) + end + end + def check_spaces_are_writeable!(spaces) unwriteable_spaces = spaces.reject do |space| can_write?(space.guid) diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index cb08319882d..521abd7b895 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -198,6 +198,32 @@ end end + context 'when access to the plan is not enabled in the target org' do + let(:target_org) { VCAP::CloudController::Organization.make } + let(:target_space) { VCAP::CloudController::Space.make organization: target_org } + let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: false) } + let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(service_plan: service_plan) } + let(:same_org_target_space) { VCAP::CloudController::Space.make organization: source_space.organization } + + before do + req_body[:data] = [ + { guid: target_space.guid }, + { guid: same_org_target_space.guid } + ] + + VCAP::CloudController::ServicePlanVisibility.make(organization: source_space.organization, service_plan: service_plan) + end + + it 'returns an error to the user' do + post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body + + error_msg = "Access to service #{service_instance.service.label} and plan #{service_plan.name} is not enabled in #{target_org.name}/#{target_space.name}" + expect(response.status).to eq 422 + expect(response.body).to include(error_msg) + expect(service_instance.shared_spaces).to_not include(target_space) + end + end + context 'when multiple target spaces do not exist' do before do req_body[:data] = [ From c39aa0e15768e0143590ae8ffcd81b7d8d001c78 Mon Sep 17 00:00:00 2001 From: Sam Gunaratne Date: Mon, 20 Nov 2017 17:09:54 +0000 Subject: [PATCH 2/2] Move share service instance plan access check into action [#152922808] Signed-off-by: Denise Yu --- app/actions/service_instance_share.rb | 49 ++++++++++++----- .../v3/service_instances_controller.rb | 9 --- .../actions/service_instance_share_spec.rb | 55 +++++++++++++++++++ .../v3/service_instance_controller_spec.rb | 26 --------- 4 files changed, 90 insertions(+), 49 deletions(-) diff --git a/app/actions/service_instance_share.rb b/app/actions/service_instance_share.rb index fae64b13749..86dc7790677 100644 --- a/app/actions/service_instance_share.rb +++ b/app/actions/service_instance_share.rb @@ -3,18 +3,9 @@ module VCAP::CloudController class ServiceInstanceShare def create(service_instance, target_spaces, user_audit_info) - supported_service_type?(service_instance) - service_instance_shareable?(service_instance) - - if target_spaces.include?(service_instance.space) - raise CloudController::Errors::ApiError.new_from_details('InvalidServiceInstanceSharingTargetSpace') - end - - target_spaces.each do |space| - if space.service_instances.map(&:name).include?(service_instance.name) - raise CloudController::Errors::ApiError.new_from_details('SharedServiceInstanceNameTaken', service_instance.name, space.name) - end - end + supported_service_type!(service_instance) + service_instance_shareable!(service_instance) + valid_target_spaces!(service_instance, target_spaces) ServiceInstance.db.transaction do target_spaces.each do |space| @@ -30,7 +21,37 @@ def create(service_instance, target_spaces, user_audit_info) private - def supported_service_type?(service_instance) + def valid_target_spaces!(service_instance, target_spaces) + no_sharing_to_self!(service_instance, target_spaces) + + target_spaces.each do |space| + plan_visibility!(service_instance, space) + name_uniqueness!(service_instance, space) + end + end + + def plan_visibility!(service_instance, space) + visible_plans = ServicePlan.organization_visible(space.organization) + + if !visible_plans.include?(service_instance.service_plan) + error_msg = "Access to service #{service_instance.service.label} and plan #{service_instance.service_plan.name} is not enabled in #{space.organization.name}/#{space.name}" + raise CloudController::Errors::ApiError.new_from_details('UnprocessableEntity', error_msg) + end + end + + def name_uniqueness!(service_instance, space) + if space.service_instances.map(&:name).include?(service_instance.name) + raise CloudController::Errors::ApiError.new_from_details('SharedServiceInstanceNameTaken', service_instance.name, space.name) + end + end + + def no_sharing_to_self!(service_instance, spaces) + if spaces.include?(service_instance.space) + raise CloudController::Errors::ApiError.new_from_details('InvalidServiceInstanceSharingTargetSpace') + end + end + + def supported_service_type!(service_instance) if service_instance.route_service? raise CloudController::Errors::ApiError.new_from_details('RouteServiceInstanceSharingNotSupported') end @@ -40,7 +61,7 @@ def supported_service_type?(service_instance) end end - def service_instance_shareable?(service_instance) + def service_instance_shareable!(service_instance) unless service_instance.shareable? raise CloudController::Errors::ApiError.new_from_details('ServiceShareIsDisabled', service_instance.service.label) end diff --git a/app/controllers/v3/service_instances_controller.rb b/app/controllers/v3/service_instances_controller.rb index 1cfe81e0546..a3dd03dc0e9 100644 --- a/app/controllers/v3/service_instances_controller.rb +++ b/app/controllers/v3/service_instances_controller.rb @@ -39,7 +39,6 @@ def share_service_instance spaces = Space.where(guid: message.guids) check_spaces_exist_and_are_readable!(message.guids, spaces) check_spaces_are_writeable!(spaces) - check_spaces_have_service_access_enabled!(spaces, service_instance) share = ServiceInstanceShare.new share.create(service_instance, spaces, user_audit_info) @@ -71,14 +70,6 @@ def unshare_service_instance private - def check_spaces_have_service_access_enabled!(spaces, service_instance) - spaces.each do |space| - visible_plans = ServicePlan.organization_visible(space.organization) - error_msg = "Access to service #{service_instance.service.label} and plan #{service_instance.service_plan.name} is not enabled in #{space.organization.name}/#{space.name}" - unprocessable!(error_msg) unless visible_plans.include?(service_instance.service_plan) - end - end - def check_spaces_are_writeable!(spaces) unwriteable_spaces = spaces.reject do |space| can_write?(space.guid) diff --git a/spec/unit/actions/service_instance_share_spec.rb b/spec/unit/actions/service_instance_share_spec.rb index f1db26c3122..f6bd2f7823f 100644 --- a/spec/unit/actions/service_instance_share_spec.rb +++ b/spec/unit/actions/service_instance_share_spec.rb @@ -147,6 +147,61 @@ module VCAP::CloudController end end end + + context 'when the service plan is private' do + let(:service_plan) { ServicePlan.make(public: false) } + let(:service_instance) { ManagedServiceInstance.make(service_plan: service_plan) } + + it 'raises an api error' do + error_msg = "Access to service #{service_instance.service.label} and plan #{service_instance.service_plan.name} is not " \ + "enabled in #{target_space1.organization.name}/#{target_space1.name}" + expect { + service_instance_share.create(service_instance, [target_space1], user_audit_info) + }.to raise_error(CloudController::Errors::ApiError, error_msg) + end + + context 'and access has been enabled for the target org' do + before do + ServicePlanVisibility.make(organization: target_space1.organization, service_plan: service_instance.service_plan) + end + + it 'creates the share' do + shared_instance = service_instance_share.create(service_instance, [target_space1], user_audit_info) + expect(shared_instance.shared_spaces.length).to eq 1 + end + end + + context 'and when the source org has service plan access disabled but the target org has service plan access enabled' do + let(:source_org) { Organization.make } + let(:space) { Space.make(organization: source_org) } + let(:service_instance) { ManagedServiceInstance.make(service_plan: service_plan, space: space) } + + before do + ServicePlanVisibility.make(organization: target_space1.organization, service_plan: service_instance.service_plan) + end + + it 'creates the share' do + shared_instance = service_instance_share.create(service_instance, [target_space1], user_audit_info) + expect(shared_instance.shared_spaces.length).to eq 1 + end + end + + context 'and when source org has had service plan access enabled and the target org has service plan access enabled' do + let(:source_org) { Organization.make } + let(:space) { Space.make(organization: source_org) } + let(:service_instance) { ManagedServiceInstance.make(service_plan: service_plan, space: space) } + + before do + ServicePlanVisibility.make(organization: target_space1.organization, service_plan: service_instance.service_plan) + ServicePlanVisibility.make(organization: source_org, service_plan: service_instance.service_plan) + end + + it 'creates the share' do + shared_instance = service_instance_share.create(service_instance, [target_space1], user_audit_info) + expect(shared_instance.shared_spaces.length).to eq 1 + end + end + end end end end diff --git a/spec/unit/controllers/v3/service_instance_controller_spec.rb b/spec/unit/controllers/v3/service_instance_controller_spec.rb index 521abd7b895..cb08319882d 100644 --- a/spec/unit/controllers/v3/service_instance_controller_spec.rb +++ b/spec/unit/controllers/v3/service_instance_controller_spec.rb @@ -198,32 +198,6 @@ end end - context 'when access to the plan is not enabled in the target org' do - let(:target_org) { VCAP::CloudController::Organization.make } - let(:target_space) { VCAP::CloudController::Space.make organization: target_org } - let(:service_plan) { VCAP::CloudController::ServicePlan.make(public: false) } - let(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make(service_plan: service_plan) } - let(:same_org_target_space) { VCAP::CloudController::Space.make organization: source_space.organization } - - before do - req_body[:data] = [ - { guid: target_space.guid }, - { guid: same_org_target_space.guid } - ] - - VCAP::CloudController::ServicePlanVisibility.make(organization: source_space.organization, service_plan: service_plan) - end - - it 'returns an error to the user' do - post :share_service_instance, service_instance_guid: service_instance.guid, body: req_body - - error_msg = "Access to service #{service_instance.service.label} and plan #{service_plan.name} is not enabled in #{target_org.name}/#{target_space.name}" - expect(response.status).to eq 422 - expect(response.body).to include(error_msg) - expect(service_instance.shared_spaces).to_not include(target_space) - end - end - context 'when multiple target spaces do not exist' do before do req_body[:data] = [