Merge branch 'dblessing-scim-identitites-new-user-flow' into 'master'

Fix regression and allow SCIM to create SAML identity

See merge request gitlab-org/gitlab!31238

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
 20200511145545
 \.

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,7 +13,6 @@ 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 } }
 
@@ -21,6 +20,7 @@ describe Users::BuildService 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