Merge branch 'dblessing-scim-create-regression' into 'master'

Allow SCIM to create an identity for an existing user

Closes #212721

See merge request gitlab-org/gitlab!28379

ee/changelogs/unreleased/dblessing-scim-create-regression.yml

+---
+title: Allow SCIM to create an identity for an existing user
+merge_request: 28379
+author:
+type: fixed

ee/lib/api/scim.rb

@@ -132,7 +132,7 @@ module API
           check_group_saml_configured
         end
 
-        desc 'Get SAML users' do
+        desc 'Get SCIM users' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
         get do
@@ -149,7 +149,7 @@ module API
           scim_error!(message: 'Unsupported Filter')
         end
 
-        desc 'Get a SAML user' do
+        desc 'Get a SCIM user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
         get ':id', requirements: USER_ID_REQUIREMENTS do
@@ -164,7 +164,7 @@ module API
           present identity, with: ::EE::API::Entities::Scim::User
         end
 
-        desc 'Create a SAML user' do
+        desc 'Create a SCIM user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
         post do
@@ -184,7 +184,7 @@ module API
           end
         end
 
-        desc 'Updates a SAML user' do
+        desc 'Updates a SCIM user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
         patch ':id', requirements: USER_ID_REQUIREMENTS do
@@ -204,7 +204,7 @@ module API
           end
         end
 
-        desc 'Removes a SAML user' do
+        desc 'Removes a SCIM user' do
           detail 'This feature was introduced in GitLab 11.10.'
         end
         delete ':id', requirements: USER_ID_REQUIREMENTS do

ee/lib/ee/gitlab/scim/provisioning_service.rb

@@ -16,16 +16,15 @@ module EE
         end
 
         def execute
-          return success_response if existing_member?
+          return error_response(errors: ["Missing params: #{missing_params}"]) unless missing_params.empty?
+          return success_response if existing_identity_and_member?
 
           clear_memoization(:identity)
 
-          if user.save && member.errors.empty?
-            success_response
-          else
-            error_response
-          end
+          return create_identity if create_identity_only?
+          return create_identity_and_member if existing_user?
 
+          create_user_and_member
         rescue => e
           logger.error(error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
 
@@ -34,8 +33,22 @@ module EE
 
         private
 
-        def success_response
-          ProvisioningResponse.new(status: :success, identity: identity)
+        def create_identity
+          return success_response if identity.save
+
+          error_response(objects: [identity])
+        end
+
+        def create_identity_and_member
+          return success_response if identity.save && member.errors.empty?
+
+          error_response(objects: [identity, member])
+        end
+
+        def create_user_and_member
+          return success_response if user.save && member.errors.empty?
+
+          error_response(objects: [user, identity, member])
         end
 
         def scim_identities_enabled?
@@ -54,20 +67,49 @@ module EE
 
         def identity
           strong_memoize(:identity) do
-            if scim_identities_enabled?
-              @group.scim_identities.with_extern_uid(@parsed_hash[:extern_uid]).first
-            else
-              ::Identity.with_extern_uid(identity_provider, @parsed_hash[:extern_uid]).first
-            end
+            next saml_identity unless scim_identities_enabled?
+
+            identity = @group.scim_identities.with_extern_uid(@parsed_hash[:extern_uid]).first
+            next identity if identity
+
+            build_scim_identity
           end
         end
 
+        def saml_identity
+          ::Identity.with_extern_uid(identity_provider, @parsed_hash[:extern_uid]).first
+        end
+
         def user
-          @user ||= ::Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
+          strong_memoize(:user) do
+            next build_user unless scim_identities_enabled?
+
+            user = ::User.find_by_any_email(@parsed_hash[:email])
+            next user if user
+
+            build_user
+          end
         end
 
-        def error_response(errors: nil)
-          errors ||= [user, identity, member].compact.flat_map { |obj| obj.errors.full_messages }
+        def build_user
+          ::Users::BuildService.new(nil, user_params).execute(skip_authorization: true)
+        end
+
+        def build_scim_identity
+          @scim_identity ||=
+            @group.scim_identities.new(
+              user: user,
+              extern_uid: @parsed_hash[:extern_uid],
+              active: true
+            )
+        end
+
+        def success_response
+          ProvisioningResponse.new(status: :success, identity: identity)
+        end
+
+        def error_response(errors: nil, objects: [])
+          errors ||= objects.compact.flat_map { |obj| obj.errors.full_messages }
           conflict = errors.any? { |error| error.include?('has already been taken') }
 
           ProvisioningResponse.new(status: conflict ? :conflict : :error, message: errors.to_sentence)
@@ -104,14 +146,32 @@ module EE
           Uniquify.new.string(clean_username) { |s| !NamespacePathValidator.valid_path?(s) }
         end
 
+        def missing_params
+          @missing_params ||= ([:extern_uid, :email, :username] - @parsed_hash.keys)
+        end
+
         def member
           strong_memoize(:member) do
+            next @group.group_member(user) if existing_member?(user)
+
             @group.add_user(user, DEFAULT_ACCESS) if user.valid?
           end
         end
 
-        def existing_member?
-          identity && ::GroupMember.member_of_group?(@group, identity.user)
+        def create_identity_only?
+          scim_identities_enabled? && existing_user? && existing_member?(user)
+        end
+
+        def existing_identity_and_member?
+          identity&.persisted? && existing_member?(identity.user)
+        end
+
+        def existing_member?(user)
+          ::GroupMember.member_of_group?(@group, user)
+        end
+
+        def existing_user?
+          user&.persisted?
         end
       end
     end

ee/spec/lib/ee/gitlab/scim/provisioning_service_spec.rb

@@ -11,6 +11,16 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
       stub_licensed_features(group_saml: true)
     end
 
+    shared_examples 'success response' do
+      it 'contains a success status' do
+        expect(service.execute.status).to eq(:success)
+      end
+
+      it 'contains an identity in the response' do
+        expect(service.execute.identity).to be_a(Identity).or be_a(ScimIdentity)
+      end
+    end
+
     shared_examples 'scim provisioning' do
       context 'valid params' do
         let_it_be(:service_params) do
@@ -26,9 +36,7 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
           User.find_by(email: service_params[:email])
         end
 
-        it 'succeeds' do
-          expect(service.execute.status).to eq(:success)
-        end
+        it_behaves_like 'success response'
 
         it 'creates the user' do
           expect { service.execute }.to change { User.count }.by(1)
@@ -68,20 +76,6 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
             expect { service.execute }.to change { User.count }.by(1)
           end
         end
-
-        context 'existing user' do
-          before do
-            create(:user, email: 'work@example.com')
-          end
-
-          it 'does not create a new user' do
-            expect { service.execute }.not_to change { User.count }
-          end
-
-          it 'fails with conflict' do
-            expect(service.execute.status).to eq(:conflict)
-          end
-        end
       end
 
       context 'invalid params' do
@@ -96,6 +90,23 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         it 'fails with error' do
           expect(service.execute.status).to eq(:error)
         end
+
+        it 'fails with missing params' do
+          expect(service.execute.message).to eq("Missing params: [:username]")
+        end
+      end
+    end
+
+    shared_examples 'existing user when scim identities are enabled' do
+      it 'does not create a new user' do
+        expect { service.execute }.not_to change { User.count }
+      end
+
+      it_behaves_like 'success response'
+
+      it 'creates the identity' do
+        expect { service.execute }.to change { ScimIdentity.count }.by(1)
+        expect { service.execute }.not_to change { Identity.count }
       end
     end
 
@@ -105,6 +116,8 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         create(:saml_provider, group: group)
       end
 
+      it_behaves_like 'scim provisioning'
+
       let_it_be(:service_params) do
         {
           email: 'work@example.com',
@@ -114,12 +127,24 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         }
       end
 
-      it_behaves_like 'scim provisioning'
-
       it 'creates the identity' do
         expect { service.execute }.to change { Identity.count }.by(1)
         expect { service.execute }.not_to change { ScimIdentity.count }
       end
+
+      context 'existing user' do
+        before do
+          create(:user, email: 'work@example.com')
+        end
+
+        it 'does not create a new user' do
+          expect { service.execute }.not_to change { User.count }
+        end
+
+        it 'fails with conflict' do
+          expect(service.execute.status).to eq(:conflict)
+        end
+      end
     end
 
     context 'when scim_identities is enabled' do
@@ -127,6 +152,8 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         stub_feature_flags(scim_identities: true)
       end
 
+      it_behaves_like 'scim provisioning'
+
       let_it_be(:service_params) do
         {
           email: 'work@example.com',
@@ -136,12 +163,37 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         }
       end
 
-      it_behaves_like 'scim provisioning'
-
       it 'creates the scim identity' do
         expect { service.execute }.to change { ScimIdentity.count }.by(1)
         expect { service.execute }.not_to change { Identity.count }
       end
+
+      context 'existing user' do
+        before do
+          create(:email, user: user, email: 'work@example.com')
+        end
+        let(:user) { create(:user) }
+
+        context 'when user is not an existing group member' do
+          it_behaves_like 'existing user when scim identities are enabled'
+
+          it 'creates the group member' do
+            expect { service.execute }.to change { GroupMember.count }.by(1)
+          end
+        end
+
+        context 'when user is an existing group member' do
+          before do
+            group.add_guest(user)
+          end
+
+          it_behaves_like 'existing user when scim identities are enabled'
+
+          it 'does not create the group member' do
+            expect { service.execute }.not_to change { GroupMember.count }
+          end
+        end
+      end
     end
   end
 end