Require authentication when changing broker URL origin

app/actions/v3/service_broker_update.rb

@@ -2,6 +2,7 @@
 require 'actions/metadata_update'
 require 'jobs/enqueuer'
 require 'jobs/queues'
+require 'uri'
 
 module VCAP::CloudController
   module V3
@@ -30,6 +31,12 @@ def update_sync
       end
 
       def enqueue_update
+        raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress') if broker.in_transitional_state?
+
+        if message.requested?(:url) && !message.requested?(:authentication) && origin_changed?
+          raise InvalidServiceBroker.new('Authentication must be provided when changing the broker URL origin')
+        end
+
         params = {}
         params[:broker_url] = message.url if message.requested?(:url)
         params[:authentication] = message.authentication.to_json if message.requested?(:authentication)
@@ -40,8 +47,6 @@ def enqueue_update
           params[:name] = message.name
         end
 
-        raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress') if broker.in_transitional_state?
-
         pollable_job = nil
         previous_broker_state = broker.state
         ServiceBrokerUpdateRequest.db.transaction do
@@ -71,6 +76,16 @@ def enqueue_update
       def unique_name!
         raise InvalidServiceBroker.new('Name must be unique')
       end
+
+      def origin_changed?
+        new_uri = URI.parse(message.url)
+        old_uri = URI.parse(broker.broker_url)
+        new_uri.scheme != old_uri.scheme ||
+          new_uri.host.to_s.downcase != old_uri.host.to_s.downcase ||
+          new_uri.port != old_uri.port
+      rescue URI::InvalidURIError
+        true
+      end
     end
   end
 end

spec/request/service_brokers_spec.rb

@@ -667,6 +667,87 @@ def expect_empty_list(user_headers)
         end
       end
 
+      context 'for a space-scoped broker' do
+        let!(:broker) do
+          create(:service_broker,
+                 name: 'space-scoped broker',
+                 broker_url: 'http://example.org/old-broker-url',
+                 auth_username: 'old-admin',
+                 auth_password: 'not-welcome',
+                 state: VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE,
+                 space: space)
+        end
+        let(:space_developer_headers) do
+          org.add_user(user)
+          space.add_developer(user)
+          headers_for(user)
+        end
+
+        context 'when changing the broker URL origin without providing authentication' do
+          it 'returns 422 and does not enqueue an update job' do
+            patch "/v3/service_brokers/#{broker.guid}", { url: 'http://attacker.example.com/broker' }.to_json, space_developer_headers
+
+            expect_error(
+              status: 422,
+              error: 'UnprocessableEntity',
+              description: 'Authentication must be provided when changing the broker URL origin'
+            )
+
+            expect(VCAP::CloudController::PollableJobModel.count).to eq(0)
+            expect(broker.reload.broker_url).to eq('http://example.org/old-broker-url')
+            expect(broker.auth_username).to eq('old-admin')
+            expect(broker.auth_password).to eq('not-welcome')
+            expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE)
+          end
+        end
+
+        context 'when changing only the broker URL path on the same origin without providing authentication' do
+          before do
+            stub_request(:get, 'http://example.org/different-path/v2/catalog').
+              to_return(status: 200, body: catalog(space_broker_id).to_json, headers: {})
+          end
+
+          it 'accepts the update' do
+            patch "/v3/service_brokers/#{broker.guid}", { url: 'http://example.org/different-path' }.to_json, space_developer_headers
+
+            expect(last_response).to have_status_code(202)
+          end
+        end
+      end
+
+      context 'for a global broker' do
+        context 'when changing the broker URL origin without providing authentication' do
+          it 'returns 422 and does not enqueue an update job' do
+            patch "/v3/service_brokers/#{broker.guid}", { url: 'http://attacker.example.com/broker' }.to_json, admin_headers
+
+            expect_error(
+              status: 422,
+              error: 'UnprocessableEntity',
+              description: 'Authentication must be provided when changing the broker URL origin'
+            )
+
+            expect(VCAP::CloudController::PollableJobModel.count).to eq(0)
+            expect(broker.reload.broker_url).to eq('http://example.org/broker-url')
+            expect(broker.auth_username).to eq('admin')
+            expect(broker.auth_password).to eq('welcome')
+            expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE)
+          end
+        end
+
+        context 'when changing only the broker URL path on the same origin without providing authentication' do
+          before do
+            stub_request(:get, 'http://example.org/different-path/v2/catalog').
+              to_return(status: 200, body: { services: [] }.to_json, headers: {})
+          end
+
+          it 'accepts the update' do
+            patch "/v3/service_brokers/#{broker.guid}", { url: 'http://example.org/different-path' }.to_json, admin_headers
+
+            expect(last_response).to have_status_code(202)
+          end
+        end
+      end
+
       context 'when there is a sql validation error while syncing' do
         let!(:service_offering) { create(:service, service_broker: broker, unique_id: global_broker_id + '-1') }
         let!(:service_plan) { create(:service_plan, service: service_offering, name: 'plan_name-1', unique_id: Sham.guid) }

spec/unit/actions/v3/service_broker_update_spec.rb

@@ -206,6 +206,15 @@ module CloudController
             end
           end
 
+          context 'and the update also changes the broker URL origin without providing authentication' do
+            let(:state) { ServiceBrokerStateEnum::SYNCHRONIZING }
+            let(:request) { { url: 'http://attacker.example.com/broker' } }
+
+            it 'raises the transitional state error first' do
+              expect { action.enqueue_update }.to raise_error(V3::ServiceBrokerUpdate::InvalidServiceBroker, expected_error_message)
+            end
+          end
+
           context 'If broker is in DELETE_IN_PROGRESS state' do
             let(:state) { ServiceBrokerStateEnum::DELETE_IN_PROGRESS }
 
@@ -233,6 +242,51 @@ module CloudController
           end
         end
 
+        context 'when changing the broker URL origin without providing authentication' do
+          let(:request) { { url: 'http://attacker.example.com/broker' } }
+
+          it 'raises an InvalidServiceBroker error' do
+            expect { action.enqueue_update }.to raise_error(
+              V3::ServiceBrokerUpdate::InvalidServiceBroker,
+              'Authentication must be provided when changing the broker URL origin'
+            )
+          end
+        end
+
+        context 'when changing the broker URL origin together with authentication' do
+          let(:request) do
+            {
+              url: 'http://attacker.example.com/broker',
+              authentication: {
+                credentials: {
+                  username: 'new-admin',
+                  password: 'welcome'
+                }
+              }
+            }
+          end
+
+          it 'does not raise an error' do
+            expect { action.enqueue_update }.not_to raise_error
+          end
+        end
+
+        context 'when changing only the broker URL path (same origin)' do
+          let(:request) { { url: 'http://example.org/different-path' } }
+
+          it 'does not raise an error' do
+            expect { action.enqueue_update }.not_to raise_error
+          end
+        end
+
+        context 'when changing only the broker URL path with different hostname casing (same origin)' do
+          let(:request) { { url: 'http://EXAMPLE.ORG/different-path' } }
+
+          it 'does not raise an error' do
+            expect { action.enqueue_update }.not_to raise_error
+          end
+        end
+
         context 'legacy service brokers' do
           let!(:state) { '' } # broker creating from V2 endpoint will not have a state