Merge branch 'ee-6260-remove-user-on-enabling-group-managed-accounts' into 'master'

Clean up  group users on enabling group managed accounts

See merge request gitlab-org/gitlab-ee!9766

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/models/saml_provider.rb

@@ -27,6 +27,14 @@ class SamlProvider < ActiveRecord::Base
     @defaults ||= DefaultOptions.new(group.full_path)
   end
 
+  def enforced_sso?
+    enabled? && super
+  end
+
+  def enforced_group_managed_accounts?
+    enforced_sso? && super
+  end
+
   class DefaultOptions
     include Gitlab::Routing
 

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/lib/gitlab/auth/group_saml/membership_enforcer.rb

@@ -10,7 +10,7 @@ module Gitlab
 
         def can_add_user?(user)
           return true unless ::Feature.enabled?(:enforced_sso, @group)
-          return true unless root_group&.saml_provider&.enforced_sso
+          return true unless root_group&.saml_provider&.enforced_sso?
 
           GroupSamlIdentityFinder.new(user: user).find_linked(group: root_group)
         end

ee/spec/features/groups/saml_providers_spec.rb

@@ -118,6 +118,32 @@ describe 'SAML provider settings' do
           expect(page).not_to have_selector('#saml_provider_enforced_sso')
         end
       end
+
+      context 'enforced_group_managed_accounts enabled' do
+        it 'updates the flag' do
+          stub_feature_flags(group_managed_accounts: true)
+
+          visit group_saml_providers_path(group)
+
+          find('input#saml_provider_enforced_group_managed_accounts').click
+
+          expect(page).to have_selector('#saml_provider_enforced_group_managed_accounts')
+          expect do
+            submit
+            saml_provider.reload
+          end.to change { saml_provider.enforced_group_managed_accounts }.to(true)
+        end
+      end
+
+      context 'enforced_group_managed_accounts disabled' do
+        it 'does not update the flag' do
+          stub_feature_flags(group_managed_accounts: false)
+
+          visit group_saml_providers_path(group)
+
+          expect(page).not_to have_selector('#saml_provider_enforced_group_managed_accounts')
+        end
+      end
     end
 
     describe 'test button' do

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/lib/gitlab/auth/group_saml/membership_enforcer_spec.rb

@@ -7,7 +7,7 @@ describe Gitlab::Auth::GroupSaml::MembershipEnforcer do
   let(:group) { identity.saml_provider.group }
 
   before do
-    allow_any_instance_of(SamlProvider).to receive(:enforced_sso).and_return(true)
+    allow_any_instance_of(SamlProvider).to receive(:enforced_sso?).and_return(true)
   end
 
   it 'allows adding the group member' do

ee/spec/models/saml_provider_spec.rb

@@ -90,4 +90,62 @@ describe SamlProvider do
       expect(settings[:idp_sso_target_url]).to eq saml_provider.sso_url
     end
   end
+
+  describe '#enforced_sso?' do
+    context 'when provider is enabled' do
+      before do
+        subject.enabled = true
+      end
+
+      it 'matches attribute' do
+        subject.enforced_sso = true
+        expect(subject).to be_enforced_sso
+        subject.enforced_sso = false
+        expect(subject).not_to be_enforced_sso
+      end
+    end
+
+    context 'when provider is disabled' do
+      before do
+        subject.enabled = false
+      end
+
+      it 'ignores attribute value' do
+        subject.enforced_sso = true
+        expect(subject).not_to be_enforced_sso
+        subject.enforced_sso = false
+        expect(subject).not_to be_enforced_sso
+      end
+    end
+  end
+
+  describe '#enforced_group_managed_accounts?' do
+    context 'when enforced_sso is enabled' do
+      before do
+        subject.enabled = true
+        subject.enforced_sso = true
+      end
+
+      it 'matches attribute' do
+        subject.enforced_group_managed_accounts = true
+        expect(subject).to be_enforced_group_managed_accounts
+        subject.enforced_group_managed_accounts = false
+        expect(subject).not_to be_enforced_group_managed_accounts
+      end
+    end
+
+    context 'when enforced_sso is disabled' do
+      before do
+        subject.enabled = true
+        subject.enforced_sso = false
+      end
+
+      it 'ignores attribute value' do
+        subject.enforced_group_managed_accounts = true
+        expect(subject).not_to be_enforced_group_managed_accounts
+        subject.enforced_group_managed_accounts = false
+        expect(subject).not_to be_enforced_group_managed_accounts
+      end
+    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

ee/spec/support/shared_examples/models/member_shared_examples.rb

@@ -15,7 +15,7 @@ shared_examples_for 'member validations' do
 
       context 'enforced SSO enabled' do
         before do
-          allow_any_instance_of(SamlProvider).to receive(:enforced_sso).and_return(true)
+          allow_any_instance_of(SamlProvider).to receive(:enforced_sso?).and_return(true)
         end
 
         it 'allows adding the group member' do

locale/gitlab.pot

@@ -1767,6 +1767,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 ""