Skip to content

Commit

Permalink
First check whether user exists in UAA before trying to create in /v3…
Browse files Browse the repository at this point in the history
…/users
  • Loading branch information
svkrieger committed Dec 17, 2024
1 parent ebb0231 commit a7d0413
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 7 deletions.
10 changes: 8 additions & 2 deletions app/actions/user_create.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions app/models/runtime/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 19 additions & 4 deletions spec/request/users_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
13 changes: 12 additions & 1 deletion spec/unit/actions/user_create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
40 changes: 40 additions & 0 deletions spec/unit/models/runtime/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit a7d0413

Please sign in to comment.