Fix regression and allow SCIM to create SAML identity

Prior to `scim_identities` SCIM would create new users along with
their SAML identity. This allowed new users creating by SCIM to
sign in with the SAML identity without requiring them to reset
their GitLab local password. This change restores that ability.

changelogs/unreleased/dblessing-scim-identitites-new-user-flow.yml

+---
+title: Fix regression and allow SCIM to create SAML identity
+merge_request: 31238
+author:
+type: fixed

db/post_migrate/20200506154421_migrate_scim_identities_to_saml_for_new_users.rb

+# frozen_string_literal: true
+
+class MigrateScimIdentitiesToSamlForNewUsers < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  class ScimIdentity < ActiveRecord::Base
+    self.table_name = 'scim_identities'
+
+    belongs_to :user
+
+    include ::EachBatch
+  end
+
+  class Identity < ActiveRecord::Base
+    self.table_name = 'identities'
+
+    belongs_to :saml_provider
+  end
+
+  def up
+    users_with_saml_provider = Identity.select('user_id').joins(:saml_provider)
+
+    ScimIdentity.each_batch do |relation|
+      identity_records = relation
+        .select("scim_identities.extern_uid, 'group_saml', scim_identities.user_id, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, saml_providers.id")
+        .joins(:user)
+        .joins('inner join saml_providers on saml_providers.group_id=scim_identities.group_id')
+        .where("date_trunc('second',scim_identities.created_at) at time zone 'UTC' = date_trunc('second',users.created_at)")
+        .where.not(user_id: users_with_saml_provider)
+
+      execute "insert into identities (extern_uid, provider, user_id, created_at, updated_at, saml_provider_id) #{identity_records.to_sql} on conflict do nothing"
+    end
+  end
+
+  def down
+  end
+end

db/structure.sql

@@ -13764,6 +13764,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200505172405
 20200506085748
 20200506125731
+20200506154421
 20200507221434
 \.
 

ee/app/services/ee/users/build_service.rb

@@ -8,6 +8,9 @@ module EE
 
       attr_reader :group_id_for_saml
 
+      GROUP_SAML_PROVIDER = 'group_saml'
+      GROUP_SCIM_PROVIDER = 'group_scim'
+
       override :initialize
       def initialize(current_user, params = {})
         super
@@ -53,7 +56,10 @@ module EE
 
       override :build_identity
       def build_identity(user)
-        return build_scim_identity(user) if params[:provider] == 'group_scim'
+        return super unless params[:provider] == GROUP_SCIM_PROVIDER
+
+        build_scim_identity(user)
+        identity_params[:provider] = GROUP_SAML_PROVIDER
 
         super
       end

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

@@ -59,9 +59,9 @@ module EE
 
         def identity_provider
           strong_memoize(:identity_provider) do
-            next 'group_scim' if scim_identities_enabled?
+            next ::Users::BuildService::GROUP_SCIM_PROVIDER if scim_identities_enabled?
 
-            'group_saml'
+            ::Users::BuildService::GROUP_SAML_PROVIDER
           end
         end
 

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

@@ -104,8 +104,11 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
 
       it_behaves_like 'success response'
 
-      it 'creates the identity' do
+      it 'creates the SCIM identity' do
         expect { service.execute }.to change { ScimIdentity.count }.by(1)
+      end
+
+      it 'does not create the SAML identity' do
         expect { service.execute }.not_to change { Identity.count }
       end
     end
@@ -127,8 +130,11 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         }
       end
 
-      it 'creates the identity' do
+      it 'creates the SAML identity' do
         expect { service.execute }.to change { Identity.count }.by(1)
+      end
+
+      it 'does not create the SCIM identity' do
         expect { service.execute }.not_to change { ScimIdentity.count }
       end
 
@@ -150,6 +156,7 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
     context 'when scim_identities is enabled' do
       before do
         stub_feature_flags(scim_identities: true)
+        create(:saml_provider, group: group)
       end
 
       it_behaves_like 'scim provisioning'
@@ -163,9 +170,12 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
         }
       end
 
-      it 'creates the scim identity' do
+      it 'creates the SCIM identity' do
         expect { service.execute }.to change { ScimIdentity.count }.by(1)
-        expect { service.execute }.not_to change { Identity.count }
+      end
+
+      it 'creates the SAML identity' do
+        expect { service.execute }.to change { Identity.count }.by(1)
       end
 
       context 'existing user' do

ee/spec/migrations/migrate_scim_identities_to_saml_for_new_users_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200506154421_migrate_scim_identities_to_saml_for_new_users.rb')
+
+describe MigrateScimIdentitiesToSamlForNewUsers, :migration do
+  let(:group1) { table(:namespaces).create!(name: 'group1', path: 'group1') }
+  let(:group2) { table(:namespaces).create!(name: 'group2', path: 'group2') }
+  let(:saml_provider1) { table(:saml_providers).create!(enabled: true, group_id: group1.id, certificate_fingerprint: '123abc', sso_url: 'https://sso1.example.com') }
+  let(:saml_provider2) { table(:saml_providers).create!(enabled: true, group_id: group2.id, certificate_fingerprint: '123abc', sso_url: 'https://sso2.example.com') }
+
+  let(:users) { table(:users) }
+  let(:scim_identities) { table(:scim_identities) }
+  let(:identities) { table(:identities) }
+
+  before do
+    saml_provider1
+    saml_provider2
+  end
+
+  context 'when a user and scim_identity were created at the same time' do
+    context 'a mix of SAML identities exist and need created' do
+      before do
+        create_user_and_scim_identity(1, scim_identity_options: { group_id: group1.id })
+        create_user_and_scim_identity(2, scim_identity_options: { group_id: group2.id })
+        create_user_and_both_identities(
+          10,
+          scim_identity_options: { group_id: group1.id },
+          saml_identity_options: { saml_provider_id: saml_provider1.id }
+        )
+      end
+
+      it 'creates group SAML identities' do
+        migrate!
+
+        saml_identity1 = identities.find_by(extern_uid: 'user1')
+        saml_identity2 = identities.find_by(extern_uid: 'user2')
+
+        expect(saml_identity1.provider).to eq('group_saml')
+        expect(saml_identity1.user_id).to eq(1)
+        expect(saml_identity1.saml_provider_id).to eq(saml_provider1.id)
+
+        expect(saml_identity2.provider).to eq('group_saml')
+        expect(saml_identity2.user_id).to eq(2)
+        expect(saml_identity2.saml_provider_id).to eq(saml_provider2.id)
+      end
+
+      it 'does not create extra identities' do
+        expect { migrate! }.to change { identities.count }.by(2)
+      end
+    end
+
+    context 'SAML identities already exist' do
+      it 'does not create any SAML identities' do
+        create_user_and_both_identities(
+          20,
+          scim_identity_options: { group_id: group1.id },
+          saml_identity_options: { saml_provider_id: saml_provider1.id }
+        )
+        create_user_and_both_identities(
+          21,
+          scim_identity_options: { group_id: group2.id },
+          saml_identity_options: { saml_provider_id: saml_provider2.id }
+        )
+
+        expect { migrate! }.not_to change { identities.count }
+      end
+    end
+  end
+
+  context 'when a user and scim_identity were created at different times' do
+    it 'does not create an identity' do
+      create_user_and_scim_identity(30, scim_identity_options: { group_id: group1.id, created_at: 1.hour.ago })
+
+      migrate!
+
+      saml_identity = identities.find_by(extern_uid: 'user30')
+
+      expect(saml_identity).to be_nil
+    end
+  end
+
+  def create_user_and_scim_identity(id, scim_identity_options: {})
+    default_user_options = {
+      id: id,
+      username: "user#{id}",
+      email: "user#{id}@example.com",
+      projects_limit: 10
+    }
+
+    default_identity_options = {
+      id: id,
+      user_id: id,
+      extern_uid: "user#{id}"
+    }
+
+    users.create!(default_user_options)
+    scim_identities.create!(default_identity_options.merge(scim_identity_options))
+  end
+
+  def create_user_and_both_identities(id, scim_identity_options: {}, saml_identity_options: {})
+    default_identity_options = {
+      id: id,
+      user_id: id,
+      extern_uid: "user#{id}",
+      provider: 'group_saml'
+    }
+
+    create_user_and_scim_identity(id, scim_identity_options: scim_identity_options)
+    identities.create!(default_identity_options.merge(saml_identity_options))
+  end
+end

ee/spec/services/ee/users/build_service_spec.rb

@@ -13,14 +13,14 @@ describe Users::BuildService do
       let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
 
       context 'allowed params' do
-        context 'with identity' do
-          let(:provider) { create(:saml_provider) }
-          let(:identity_params) { { extern_uid: 'uid', provider: 'group_saml', saml_provider_id: provider.id } }
+        let(:provider) { create(:saml_provider) }
+        let(:identity_params) { { extern_uid: 'uid', provider: 'group_saml', saml_provider_id: provider.id } }
 
-          before do
-            params.merge!(identity_params)
-          end
+        before do
+          params.merge!(identity_params)
+        end
 
+        context 'with identity' do
           it 'sets all allowed attributes' do
             expect(Identity).to receive(:new).with(hash_including(identity_params)).and_call_original
             expect(ScimIdentity).not_to receive(:new)
@@ -35,11 +35,11 @@ describe Users::BuildService do
           end
           let_it_be(:scim_identity_params) { { extern_uid: 'uid', provider: 'group_scim', group_id: 1 } }
 
-          it 'passes allowed attributes to scim identity' do
+          it 'passes allowed attributes to both scim and saml identity' do
             scim_identity_params.delete(:provider)
 
             expect(ScimIdentity).to receive(:new).with(hash_including(scim_identity_params)).and_call_original
-            expect(Identity).not_to receive(:new)
+            expect(Identity).to receive(:new).with(hash_including(identity_params)).and_call_original
 
             service.execute
           end