Adjust error handling and add unit tests

Author: svkrieger

app/actions/user_create.rb

@@ -4,15 +4,8 @@ class Error < StandardError
     end
 
     def create(message:)
-      begin
-        shadow_user = User.create_uaa_shadow_user(message.username, message.origin) if message.username && message.origin
-      rescue CF::UAA::TargetError => e
-        raise e unless e.info['error'] == 'scim_resource_already_exists'
-
-        existing_guid = e.info['user_id']
-      end
-
-      user_guid = existing_guid || shadow_user['id'] || message.guid
+      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
 
       user = User.create(guid: user_guid)
       User.db.transaction do

app/controllers/v3/users_controller.rb

@@ -39,9 +39,12 @@ def show
 
   def create
     message = UserCreateMessage.new(hashed_params[:body])
-    unauthorized! unless permission_queryer.can_write_globally? || org_managers_can_create_users?(message:)
+    unauthorized! unless permission_queryer.can_write_globally? || org_managers_can_create_users?
     unprocessable!(message.errors.full_messages) unless message.valid?
 
+    # prevent org_managers from creating users by guid
+    unauthorized! if !permission_queryer.can_write_globally? && !(!message.guid && org_managers_can_create_users?)
+
     user = UserCreate.new.create(message:)
 
     render status: :created, json: Presenters::V3::UserPresenter.new(user, uaa_users: User.uaa_users_info([user.guid]))
@@ -94,7 +97,7 @@ def user_not_found!
     resource_not_found!(:user)
   end
 
-  def org_managers_can_create_users?(message:)
-    VCAP::CloudController::Config.config.get(:allow_user_creation_by_org_manager) && permission_queryer.is_org_manager? && !message.guid
+  def org_managers_can_create_users?
+    VCAP::CloudController::Config.config.get(:allow_user_creation_by_org_manager) && permission_queryer.is_org_manager?
   end
 end

app/messages/user_create_message.rb

@@ -4,7 +4,6 @@ module VCAP::CloudController
   class UserCreateMessage < MetadataBaseMessage
     register_allowed_keys %i[guid origin username]
 
-
     class UserCreateValidator < ActiveModel::Validator
       def validate(record)
         if record.guid

lib/cloud_controller/dependency_locator.rb

@@ -289,7 +289,7 @@ def uaa_username_lookup_client
     end
 
     def uaa_shadow_user_creation_client
-      client = config.get(:uaa, :clients)&.find { |client| client['name'] == 'cloud_controller_shadow_user_creation' }
+      client = config.get(:uaa, :clients)&.find { |client_config| client_config['name'] == 'cloud_controller_shadow_user_creation' }
 
       return unless client
 

lib/cloud_controller/uaa/uaa_client.rb

@@ -97,8 +97,12 @@ def origins_for_username(username)
     end
 
     def create_shadow_user(username, origin)
-      with_cache_retry { scim.add(:user, { username: username, origin: origin, emails: [{ primary: true, value: username}]}) }
-    rescue CF::UAA::BadTarget => e
+      with_cache_retry { scim.add(:user, { username: username, origin: origin, emails: [{ primary: true, value: username }] }) }
+    rescue CF::UAA::TargetError => e
+      raise e unless e.info['error'] == 'scim_resource_already_exists'
+
+      { 'id' => e.info['user_id'] }
+    rescue CF::UAA::UAAError => e
       logger.error("UAA request for creating a user failed: #{e.inspect}")
       raise UaaUnavailable
     end

spec/request/users_spec.rb

@@ -720,7 +720,7 @@
         let(:uaa_client_id) { 'cc_routing' }
 
         before do
-          allow(uaa_client).to receive_messages(users_for_ids: {}, get_clients: [{ client_id: uaa_client_id }])
+          allow(uaa_client).to receive_messages(users_for_ids: {})
         end
 
         let(:api_call) { ->(user_headers) { post '/v3/users', params.to_json, user_headers } }
@@ -814,6 +814,159 @@
         end
       end
     end
+
+    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)
+      end
+
+      describe 'when creating a user by guid' do
+        before do
+          allow(uaa_client).to receive(:users_for_ids).and_return({})
+        end
+
+        let(:api_call) { ->(user_headers) { post '/v3/users', params.to_json, user_headers } }
+
+        let(:user_json) do
+          {
+            guid: params[:guid],
+            created_at: iso8601,
+            updated_at: iso8601,
+            username: nil,
+            presentation_name: params[:guid],
+            origin: nil,
+            metadata: {
+              labels: {},
+              annotations: {}
+            },
+            links: {
+              self: { href: %r{#{Regexp.escape(link_prefix)}/v3/users/#{params[:guid]}} }
+            }
+          }
+        end
+
+        let(:expected_codes_and_responses) do
+          h = Hash.new(
+            code: 403
+          )
+          h['admin'] = {
+            code: 201,
+            response_object: user_json
+          }
+          h
+        end
+
+        it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+      end
+
+      describe 'when creating a user by username and origin' do
+        let(:params) do
+          {
+            username: 'some-user',
+            origin: 'idp.local'
+          }
+        end
+        let(:user_guid) { 'new-user-guid' }
+
+        let(:api_call) { ->(user_headers) { post '/v3/users', params.to_json, user_headers } }
+
+        let(:user_json) do
+          {
+            guid: user_guid,
+            created_at: iso8601,
+            updated_at: iso8601,
+            username: params[:username],
+            presentation_name: params[:username],
+            origin: params[:origin],
+            metadata: {
+              labels: {},
+              annotations: {}
+            },
+            links: {
+              self: { href: %r{#{Regexp.escape(link_prefix)}/v3/users/#{user_guid}} }
+            }
+          }
+        end
+
+        let(:expected_codes_and_responses) do
+          h = Hash.new(
+            code: 403
+          )
+          h['admin'] = {
+            code: 201,
+            response_object: user_json
+          }
+          h['org_manager'] = {
+            code: 201,
+            response_object: user_json
+          }
+          h
+        end
+
+        before do
+          allow(uaa_client).to receive(:users_for_ids).with(['new-user-guid']).and_return(
+            {
+              user_guid => {
+                'username' => 'some-user',
+                'origin' => 'idp.local'
+              }
+            }
+          )
+          allow(uaa_client).to receive(:create_shadow_user).and_return({ 'id' => user_guid })
+        end
+
+        it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+      end
+
+      context 'when parameters are invalid' do
+        let(:params) do
+          {
+            guid: user_guid,
+            username: 'some-user',
+            origin: 'idp.local'
+          }
+        end
+        let(:user_guid) { 'new-user-guid' }
+
+        let(:api_call) { ->(user_headers) { post '/v3/users', params.to_json, user_headers } }
+
+        let(:user_json) do
+          {
+            guid: user_guid,
+            created_at: iso8601,
+            updated_at: iso8601,
+            username: params[:username],
+            presentation_name: params[:username],
+            origin: params[:origin],
+            metadata: {
+              labels: {},
+              annotations: {}
+            },
+            links: {
+              self: { href: %r{#{Regexp.escape(link_prefix)}/v3/users/#{user_guid}} }
+            }
+          }
+        end
+
+        let(:expected_codes_and_responses) do
+          h = Hash.new(
+            code: 403
+          )
+          h['admin'] = {
+            code: 422,
+            response_object: user_json
+          }
+          h['org_manager'] = {
+            code: 422,
+            response_object: user_json
+          }
+          h
+        end
+
+        it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
+      end
+    end
   end
 
   describe 'PATCH /v3/users/:guid' do

spec/unit/actions/user_create_spec.rb

@@ -55,12 +55,16 @@ module VCAP::CloudController
       end
 
       describe 'creating users' do
-        context 'when creating a UAA user' do
+        before do
+          allow(User).to receive(:create_uaa_shadow_user).and_return({ 'id' => guid })
+        end
+
+        context 'when creating a UAA user by guid' do
           let(:message) do
             UserCreateMessage.new({ guid:, metadata: })
           end
 
-          it 'creates a user' do
+          it 'creates a user in ccdb' do
             created_user = nil
             expect do
               created_user = subject.create(message:)
@@ -73,6 +77,46 @@ module VCAP::CloudController
             )
             expect(created_user).to have_annotations({ key_name: 'anno', value: 'tations' })
           end
+
+          it 'does not create a shadow user in uaa' do
+            subject.create(message:)
+            expect(User).not_to have_received(:create_uaa_shadow_user)
+          end
+        end
+
+        context 'when creating a UAA user by username and origin' do
+          let(:message) do
+            UserCreateMessage.new({ username:, origin:, metadata: })
+          end
+
+          it 'creates a user in ccdb' do
+            created_user = nil
+            expect do
+              created_user = subject.create(message:)
+            end.to change(User, :count).by(1)
+
+            expect(created_user.guid).to eq guid
+            expect(created_user).to have_labels(
+                                      { prefix: nil, key_name: 'release', value: 'stable' },
+                                      { prefix: 'seriouseats.com', key_name: 'potato', value: 'mashed' }
+                                    )
+            expect(created_user).to have_annotations({ key_name: 'anno', value: 'tations' })
+          end
+
+          it 'creates a shadow user in uaa' do
+            subject.create(message:)
+            expect(User).to have_received(:create_uaa_shadow_user).with(username, origin)
+          end
+
+          context 'when an UaaUnavailable error is raised' do
+            before do
+              allow(User).to receive(:create_uaa_shadow_user).and_raise(UaaUnavailable)
+            end
+
+            it 'raises the error' do
+              expect { subject.create(message:) }.to raise_error(UaaUnavailable)
+            end
+          end
         end
 
         context 'when creating a UAA client' do

spec/unit/lib/uaa/uaa_client_spec.rb

@@ -469,6 +469,61 @@ module VCAP::CloudController
       end
     end
 
+    describe '#create_shadow_user' do
+      context 'when user does not exist in uaa' do
+        before do
+          scim = instance_double(CF::UAA::Scim)
+          allow(scim).to receive(:add).and_return({ 'id' => '7b5fe025-fe6b-4f82-af6e-2534523233a6', 'username' => '[email protected]', 'origin' => 'idp.local' })
+          allow(uaa_client).to receive_messages(scim:)
+        end
+
+        it 'creates the user and returns the id' do
+          shadow_user = uaa_client.create_shadow_user('[email protected]', 'idp.local')
+          expect(shadow_user['id']).to eq('7b5fe025-fe6b-4f82-af6e-2534523233a6')
+          expect(shadow_user['username']).to eq('[email protected]')
+          expect(shadow_user['origin']).to eq('idp.local')
+        end
+      end
+
+      context 'when user already exists in uaa' do
+        before do
+          scim = instance_double(CF::UAA::Scim)
+          allow(scim).to receive(:add).and_raise(
+            CF::UAA::TargetError.new({
+                                       'user_id' => 'ea2824df-b2b5-4444-a18f-b4c7b227a184',
+                                       'error_description' => 'Username already in use: [email protected]',
+                                       'verified' => true,
+                                       'active' => true,
+                                       'error' => 'scim_resource_already_exists',
+                                       'message' => 'Username already in use: [email protected]'
+                                     })
+          )
+          allow(uaa_client).to receive_messages(scim:)
+        end
+
+        it 'catches the exception and returns the id' do
+          shadow_user = uaa_client.create_shadow_user('[email protected]', 'idp.local')
+          expect(shadow_user['id']).to eq('ea2824df-b2b5-4444-a18f-b4c7b227a184')
+        end
+      end
+
+      context 'when something goes wrong communicating with uaa' do
+        let(:uaa_error) { CF::UAA::UAAError.new('some error') }
+        let(:mock_logger) { double(:steno_logger, error: nil) }
+
+        before do
+          scim = instance_double(CF::UAA::Scim)
+          allow(scim).to receive(:add).and_raise(uaa_error)
+          allow(uaa_client).to receive_messages(scim: scim, logger: mock_logger)
+        end
+
+        it 'logs the error and raises UaaUnavailable' do
+          expect { uaa_client.create_shadow_user('[email protected]', 'idp.local') }.to raise_error(UaaUnavailable)
+          expect(mock_logger).to have_received(:error).with("UAA request for creating a user failed: #{uaa_error.inspect}")
+        end
+      end
+    end
+
     describe '#id_for_username' do
       let(:username) { '[email protected]' }