From 4b24f0d4baab62b18e2ffd72d1a72390c2fbf2ca Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Wed, 8 Dec 2021 11:24:59 -0600 Subject: [PATCH] Self-managed SAML Group Sync Add support for SAML Group Sync in self-managed installations. This feature builds on existing support for Group SAML Group Sync. Changelog: added EE: true --- config/sidekiq_queues.yml | 2 + ee/app/models/ee/group.rb | 1 + ee/app/workers/all_queues.yml | 9 ++ ee/app/workers/auth/saml_group_sync_worker.rb | 105 +++++++++++++++++ ee/lib/gitlab/auth/saml/membership_updater.rb | 2 +- .../auth/saml/membership_updater_spec.rb | 8 +- .../auth/saml_group_sync_worker_spec.rb | 110 ++++++++++++++++++ 7 files changed, 232 insertions(+), 5 deletions(-) create mode 100644 ee/app/workers/auth/saml_group_sync_worker.rb rename ee/spec/lib/{ee => }/gitlab/auth/saml/membership_updater_spec.rb (82%) create mode 100644 ee/spec/workers/auth/saml_group_sync_worker_spec.rb diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 11c232df4e0097..fc75752061fb3e 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -47,6 +47,8 @@ - 1 - - audit_events_user_impersonation_event_create - 1 +- - auth_saml_group_sync + - 1 - - authorized_keys - 2 - - authorized_project_update diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index c3fbf1db5e7599..fb983922b1f2a1 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -87,6 +87,7 @@ module Group scope :with_deletion_schedule_only, -> { preload(:deletion_schedule) } scope :with_saml_provider, -> { preload(:saml_provider) } + scope :with_saml_group_links, -> { joins(:saml_group_links) } scope :where_group_links_with_provider, ->(provider) do joins(:ldap_group_links).where(ldap_group_links: { provider: provider }) diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 341c6b08a62993..5ddf84efaee84f 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -966,6 +966,15 @@ :weight: 1 :idempotent: :tags: [] +- :name: auth_saml_group_sync + :worker_name: Auth::SamlGroupSyncWorker + :feature_category: :authentication_and_authorization + :has_external_dependencies: + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] - :name: ci_batch_reset_minutes :worker_name: Ci::BatchResetMinutesWorker :feature_category: :continuous_integration diff --git a/ee/app/workers/auth/saml_group_sync_worker.rb b/ee/app/workers/auth/saml_group_sync_worker.rb new file mode 100644 index 00000000000000..750ae70a6d0754 --- /dev/null +++ b/ee/app/workers/auth/saml_group_sync_worker.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +# Self-managed SAML Group Sync Worker +# +# When a user signs in with SAML this worker will +# be triggered to manage that user's group membership. +module Auth + class SamlGroupSyncWorker + include ApplicationWorker + include Gitlab::Utils::StrongMemoize + + data_consistency :always + + feature_category :authentication_and_authorization + idempotent! + + loggable_arguments 1 + + def perform(user_id, group_link_ids) + @group_link_ids = group_link_ids + @user = User.find_by_id(user_id) + + return unless user && sync_enabled? && groups_to_sync? + + sync_groups + end + + private + + attr_reader :group_link_ids, :user + + def sync_enabled? + Gitlab::Auth::Saml::Config.group_sync_enabled? + end + + def groups_to_sync? + group_links.any? || group_ids_by_root_ancestor_id.any? + end + + def sync_groups + group_ids_to_manage = group_ids_by_root_ancestor_id.dup + + # Sync groups user for which user should be a member + group_links_by_root_ancestor.each do |root_ancestor, group_links| + Groups::SyncService.new( + root_ancestor, user, + group_links: group_links, manage_group_ids: group_ids_to_manage.delete(root_ancestor.id) + ).execute + end + + return if group_ids_to_manage.empty? + + root_ancestors = preload_groups(group_ids_to_manage.keys) + + # Sync groups with links for which user should not be a member + group_ids_to_manage.each do |root_ancestor_id, group_ids| + Groups::SyncService.new( + root_ancestors[root_ancestor_id], user, group_links: [], manage_group_ids: group_ids + ).execute + end + end + + def group_links + strong_memoize(:group_links) do + SamlGroupLink.id_in(group_link_ids).preload_group + end + end + + def group_ids_by_root_ancestor_id + strong_memoize(:group_ids_by_root_ancestor_id) do + grouped = {} + groups = Group.with_saml_group_links.select(:id, 'traversal_ids[1] as root_id') + + groups.each do |group| + grouped[group.root_id] ||= [] + + grouped[group.root_id].push(group.id) + end + + grouped + end + end + + def group_links_by_root_ancestor + strong_memoize(:group_links_by_root_ancestor) do + grouped = {} + groups = group_links.map(&:group) + Preloaders::GroupRootAncestorPreloader.new(groups, [:route]).execute + + group_links.each do |link| + root_ancestor = link.group.root_ancestor + grouped[root_ancestor] ||= [] + + grouped[root_ancestor].push(link) + end + + grouped + end + end + + def preload_groups(group_ids) + Group.by_id(group_ids).group_by(&:id).transform_values(&:first) + end + end +end diff --git a/ee/lib/gitlab/auth/saml/membership_updater.rb b/ee/lib/gitlab/auth/saml/membership_updater.rb index 0e81b7bc2ac594..1e0899d413cc81 100644 --- a/ee/lib/gitlab/auth/saml/membership_updater.rb +++ b/ee/lib/gitlab/auth/saml/membership_updater.rb @@ -20,7 +20,7 @@ def execute attr_reader :user, :auth_hash def enqueue_group_sync - group_link_ids + ::Auth::SamlGroupSyncWorker.perform_async(user.id, group_link_ids) end def sync_groups? diff --git a/ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb b/ee/spec/lib/gitlab/auth/saml/membership_updater_spec.rb similarity index 82% rename from ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb rename to ee/spec/lib/gitlab/auth/saml/membership_updater_spec.rb index 52a16abcd0d43c..103e14ebf18a76 100644 --- a/ee/spec/lib/ee/gitlab/auth/saml/membership_updater_spec.rb +++ b/ee/spec/lib/gitlab/auth/saml/membership_updater_spec.rb @@ -30,7 +30,7 @@ def stub_saml_group_sync_enabled(enabled) end it 'does not enqueue group sync' do - expect(::SamlGroupLink).not_to receive(:by_saml_group_name) + expect(::Auth::SamlGroupSyncWorker).not_to receive(:perform_async) update_membership end @@ -42,7 +42,7 @@ def stub_saml_group_sync_enabled(enabled) end it 'enqueues group sync' do - expect(::SamlGroupLink).to receive(:by_saml_group_name).and_call_original + expect(::Auth::SamlGroupSyncWorker).to receive(:perform_async).with(user.id, match_array(group_link.id)) update_membership end @@ -55,7 +55,7 @@ def stub_saml_group_sync_enabled(enabled) end it 'enqueues group sync' do - expect(::SamlGroupLink).not_to receive(:by_saml_group_name) + expect(::Auth::SamlGroupSyncWorker).to receive(:perform_async).with(user.id, []) update_membership end @@ -67,7 +67,7 @@ def stub_saml_group_sync_enabled(enabled) end it 'enqueues group sync' do - expect(::SamlGroupLink).to receive(:by_saml_group_name).and_call_original + expect(::Auth::SamlGroupSyncWorker).to receive(:perform_async).with(user.id, []) update_membership end diff --git a/ee/spec/workers/auth/saml_group_sync_worker_spec.rb b/ee/spec/workers/auth/saml_group_sync_worker_spec.rb new file mode 100644 index 00000000000000..fb76ba0485e58d --- /dev/null +++ b/ee/spec/workers/auth/saml_group_sync_worker_spec.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Auth::SamlGroupSyncWorker do + describe '#perform' do + let_it_be(:user) { create(:user) } + + let_it_be(:top_level_group1) { create(:group) } + let_it_be(:top_level_group_link1) { create(:saml_group_link, group: top_level_group1) } + + let_it_be(:group1_subgroup) { create(:group, parent: top_level_group1) } + let_it_be(:group1_subgroup_group_link) { create(:saml_group_link, group: group1_subgroup) } + + let(:worker) { described_class.new } + + context 'when saml_group_sync feature is not licensed' do + it 'does not call the sync service' do + expect(Groups::SyncService).not_to receive(:new) + + perform([top_level_group_link1.id]) + end + end + + context 'when the saml_group_sync feature is licensed' do + before do + stub_licensed_features(saml_group_sync: true) + end + + context 'when SAML is not enabled' do + it 'does not call the sync service' do + expect(Groups::SyncService).not_to receive(:new) + + perform([top_level_group_link1.id]) + end + end + + context 'when SAML is enabled' do + before do + stub_saml_group_sync_enabled(true) + end + + it 'calls the sync service with the group links' do + expect_sync_service_call(group_links: [top_level_group_link1, group1_subgroup_group_link]) + + perform([top_level_group_link1.id, group1_subgroup_group_link.id]) + end + + it 'does not call the sync service when the user does not exist' do + expect(Groups::SyncService).not_to receive(:new) + + described_class.new.perform(non_existing_record_id, [group1_subgroup_group_link]) + end + + context 'when group links exist in hierarchies which the user should not be a member of' do + let_it_be(:top_level_group2) { create(:group) } + let_it_be(:top_level_group_link2) { create(:saml_group_link, group: top_level_group2) } + + it 'calls the service for top level groups with links that the user should not be a member of' do + expect_sync_service_call( + group_links: [top_level_group_link1], + manage_group_ids: [top_level_group1.id, group1_subgroup.id] + ) + expect_sync_service_call( + group_links: [], + manage_group_ids: [top_level_group2.id], + top_level_group: top_level_group2 + ) + + perform([top_level_group_link1.id]) + end + end + + context 'with a group in the hierarchy that has no group links' do + let(:group_without_links) { create(:group, parent: top_level_group1) } + + it 'is not included in manage_group_ids' do + expect_sync_service_call(group_links: [top_level_group_link1, group1_subgroup_group_link]) + + perform([top_level_group_link1.id, group1_subgroup_group_link.id]) + end + end + + context 'when the worker receives no group link ids' do + it 'calls the sync service' do + expect_sync_service_call(group_links: []) + + perform([]) + end + end + end + end + + def expect_sync_service_call(group_links:, manage_group_ids: nil, top_level_group: top_level_group1) + manage_group_ids = [top_level_group1.id, group1_subgroup.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 perform(group_links) + worker.perform(user.id, group_links) + end + + def stub_saml_group_sync_enabled(enabled) + allow(::Gitlab::Auth::Saml::Config).to receive(:group_sync_enabled?).and_return(enabled) + end + end +end -- GitLab