Cleanup group memeber with group managed accounts enabled

When a group owner enables "group managed accounts" feature
we should remove all users which are not managed by the group
except group owners.

app/finders/group_members_finder.rb

@@ -34,3 +34,5 @@ class GroupMembersFinder
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end
+
+GroupMembersFinder.prepend(EE::GroupMembersFinder)

app/services/members/base_service.rb

@@ -51,7 +51,9 @@ module Members
     def enqueue_delete_todos(member)
       type = member.is_a?(GroupMember) ? 'Group' : 'Project'
       # don't enqueue immediately to prevent todos removal in case of a mistake
-      TodosDestroyer::EntityLeaveWorker.perform_in(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type)
+      member.run_after_commit_or_now do
+        TodosDestroyer::EntityLeaveWorker.perform_in(Todo::WAIT_FOR_DELETE, member.user_id, member.source_id, type)
+      end
     end
   end
 end

ee/app/controllers/groups/saml_providers_controller.rb

@@ -21,7 +21,7 @@ class Groups::SamlProvidersController < Groups::ApplicationController
   def update
     @saml_provider = @group.saml_provider
 
-    @saml_provider.update(saml_provider_params)
+    GroupSaml::SamlProvider::UpdateService.new(current_user, @saml_provider, params: saml_provider_params).execute
 
     render :show
   end

ee/app/finders/ee/group_members_finder.rb

+# frozen_string_literal: true
+
+module EE::GroupMembersFinder
+  extend ActiveSupport::Concern
+
+  prepended do
+    attr_reader :group
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def not_managed
+    group.group_members.non_owners.joins(:user)
+      .where("users.managing_group_id IS NULL OR users.managing_group_id != ?", group.id)
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
+end

ee/app/models/ee/group_member.rb

@@ -13,6 +13,8 @@ module EE
       scope :with_identity_provider, ->(provider) do
         joins(user: :identities).where(identities: { provider: provider })
       end
+
+      scope :non_owners, -> { where("members.access_level < ?", ::Gitlab::Access::OWNER) }
     end
   end
 end

ee/app/services/group_saml/group_managed_accounts/clean_up_members_service.rb

+# frozen_string_literal: true
+module GroupSaml
+  module GroupManagedAccounts
+    class CleanUpMembersService
+      attr_reader :group, :current_user
+
+      def initialize(current_user, group)
+        @current_user = current_user
+        @group = group
+      end
+
+      def execute
+        non_group_managed_accounts.all? do |group_membership|
+          membership = Members::DestroyService.new(current_user).execute(group_membership)
+          membership.destroyed?
+        end
+      end
+
+      private
+
+      def non_group_managed_accounts
+        @non_group_managed_accounts ||= GroupMembersFinder.new(group).not_managed
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+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
+    end
+  end
+end

ee/spec/finders/ee/group_members_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupMembersFinder do
+  subject(:finder) { described_class.new(group) }
+  let(:group) { create :group }
+
+  let(:non_owner_access_level) { Gitlab::Access.options.values.sample }
+  let!(:group_owner_membership) { group.add_user(create(:user), Gitlab::Access::OWNER) }
+  let!(:group_member_membership) { group.add_user(create(:user), non_owner_access_level) }
+  let!(:dedicated_member_account_membership) do
+    group.add_user(create(:user, managing_group: group), non_owner_access_level)
+  end
+
+  describe '#not_managed' do
+    it 'returns non-owners without group managed accounts' do
+      expect(finder.not_managed).to eq([group_member_membership])
+    end
+  end
+end

ee/spec/services/group_saml/group_managed_accounts/clean_up_members_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GroupSaml::GroupManagedAccounts::CleanUpMembersService do
+  subject(:service) { described_class.new(current_user, group) }
+  let(:group) { Group.new }
+  let(:current_user) { instance_double('User') }
+  let(:destroy_member_service_spy) { spy('GroupSaml::GroupManagedAccounts::CleanUpMembersService') }
+
+  let(:group_member_membership) { instance_double('Member')}
+  before do
+    allow(Members::DestroyService)
+      .to receive(:new).with(current_user).and_return(destroy_member_service_spy)
+    allow(GroupMembersFinder)
+      .to receive(:new).with(group)
+            .and_return(instance_double('GroupMembersFinder', not_managed: [group_member_membership]))
+  end
+
+  it 'removes non-owner members without dedicated accounts from the group' do
+    service.execute
+    expect(destroy_member_service_spy).to have_received(:execute).with(group_member_membership)
+  end
+
+  it 'returns true' do
+    expect(service.execute).to eq true
+  end
+
+  context 'when at least one non-owner member was not removed' do
+    before do
+      allow(destroy_member_service_spy).to receive(:destroyed?).and_return(false)
+    end
+
+    it 'returns false' do
+      expect(service.execute).to eq false
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+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
+
+  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
+end

locale/gitlab.pot

@@ -1761,6 +1761,9 @@ msgstr ""
 msgid "Can't find HEAD commit for this branch"
 msgstr ""
 
+msgid "Can't remove group members without group managed account"
+msgstr ""
+
 msgid "Canary Deployments is a popular CI strategy, where a small portion of the fleet is updated to the new version of your application."
 msgstr ""