Merge branch 'dblessing-add-scim-identities' into 'master'

Separate SCIM Identities from SAML

See merge request gitlab-org/gitlab!26124

changelogs/unreleased/dblessing-add-scim-identities.yml

+---
+title: Create scim_identities table in preparation for newer SCIM features in the
+  future
+merge_request: 26124
+author:
+type: added

db/migrate/20200227164113_create_scim_identities.rb

+# frozen_string_literal: true
+
+class CreateScimIdentities < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    create_table :scim_identities do |t|
+      t.references :group, foreign_key: { to_table: :namespaces, on_delete: :cascade }, null: false
+      t.references :user, index: false, foreign_key: { on_delete: :cascade }, null: false
+      t.timestamps_with_timezone
+      t.boolean :active, default: false
+      t.string :extern_uid, null: false, limit: 255
+
+      t.index 'LOWER(extern_uid),group_id', name: 'index_scim_identities_on_lower_extern_uid_and_group_id', unique: true
+      t.index [:user_id, :group_id], unique: true
+    end
+  end
+end

db/schema.rb

@@ -3819,6 +3819,18 @@ ActiveRecord::Schema.define(version: 2020_03_04_160823) do
     t.index ["group_id"], name: "index_saml_providers_on_group_id"
   end
 
+  create_table "scim_identities", force: :cascade do |t|
+    t.bigint "group_id", null: false
+    t.bigint "user_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.boolean "active", default: false
+    t.string "extern_uid", limit: 255, null: false
+    t.index "lower((extern_uid)::text), group_id", name: "index_scim_identities_on_lower_extern_uid_and_group_id", unique: true
+    t.index ["group_id"], name: "index_scim_identities_on_group_id"
+    t.index ["user_id", "group_id"], name: "index_scim_identities_on_user_id_and_group_id", unique: true
+  end
+
   create_table "scim_oauth_access_tokens", id: :serial, force: :cascade do |t|
     t.datetime_with_timezone "created_at", null: false
     t.datetime_with_timezone "updated_at", null: false
@@ -5037,6 +5049,8 @@ ActiveRecord::Schema.define(version: 2020_03_04_160823) do
   add_foreign_key "reviews", "projects", on_delete: :cascade
   add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify
   add_foreign_key "saml_providers", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "scim_identities", "namespaces", column: "group_id", on_delete: :cascade
+  add_foreign_key "scim_identities", "users", on_delete: :cascade
   add_foreign_key "scim_oauth_access_tokens", "namespaces", column: "group_id", on_delete: :cascade
   add_foreign_key "security_scans", "ci_builds", column: "build_id", on_delete: :cascade
   add_foreign_key "self_managed_prometheus_alert_events", "environments", on_delete: :cascade

ee/app/finders/scim_finder.rb

 # frozen_string_literal: true
 
 class ScimFinder
-  attr_reader :saml_provider
+  include ::Gitlab::Utils::StrongMemoize
+
+  attr_reader :group, :saml_provider
 
   UnsupportedFilter = Class.new(StandardError)
 
   def initialize(group)
+    @group = group
     @saml_provider = group&.saml_provider
   end
 
   def search(params)
-    return Identity.none unless saml_provider&.enabled?
-    return saml_provider.identities if unfiltered?(params)
+    return null_identity unless saml_provider&.enabled?
+    return all_identities if unfiltered?(params)
 
     filter_identities(params)
   end
 
   private
 
+  def scim_identities_enabled?
+    strong_memoize(:scim_identities_enabled) do
+      Feature.enabled?(:scim_identities, group)
+    end
+  end
+
+  def null_identity
+    return ScimIdentity.none if scim_identities_enabled?
+
+    Identity.none
+  end
+
+  def all_identities
+    return group.scim_identities if scim_identities_enabled?
+
+    saml_provider.identities
+  end
+
   def unfiltered?(params)
     params[:filter].blank?
   end
@@ -39,6 +60,8 @@ class ScimFinder
   end
 
   def by_extern_uid(parser)
+    return group.scim_identities.with_extern_uid(parser.filter_params[:extern_uid]) if scim_identities_enabled?
+
     Identity.where_group_saml_uid(saml_provider, parser.filter_params[:extern_uid])
   end
 
@@ -48,6 +71,9 @@ class ScimFinder
 
   def by_username(parser)
     user = User.find_by_username(parser.filter_params[:username])
+
+    return group.scim_identities.for_user(user) if scim_identities_enabled?
+
     saml_provider.identities.for_user(user)
   end
 end

ee/app/models/ee/group.rb

@@ -19,6 +19,7 @@ module EE
       has_many :epics
 
       has_one :saml_provider
+      has_many :scim_identities
       has_many :ip_restrictions, autosave: true
       has_one :insight, foreign_key: :namespace_id
       accepts_nested_attributes_for :insight, allow_destroy: true

ee/app/models/ee/user.rb

@@ -54,6 +54,7 @@ module EE
       has_many :protected_branch_unprotect_access_levels, dependent: :destroy, class_name: "::ProtectedBranch::UnprotectAccessLevel" # rubocop:disable Cop/ActiveRecordDependent
 
       has_many :smartcard_identities
+      has_many :scim_identities
 
       belongs_to :managing_group, class_name: 'Group', optional: true, inverse_of: :managed_users
 

ee/app/models/scim_identity.rb

+# frozen_string_literal: true
+
+class ScimIdentity < ApplicationRecord
+  include Sortable
+  include CaseSensitivity
+  include ScimPaginatable
+
+  belongs_to :group
+  belongs_to :user
+
+  validates :group, presence: true
+  validates :user, presence: true, uniqueness: { scope: [:group_id] }
+  validates :extern_uid, presence: true,
+            uniqueness: { case_sensitive: false, scope: [:group_id] }
+
+  scope :for_user, ->(user) { where(user: user) }
+  scope :with_extern_uid, ->(extern_uid) { iwhere(extern_uid: extern_uid) }
+end

ee/spec/factories/scim_identities.rb

+# frozen_string_literal: true
+FactoryBot.define do
+  factory :scim_identity do
+    extern_uid { generate(:username) }
+    group
+    user
+    active { true }
+  end
+end

ee/spec/finders/scim_finder_spec.rb

@@ -10,9 +10,17 @@ describe ScimFinder do
 
   describe '#search' do
     context 'without a SAML provider' do
-      it 'returns an empty relation when there is no saml provider' do
+      it 'returns an empty identity relation when scim_identities is disabled' do
+        stub_feature_flags(scim_identities: false)
+
         expect(finder.search(unused_params)).to eq Identity.none
       end
+
+      it 'returns an empty scim identity relation when scim_identities is enabled' do
+        stub_feature_flags(scim_identities: true)
+
+        expect(finder.search(unused_params)).to eq ScimIdentity.none
+      end
     end
 
     context 'SCIM/SAML is not enabled' do
@@ -20,35 +28,70 @@ describe ScimFinder do
         create(:saml_provider, group: group, enabled: false)
       end
 
-      it 'returns an empty relation when SCIM/SAML is not enabled' do
+      it 'returns an empty identity relation when scim_identities is disabled' do
+        stub_feature_flags(scim_identities: false)
+
         expect(finder.search(unused_params)).to eq Identity.none
       end
+
+      it 'returns an empty scim identity relation when scim_identities is enabled' do
+        stub_feature_flags(scim_identities: true)
+
+        expect(finder.search(unused_params)).to eq ScimIdentity.none
+      end
     end
 
     context 'with SCIM enabled' do
       let!(:saml_provider) { create(:saml_provider, group: group) }
 
       context 'with an eq filter' do
-        let!(:identity) { create(:group_saml_identity, saml_provider: saml_provider) }
-        let!(:other_identity) { create(:group_saml_identity, saml_provider: saml_provider) }
-
+        shared_examples 'valid lookups' do
           it 'allows identity lookup by id/externalId' do
-          expect(finder.search(filter: "id eq #{identity.extern_uid}")).to be_a ActiveRecord::Relation
-          expect(finder.search(filter: "id eq #{identity.extern_uid}").first).to eq identity
-          expect(finder.search(filter: "externalId eq #{identity.extern_uid}").first).to eq identity
+            expect(finder.search(filter: "id eq #{id.extern_uid}")).to be_a ActiveRecord::Relation
+            expect(finder.search(filter: "id eq #{id.extern_uid}").first).to eq id
+            expect(finder.search(filter: "externalId eq #{id.extern_uid}").first).to eq id
           end
 
           it 'allows lookup by userName' do
-          expect(finder.search(filter: "userName eq \"#{identity.user.username}\"").first).to eq identity
+            expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id
+          end
+        end
+
+        context 'when scim_identities is disabled' do
+          before do
+            stub_feature_flags(scim_identities: false)
+          end
+          let(:id) { create(:group_saml_identity, saml_provider: saml_provider) }
+
+          it_behaves_like 'valid lookups'
+        end
+
+        context 'when scim_identities is enabled' do
+          before do
+            stub_feature_flags(scim_identities: true)
+          end
+          let(:id) { create(:scim_identity, group: group) }
+
+          it_behaves_like 'valid lookups'
         end
       end
 
-      it 'returns all related identities if there is no filter' do
+      context 'with no filter' do
+        it 'returns all related identities when scim_identities is disabled' do
+          stub_feature_flags(scim_identities: false)
           create_list(:group_saml_identity, 2, saml_provider: saml_provider)
 
           expect(finder.search({}).count).to eq 2
         end
 
+        it 'returns all related identities when scim_identities is enabled' do
+          stub_feature_flags(scim_identities: true)
+          create_list(:scim_identity, 4, group: group)
+
+          expect(finder.search({}).count).to eq 4
+        end
+      end
+
       it 'raises an error if the filter is unsupported' do
         expect { finder.search(filter: 'id ne 1').count }.to raise_error(ScimFinder::UnsupportedFilter)
       end

ee/spec/models/scim_identity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ScimIdentity do
+  describe 'relations' do
+    it { is_expected.to belong_to(:group) }
+    it { is_expected.to belong_to(:user) }
+  end
+
+  describe 'validations' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:group) { create(:group) }
+
+    context 'with existing user and group' do
+      before do
+        create(:scim_identity, user: user, group: group, extern_uid: user.email)
+      end
+
+      it 'returns false for a duplicate identity with the same extern_uid' do
+        identity = user.scim_identities.build(group: group, extern_uid: user.email)
+
+        expect(identity.validate).to eq(false)
+      end
+
+      it 'returns false for a duplicate identity with different extern_uid' do
+        identity = user.scim_identities.build(group: group, extern_uid: '1234abcd')
+
+        expect(identity.validate).to eq(false)
+      end
+
+      it 'returns true when a different group is used' do
+        other_group = create(:group)
+        identity = user.scim_identities.build(group: other_group, extern_uid: user.email)
+
+        expect(identity.validate).to eq(true)
+      end
+
+      it 'returns false for a duplicate extern_uid with different case' do
+        identity = user.scim_identities.build(group: group, extern_uid: user.email.upcase)
+
+        expect(identity.validate).to eq(false)
+      end
+    end
+  end
+
+  describe '.with_extern_uid' do
+    it 'finds identity regardless of case' do
+      user = create(:user)
+      group = create(:group)
+
+      identity = user.scim_identities.create(group: group, extern_uid: user.email)
+
+      expect(group.scim_identities.with_extern_uid(user.email.upcase).first).to eq identity
+    end
+  end
+end

ee/spec/requests/api/scim_spec.rb

@@ -9,6 +9,7 @@ describe API::Scim do
 
   before do
     stub_licensed_features(group_allowed_email_domains: true, group_saml: true)
+    stub_feature_flags(scim_identities: false)
 
     group.add_owner(user)
   end