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

Full SCIM Identities Support

See merge request gitlab-org/gitlab!26704

app/services/users/build_service.rb

@@ -28,9 +28,7 @@ module Users
         end
       end
 
-      unless identity_params.empty?
-        user.identities.build(identity_params)
-      end
+      build_identity(user)
 
       user
     end
@@ -41,6 +39,12 @@ module Users
       [:extern_uid, :provider]
     end
 
+    def build_identity(user)
+      return if identity_params.empty?
+
+      user.identities.build(identity_params)
+    end
+
     def can_create_user?
       (current_user.nil? && Gitlab::CurrentSettings.allow_signup?) || current_user&.admin?
     end

ee/app/finders/scim_finder.rb

@@ -23,7 +23,7 @@ class ScimFinder
 
   def scim_identities_enabled?
     strong_memoize(:scim_identities_enabled) do
-      Feature.enabled?(:scim_identities, group)
+      ::EE::Gitlab::Scim::Feature.scim_identities_enabled?(group)
     end
   end
 
@@ -47,9 +47,9 @@ class ScimFinder
     parser = EE::Gitlab::Scim::ParamsParser.new(params)
 
     if eq_filter_on_extern_uid?(parser)
-      by_extern_uid(parser)
+      by_extern_uid(parser.filter_params[:extern_uid])
     elsif eq_filter_on_username?(parser)
-      by_username(parser)
+      by_username(parser.filter_params[:username])
     else
       raise UnsupportedFilter
     end
@@ -59,18 +59,18 @@ class ScimFinder
     parser.filter_operator == :eq && parser.filter_params[:extern_uid].present?
   end
 
-  def by_extern_uid(parser)
-    return group.scim_identities.with_extern_uid(parser.filter_params[:extern_uid]) if scim_identities_enabled?
+  def by_extern_uid(extern_uid)
+    return group.scim_identities.with_extern_uid(extern_uid) if scim_identities_enabled?
 
-    Identity.where_group_saml_uid(saml_provider, parser.filter_params[:extern_uid])
+    Identity.where_group_saml_uid(saml_provider, extern_uid)
   end
 
   def eq_filter_on_username?(parser)
     parser.filter_operator == :eq && parser.filter_params[:username].present?
   end
 
-  def by_username(parser)
-    user = User.find_by_username(parser.filter_params[:username])
+  def by_username(username)
+    user = User.find_by_username(username) || User.find_by_any_email(username)
 
     return group.scim_identities.for_user(user) if scim_identities_enabled?
 

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

@@ -51,6 +51,13 @@ module EE
         super.push(:saml_provider_id)
       end
 
+      override :build_identity
+      def build_identity(user)
+        return build_scim_identity(user) if params[:provider] == 'group_scim'
+
+        super
+      end
+
       override :identity_params
       def identity_params
         if group_id_for_saml.present?
@@ -60,6 +67,10 @@ module EE
         end
       end
 
+      def scim_identity_attributes
+        [:group_id, :extern_uid]
+      end
+
       def saml_provider_id
         strong_memoize(:saml_provider_id) do
           group = GroupFinder.new(current_user).execute(id: group_id_for_saml)
@@ -75,6 +86,12 @@ module EE
                                           issuer: params[:certificate_issuer])
         end
       end
+
+      def build_scim_identity(user)
+        scim_identity_params = params.slice(*scim_identity_attributes)
+
+        user.scim_identities.build(scim_identity_params.merge(active: true))
+      end
     end
   end
 end

ee/lib/api/scim.rb

@@ -2,6 +2,8 @@
 
 module API
   class Scim < Grape::API
+    include ::Gitlab::Utils::StrongMemoize
+
     prefix 'api/scim'
     version 'v2'
     content_type :json, 'application/scim+json'
@@ -19,16 +21,6 @@ module API
           API.logger
         end
 
-        def destroy_identity(identity)
-          GroupSaml::Identity::DestroyService.new(identity).execute(transactional: true)
-
-          true
-        rescue => e
-          logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
-
-          false
-        end
-
         def render_scim_error(error_class, message)
           error!({ with: error_class }.merge(detail: message), error_class::STATUS)
         end
@@ -69,25 +61,70 @@ module API
           parsed_hash = parser.update_params
 
           if parser.deprovision_user?
-            destroy_identity(identity)
+            deprovision(identity)
+          elsif reprovisionable?(identity) && parser.reprovision_user?
+            reprovision(identity)
           elsif parsed_hash[:extern_uid]
             identity.update(parsed_hash.slice(:extern_uid))
           else
             scim_conflict!(message: 'Email has already been taken') if email_taken?(parsed_hash[:email], identity)
 
             result = ::Users::UpdateService.new(identity.user,
-                                                parsed_hash.except(:extern_uid)
+                                                parsed_hash.except(:extern_uid, :active)
                                                   .merge(user: identity.user)).execute
 
             result[:status] == :success
           end
         end
 
+        def reprovisionable?(identity)
+          return true if identity.respond_to?(:active) && !identity.active?
+
+          false
+        end
+
         def email_taken?(email, identity)
           return unless email
 
           User.by_any_email(email.downcase).where.not(id: identity.user.id).exists?
         end
+
+        def find_user_identity(group, extern_uid)
+          return unless group.saml_provider
+          return group.scim_identities.with_extern_uid(extern_uid).first if scim_identities_enabled?
+
+          GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: extern_uid)
+        end
+
+        def scim_identities_enabled?
+          strong_memoize(:scim_identities_enabled) do
+            ::EE::Gitlab::Scim::Feature.scim_identities_enabled?(@group)
+          end
+        end
+
+        def deprovision(identity)
+          if scim_identities_enabled?
+            ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
+          else
+            GroupSaml::Identity::DestroyService.new(identity).execute(transactional: true)
+          end
+
+          true
+        rescue => e
+          logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
+
+          false
+        end
+
+        def reprovision(identity)
+          ::EE::Gitlab::Scim::ReprovisionService.new(identity).execute
+
+          true
+        rescue => e
+          logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
+
+          false
+        end
       end
 
       resource :Users do
@@ -118,7 +155,7 @@ module API
         get ':id', requirements: USER_ID_REQUIREMENTS do
           group = find_and_authenticate_group!(params[:group])
 
-          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
+          identity = find_user_identity(group, params[:id])
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
@@ -154,7 +191,7 @@ module API
           scim_error!(message: 'Missing ID') unless params[:id]
 
           group = find_and_authenticate_group!(params[:group])
-          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
+          identity = find_user_identity(group, params[:id])
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
@@ -174,11 +211,11 @@ module API
           scim_error!(message: 'Missing ID') unless params[:id]
 
           group = find_and_authenticate_group!(params[:group])
-          identity = GroupSamlIdentityFinder.find_by_group_and_uid(group: group, uid: params[:id])
+          identity = find_user_identity(group, params[:id])
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
-          destroyed = destroy_identity(identity)
+          destroyed = deprovision(identity)
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed
 

ee/lib/ee/api/entities/scim/user.rb

@@ -29,9 +29,11 @@ module EE
           end
 
           def active
-            # We don't block the user yet when deprovisioning
-            # So the user is always active, until the identity link is removed.
-            true
+            object_active = object.try(:active)
+
+            return true if object_active.nil?
+
+            object_active
           end
 
           def email_type

ee/lib/ee/gitlab/scim/deprovision_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Scim
+      class DeprovisionService
+        attr_reader :identity
+
+        delegate :user, :group, to: :identity
+
+        def initialize(identity)
+          @identity = identity
+        end
+
+        def execute
+          ScimIdentity.transaction do
+            identity.update!(active: false)
+            remove_group_access
+          end
+        end
+
+        private
+
+        def remove_group_access
+          return unless group_membership
+          return if group.last_owner?(user)
+
+          ::Members::DestroyService.new(user).execute(group_membership)
+        end
+
+        def group_membership
+          @group_membership ||= group.group_member(user)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/scim/feature.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Scim
+      class Feature
+        def self.scim_identities_enabled?(group)
+          ::Feature.enabled?(:scim_identities, group)
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/scim/params_parser.rb

@@ -15,6 +15,10 @@ module EE
           update_params[:active] == false
         end
 
+        def reprovision_user?
+          !deprovision_user?
+        end
+
         def post_params
           @post_params ||= process_post_params
         end

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

@@ -6,7 +6,6 @@ module EE
       class ProvisioningService
         include ::Gitlab::Utils::StrongMemoize
 
-        IDENTITY_PROVIDER = 'group_saml'
         PASSWORD_AUTOMATICALLY_SET = true
         SKIP_EMAIL_CONFIRMATION = false
         DEFAULT_ACCESS = :guest
@@ -39,9 +38,27 @@ module EE
           ProvisioningResponse.new(status: :success, identity: identity)
         end
 
+        def scim_identities_enabled?
+          strong_memoize(:scim_identities_enabled) do
+            ::EE::Gitlab::Scim::Feature.scim_identities_enabled?(@group)
+          end
+        end
+
+        def identity_provider
+          strong_memoize(:identity_provider) do
+            next 'group_scim' if scim_identities_enabled?
+
+            'group_saml'
+          end
+        end
+
         def identity
           strong_memoize(:identity) do
-            ::Identity.with_extern_uid(IDENTITY_PROVIDER, @parsed_hash[:extern_uid]).first
+            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
           end
         end
 
@@ -67,8 +84,9 @@ module EE
         def user_params
           @parsed_hash.tap do |hash|
             hash[:skip_confirmation] = SKIP_EMAIL_CONFIRMATION
-            hash[:saml_provider_id] = @group.saml_provider.id
-            hash[:provider] = IDENTITY_PROVIDER
+            hash[:saml_provider_id] = @group.saml_provider&.id
+            hash[:group_id] = @group.id
+            hash[:provider] = identity_provider
             hash[:email_confirmation] = hash[:email]
             hash[:username] = valid_username
             hash[:password] = hash[:password_confirmation] = random_password

ee/lib/ee/gitlab/scim/reprovision_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Scim
+      class ReprovisionService
+        attr_reader :identity
+
+        delegate :user, :group, to: :identity
+
+        DEFAULT_ACCESS = :guest
+
+        def initialize(identity)
+          @identity = identity
+        end
+
+        def execute
+          ScimIdentity.transaction do
+            identity.update!(active: true)
+            add_member unless existing_member?
+          end
+        end
+
+        private
+
+        def add_member
+          group.add_user(user, DEFAULT_ACCESS)
+        end
+
+        def existing_member?
+          ::GroupMember.member_of_group?(group, user)
+        end
+      end
+    end
+  end
+end

ee/spec/finders/scim_finder_spec.rb

@@ -55,6 +55,10 @@ describe ScimFinder do
           it 'allows lookup by userName' do
             expect(finder.search(filter: "userName eq \"#{id.user.username}\"").first).to eq id
           end
+
+          it 'allows lookup by userName when userName is an email address' do
+            expect(finder.search(filter: "userName eq #{id.user.email}").first).to eq id
+          end
         end
 
         context 'when scim_identities is disabled' do

ee/spec/lib/ee/api/entities/scim/user_spec.rb

@@ -39,4 +39,14 @@ describe ::EE::API::Entities::Scim::User do
   it 'contains the resource type' do
     expect(subject[:meta][:resourceType]).to eq('User')
   end
+
+  context 'with a SCIM identity' do
+    let(:identity) { build(:scim_identity, user: user) }
+
+    it 'contains active false when the identity is not active' do
+      identity.active = false
+
+      expect(subject[:active]).to be false
+    end
+  end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::EE::Gitlab::Scim::DeprovisionService do
+  describe '#execute' do
+    let_it_be(:identity) { create(:scim_identity, active: true) }
+    let_it_be(:group) { identity.group }
+    let_it_be(:user) { identity.user }
+
+    let(:service) { described_class.new(identity) }
+
+    it 'deactivates scim identity' do
+      service.execute
+
+      expect(identity.active).to be false
+    end
+
+    it 'removes group access' do
+      create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
+
+      service.execute
+
+      expect(group.members.pluck(:user_id)).not_to include(user.id)
+    end
+
+    it 'does not remove the last owner' do
+      create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
+
+      service.execute
+
+      expect(identity.group.members.pluck(:user_id)).to include(user.id)
+    end
+
+    it 'does not change group membership when the user is not a member' do
+      expect { service.execute }.not_to change { group.members.count }
+    end
+  end
+end

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

@@ -98,4 +98,18 @@ describe EE::Gitlab::Scim::ParamsParser do
       expect(described_class.new(Operations: operations).deprovision_user?).to be false
     end
   end
+
+  describe '#reprovision_user?' do
+    it 'returns true when reprovisioning' do
+      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'True' }]
+
+      expect(described_class.new(Operations: operations).reprovision_user?).to be true
+    end
+
+    it 'returns false when not reprovisioning' do
+      operations = [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }]
+
+      expect(described_class.new(Operations: operations).reprovision_user?).to be false
+    end
+  end
 end

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

@@ -9,86 +9,138 @@ describe ::EE::Gitlab::Scim::ProvisioningService do
 
     before do
       stub_licensed_features(group_saml: true)
-      create(:saml_provider, group: group)
     end
 
-    context 'valid params' do
-      let(:service_params) do
-        {
-          email: 'work@example.com',
-          name: 'Test Name',
-          extern_uid: 'test_uid',
-          username: 'username'
-        }.freeze
-      end
+    shared_examples 'scim provisioning' do
+      context 'valid params' do
+        let_it_be(:service_params) do
+          {
+            email: 'work@example.com',
+            name: 'Test Name',
+            extern_uid: 'test_uid',
+            username: 'username'
+          }
+        end
 
-      it 'succeeds' do
-        expect(service.execute.status).to eq(:success)
-      end
+        def user
+          User.find_by(email: service_params[:email])
+        end
 
-      it 'creates the identity' do
-        expect { service.execute }.to change { Identity.count }.by(1)
-      end
+        it 'succeeds' do
+          expect(service.execute.status).to eq(:success)
+        end
 
-      it 'creates the user' do
-        expect { service.execute }.to change { User.count }.by(1)
-      end
+        it 'creates the user' do
+          expect { service.execute }.to change { User.count }.by(1)
+        end
 
-      it 'creates the group member' do
-        expect { service.execute }.to change { GroupMember.count }.by(1)
-      end
+        it 'creates the group member' do
+          expect { service.execute }.to change { GroupMember.count }.by(1)
+        end
 
-      it 'creates the correct user attributes' do
-        service.execute
+        it 'creates the correct user attributes' do
+          service.execute
 
-        expect(User.find_by(service_params.except(:extern_uid))).to be_a(User)
-      end
+          expect(user).to be_a(User)
+        end
 
-      it 'user record requires confirmation' do
-        service.execute
+        it 'creates the member with guest access level' do
+          service.execute
 
-        user = User.find_by(email: service_params[:email])
+          access_level = group.group_member(user).access_level
 
-        expect(user).to be_present
-        expect(user).not_to be_confirmed
-      end
+          expect(access_level).to eq(Gitlab::Access::GUEST)
+        end
 
-      context 'when the current minimum password length is different from the default minimum password length' do
-        before do
-          stub_application_setting minimum_password_length: 21
+        it 'user record requires confirmation' do
+          service.execute
+
+          expect(user).to be_present
+          expect(user).not_to be_confirmed
         end
 
-        it 'creates the user' do
-          expect { service.execute }.to change { User.count }.by(1)
+        context 'when the current minimum password length is different from the default minimum password length' do
+          before do
+            stub_application_setting minimum_password_length: 21
+          end
+
+          it 'creates the user' do
+            expect { service.execute }.to change { User.count }.by(1)
+          end
         end
-      end
 
-      context 'existing user' do
-        before do
-          create(:user, email: 'work@example.com')
+        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
 
-        it 'does not create a new user' do
-          expect { service.execute }.not_to change { User.count }
+      context 'invalid params' do
+        let_it_be(:service_params) do
+          {
+            email: 'work@example.com',
+            name: 'Test Name',
+            extern_uid: 'test_uid'
+          }
         end
 
-        it 'fails with conflict' do
-          expect(service.execute.status).to eq(:conflict)
+        it 'fails with error' do
+          expect(service.execute.status).to eq(:error)
         end
       end
     end
 
-    context 'invalid params' do
-      let(:service_params) do
+    context 'when scim_identities is disabled' do
+      before do
+        stub_feature_flags(scim_identities: false)
+        create(:saml_provider, group: group)
+      end
+
+      let_it_be(:service_params) do
         {
           email: 'work@example.com',
           name: 'Test Name',
-          extern_uid: 'test_uid'
-        }.freeze
+          extern_uid: 'test_uid',
+          username: 'username'
+        }
       end
 
-      it 'fails with error' do
-        expect(service.execute.status).to eq(:error)
+      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
+    end
+
+    context 'when scim_identities is enabled' do
+      before do
+        stub_feature_flags(scim_identities: true)
+      end
+
+      let_it_be(:service_params) do
+        {
+          email: 'work@example.com',
+          name: 'Test Name',
+          extern_uid: 'test_uid',
+          username: 'username'
+        }
+      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
     end
   end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::EE::Gitlab::Scim::ReprovisionService do
+  describe '#execute' do
+    let_it_be(:identity) { create(:scim_identity, active: false) }
+    let_it_be(:group) { identity.group }
+    let_it_be(:user) { identity.user }
+
+    let(:service) { described_class.new(identity) }
+
+    it 'activates scim identity' do
+      service.execute
+
+      expect(identity.active).to be true
+    end
+
+    it 'creates the member' do
+      service.execute
+
+      expect(group.members.pluck(:user_id)).to include(user.id)
+    end
+
+    it 'creates the member with guest access level' do
+      service.execute
+
+      access_level = group.group_member(user).access_level
+
+      expect(access_level).to eq(Gitlab::Access::GUEST)
+    end
+
+    it 'does not change group membership when the user is already a member' do
+      create(:group_member, group: group, user: user)
+
+      expect { service.execute }.not_to change { group.members.count }
+    end
+  end
+end

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

@@ -23,6 +23,23 @@ describe Users::BuildService 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)
+
+            service.execute
+          end
+        end
+
+        context 'with scim identity' do
+          before do
+            params.merge!(scim_identity_params)
+          end
+          let_it_be(:scim_identity_params) { { extern_uid: 'uid', provider: 'group_scim', group_id: 1 } }
+
+          it 'passes allowed attributes to scim 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)
 
             service.execute
           end