Return errors if deprovision unsuccessful

Return errors before updating active false

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57178

https://gitlab.com/gitlab-org/gitlab/-/issues/321883
https://gitlab.com/gitlab-org/gitlab/-/issues/325729

ee/changelogs/unreleased/sfang-deprovision-service-error.yml

+---
+title: Allow SCIM deprovisioning for minimal access users
+merge_request: 57178
+author:
+type: fixed

ee/lib/api/scim.rb

@@ -69,7 +69,7 @@ module API
           parsed_hash = parser.update_params
 
           if parser.deprovision_user?
-            deprovision(identity)
+            patch_deprovision(identity)
           elsif reprovisionable?(identity) && parser.reprovision_user?
             reprovision(identity)
           elsif parsed_hash[:extern_uid]
@@ -103,14 +103,28 @@ module API
           group.scim_identities.with_extern_uid(extern_uid).first
         end
 
-        def deprovision(identity)
-          ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
+        # delete_deprovision handles the response and returns either no_content! or a detailed error message.
+        def delete_deprovision(identity)
+          service = ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
 
-          true
-        rescue => e
-          logger.error(identity: identity, error: e.class.name, message: e.message, source: "#{__FILE__}:#{__LINE__}")
+          if service.success?
+            no_content!
+          else
+            logger.error(identity: identity, error: service.class.name, message: service.message, source: "#{__FILE__}:#{__LINE__}")
+            scim_error!(message: service.message)
+          end
+        end
 
-          false
+        # The method that calls patch_deprovision, update_scim_user, expects a truthy/falsey value, and then continues to handle the request.
+        def patch_deprovision(identity)
+          service = ::EE::Gitlab::Scim::DeprovisionService.new(identity).execute
+
+          if service.success?
+            true
+          else
+            logger.error(identity: identity, error: service.class.name, message: service.message, source: "#{__FILE__}:#{__LINE__}")
+            false
+          end
         end
 
         def reprovision(identity)
@@ -212,11 +226,7 @@ module API
 
           scim_not_found!(message: "Resource #{params[:id]} not found") unless identity
 
-          destroyed = deprovision(identity)
-
-          scim_not_found!(message: "Resource #{params[:id]} not found") unless destroyed
-
-          no_content!
+          delete_deprovision(identity)
         end
       end
     end

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

@@ -13,23 +13,29 @@ module EE
         end
 
         def execute
+          return error(_("Could not remove %{user} from %{group}. User is not a group member.") % { user: user.name, group: group.name }) unless group_membership
+          return error(_("Could not remove %{user} from %{group}. Cannot remove last group owner.") % { user: user.name, group: group.name }) if group.last_owner?(user)
+
           ScimIdentity.transaction do
             identity.update!(active: false)
             remove_group_access
           end
+
+          ServiceResponse.success(message: _("User %{user} was removed from %{group}.") % { user: user.name, group: group.name })
         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)
+          @group_membership ||= group.all_group_members.with_user(user).first
+        end
+
+        def error(message)
+          ServiceResponse.error(message: message)
         end
       end
     end

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

@@ -4,36 +4,89 @@ require 'spec_helper'
 
 RSpec.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(:identity) { create(:scim_identity, active: true) }
+    let(:group) { identity.group }
+    let(:user) { identity.user }
 
     let(:service) { described_class.new(identity) }
 
-    it 'deactivates scim identity' do
-      service.execute
+    context 'when user is successfully removed' do
+      before do
+        create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
+      end
 
-      expect(identity.active).to be false
-    end
+      it 'deactivates scim identity' do
+        expect { service.execute }.to change { identity.active }.from(true).to(false)
+      end
+
+      it 'removes group access' do
+        service.execute
 
-    it 'removes group access' do
-      create(:group_member, group: group, user: user, access_level: GroupMember::REPORTER)
+        expect(group.all_group_members.pluck(:user_id)).not_to include(user.id)
+      end
 
-      service.execute
+      it 'returns the successful deprovision message' do
+        response = service.execute
 
-      expect(group.members.pluck(:user_id)).not_to include(user.id)
+        expect(response.message).to include("User #{user.name} was removed from #{group.name}.")
+      end
     end
 
-    it 'does not remove the last owner' do
-      create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
+    context 'with minimal access role' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, group: group, user: user, access_level: ::Gitlab::Access::MINIMAL_ACCESS)
+      end
+
+      it 'deactivates scim identity' do
+        expect { service.execute }.to change { identity.active }.from(true).to(false)
+      end
 
-      service.execute
+      it 'removes group access' do
+        service.execute
 
-      expect(identity.group.members.pluck(:user_id)).to include(user.id)
+        expect(group.all_group_members.pluck(:user_id)).not_to include(user.id)
+      end
+
+      it 'returns the successful deprovision message' do
+        response = service.execute
+
+        expect(response.message).to include("User #{user.name} was removed from #{group.name}.")
+      end
     end
 
-    it 'does not change group membership when the user is not a member' do
-      expect { service.execute }.not_to change { group.members.count }
+    context 'when user is not successfully removed' do
+      context 'when user is the last owner' do
+        before do
+          create(:group_member, group: group, user: user, access_level: GroupMember::OWNER)
+        end
+
+        it 'does not remove the last owner' do
+          service.execute
+
+          expect(identity.group.members.pluck(:user_id)).to include(user.id)
+        end
+
+        it 'returns the last group owner error' do
+          response = service.execute
+
+          expect(response.error?).to be true
+          expect(response.errors).to include("Could not remove #{user.name} from #{group.name}. Cannot remove last group owner.")
+        end
+      end
+
+      context 'when user is not a group member' do
+        it 'does not change group membership when the user is not a member' do
+          expect { service.execute }.not_to change { group.members.count }
+        end
+
+        it 'returns the group membership error' do
+          response = service.execute
+
+          expect(response.error?).to be true
+          expect(response.errors).to include("Could not remove #{user.name} from #{group.name}. User is not a group member.")
+        end
+      end
     end
   end
 end

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

@@ -38,5 +38,16 @@ RSpec.describe ::EE::Gitlab::Scim::ReprovisionService do
 
       expect { service.execute }.not_to change { group.members.count }
     end
+
+    context 'with minimal access user' do
+      before do
+        stub_licensed_features(minimal_access_role: true)
+        create(:group_member, group: group, user: user, access_level: ::Gitlab::Access::MINIMAL_ACCESS)
+      end
+
+      it 'does not change group membership when the user is already a member' do
+        expect { service.execute }.not_to change { group.all_group_members.count }
+      end
+    end
   end
 end

ee/spec/requests/api/scim_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe API::Scim do
 
   before do
     stub_licensed_features(group_allowed_email_domains: true, group_saml: true)
-    group.add_owner(user)
+    group.add_developer(user)
     create(:saml_provider, group: group, default_membership_role: Gitlab::Access::DEVELOPER)
   end
 
@@ -337,6 +337,27 @@ RSpec.describe API::Scim do
         expect(identity.reload.active).to be false
       end
 
+      context 'with owner' do
+        params = { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'False' }] }.to_query
+
+        before do
+          group.add_owner(user)
+          call_patch_api(params)
+        end
+
+        it 'responds with 412' do
+          expect(response).to have_gitlab_http_status(:precondition_failed)
+        end
+
+        it 'returns the last group owner error' do
+          expect(response.body).to include("Error updating #{user.name}")
+        end
+
+        it 'does not deactivate the identity' do
+          expect(identity.reload.active).to be true
+        end
+      end
+
       context 'Reprovision user' do
         let_it_be(:params) { { Operations: [{ 'op': 'Replace', 'path': 'active', 'value': 'true' }] }.to_query }
 
@@ -450,6 +471,25 @@ RSpec.describe API::Scim do
         end
       end
 
+      context 'with owner' do
+        before do
+          group.add_owner(user)
+          delete scim_api("scim/v2/groups/#{group.full_path}/Users/#{identity.extern_uid}")
+        end
+
+        it 'responds with 412' do
+          expect(response).to have_gitlab_http_status(:precondition_failed)
+        end
+
+        it 'returns the last group owner error' do
+          expect(response.body).to include("Could not remove #{user.name} from #{group.name}. Cannot remove last group owner.")
+        end
+
+        it 'does not deactivate the identity' do
+          expect(identity.reload.active).to be true
+        end
+      end
+
       it 'responds with 404 if there is no user' do
         delete scim_api("scim/v2/groups/#{group.full_path}/Users/123")
 

locale/gitlab.pot

@@ -8726,6 +8726,12 @@ msgstr ""
 msgid "Could not load usage counts. Please refresh the page to try again."
 msgstr ""
 
+msgid "Could not remove %{user} from %{group}. Cannot remove last group owner."
+msgstr ""
+
+msgid "Could not remove %{user} from %{group}. User is not a group member."
+msgstr ""
+
 msgid "Could not remove the trigger."
 msgstr ""
 
@@ -33165,6 +33171,9 @@ msgstr ""
 msgid "User %{username} was successfully removed."
 msgstr ""
 
+msgid "User %{user} was removed from %{group}."
+msgstr ""
+
 msgid "User ID"
 msgstr ""