Add validation for credentials in SB update to be not blank

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)

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

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 }
 

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

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

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