Merge branch 'optimize_ldap_group_sync_2' into 'master'

Optimize LDAP group sync operations

Separate the LDAP group sync process from the regular LDAP access checks and optimize. 

So far, this is a somewhat working PoC that splits the group sync operation to a new worker. It updates all members for a group as it iterates, instead of looping through users and updating groups that way. I am adding lots of logging because this would have been extremely helpful in the past.

There are lots of things still broken, or not considered. However, at least group members are added and updated when I use it in my idyllic dev environment 😃 

- [x] Sync groups
- [x] Sync admins
- [x] Make it work with all type of LDAP groups (with member, member_uid, etc. attributes)
- [x] Update tests
- [ ] Document
- [x] Add scheduled job? (and associated config)

Do these in another merge request

- [ ] Answer: Do we still need some sort of sync on user sign in?
- [ ] Answer: Should sync time be configurable?
- [ ] Answer: Should the group button to 'Reset cache' be changed to 'Sync now'?

Can the last 3 questions be addressed in a subsequent MR?
Are there other things we're not considering yet?

See merge request !229

app/models/group.rb

@@ -36,6 +36,10 @@ class Group < Namespace
   after_create :post_create_hook
   after_destroy :post_destroy_hook
 
+  scope :where_group_links_with_provider, ->(provider) do
+    joins(:ldap_group_links).where(ldap_group_links: { provider: provider })
+  end
+
   class << self
     # Searches for groups matching the given query.
     #

app/models/identity.rb

@@ -19,6 +19,8 @@ class Identity < ActiveRecord::Base
   validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider }
   validates :user_id, uniqueness: { scope: :provider }
 
+  scope :with_provider, ->(provider) { where(provider: provider) }
+
   def ldap?
     provider.starts_with?('ldap')
   end

app/models/members/group_member.rb

@@ -31,6 +31,10 @@ class GroupMember < Member
   scope :with_group, ->(group) { where(source_id: group.id) }
   scope :with_user, ->(user) { where(user_id: user.id) }
   scope :with_ldap_dn, -> { joins(user: :identities).where("identities.provider LIKE ?", 'ldap%') }
+  scope :select_access_level_and_user, -> { select(:access_level, :user_id) }
+  scope :with_identity_provider, ->(provider) do
+    joins(user: :identities).where(identities: { provider: provider })
+  end
 
   def self.access_level_roles
     Gitlab::Access.options_with_owner

app/models/user.rb

@@ -227,6 +227,9 @@ class User < ActiveRecord::Base
   scope :ldap, -> { joins(:identities).where('identities.provider LIKE ?', 'ldap%') }
   scope :with_two_factor,    -> { where(two_factor_enabled: true) }
   scope :without_two_factor, -> { where(two_factor_enabled: false) }
+  scope :with_provider, ->(provider) do
+    joins(:identities).where(identities: { provider: provider })
+  end
 
   #
   # Class methods

app/workers/ldap_group_links_worker.rb

-class LdapGroupLinksWorker
-  include Sidekiq::Worker
-
-  def perform(user_id)
-    user = User.find(user_id)
-    logger.info "Updating LDAP group memberships for user #{user.id} (#{user.email})"
-    access = Gitlab::LDAP::Access.new(user)
-    access.update_ldap_group_links
-  end
-end

app/workers/ldap_group_sync_worker.rb

+class LdapGroupSyncWorker
+  include Sidekiq::Worker
+
+  sidekiq_options retry: false
+
+  def perform
+    logger.info 'Started LDAP group sync'
+    Gitlab::LDAP::GroupSync.execute
+    logger.info 'Finished LDAP group sync'
+  end
+end

config/initializers/1_settings.rb

@@ -330,6 +330,9 @@ Settings.cron_jobs['update_all_mirrors_worker']['job_class'] = 'UpdateAllMirrors
 Settings.cron_jobs['ldap_sync_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['ldap_sync_worker']['cron'] ||= '30 1 * * *'
 Settings.cron_jobs['ldap_sync_worker']['job_class'] = 'LdapSyncWorker'
+Settings.cron_jobs['ldap_group_sync_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['ldap_group_sync_worker']['cron'] ||= '0 * * * *'
+Settings.cron_jobs['ldap_group_sync_worker']['job_class'] = 'LdapGroupSyncWorker'
 Settings.cron_jobs['geo_bulk_notify_worker'] ||= Settingslogic.new({})
 Settings.cron_jobs['geo_bulk_notify_worker']['cron'] ||= '*/10 * * * * *'
 Settings.cron_jobs['geo_bulk_notify_worker']['job_class'] ||= 'GeoBulkNotifyWorker'

db/migrate/20160303210802_add_secondary_extern_uid_to_identities.rb

+class AddSecondaryExternUidToIdentities < ActiveRecord::Migration
+  def change
+    add_column :identities, :secondary_extern_uid, :string
+  end
+end

db/migrate/20160317191509_add_last_sync_time_to_groups.rb

+class AddLastSyncTimeToGroups < ActiveRecord::Migration
+  def change
+    add_column :namespaces, :last_ldap_sync_at, :datetime
+    add_index :namespaces, :last_ldap_sync_at
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160316124047) do
+ActiveRecord::Schema.define(version: 20160317191509) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -455,6 +455,7 @@ ActiveRecord::Schema.define(version: 20160316124047) do
     t.integer  "user_id"
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string   "secondary_extern_uid"
   end
 
   add_index "identities", ["created_at", "id"], name: "index_identities_on_created_at_and_id", using: :btree
@@ -676,9 +677,11 @@ ActiveRecord::Schema.define(version: 20160316124047) do
     t.string   "avatar"
     t.boolean  "membership_lock",       default: false
     t.boolean  "share_with_group_lock", default: false
+    t.datetime "last_ldap_sync_at"
   end
 
   add_index "namespaces", ["created_at", "id"], name: "index_namespaces_on_created_at_and_id", using: :btree
+  add_index "namespaces", ["last_ldap_sync_at"], name: "index_namespaces_on_last_ldap_sync_at", using: :btree
   add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree
   add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
   add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree
@@ -831,7 +834,7 @@ ActiveRecord::Schema.define(version: 20160316124047) do
     t.boolean  "pending_delete",                   default: false
     t.boolean  "public_builds",                    default: true,     null: false
     t.string   "main_language"
-    t.integer  "pushes_since_gc",        default: 0
+    t.integer  "pushes_since_gc",                  default: 0
   end
 
   add_index "projects", ["builds_enabled", "shared_runners_enabled"], name: "index_projects_on_builds_enabled_and_shared_runners_enabled", using: :btree

lib/gitlab/ldap/access.rb

@@ -18,7 +18,6 @@ module Gitlab
         self.open(user) do |access|
           # Whether user is allowed, or not, we should update
           # permissions to keep things clean
-          access.update_permissions(options)
           if access.allowed?
             access.update_user
             user.last_credential_check_at = Time.now
@@ -75,17 +74,6 @@ module Gitlab
         update_kerberos_identity if import_kerberos_identities?
       end
 
-      def update_permissions(options)
-        if group_base.present?
-          if options[:update_ldap_group_links_synchronously]
-            update_ldap_group_links
-          else
-            LdapGroupLinksWorker.perform_async(user.id)
-          end
-        end
-        update_admin_status if admin_group.present?
-      end
-
       # Update user ssh keys if they changed in LDAP
       def update_ssh_keys
         remove_old_ssh_keys
@@ -148,63 +136,6 @@ module Gitlab
         user.update(email: ldap_email)
       end
 
-      def update_admin_status
-        admin_group = Gitlab::LDAP::Group.find_by_cn(ldap_config.admin_group, adapter)
-        admin_user = Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
-
-        if admin_group && admin_group.has_member?(admin_user)
-          unless user.admin?
-            user.admin = true
-            user.save
-          end
-        else
-          if user.admin?
-            user.admin = false
-            user.save
-          end
-        end
-      end
-
-      # Loop through all ldap connected groups, and update the users link with it
-      #
-      # We documented what sort of queries an LDAP server can expect from
-      # GitLab EE in doc/integration/ldap.md. Please remember to update that
-      # documentation if you change the algorithm below.
-      def update_ldap_group_links
-        gitlab_groups_with_ldap_link.each do |group|
-          active_group_links = group.ldap_group_links.where(cn: cns_with_access)
-
-          if active_group_links.any?
-            max_access = active_group_links.maximum(:group_access)
-
-            # Ensure we don't leave a group without an owner
-            if max_access < Gitlab::Access::OWNER && group.last_owner?(user)
-              logger.warn "#{self.class.name}: LDAP group sync cannot demote #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
-            else
-              group.add_users([user.id], max_access, skip_notification: true)
-            end
-          elsif group.last_owner?(user)
-            logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
-          else
-            group.users.delete(user)
-          end
-        end
-      end
-
-      def ldap_groups
-        @ldap_groups ||= ::LdapGroupLink.with_provider(provider).distinct(:cn).pluck(:cn).map do |cn|
-          Gitlab::LDAP::Group.find_by_cn(cn, adapter)
-        end.compact
-      end
-
-      # returns a collection of cn strings to which the user has access
-      def cns_with_access
-        return [] unless ldap_user.present?
-        @ldap_groups_with_access ||= ldap_groups.select do |ldap_group|
-          ldap_group.has_member?(ldap_user)
-        end.map(&:cn)
-      end
-
       def sync_ssh_keys?
         ldap_config.sync_ssh_keys?
       end
@@ -214,22 +145,8 @@ module Gitlab
         ldap_config.active_directory && (Gitlab.config.kerberos.enabled || AuthHelper.kerberos_enabled? )
       end
 
-      def group_base
-        ldap_config.group_base
-      end
-
-      def admin_group
-        ldap_config.admin_group
-      end
-
       private
 
-      def gitlab_groups_with_ldap_link
-        ::Group.includes(:ldap_group_links).references(:ldap_group_links).
-          where.not(ldap_group_links: { id: nil }).
-          where(ldap_group_links: { provider: provider })
-      end
-
       def logger
         Rails.logger
       end

lib/gitlab/ldap/adapter.rb

@@ -93,6 +93,16 @@ module Gitlab
                     attributes: %w{dn}).any?
       end
 
+      def dns_for_filter(filter)
+        ldap_search(
+          base: config.base,
+          filter: filter,
+          scope: Net::LDAP::SearchScope_WholeSubtree,
+          attributes: %w{dn}
+        ).map(&:dn)
+      end
+
+
       def ldap_search(*args)
         # Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
         Timeout.timeout(config.timeout) do

lib/gitlab/ldap/group.rb

@@ -14,6 +14,10 @@ module Gitlab
         @adapter = adapter
       end
 
+      def active_directory?
+        adapter.config.active_directory
+      end
+
       def cn
         entry.cn.first
       end
@@ -34,22 +38,12 @@ module Gitlab
         entry.memberuid
       end
 
-      def has_member?(user)
-        user_uid = user.uid.downcase
-        user_dn = user.dn.downcase
-
-        if memberuid?
-          member_uids.any? { |member_uid| member_uid.downcase == user_uid }
-        elsif member_dns.any? { |member_dn| member_dn.downcase == user_dn }
-          true
-        elsif member_dns.any? { |member_dn| member_dn.downcase == "uid=" + user_uid }
-          true
-        elsif adapter.config.active_directory
-          adapter.dn_matches_filter?(user.dn, active_directory_recursive_memberof_filter)
+      def member_dns
+        if active_directory?
+          dns = adapter.dns_for_filter(active_directory_recursive_memberof_filter)
+          return dns unless dns.empty?
         end
-      end
 
-      def member_dns
         if (entry.respond_to? :member) && (entry.respond_to? :submember)
           entry.member + entry.submember
         elsif entry.respond_to? :member