Merge branch '35365-bugfix-gma-group-members-cleanup' into 'master'

Add group cleanup for GMA when SAML provider is created

Closes #35365

See merge request gitlab-org/gitlab!20157

changelogs/unreleased/35365-bugfix-gma-group-members-cleanup.yml

+---
+title: Fix group managed accounts members cleanup
+merge_request: 20157
+author:
+type: fixed

ee/app/controllers/groups/saml_providers_controller.rb

@@ -18,9 +18,11 @@ class Groups::SamlProvidersController < Groups::ApplicationController
   end
 
   def create
-    @saml_provider = @group.build_saml_provider(saml_provider_params)
+    create_service = GroupSaml::SamlProvider::CreateService.new(current_user, @group, params: saml_provider_params)
 
-    @saml_provider.save
+    create_service.execute
+
+    @saml_provider = create_service.saml_provider
 
     render :show
   end

ee/app/services/group_saml/saml_provider/base_service.rb

+# frozen_string_literal: true
+
+module GroupSaml
+  module SamlProvider
+    module BaseService
+      extend FastGettext::Translation
+
+      attr_reader :saml_provider, :params, :current_user
+
+      delegate :group, to: :saml_provider
+
+      def initialize(current_user, saml_provider, params:)
+        @saml_provider = saml_provider
+        @current_user = current_user
+        @params = params
+      end
+
+      def execute
+        ::SamlProvider.transaction do
+          group_managed_accounts_was_enforced = saml_provider.enforced_group_managed_accounts?
+
+          updated = saml_provider.update(params)
+
+          if updated && saml_provider.enforced_group_managed_accounts? && !group_managed_accounts_was_enforced
+            cleanup_members!
+          end
+        end
+      end
+
+      private
+
+      def cleanup_members!
+        return if GroupManagedAccounts::CleanUpMembersService.new(current_user, group).execute
+
+        saml_provider.errors.add(:base, _("Can't remove group members without group managed account"))
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+end

ee/app/services/group_saml/saml_provider/create_service.rb

+# frozen_string_literal: true
+
+module GroupSaml
+  module SamlProvider
+    class CreateService
+      include BaseService
+
+      def initialize(current_user, group, params:)
+        @group = group
+        super(current_user, group.build_saml_provider, params: params)
+      end
+    end
+  end
+end

ee/app/services/group_saml/saml_provider/update_service.rb

@@ -3,37 +3,7 @@
 module GroupSaml
   module SamlProvider
     class UpdateService
-      extend FastGettext::Translation
-
-      attr_reader :saml_provider, :params, :current_user
-
-      delegate :group, to: :saml_provider
-
-      def initialize(current_user, saml_provider, params:)
-        @current_user = current_user
-        @saml_provider = saml_provider
-        @params = params
-      end
-
-      def execute
-        ::SamlProvider.transaction do
-          group_managed_accounts_was_enforced = saml_provider.enforced_group_managed_accounts?
-          saml_provider.update(params)
-
-          if saml_provider.enforced_group_managed_accounts? && !group_managed_accounts_was_enforced
-            cleanup_members
-          end
-        end
-      end
-
-      private
-
-      def cleanup_members
-        return if GroupManagedAccounts::CleanUpMembersService.new(current_user, group).execute
-
-        saml_provider.errors.add(:base, _("Can't remove group members without group managed account"))
-        raise ActiveRecord::Rollback
-      end
+      include BaseService
     end
   end
 end

ee/spec/services/group_saml/saml_provider/create_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupSaml::SamlProvider::CreateService do
+  subject(:service) { described_class.new(nil, group, params: params) }
+
+  let(:group) { create :group }
+
+  include_examples 'base SamlProvider service'
+end

ee/spec/services/group_saml/saml_provider/update_service_spec.rb

@@ -5,83 +5,10 @@ require 'spec_helper'
 describe GroupSaml::SamlProvider::UpdateService do
   subject(:service) { described_class.new(nil, saml_provider, params: params) }
 
-  let(:params) do
-    {
-      sso_url: 'https://test',
-      certificate_fingerprint: fingerprint,
-      enabled: true,
-      enforced_sso: true,
-      enforced_group_managed_accounts: true
-    }
-  end
   let(:saml_provider) do
     create :saml_provider, enabled: false, enforced_sso: false, enforced_group_managed_accounts: enforced_group_managed_accounts
   end
-  let(:enforced_group_managed_accounts) { false }
-  let(:fingerprint) { '11:22:33:44:55:66:77:88:99:11:22:33:44:55:66:77:88:99' }
-  let(:cleanup_members_service_spy) { spy('GroupSaml::GroupManagedAccounts::CleanUpMembersService') }
-
-  before do
-    allow(GroupSaml::GroupManagedAccounts::CleanUpMembersService)
-      .to receive(:new).with(nil, saml_provider.group).and_return(cleanup_members_service_spy)
-  end
+  let(:group) { saml_provider.group }
 
-  it 'updates SAML provider with given params' do
-    expect do
-      service.execute
-      saml_provider.reload
-    end.to change { saml_provider.sso_url }.to('https://test')
-             .and change { saml_provider.certificate_fingerprint }.to(fingerprint)
-             .and change { saml_provider.enabled? }.to(true)
-             .and change { saml_provider.enforced_sso? }.to(true)
-             .and change { saml_provider.enforced_group_managed_accounts? }.to(true)
-  end
-
-  context 'when enforced_group_managed_accounts is enabled' do
-    it 'cleans up group members' do
-      service.execute
-
-      expect(cleanup_members_service_spy).to have_received(:execute)
-    end
-
-    context 'when clean up fails' do
-      before do
-        allow(cleanup_members_service_spy).to receive(:execute).and_return(false)
-      end
-
-      it 'adds an error to saml provider' do
-        expect { service.execute }.to change { saml_provider.errors[:base] }
-                 .to(["Can't remove group members without group managed account"])
-      end
-
-      it 'does not update saml_provider' do
-        expect do
-          service.execute
-          saml_provider.reload
-        end.not_to change { saml_provider.enforced_group_managed_accounts? }
-      end
-    end
-  end
-
-  context 'when group managed accounts was enabled beforehand' do
-    let(:enforced_group_managed_accounts) { true }
-    let(:params) { { enforced_group_managed_accounts: true } }
-
-    it 'does not clean up group members' do
-      service.execute
-
-      expect(cleanup_members_service_spy).not_to have_received(:execute)
-    end
-  end
-
-  context 'when enforced_group_managed_accounts is disabled' do
-    let(:enforced_group_managed_accounts) { true }
-    let(:params) { { enforced_group_managed_accounts: false } }
-
-    it 'does  not clean up group members' do
-      service.execute
-
-      expect(cleanup_members_service_spy).not_to have_received(:execute)
-    end
-  end
+  include_examples 'base SamlProvider service'
 end

ee/spec/support/shared_examples/services/group_saml/saml_provider/base_service_shared_examples.rb

+# frozen_string_literal: true
+
+shared_examples 'base SamlProvider service' do
+  let(:params) do
+    {
+      sso_url: 'https://test',
+      certificate_fingerprint: fingerprint,
+      enabled: true,
+      enforced_sso: true,
+      enforced_group_managed_accounts: true
+    }
+  end
+
+  let(:enforced_group_managed_accounts) { false }
+  let(:fingerprint) { '11:22:33:44:55:66:77:88:99:11:22:33:44:55:66:77:88:99' }
+  let(:cleanup_members_service_spy) { spy('GroupSaml::GroupManagedAccounts::CleanUpMembersService') }
+
+  before do
+    allow(GroupSaml::GroupManagedAccounts::CleanUpMembersService)
+      .to receive(:new).with(nil, group).and_return(cleanup_members_service_spy)
+  end
+
+  it 'updates SAML provider with given params' do
+    expect do
+      service.execute
+      group.reload
+    end.to change { group.saml_provider&.sso_url }.to('https://test')
+             .and change { group.saml_provider&.certificate_fingerprint }.to(fingerprint)
+             .and change { group.saml_provider&.enabled? }.to(true)
+             .and change { group.saml_provider&.enforced_sso? }.to(true)
+             .and change { group.saml_provider&.enforced_group_managed_accounts? }.to(true)
+  end
+
+  context 'when enforced_group_managed_accounts is enabled' do
+    it 'cleans up group members' do
+      service.execute
+
+      expect(cleanup_members_service_spy).to have_received(:execute)
+    end
+
+    context 'when save fails' do
+      let(:params) do
+        super().merge(sso_url: 'NOTANURL<>&*^')
+      end
+
+      it 'does not trigger members cleanup' do
+        service.execute
+        expect(cleanup_members_service_spy).not_to have_received(:execute)
+      end
+    end
+
+    context 'when clean up fails' do
+      before do
+        allow(cleanup_members_service_spy).to receive(:execute).and_return(false)
+      end
+
+      it 'adds an error to saml provider' do
+        expect { service.execute }.to change { group.saml_provider && group.saml_provider.errors[:base] }
+                                        .to(["Can't remove group members without group managed account"])
+      end
+
+      it 'does not change saml_provider' do
+        expect do
+          service.execute
+          group.reload
+        end.not_to change { group.saml_provider }
+      end
+    end
+  end
+
+  context 'when group managed accounts was enabled beforehand' do
+    let(:enforced_group_managed_accounts) { true }
+    let(:params) { { enforced_group_managed_accounts: true } }
+
+    it 'does not clean up group members' do
+      service.execute
+
+      expect(cleanup_members_service_spy).not_to have_received(:execute)
+    end
+  end
+
+  context 'when enforced_group_managed_accounts is disabled' do
+    let(:enforced_group_managed_accounts) { true }
+    let(:params) { { enforced_group_managed_accounts: false } }
+
+    it 'does  not clean up group members' do
+      service.execute
+
+      expect(cleanup_members_service_spy).not_to have_received(:execute)
+    end
+  end
+end