Allow SAML Group Sync to remove old memberships

In addition to adding new members, SAML Group Sync should also
remove existing memberships where the SAML response indicates the
user is no longer a member of the group in the IdP. Old memberships
will only be removed for groups with any group links.

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