From 81a91a34b520d8d8a307dc0f49c987b7ec6394c0 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Wed, 17 Aug 2016 08:29:33 -0500 Subject: [PATCH] Allow syncing a group against all providers at once --- CHANGELOG-EE | 1 + lib/ee/gitlab/ldap/sync/group.rb | 75 ++++++++++---- spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 115 +++++++++++++++------ 3 files changed, 140 insertions(+), 51 deletions(-) diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 1d59d579ddc282..e1506186806e96 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -6,6 +6,7 @@ v 8.11.0 (unreleased) - Performance improvement of push rules - Temporary fix for #825 - LDAP sync converts access requests to members. !655 - Optimize commit and diff changes access check to reduce git operations + - Allow syncing a group against all providers at once - Change LdapGroupSyncWorker to use new LDAP group sync classes - Removed unused GitLab GEO database index - Enable monitoring for ES classes diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index a0e489ceac14ff..403e575eb95203 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -5,8 +5,58 @@ module Sync class Group attr_reader :provider, :group, :proxy - def self.execute(group, proxy) - self.new(group, proxy).update_permissions + class << self + # Sync members across all providers for the given group. + def execute_all_providers(group) + return unless ldap_sync_ready?(group) + + group.start_ldap_sync + Rails.logger.debug { "Started syncing all providers for '#{group.name}' group" } + + # Shuffle providers to prevent a scenario where sync fails after a time + # and only the first provider or two get synced. This shuffles the order + # so subsequent syncs should eventually get to all providers. Obviously + # we should avoid failure, but this is an additional safeguard. + ::Gitlab::LDAP::Config.providers.shuffle.each do |provider| + Sync::Proxy.open(provider) do |proxy| + new(group, proxy).update_permissions + end + end + + group.finish_ldap_sync + Rails.logger.debug { "Finished syncing all providers for '#{group.name}' group" } + end + + # Sync members across a single provider for the given group. + def execute(group, proxy) + return unless ldap_sync_ready?(group) + + group.start_ldap_sync + Rails.logger.debug { "Started syncing '#{proxy.provider}' provider for '#{group.name}' group" } + + sync_group = new(group, proxy) + sync_group.update_permissions + + group.finish_ldap_sync + Rails.logger.debug { "Finished syncing '#{proxy.provider}' provider for '#{group.name}' group" } + end + + def ldap_sync_ready?(group) + fail_stuck_group(group) + + return true unless group.ldap_sync_started? + + Rails.logger.warn "Group '#{group.name}' is not ready for LDAP sync. Skipping" + false + end + + def fail_stuck_group(group) + return unless group.ldap_sync_started? + + if group.ldap_sync_last_sync_at < 1.hour.ago + group.mark_ldap_sync_as_failed('The sync took too long to complete.') + end + end end def initialize(group, proxy) @@ -16,15 +66,10 @@ def initialize(group, proxy) end def update_permissions - fail_stuck_group(group) - - if group.ldap_sync_started? - logger.debug { "Group '#{group.name}' is not ready for LDAP sync. Skipping" } + unless group.ldap_sync_started? + logger.warn "Group '#{group.name}' LDAP sync status must be 'started' before updating permissions" return end - group.start_ldap_sync - - logger.debug { "Syncing '#{group.name}' group" } access_levels = AccessLevels.new # Only iterate over group links for the current provider @@ -39,10 +84,6 @@ def update_permissions update_existing_group_membership(group, access_levels) add_new_members(group, access_levels) - - group.finish_ldap_sync - - logger.debug { "Finished syncing '#{group.name}' group" } end private @@ -154,14 +195,6 @@ def select_and_preload_group_members(group) .with_identity_provider(provider).preload(:user) end - def fail_stuck_group(group) - return false unless group.ldap_sync_started? - - if group.ldap_sync_last_sync_at < 1.hour.ago - group.mark_ldap_sync_as_failed('The sync took too long to complete.') - end - end - def logger Rails.logger end diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb index 35a4a47aaa3636..a7393555ec020d 100644 --- a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -13,44 +13,106 @@ stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter) end - describe '#update_permissions' do + shared_examples :group_state_machine do + it 'uses the ldap sync state machine' do + expect(group).to receive(:start_ldap_sync) + expect(group).to receive(:finish_ldap_sync) + expect(EE::Gitlab::LDAP::Sync::Group) + .to receive(:new).at_most(:twice).and_call_original + + execute + end + + it 'fails a stuck group older than 1 hour' do + group.start_ldap_sync + group.update_column(:ldap_sync_last_sync_at, 61.minutes.ago) + + expect(group).to receive(:mark_ldap_sync_as_failed) + + execute + end + + context 'when the group ldap sync is already started' do + it 'logs a debug message' do + group.start_ldap_sync + + expect(Rails.logger) + .to receive(:warn) + .with(/^Group '\w*' is not ready for LDAP sync. Skipping/) + .at_least(:once) + + execute + end + + it 'does not update permissions' do + group.start_ldap_sync + + expect_any_instance_of(EE::Gitlab::LDAP::Sync::Group) + .not_to receive(:update_permissions) + + execute + end + end + end + + describe '.execute_all_providers' do + def execute + described_class.execute_all_providers(group) + end + + before do + allow(Gitlab::LDAP::Config) + .to receive(:providers).and_return(['main', 'secondary']) + allow(EE::Gitlab::LDAP::Sync::Proxy) + .to receive(:open).and_yield(double('proxy').as_null_object) + end + let(:group) do create(:group_with_ldap_group_link, cn: 'ldap_group1', group_access: ::Gitlab::Access::DEVELOPER) end + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } - context 'with all functionality against one LDAP group type' do - context 'with basic add/update actions' do - let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + include_examples :group_state_machine + end - it 'fails a stuck group older than 1 hour' do - group.start_ldap_sync - group.update_column(:ldap_sync_last_sync_at, 61.minutes.ago) + describe '.execute' do + def execute + described_class.execute(group, proxy(adapter)) + end - expect(group).to receive(:mark_ldap_sync_as_failed) + let(:group) do + create(:group_with_ldap_group_link, + cn: 'ldap_group1', + group_access: ::Gitlab::Access::DEVELOPER) + end + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } - sync_group.update_permissions - end + include_examples :group_state_machine + end - context 'when the group ldap sync is already started' do - it 'logs a debug message' do - group.start_ldap_sync + describe '#update_permissions' do + before { group.start_ldap_sync } + after { group.finish_ldap_sync } - expect(Rails.logger).to receive(:debug) do |&block| - expect(block.call).to match /^Group '\w*' is not ready for LDAP sync. Skipping/ - end.at_least(1).times + let(:group) do + create(:group_with_ldap_group_link, + cn: 'ldap_group1', + group_access: ::Gitlab::Access::DEVELOPER) + end - sync_group.update_permissions - end + context 'with all functionality against one LDAP group type' do + context 'with basic add/update actions' do + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } - it 'does not add new members' do - group.start_ldap_sync + it 'does not update permissions unless ldap sync status is started' do + group.finish_ldap_sync - sync_group.update_permissions + expect(Rails.logger) + .to receive(:warn).with(/status must be 'started' before updating permissions/) - expect(group.members.pluck(:user_id)).not_to include(user.id) - end + sync_group.update_permissions end it 'adds new members' do @@ -96,13 +158,6 @@ expect(group.members.find_by(user_id: user.id).access_level) .to eq(::Gitlab::Access::DEVELOPER) end - - it 'uses the ldap sync state machine' do - expect(group).to receive(:start_ldap_sync) - expect(group).to receive(:finish_ldap_sync) - - sync_group.update_permissions - end end context 'when existing user is no longer in LDAP group' do -- GitLab