diff --git a/app/jobs/v3/services/update_broker_job.rb b/app/jobs/v3/services/update_broker_job.rb index ee96ec40855..b2edf381ffb 100644 --- a/app/jobs/v3/services/update_broker_job.rb +++ b/app/jobs/v3/services/update_broker_job.rb @@ -53,6 +53,8 @@ def initialize(update_request_guid, previous_broker_state, user_audit_info:) end def perform + raise CloudController::Errors::V3::ApiError.new_from_details('ServiceBrokerGone') if broker.nil? + ServiceBroker.db.transaction do broker.update(update_params) @@ -64,11 +66,7 @@ def perform @warnings rescue => e - begin - broker.update(state: previous_broker_state) - rescue - raise CloudController::Errors::V3::ApiError.new_from_details('ServiceBrokerGone') if broker.nil? - end + ServiceBroker.where(id: update_request.service_broker_id).update(state: previous_broker_state) if e.is_a?(Sequel::ValidationFailed) raise V3::ServiceBrokerUpdate::InvalidServiceBroker.new(e.message) diff --git a/app/messages/basic_credentials_message.rb b/app/messages/basic_credentials_message.rb index b0da9458b3d..1f6f532b63e 100644 --- a/app/messages/basic_credentials_message.rb +++ b/app/messages/basic_credentials_message.rb @@ -5,7 +5,7 @@ class BasicCredentialsMessage < BaseMessage validates_with NoAdditionalKeysValidator - validates :username, string: true - validates :password, string: true + validates :username, string: true, presence: true + validates :password, string: true, presence: true end end diff --git a/spec/unit/jobs/v3/services/update_broker_job_spec.rb b/spec/unit/jobs/v3/services/update_broker_job_spec.rb index c66e9ec9731..01971d0ef95 100644 --- a/spec/unit/jobs/v3/services/update_broker_job_spec.rb +++ b/spec/unit/jobs/v3/services/update_broker_job_spec.rb @@ -182,8 +182,8 @@ module V3 describe 'when there is a model level validation error' do before do allow_any_instance_of(ServiceBroker).to receive(:validate) do |record| - # Make setting the state to 'AVAILABLE' fail, but nothing else - if record.state == 'AVAILABLE' + # Make setting the name to 'new-name' fail, but nothing else + if record.name == 'new-name' record.errors.add(:something, 'is not right') end end @@ -201,6 +201,31 @@ module V3 end end + describe 'when there is a model level validation error i.e. blank credentials' do + let(:update_broker_request) do + ServiceBrokerUpdateRequest.create( + authentication: '{"credentials":{"username":"","password":""}}', + service_broker_id: broker.id + ) + end + + it 'fails to update and raises InvalidServiceBroker' do + expect { + job.perform + }.to raise_error(V3::ServiceBrokerUpdate::InvalidServiceBroker) { |error| + expect(error.message).to include('auth_username presence') + expect(error.message).to include('auth_password presence') + } + + broker.reload + expect(broker.name).to eq('test-broker') + expect(broker.broker_url).to eq('http://example.org/broker-url') + expect(broker.auth_username).to eq('username') + expect(broker.auth_password).to eq('password') + expect(broker.state).to eq(ServiceBrokerStateEnum::AVAILABLE) + end + end + context 'when catalog returned by broker is invalid' do before { setup_broker_with_invalid_catalog } diff --git a/spec/unit/messages/service_broker_create_message_spec.rb b/spec/unit/messages/service_broker_create_message_spec.rb index 9e06d959290..24132e4cef1 100644 --- a/spec/unit/messages/service_broker_create_message_spec.rb +++ b/spec/unit/messages/service_broker_create_message_spec.rb @@ -119,18 +119,37 @@ module VCAP::CloudController context 'when authentication is not valid' do let(:request_body) do + valid_body[:authentication] = valid_body[:authentication].except(:type) valid_body end - before do - allow_any_instance_of(VCAP::CloudController::Validators::AuthenticationValidator).to receive(:validate) do |_, record| - record.errors.add(:authentication, 'this authentication is not valid!') - end + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include('authentication.type must be one of ["basic"]') + end + end + + context 'when username is an empty string' do + let(:request_body) do + valid_body[:authentication][:credentials][:username] = '' + valid_body + end + + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include(/Username can't be blank/) + end + end + + context 'when password is an empty string' do + let(:request_body) do + valid_body[:authentication][:credentials][:password] = '' + valid_body end it 'is not valid' do expect(message).not_to be_valid - expect(message.errors_on(:authentication)).to include('this authentication is not valid!') + expect(message.errors_on(:authentication)).to include(/Password can't be blank/) end end end diff --git a/spec/unit/messages/service_broker_update_message_spec.rb b/spec/unit/messages/service_broker_update_message_spec.rb index e00a08c77ab..efafa9c4265 100644 --- a/spec/unit/messages/service_broker_update_message_spec.rb +++ b/spec/unit/messages/service_broker_update_message_spec.rb @@ -149,18 +149,37 @@ module VCAP::CloudController context 'when authentication is not valid' do let(:request_body) do + valid_body[:authentication] = valid_body[:authentication].except(:type) valid_body end - before do - allow_any_instance_of(VCAP::CloudController::Validators::AuthenticationValidator).to receive(:validate) do |_, record| - record.errors.add(:authentication, 'this authentication is not valid!') - end + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include('authentication.type must be one of ["basic"]') + end + end + + context 'when username is an empty string' do + let(:request_body) do + valid_body[:authentication][:credentials][:username] = '' + valid_body + end + + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include(/Username can't be blank/) + end + end + + context 'when password is an empty string' do + let(:request_body) do + valid_body[:authentication][:credentials][:password] = '' + valid_body end it 'is not valid' do expect(message).not_to be_valid - expect(message.errors_on(:authentication)).to include('this authentication is not valid!') + expect(message.errors_on(:authentication)).to include(/Password can't be blank/) end end end diff --git a/spec/unit/messages/validators/authentication_validator_spec.rb b/spec/unit/messages/validators/authentication_validator_spec.rb index 89d03785f4a..76ff5294434 100644 --- a/spec/unit/messages/validators/authentication_validator_spec.rb +++ b/spec/unit/messages/validators/authentication_validator_spec.rb @@ -118,5 +118,37 @@ def self.model_name expect(message.errors_on(:authentication)).to include(/Field\(s\) \["password"\] must be valid/) end end + + context 'when username is an empty string' do + let(:authentication) do + { + credentials: { + username: '', + password: 'pass' + }, + } + end + + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include(/Username can't be blank/) + end + end + + context 'when password is an empty string' do + let(:authentication) do + { + credentials: { + username: 'user', + password: '' + }, + } + end + + it 'is not valid' do + expect(message).not_to be_valid + expect(message.errors_on(:authentication)).to include(/Password can't be blank/) + end + end end end