diff --git a/app/actions/user_create.rb b/app/actions/user_create.rb index 070e10a5685..b9d14b41b61 100644 --- a/app/actions/user_create.rb +++ b/app/actions/user_create.rb @@ -4,9 +4,15 @@ class Error < StandardError end def create(message:) - shadow_user = User.create_uaa_shadow_user(message.username, message.origin) if message.username && message.origin - user_guid = shadow_user ? shadow_user['id'] : message.guid + if message.username && message.origin + existing_user_guid = User.get_user_id_by_username_and_origin(message.username, message.origin) + shadow_user = User.create_uaa_shadow_user(message.username, message.origin) unless existing_user_guid + + user_guid = existing_user_guid || shadow_user['id'] + else + user_guid = message.guid + end user = User.create(guid: user_guid) User.db.transaction do MetadataUpdate.update(user, message) diff --git a/app/models/runtime/user.rb b/app/models/runtime/user.rb index 5a51f001e13..212ce236089 100644 --- a/app/models/runtime/user.rb +++ b/app/models/runtime/user.rb @@ -252,6 +252,11 @@ def self.uaa_users_info(user_guids) uaa_username_lookup_client.users_for_ids(user_guids) end + def self.get_user_id_by_username_and_origin(username, origin) + uaa_username_lookup_client = CloudController::DependencyLocator.instance.uaa_username_lookup_client + uaa_username_lookup_client.ids_for_usernames_and_origins([username], [origin]).first + end + def self.create_uaa_shadow_user(username, origin) uaa_shadow_user_creation_client = CloudController::DependencyLocator.instance.uaa_shadow_user_creation_client uaa_shadow_user_creation_client.create_shadow_user(username, origin) diff --git a/spec/request/users_spec.rb b/spec/request/users_spec.rb index 1424e1a1bcb..9da5256fb74 100644 --- a/spec/request/users_spec.rb +++ b/spec/request/users_spec.rb @@ -705,8 +705,8 @@ end before do - allow(CloudController::DependencyLocator.instance).to receive(:uaa_shadow_user_creation_client).and_return(uaa_client) - allow(uaa_client).to receive(:create_shadow_user).and_return({ 'id' => user_guid }) + allow(CloudController::DependencyLocator.instance).to receive_messages(uaa_shadow_user_creation_client: uaa_client, uaa_username_lookup_client: uaa_client) + allow(uaa_client).to receive_messages(create_shadow_user: { 'id' => user_guid }, ids_for_usernames_and_origins: []) end it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS @@ -867,7 +867,7 @@ context 'when "allow_user_creation_by_org_manager" is enabled' do before do TestConfig.override(allow_user_creation_by_org_manager: true) - allow(CloudController::DependencyLocator.instance).to receive(:uaa_shadow_user_creation_client).and_return(uaa_client) + allow(CloudController::DependencyLocator.instance).to receive_messages(uaa_shadow_user_creation_client: uaa_client, uaa_username_lookup_client: uaa_client) end describe 'when creating a user by guid' do @@ -954,7 +954,7 @@ end before do - allow(uaa_client).to receive(:create_shadow_user).and_return({ 'id' => user_guid }) + allow(uaa_client).to receive_messages(create_shadow_user: { 'id' => user_guid }, ids_for_usernames_and_origins: []) end it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS @@ -967,6 +967,21 @@ expect(uaa_client).not_to have_received(:users_for_ids) end + + context 'when user already exists in UAA' do + before do + allow(uaa_client).to receive(:ids_for_usernames_and_origins).and_return([user_guid]) + end + + it 'does not try to create a shadow user' do + post '/v3/users', params.to_json, admin_header + + expect(last_response).to have_status_code(201) + expect(parsed_response).to match_json_response(user_json) + + expect(uaa_client).not_to have_received(:create_shadow_user) + end + end end context 'when parameters are invalid' do diff --git a/spec/unit/actions/user_create_spec.rb b/spec/unit/actions/user_create_spec.rb index 2f4d9dec6f5..6d182d97549 100644 --- a/spec/unit/actions/user_create_spec.rb +++ b/spec/unit/actions/user_create_spec.rb @@ -56,7 +56,7 @@ module VCAP::CloudController describe 'creating users' do before do - allow(User).to receive(:create_uaa_shadow_user).and_return({ 'id' => guid }) + allow(User).to receive_messages(create_uaa_shadow_user: { 'id' => guid }, get_user_id_by_username_and_origin: nil) end context 'when creating a UAA user by guid' do @@ -108,6 +108,17 @@ module VCAP::CloudController expect(User).to have_received(:create_uaa_shadow_user).with(username, origin) end + context 'when user already exists in UAA' do + before do + allow(User).to receive(:get_user_id_by_username_and_origin).and_return('some-user-guid') + end + + it 'does not try to create a shadow user' do + subject.create(message:) + expect(User).not_to have_received(:create_uaa_shadow_user) + end + end + context 'when an UaaUnavailable error is raised' do before do allow(User).to receive(:create_uaa_shadow_user).and_raise(UaaUnavailable) diff --git a/spec/unit/models/runtime/user_spec.rb b/spec/unit/models/runtime/user_spec.rb index b59a5953898..ad10cdcf0d2 100644 --- a/spec/unit/models/runtime/user_spec.rb +++ b/spec/unit/models/runtime/user_spec.rb @@ -578,5 +578,45 @@ module VCAP::CloudController end end end + + describe '#get_user_id_by_username_and_origin' do + let(:uaa_client) { instance_double(UaaClient) } + + before do + allow(CloudController::DependencyLocator.instance).to receive(:uaa_username_lookup_client).and_return(uaa_client) + end + + context 'when uaa client returns user id' do + before do + allow(uaa_client).to receive(:ids_for_usernames_and_origins).and_return(['some-user-id']) + end + + it 'returns user id' do + user_id = User.get_user_id_by_username_and_origin(['shadow-user'], ['idp.local']) + expect(user_id).to eq('some-user-id') + end + end + + context 'when uaa client returns empty array' do + before do + allow(uaa_client).to receive(:ids_for_usernames_and_origins).and_return([]) + end + + it 'returns nil' do + user_id = User.get_user_id_by_username_and_origin(['shadow-user'], ['idp.local']) + expect(user_id).to be_nil + end + end + + context 'when uaa client raises UaaUnavailable' do + before do + allow(uaa_client).to receive(:ids_for_usernames_and_origins).and_raise(UaaUnavailable) + end + + it 'raises UaaUnavailable' do + expect { User.get_user_id_by_username_and_origin(['shadow-user'], ['idp.local']) }.to raise_error(UaaUnavailable) + end + end + end end end