Skip to content
Snippets Groups Projects
New issue look: Off

LDAP `sync_time` configuration duplicated/not used properly

    • Closed Issue created by Drew Blessing @dblessing

    Related to gitlab-org/gitlab-ee#132

    It seems that when we switched from old to new LDAP configuration syntax, sync_time was sort of half-way migrated. But, code references the old configuration so the 'properly' configured sync_time is never referenced.

    1_settings.rb:

    Notice the two spots I put the >>. One seems to be a 'global' sync_time that is not specific to a provider, while the other is provider specific.

    # Default settings
Settings['ldap'] ||= Settingslogic.new({})
Settings.ldap['enabled'] = false if Settings.ldap['enabled'].nil?
>> Settings.ldap['sync_time'] = 3600 if Settings.ldap['sync_time'].nil?
Settings.ldap['schedule_sync_daily'] = 1 if Settings.ldap['schedule_sync_daily'].nil?
Settings.ldap['schedule_sync_hour'] = 1 if Settings.ldap['schedule_sync_hour'].nil?
Settings.ldap['schedule_sync_minute'] = 30  if Settings.ldap['schedule_sync_minute'].nil?

# backwards compatibility, we only have one host
if Settings.ldap['enabled'] || Rails.env.test?
  if Settings.ldap['host'].present?
    # We detected old LDAP configuration syntax. Update the config to make it
    # look like it was entered with the new syntax.
    server = Settings.ldap.except('sync_time')
    Settings.ldap['servers'] = {
      'main' => server
    }
  end

  Settings.ldap['servers'].each do |key, server|
    server = Settingslogic.new(server)
    server['label'] ||= 'LDAP'
    server['timeout'] ||= 10.seconds
    server['block_auto_created_users'] = false if server['block_auto_created_users'].nil?
    server['allow_username_or_email_login'] = false if server['allow_username_or_email_login'].nil?
    server['active_directory'] = true if server['active_directory'].nil?
    server['attributes'] = {} if server['attributes'].nil?
    server['provider_name'] ||= "ldap#{key}".downcase
>>    server['sync_time'] = 3600 if server['sync_time'].nil?
    server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name'])
    Settings.ldap['servers'][key] = server
  end
end

    user.rb:

    Now look at the code in user.rb that detects if an LDAP check is necessary. It references only the global setting, not the provider specific one.

      def requires_ldap_check?
    if !Gitlab.config.ldap.enabled
      false
    elsif ldap_user?
      !last_credential_check_at || (last_credential_check_at + Gitlab.config.ldap['sync_time']) < Time.now
    else
      false
    end
  end

    If we retain the provider specific one then a lot of code will have to get a lot smarter and look up the LDAP provider before checking sync time.

    cc/ @jacobvosmaer - Mentioning you because this is related to gitlab-org/gitlab-ee#132

    Designs

      Child items ...

      2. Activity

      Please register or sign in to reply