Merge branch 'dblessing_saml_group_sync_remove' into 'master'

SAML Group Sync remove old memberships

See merge request gitlab-org/gitlab!46917

ee/app/models/saml_group_link.rb

@@ -9,4 +9,6 @@ class SamlGroupLink < ApplicationRecord
   validates :saml_group_name, presence: true, uniqueness: { scope: [:group_id] }, length: { maximum: 255 }
 
   scope :by_id_and_group_id, ->(id, group_id) { where(id: id, group_id: group_id) }
+  scope :by_group_id, ->(group_id) { where(group_id: group_id) }
+  scope :preload_group, -> { preload(group: :route) }
 end

ee/app/services/groups/sync_service.rb

@@ -3,38 +3,139 @@
 #
 # Usage example:
 #
-# Groups::SyncService.new(nil, user, group_links: array_of_group_links).execute
+# Groups::SyncService.new(
+#   top_level_group, user,
+#   group_links: array_of_group_links,
+#   manage_group_ids: array_of_group_ids
+# ).execute
 #
 # Given group links must respond to `group_id` and `access_level`.
 #
 # This is a generic group sync service, reusable by many IdP-specific
 # implementations. The worker (caller) is responsible for providing the
 # specific group links, which this service then iterates over
-# and adds users to respective groups. See `SamlGroupSyncWorker` for an
-# example.
+# and adds/removes users from respective groups.
+#
+# When `manage_group_ids` is present, users will only be removed from these
+# groups if they should no longer be a member. When not present, users are
+# removed from all groups where they should no longer be a member. This is
+# useful when it's desired to only manage groups with group links and
+# allow other groups to manage members manually.
+#
+# See `GroupSamlGroupSyncWorker` for an example.
 #
 module Groups
   class SyncService < Groups::BaseService
+    include Gitlab::Utils::StrongMemoize
+    extend Gitlab::Utils::Override
+
+    attr_reader :updated_membership
+
+    override :initialize
+    def initialize(group, user, params = {})
+      @updated_membership = {
+        added: 0,
+        updated: 0,
+        removed: 0
+      }
+
+      super
+    end
+
     def execute
-      group_links_by_group.each do |group_id, group_links|
+      return unless group
+
+      remove_old_memberships
+      update_current_memberships
+
+      ServiceResponse.success(payload: updated_membership)
+    end
+
+    private
+
+    def remove_old_memberships
+      members_to_remove.each do |member|
+        Members::DestroyService.new(current_user).execute(member, skip_authorization: true)
+
+        next unless member.destroyed?
+
+        log_membership_update(
+          group_id: member.source_id,
+          action: :removed,
+          prior_access_level: member.access_level,
+          access_level: nil
+        )
+      end
+    end
+
+    def update_current_memberships
+      group_links_by_group.each do |group, group_links|
         access_level = max_access_level(group_links)
-        Group.find_by_id(group_id)&.add_user(current_user, access_level)
+        existing_member = existing_member_by_group(group)
+
+        next if correct_access_level?(existing_member, access_level) || group.last_owner?(current_user)
+
+        add_member(group, access_level, existing_member)
       end
     end
 
-    private
+    def add_member(group, access_level, existing_member)
+      member = group.add_user(current_user, access_level)
+
+      return member unless member.persisted? && member.access_level == access_level
+
+      log_membership_update(
+        group_id: group.id,
+        action: (existing_member ? :updated : :added),
+        prior_access_level: existing_member&.access_level,
+        access_level: access_level
+      )
+    end
+
+    def correct_access_level?(member, access_level)
+      member && member.access_level == access_level
+    end
+
+    def members_to_remove
+      existing_members.select do |member|
+        group_id = member.source_id
+
+        !member_in_groups_to_be_updated?(group_id) && manage_group?(group_id)
+      end
+    end
+
+    def member_in_groups_to_be_updated?(group_id)
+      group_links_by_group.keys.map(&:id).include?(group_id)
+    end
+
+    def manage_group?(group_id)
+      params[:manage_group_ids].blank? || params[:manage_group_ids].include?(group_id)
+    end
+
+    def existing_member_by_group(group)
+      existing_members.find { |member| member.source_id == group.id }
+    end
+
+    def existing_members
+      strong_memoize(:existing_members) do
+        group.members_with_descendants.with_user(current_user).to_a
+      end
+    end
 
     def group_links_by_group
-      params[:group_links].group_by(&:group_id)
+      strong_memoize(:group_links_by_group) do
+        params[:group_links].group_by(&:group)
+      end
     end
 
     def max_access_level(group_links)
-      human_access_level = group_links.map(&:access_level)
-      human_access_level.map { |level| integer_access_level(level) }.max
+      group_links.map(&:access_level_before_type_cast).max
     end
 
-    def integer_access_level(human_access_level)
-      ::Gitlab::Access.options_with_owner[human_access_level]
+    def log_membership_update(group_id:, action:, prior_access_level:, access_level:)
+      @updated_membership[action] += 1
+
+      Gitlab::AppLogger.debug(message: "#{self.class.name} User: #{current_user.username} (#{current_user.id}), Action: #{action}, Group: #{group_id}, Prior Access: #{prior_access_level}, New Access: #{access_level}")
     end
   end
 end

ee/app/workers/group_saml_group_sync_worker.rb

@@ -2,19 +2,25 @@
 
 class GroupSamlGroupSyncWorker
   include ApplicationWorker
+  include Gitlab::Utils::StrongMemoize
 
   feature_category :authentication_and_authorization
   idempotent!
 
+  loggable_arguments 2
+
+  attr_reader :top_level_group, :group_link_ids, :user
+
   def perform(user_id, top_level_group_id, group_link_ids)
-    top_level_group = Group.find_by_id(top_level_group_id)
-    user = User.find_by_id(user_id)
+    @top_level_group = Group.find_by_id(top_level_group_id)
+    @group_link_ids = group_link_ids
+    @user = User.find_by_id(user_id)
 
-    return unless user && feature_available?(top_level_group)
+    return unless user && feature_available?(top_level_group) && groups_to_sync?
 
-    group_links = find_group_links(group_link_ids, top_level_group)
+    response = sync_groups
 
-    Groups::SyncService.new(nil, user, group_links: group_links).execute
+    log_extra_metadata_on_done(:stats, response.payload)
   end
 
   private
@@ -23,7 +29,32 @@ class GroupSamlGroupSyncWorker
     group && group.saml_group_sync_available?
   end
 
-  def find_group_links(group_link_ids, top_level_group)
-    SamlGroupLink.by_id_and_group_id(group_link_ids, top_level_group.self_and_descendants.select(:id))
+  def groups_to_sync?
+    group_links.any? || group_ids_with_any_links.any?
+  end
+
+  def sync_groups
+    Groups::SyncService.new(
+      top_level_group, user,
+      group_links: group_links, manage_group_ids: group_ids_with_any_links
+    ).execute
+  end
+
+  def group_links
+    strong_memoize(:group_links) do
+      SamlGroupLink.by_id_and_group_id(group_link_ids, group_ids_in_hierarchy).preload_group
+    end
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def group_ids_with_any_links
+    strong_memoize(:group_ids_with_any_links) do
+      SamlGroupLink.by_group_id(group_ids_in_hierarchy).pluck(:group_id).uniq
+    end
+  end
+
+  def group_ids_in_hierarchy
+    top_level_group.self_and_descendants.pluck(:id)
   end
+  # rubocop: enable CodeReuse/ActiveRecord
 end

ee/spec/services/groups/sync_service_spec.rb

@@ -2,14 +2,13 @@
 
 require 'spec_helper'
 
-RSpec.describe Groups::SyncService, '#execute' do
+RSpec.describe Groups::SyncService do
   let(:user) { create(:user) }
 
   describe '#execute' do
-    subject(:sync) { described_class.new(nil, user, group_links: group_links).execute }
-
     let_it_be(:top_level_group) { create(:group) }
     let_it_be(:group1) { create(:group, parent: top_level_group) }
+    let_it_be(:group2) { create(:group, parent: top_level_group) }
 
     let_it_be(:group_links) do
       [
@@ -19,6 +18,15 @@ RSpec.describe Groups::SyncService, '#execute' do
       ]
     end
 
+    let_it_be(:manage_group_ids) { [top_level_group.id, group1.id, group2.id] }
+
+    subject(:sync) do
+      described_class.new(
+        top_level_group, user,
+        group_links: group_links, manage_group_ids: manage_group_ids
+      ).execute
+    end
+
     it 'adds two new group member records' do
       expect { sync }.to change { GroupMember.count }.by(2)
     end
@@ -37,6 +45,14 @@ RSpec.describe Groups::SyncService, '#execute' do
         .to eq(::Gitlab::Access::DEVELOPER)
     end
 
+    it 'returns a success response' do
+      expect(sync.success?).to eq(true)
+    end
+
+    it 'returns sync stats as payload' do
+      expect(sync.payload).to include({ added: 2, removed: 0, updated: 0 })
+    end
+
     context 'when the user is already a member' do
       context 'with the correct access level' do
         before do
@@ -53,22 +69,114 @@ RSpec.describe Groups::SyncService, '#execute' do
           expect(group1.members.find_by(user_id: user.id).access_level)
             .to eq(::Gitlab::Access::DEVELOPER)
         end
+
+        it 'does not call Group find_by_id' do
+          expect(Group).not_to receive(:find_by_id).with(group1.id)
+
+          sync
+        end
       end
 
       context 'with a different access level' do
+        context 'when the user is not the last owner' do
+          before do
+            top_level_group.add_user(user, ::Gitlab::Access::MAINTAINER)
+          end
+
+          it 'does not change the group member count' do
+            expect { sync }.not_to change { top_level_group.members.count }
+          end
+
+          it 'updates the access_level' do
+            sync
+
+            expect(top_level_group.members.find_by(user_id: user.id).access_level)
+              .to eq(::Gitlab::Access::GUEST)
+          end
+
+          it 'returns sync stats as payload' do
+            expect(sync.payload).to include({ added: 1, removed: 0, updated: 1 })
+          end
+        end
+
+        context 'when the user is the last owner' do
+          before do
+            top_level_group.add_user(user, ::Gitlab::Access::OWNER)
+          end
+
+          it 'does not change the group member count' do
+            expect { sync }.not_to change { top_level_group.members.count }
+          end
+
+          it 'does not update the access_level' do
+            sync
+
+            expect(top_level_group.members.find_by(user_id: user.id).access_level)
+              .to eq(::Gitlab::Access::OWNER)
+          end
+
+          it 'returns sync stats as payload' do
+            expect(sync.payload).to include({ added: 0, removed: 0, updated: 0 })
+          end
+        end
+      end
+
+      context 'but should no longer be a member' do
+        shared_examples 'removes the member' do
+          before do
+            group2.add_user(user, ::Gitlab::Access::DEVELOPER)
+          end
+
+          it 'reduces group member count by 1' do
+            expect { sync }.to change { group2.members.count }.by(-1)
+          end
+
+          it 'removes the matching user' do
+            sync
+
+            expect(group2.members).not_to include(user)
+          end
+
+          it 'returns sync stats as payload' do
+            expect(sync.payload).to include({ added: 2, removed: 1, updated: 0 })
+          end
+        end
+
+        context 'when manage_group_ids is present' do
+          let_it_be(:manage_group_ids) { [group2.id] }
+
+          include_examples 'removes the member'
+        end
+
+        context 'when manage_group_ids is empty' do
+          let_it_be(:manage_group_ids) { [] }
+
+          include_examples 'removes the member'
+        end
+
+        context 'when manage_groups_ids is nil' do
+          let_it_be(:manage_group_ids) { nil }
+
+          include_examples 'removes the member'
+        end
+      end
+
+      context 'in a group that is not managed' do
+        let_it_be(:manage_group_ids) { [top_level_group.id, group1.id] }
+
         before do
-          top_level_group.add_user(user, ::Gitlab::Access::MAINTAINER)
+          group2.add_user(user, ::Gitlab::Access::REPORTER)
         end
 
         it 'does not change the group member count' do
-          expect { sync }.not_to change { top_level_group.members.count }
+          expect { sync }.not_to change { group2.members.count }
         end
 
-        it 'updates the access_level' do
+        it 'retains the correct access level' do
           sync
 
-          expect(top_level_group.members.find_by(user_id: user.id).access_level)
-            .to eq(::Gitlab::Access::GUEST)
+          expect(group2.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::REPORTER)
         end
       end
     end

ee/spec/workers/group_saml_group_sync_worker_spec.rb

@@ -12,6 +12,8 @@ RSpec.describe GroupSamlGroupSyncWorker do
     let_it_be(:group) { create(:group, parent: top_level_group) }
     let_it_be(:group_link) { create(:saml_group_link, group: group) }
 
+    let(:worker) { described_class.new }
+
     context 'when the group does not have group_saml_group_sync feature licensed' do
       before do
         create(:saml_provider, group: top_level_group, enabled: true)
@@ -44,7 +46,8 @@ RSpec.describe GroupSamlGroupSyncWorker do
         end
 
         it 'calls the sync service with the group links' do
-          stub_sync_service_expectation([top_level_group_link, group_link])
+          expect_sync_service_call(group_links: [top_level_group_link, group_link])
+          expect_metadata_logging_call({ added: 2, updated: 0, removed: 0 })
 
           perform([top_level_group_link.id, group_link.id])
         end
@@ -55,24 +58,53 @@ RSpec.describe GroupSamlGroupSyncWorker do
           described_class.new.perform(non_existing_record_id, top_level_group.id, [group_link])
         end
 
+        it 'includes groups with links in manage_group_ids' do
+          expect_sync_service_call(
+            group_links: [top_level_group_link],
+            manage_group_ids: [top_level_group.id, group.id]
+          )
+
+          perform([top_level_group_link.id])
+        end
+
         context 'when a group link falls outside the top-level group' do
           let(:outside_group_link) { create(:saml_group_link, group: create(:group)) }
 
           it 'drops group links outside the top level group' do
-            stub_sync_service_expectation([group_link])
+            expect_sync_service_call(group_links: [group_link])
+            expect_metadata_logging_call({ added: 1, updated: 0, removed: 0 })
 
             perform([outside_group_link.id, group_link])
           end
         end
+
+        context 'with a group in the hierarchy that has no group links' do
+          let(:group_without_links) { create(:group, parent: group) }
+
+          it 'is not included in manage_group_ids' do
+            expect_sync_service_call(group_links: [top_level_group_link, group_link])
+            expect_metadata_logging_call({ added: 2, updated: 0, removed: 0 })
+
+            perform([top_level_group_link.id, group_link.id])
+          end
+        end
       end
     end
 
-    def stub_sync_service_expectation(group_links)
-      expect(Groups::SyncService).to receive(:new).with(nil, user, group_links: group_links).and_call_original
+    def expect_sync_service_call(group_links:, manage_group_ids: nil)
+      manage_group_ids = [top_level_group.id, group.id] if manage_group_ids.nil?
+
+      expect(Groups::SyncService).to receive(:new).with(
+        top_level_group, user, group_links: group_links, manage_group_ids: manage_group_ids
+      ).and_call_original
+    end
+
+    def expect_metadata_logging_call(stats)
+      expect(worker).to receive(:log_extra_metadata_on_done).with(:stats, stats)
     end
 
     def perform(group_links)
-      described_class.new.perform(user.id, top_level_group.id, group_links)
+      worker.perform(user.id, top_level_group.id, group_links)
     end
   end
 end