Merge branch 'sh-handle-issue-9613' into 'master'

Guard against ldap_sync_last_sync_at being nil

Closes #1529

See merge request gitlab-org/gitlab-ee!10507

ee/app/models/ee/group.rb

@@ -131,11 +131,20 @@ module EE
       (::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

ee/changelogs/unreleased/sh-handle-issue-9613.yml

+---
+title: Guard against ldap_sync_last_sync_at being nil
+merge_request: 10505
+author:
+type: fixed

ee/lib/ee/gitlab/auth/ldap/sync/group.rb

@@ -66,10 +66,24 @@ module EE
               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
+
+                group.mark_ldap_sync_as_failed(message, skip_validation: true)
+              end
             end
 
             def initialize(group, proxy)

ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb

@@ -117,6 +117,28 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
     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 }