diff --git a/app/actions/route_transfer_owner.rb b/app/actions/route_transfer_owner.rb new file mode 100644 index 00000000000..f24a0f8b435 --- /dev/null +++ b/app/actions/route_transfer_owner.rb @@ -0,0 +1,30 @@ +require 'repositories/route_event_repository' + +module VCAP::CloudController + class RouteTransferOwner + class Error < ::StandardError + end + + class << self + def transfer(route, target_space, user_audit_info) + return route if target_space.name == route.space.name + + original_space = route.space + Route.db.transaction do + route.space = target_space + route.remove_shared_space(target_space) + route.add_shared_space(original_space) + route.save + end + Repositories::RouteEventRepository.new.record_route_transfer_owner( + route, user_audit_info, original_space, target_space.guid) + end + + private + + def error!(message) + raise Error.new(message) + end + end + end +end diff --git a/app/controllers/v3/routes_controller.rb b/app/controllers/v3/routes_controller.rb index 76e374512c8..55c6509ea54 100644 --- a/app/controllers/v3/routes_controller.rb +++ b/app/controllers/v3/routes_controller.rb @@ -4,6 +4,7 @@ require 'messages/routes_list_message' require 'messages/route_show_message' require 'messages/route_update_message' +require 'messages/route_transfer_owner_message' require 'messages/route_update_destinations_message' require 'actions/update_route_destinations' require 'decorators/include_route_domain_decorator' @@ -16,6 +17,7 @@ require 'actions/route_update' require 'actions/route_share' require 'actions/route_unshare' +require 'actions/route_transfer_owner' require 'fetchers/app_fetcher' require 'fetchers/route_fetcher' require 'fetchers/route_destinations_list_fetcher' @@ -169,14 +171,24 @@ def relationships_shared_routes def transfer_owner FeatureFlag.raise_unless_enabled!(:route_sharing) + + message = RouteTransferOwnerMessage.new(hashed_params[:body]) + unprocessable!(message.errors.full_messages) unless message.valid? + unauthorized! unless permission_queryer.can_manage_apps_in_active_space?(route.space.guid) - target_space_guid = hashed_params['space'] - target_space = Space.first(guid: target_space_guid) - if target_space.nil? || !can_write_space?(target_space) - unprocessable!("Unable to transfer owner of route #{route.uri} to space '#{target_space_guid}'. " \ - 'Ensure the space exists and that you have access to it.') - end + target_space = Space.first(guid: message.guid) + target_space_error = if target_space.nil? || !can_read_space?(target_space) + 'Ensure the space exists and that you have access to it.' + elsif !permission_queryer.can_manage_apps_in_active_space?(target_space.guid) + 'Ensure that you have write permission for the target space.' + elsif !permission_queryer.is_space_active?(target_space.guid) + 'The target organization is suspended.' + end + unprocessable!("Unable to transfer owner of route '#{route.uri}' to space '#{message.guid}'. #{target_space_error}") unless target_space_error.nil? + + RouteTransferOwner.transfer(route, target_space, user_audit_info) + render status: :ok, json: { status: 'ok' } end diff --git a/app/messages/route_transfer_owner_message.rb b/app/messages/route_transfer_owner_message.rb new file mode 100644 index 00000000000..399791e9240 --- /dev/null +++ b/app/messages/route_transfer_owner_message.rb @@ -0,0 +1,10 @@ +require 'messages/base_message' + +module VCAP::CloudController + class RouteTransferOwnerMessage < BaseMessage + register_allowed_keys [:guid] + + validates_with NoAdditionalKeysValidator + validates :guid, presence: true, string: true, allow_nil: false, allow_blank: false + end +end diff --git a/app/repositories/route_event_repository.rb b/app/repositories/route_event_repository.rb index 50c08f349c1..c27c7f48d6f 100644 --- a/app/repositories/route_event_repository.rb +++ b/app/repositories/route_event_repository.rb @@ -77,6 +77,24 @@ def record_route_unshare(route, actor_audit_info, target_space_guid) ) end + def record_route_transfer_owner(route, actor_audit_info, original_space, target_space_guid) + Event.create( + space: original_space, + type: 'audit.route.transfer-owner', + actee: route.guid, + actee_type: 'route', + actee_name: route.host, + actor: actor_audit_info.user_guid, + actor_type: 'user', + actor_name: actor_audit_info.user_email, + actor_username: actor_audit_info.user_name, + timestamp: Sequel::CURRENT_TIMESTAMP, + metadata: { + target_space_guid: target_space_guid + } + ) + end + def record_route_delete_request(route, actor_audit_info, recursive) Event.create( type: 'audit.route.delete-request', diff --git a/spec/request/routes_spec.rb b/spec/request/routes_spec.rb index e3482fb54ec..6a7c8c93dbd 100644 --- a/spec/request/routes_spec.rb +++ b/spec/request/routes_spec.rb @@ -3148,19 +3148,15 @@ expect(route.shared_spaces).to contain_exactly(target_space_1, target_space_2, target_space_3) end end - - describe 'errors while unsharing' do - # isolation segments? - end end - describe 'PATCH /v3/routes/:guid/transfer-owner' do + describe 'PATCH /v3/routes/:guid/transfer_owner' do let(:route) { VCAP::CloudController::Route.make(space: space) } let(:api_call) { lambda { |user_headers| patch "/v3/routes/#{route.guid}/transfer_owner", request_body.to_json, user_headers } } let(:target_space) { VCAP::CloudController::Space.make(organization: org) } let(:request_body) do { - 'space' => target_space.guid + 'guid' => target_space.guid } end let(:space_dev_headers) do @@ -3191,12 +3187,32 @@ it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS end + it 'changes the route owner to the given space and logs an event' do + api_call.call(space_dev_headers) + + expect(last_response.status).to eq(200) + + event = VCAP::CloudController::Event.last + expect(event.values).to include({ + type: 'audit.route.transfer-owner', + actor: user.guid, + actee_type: 'route', + actee_name: route.host, + space_guid: space.guid, + organization_guid: space.organization.guid + }) + expect(event.metadata['target_space_guid']).to eq(target_space.guid) + + route.reload + expect(route.space).to eq target_space + end + describe 'target space to transfer to' do context 'does not exist' do let(:target_space_guid) { 'fake-target' } let(:request_body) do { - 'space' => target_space_guid + 'guid' => target_space_guid } end @@ -3207,7 +3223,7 @@ expect(parsed_response['errors']).to include( include( { - 'detail' => "Unable to transfer owner of route #{route.uri} to space '#{target_space_guid}'. " \ + 'detail' => "Unable to transfer owner of route '#{route.uri}' to space '#{target_space_guid}'. " \ 'Ensure the space exists and that you have access to it.', 'title' => 'CF-UnprocessableEntity' }) @@ -3215,11 +3231,11 @@ end end - context 'user does not have access to the target space' do + context 'user does not have read access to the target space' do let(:no_access_target_space) { VCAP::CloudController::Space.make(organization: org) } let(:request_body) do { - 'space' => no_access_target_space.guid + 'guid' => no_access_target_space.guid } end @@ -3230,13 +3246,76 @@ expect(parsed_response['errors']).to include( include( { - 'detail' => "Unable to transfer owner of route #{route.uri} to space '#{no_access_target_space.guid}'. "\ + 'detail' => "Unable to transfer owner of route '#{route.uri}' to space '#{no_access_target_space.guid}'. "\ 'Ensure the space exists and that you have access to it.', 'title' => 'CF-UnprocessableEntity' }) ) end end + context 'user does not have write access to the target space' do + let(:no_write_access_target_space) { VCAP::CloudController::Space.make(organization: org) } + let(:request_body) do + { + 'guid' => no_write_access_target_space.guid + } + end + + before do + no_write_access_target_space.add_auditor(user) + end + + it 'responds with 422 and does not share the route' do + api_call.call(space_dev_headers) + + expect(last_response.status).to eq(422) + expect(parsed_response['errors']).to include( + include( + { + 'detail' => "Unable to transfer owner of route '#{route.uri}' to space '#{no_write_access_target_space.guid}'. "\ + 'Ensure that you have write permission for the target space.', + 'title' => 'CF-UnprocessableEntity' + }) + ) + end + end + end + + it 'responds with 404 when the route does not exist' do + patch '/v3/routes/some-fake-guid/transfer_owner', request_body.to_json, space_dev_headers + + expect(last_response).to have_status_code(404) + expect(parsed_response['errors']).to include( + include( + { + 'detail' => 'Route not found', + 'title' => 'CF-ResourceNotFound' + }) + ) + end + + describe 'when the request body is invalid' do + context 'when there are additional keys' do + let(:request_body) do + { + 'guid' => target_space.guid, + 'fake-key' => 'foo' + } + end + + it 'should respond with 422' do + api_call.call(space_dev_headers) + + expect(last_response.status).to eq(422) + expect(parsed_response['errors']).to include( + include( + { + 'detail' => "Unknown field(s): 'fake-key'", + 'title' => 'CF-UnprocessableEntity' + }) + ) + end + end end describe 'when route_sharing flag is disabled' do diff --git a/spec/unit/actions/route_transfer_owner_spec.rb b/spec/unit/actions/route_transfer_owner_spec.rb new file mode 100644 index 00000000000..165aa87cdf3 --- /dev/null +++ b/spec/unit/actions/route_transfer_owner_spec.rb @@ -0,0 +1,85 @@ +require 'spec_helper' +require 'actions/route_transfer_owner' + +module VCAP::CloudController + RSpec.describe RouteTransferOwner do + let(:route_share) { RouteShare.new } + let(:route) { Route.make domain: SharedDomain.make, space: original_owning_space } + let(:original_owning_space) { Space.make name: 'original_owning_space' } + let(:target_space) { Space.make name: 'target_space' } + let(:shared_space) { Space.make name: 'shared_space' } + let(:user_audit_info) { UserAuditInfo.new(user_guid: 'user-guid-1', user_email: 'user@email.com') } + + describe '#transfer' do + before do + route_share.create(route, [shared_space], user_audit_info) + end + + it 'makes the target space the new owner' do + RouteTransferOwner.transfer(route, target_space, user_audit_info) + expect(route.space.name).to eq target_space.name + end + + context 'route was previously shared with the target space' do + before do + route_share.create(route, [target_space], user_audit_info) + end + + it 'removes the target space from the list of shared spaces' do + expect(route.shared_spaces.map(&:name)).to include target_space.name + RouteTransferOwner.transfer(route, target_space, user_audit_info) + route.reload + expect(route.shared_spaces.map(&:name)).not_to include target_space.name + end + end + + it 'shares the route with the original owning space' do + expect(route.shared_spaces.map(&:name)).not_to include original_owning_space.name + RouteTransferOwner.transfer(route, target_space, user_audit_info) + route.reload + expect(route.shared_spaces.map(&:name)).to include original_owning_space.name + end + + context 'target space is already the owning space' do + it ' it does nothing and succeeds' do + expect { RouteTransferOwner.transfer(route, original_owning_space, user_audit_info) }.not_to raise_error + expect(route.shared_spaces.map(&:name)).not_to include original_owning_space.name + expect(route.space.name).to eq original_owning_space.name + end + end + + it 'records a transfer event' do + expect_any_instance_of(Repositories::RouteEventRepository).to receive(:record_route_transfer_owner).with( + route, user_audit_info, target_space.guid) + + RouteTransferOwner.transfer(route, target_space, user_audit_info) + end + + context 'when tranfering ownership fails' do + before do + allow(route).to receive(:save).and_raise('db failure') + end + + it 'does not change the owning space' do + expect(route.space.name).to eq original_owning_space.name + expect { + RouteTransferOwner.transfer(route, target_space, user_audit_info) + }.to raise_error('db failure') + route.reload + expect(route.space.name).to eq original_owning_space.name + end + + it 'does not change the shared spaces' do + expect(route.shared_spaces.length).to eq 1 + expect(route.shared_spaces.map(&:name)).to include shared_space.name + expect { + RouteTransferOwner.transfer(route, target_space, user_audit_info) + }.to raise_error('db failure') + route.reload + expect(route.shared_spaces.map(&:name)).to include shared_space.name + expect(route.shared_spaces.length).to eq 1 + end + end + end + end +end diff --git a/spec/unit/messages/route_transfer_owner_message_spec.rb b/spec/unit/messages/route_transfer_owner_message_spec.rb new file mode 100644 index 00000000000..7bc004552fd --- /dev/null +++ b/spec/unit/messages/route_transfer_owner_message_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' +require 'messages/process_scale_message' +require 'messages/base_message' + +module VCAP::CloudController + RSpec.describe RouteTransferOwnerMessage do + context 'when unexpected keys are requested' do + let(:params) { { guid: 'some-guid', unexpected: 'foo' } } + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(params) + + expect(message).not_to be_valid + expect(message.errors.full_messages[0]).to include("Unknown field(s): 'unexpected'") + end + end + + context 'when guid is not a string' do + let(:params) { { guid: 5 } } + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(params) + + expect(message).not_to be_valid + expect(message.errors.count).to eq(1) + expect(message.errors[:guid]).to include('must be a string') + end + end + + context 'when guid is not present' do + let(:params) { { guid: '' } } + + it 'is not valid' do + message = RouteTransferOwnerMessage.new(params) + + expect(message).not_to be_valid + expect(message.errors.count).to eq(1) + expect(message.errors[:guid]).to include("can't be blank") + end + end + end +end