From fc7dc44d7d9ca60863cd69283a7a4cf6f616c915 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 29 Mar 2019 21:52:22 -0700 Subject: [PATCH 1/2] Guard against ldap_sync_last_sync_at being nil If ldap_sync_last_sync_at is nil, then the LDAP group got into an inconsistent state after the transition. Since it's possible for ldap_sync_last_sync_at to be nil, we guard against this occurrence and attempt to bubble up the error to the admin. Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/1529 --- ee/app/models/ee/group.rb | 15 ++++++++++--- .../unreleased/sh-handle-issue-9613.yml | 5 +++++ ee/lib/ee/gitlab/auth/ldap/sync/group.rb | 17 +++++++++++++- .../ee/gitlab/auth/ldap/sync/group_spec.rb | 22 +++++++++++++++++++ 4 files changed, 55 insertions(+), 4 deletions(-) create mode 100644 ee/changelogs/unreleased/sh-handle-issue-9613.yml diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 00d5005457e5e9..715f266f573531 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -131,11 +131,20 @@ def ldap_synced? (::Gitlab.config.ldap.enabled && ldap_group_links.any?(&:active?)) || super end - def mark_ldap_sync_as_failed(error_message) + def mark_ldap_sync_as_failed(error_message, skip_validation: false) return false unless ldap_sync_started? - fail_ldap_sync - update_column(:ldap_sync_error, ::Gitlab::UrlSanitizer.sanitize(error_message)) + error_message = ::Gitlab::UrlSanitizer.sanitize(error_message) + + if skip_validation + # A group that does not validate cannot transition out of its + # current state, so manually set the ldap_sync_status + update_columns(ldap_sync_error: error_message, + ldap_sync_status: 'failed') + else + fail_ldap_sync + update_column(:ldap_sync_error, error_message) + end end # This token conveys that the anonymous user is allowed to know of the group diff --git a/ee/changelogs/unreleased/sh-handle-issue-9613.yml b/ee/changelogs/unreleased/sh-handle-issue-9613.yml new file mode 100644 index 00000000000000..5848681ea262d4 --- /dev/null +++ b/ee/changelogs/unreleased/sh-handle-issue-9613.yml @@ -0,0 +1,5 @@ +--- +title: Guard against ldap_sync_last_sync_at being nil +merge_request: 10505 +author: +type: fixed diff --git a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb index 8075ab903e2a44..06f637301e1d21 100644 --- a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb +++ b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb @@ -66,10 +66,25 @@ def ldap_sync_ready?(group) def fail_stuck_group(group) return unless group.ldap_sync_started? - if group.ldap_sync_last_sync_at < 1.hour.ago + if group.ldap_sync_last_sync_at.nil? + fail_due_to_no_sync_time(group) + elsif 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 fail_due_to_no_sync_time(group) + # If the group sync is in the started state but no sync + # time was available, then something may be invalid with + # this group. Do some validation and bubble up the error. + details = group.errors.full_messages.join(', ') unless group.valid? + + message = +'The sync failed because the group is an inconsistent state' + message += ": #{details}" if details + Rails.logger.warn "Group '#{group.name}' failed to sync: #{message}" + + group.mark_ldap_sync_as_failed(message, skip_validation: true) + end end def initialize(group, proxy) diff --git a/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb b/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb index 694540321b5ecb..2dd62fec8d974c 100644 --- a/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb +++ b/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb @@ -117,6 +117,28 @@ def execute include_examples :group_state_machine end + describe '.fail_stuck_group' do + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + + it 'handles nil ldap_sync_last_sync_at' do + group = create(:group_with_ldap_group_link, + cn: 'ldap_group1', + group_access: ::Gitlab::Access::DEVELOPER, + ldap_sync_status: 'started', + ldap_sync_last_sync_at: nil, + visibility_level: Gitlab::VisibilityLevel::PUBLIC) + create(:project, group: group, visibility_level: Gitlab::VisibilityLevel::PUBLIC) + group.update_columns(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + + expect(group).not_to be_valid + + described_class.fail_stuck_group(group) + + expect(group.ldap_sync_status).to eq('failed') + expect(group.ldap_sync_error).to eq('The sync failed because the group is an inconsistent state: Visibility level private is not allowed since this group contains projects with higher visibility.') + end + end + describe '.ldap_sync_ready?' do let(:ldap_group1) { nil } -- GitLab From 8201bc865bd085f181d6bb81b067d4e4974dc82b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 1 Apr 2019 07:50:53 -0700 Subject: [PATCH 2/2] Remove LDAP group sync warning message This could spam the logs. --- ee/lib/ee/gitlab/auth/ldap/sync/group.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb index 06f637301e1d21..91e048ee74a24e 100644 --- a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb +++ b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb @@ -81,7 +81,6 @@ def fail_due_to_no_sync_time(group) message = +'The sync failed because the group is an inconsistent state' message += ": #{details}" if details - Rails.logger.warn "Group '#{group.name}' failed to sync: #{message}" group.mark_ldap_sync_as_failed(message, skip_validation: true) end -- GitLab