Merge branch 'bvl-update-ldap-user-groups-on-login' into 'master'

Add a user's memberships when logging in

Closes #906

See merge request !1819

app/controllers/omniauth_callbacks_controller.rb

@@ -34,6 +34,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
         prompt_for_two_factor(@user)
       else
         log_audit_event(@user, with: :ldap)
+        flash[:notice] = 'LDAP sync in progress. This could take a few minutes. '\
+                         'Refresh the page to see the changes.'
         sign_in_and_redirect(@user)
       end
     else

app/workers/ldap_all_groups_sync_worker.rb

+class LdapAllGroupsSyncWorker
+  include Sidekiq::Worker
+  include CronjobQueue
+
+  def perform
+    logger.info 'Started LDAP group sync'
+    EE::Gitlab::LDAP::Sync::Groups.execute
+    logger.info 'Finished LDAP group sync'
+  end
+end

app/workers/ldap_group_sync_worker.rb

 class LdapGroupSyncWorker
   include Sidekiq::Worker
-  include CronjobQueue
+  include DedicatedSidekiqQueue
 
-  def perform(group_id = nil)
-    if group_id
-      group = Group.find_by(id: group_id)
-      unless group
-        logger.warn "Could not find group #{group_id} for LDAP group sync"
-        return
+  def perform(group_ids, provider = nil)
+    groups = Group.where(id: Array(group_ids))
+
+    if provider
+      EE::Gitlab::LDAP::Sync::Proxy.open(provider) do |proxy|
+        sync_groups(groups, proxy: proxy)
       end
+    else
+      sync_groups(groups)
+    end
+  end
 
-      logger.info "Started LDAP group sync for group #{group.name} (#{group.id})"
-      EE::Gitlab::LDAP::Sync::Group.execute_all_providers(group)
-      logger.info "Finished LDAP group sync for group #{group.name} (#{group.id})"
+  def sync_groups(groups, proxy: nil)
+    groups.each { |group| sync_group(group, proxy: proxy) }
+  end
+
+  def sync_group(group, proxy: nil)
+    logger.info "Started LDAP group sync for group #{group.name} (#{group.id})"
+    if proxy
+      EE::Gitlab::LDAP::Sync::Group.execute(group, proxy)
     else
-      logger.info 'Started LDAP group sync'
-      EE::Gitlab::LDAP::Sync::Groups.execute
-      logger.info 'Finished LDAP group sync'
+      EE::Gitlab::LDAP::Sync::Group.execute_all_providers(group)
     end
+    logger.info "Finished LDAP group sync for group #{group.name} (#{group.id})"
   end
 end

changelogs/unreleased-ee/bvl-update-ldap-user-groups-on-login.yml

+---
+title: Add a user's memberships when logging in through LDAP
+merge_request: 1819
+author:

config/initializers/1_settings.rb

@@ -392,7 +392,7 @@ 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['ldap_group_sync_worker']['job_class'] = 'LdapAllGroupsSyncWorker'
 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'

config/sidekiq_queues.yml

@@ -55,6 +55,7 @@
   - [update_user_activity, 1]
   - [propagate_service_template, 1]
   # EE specific queues
+  - [ldap_group_sync, 2]
   - [geo, 1]
   - [project_mirror, 1]
   - [project_update_repository_storage, 1]

doc/administration/auth/ldap-ee.md

@@ -29,6 +29,11 @@ The process will also update the following user information:
 
 ## Group Sync
 
+If your LDAP supports the `memberof` property, GitLab will add the user to any
+new groups they might be added to when the user logs in. That way they don't need
+to wait for the hourly sync to be granted access to the groups that they are in
+in LDAP.
+
 If `group_base` is set in LDAP configuration, a group sync process will run
 every hour, on the hour. This allows GitLab group membership to be automatically
 updated based on LDAP group members.

lib/gitlab/ldap/access.rb

@@ -89,6 +89,7 @@ module Gitlab
 
       def update_user
         update_email
+        update_memberships
         update_ssh_keys if sync_ssh_keys?
         update_kerberos_identity if import_kerberos_identities?
       end
@@ -162,6 +163,16 @@ module Gitlab
         ldap_config.active_directory && (Gitlab.config.kerberos.enabled || AuthHelper.kerberos_enabled? )
       end
 
+      def update_memberships
+        return if ldap_user.nil? || ldap_user.group_cns.empty?
+
+        group_ids = LdapGroupLink.where(cn: ldap_user.group_cns, provider: provider)
+                      .distinct(:group_id)
+                      .pluck(:group_id)
+
+        LdapGroupSyncWorker.perform_async(group_ids, provider) if group_ids.any?
+      end
+
       private
 
       def logger

lib/gitlab/ldap/adapter.rb

@@ -107,7 +107,7 @@ module Gitlab
       end
 
       def user_attributes
-        %W(#{config.uid} cn mail dn)
+        %W(#{config.uid} cn mail dn memberof)
       end
     end
   end

lib/gitlab/ldap/person.rb

@@ -45,6 +45,22 @@ module Gitlab
         attribute_value(:email)
       end
 
+      def memberof
+        return [] unless entry.attribute_names.include?(:memberof)
+
+        entry.memberof
+      end
+
+      def group_cns
+        memberof.map { |memberof_value| cn_from_memberof(memberof_value) }
+      end
+
+      def cn_from_memberof(memberof)
+        # Only get the first CN value of the string, that's the one that contains
+        # the group name
+        memberof.match(/(?:cn=([\w\s]+))/i)&.captures&.first
+      end
+
       delegate :dn, to: :entry
 
       private

spec/lib/ee/gitlab/ldap/adapter_spec.rb

@@ -43,7 +43,7 @@ describe Gitlab::LDAP::Adapter, lib: true do
   describe '#user_attributes' do
     it 'appends EE-specific attributes' do
       stub_ldap_config(uid: 'uid', sync_ssh_keys: 'sshPublicKey')
-      expect(adapter.user_attributes).to match_array(%w(uid dn cn mail sshPublicKey))
+      expect(adapter.user_attributes).to match_array(%w(uid dn cn mail sshPublicKey memberof))
     end
   end
 end

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

@@ -65,10 +65,11 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
     end
 
     before do
-      allow(Gitlab::LDAP::Config)
-        .to receive(:providers).and_return(%w(main secondary))
-      allow(EE::Gitlab::LDAP::Sync::Proxy)
-        .to receive(:open).and_yield(double('proxy').as_null_object)
+      stub_ldap_config(providers: %w[main secundary])
+
+      adapter = ldap_adapter('main')
+      proxy = proxy(adapter, 'main')
+      allow(EE::Gitlab::LDAP::Sync::Proxy).to receive(:open).and_yield(proxy)
     end
 
     let(:group) do

spec/lib/gitlab/ldap/access_spec.rb

 require 'spec_helper'
 
 describe Gitlab::LDAP::Access, lib: true do
+  include LdapHelpers
   let(:access) { Gitlab::LDAP::Access.new user }
   let(:user) { create(:omniauth_user) }
 
@@ -173,6 +174,12 @@ describe Gitlab::LDAP::Access, lib: true do
       subject
     end
 
+    it 'updates the group memberships' do
+      expect(access).to receive(:update_memberships).once
+
+      subject
+    end
+
     it 'syncs ssh keys if enabled by configuration' do
       allow(access).to receive_messages(group_base: '', sync_ssh_keys?: true)
       expect(access).to receive(:update_ssh_keys).once
@@ -305,4 +312,50 @@ describe Gitlab::LDAP::Access, lib: true do
       expect{ access.update_email }.to change(user, :email)
     end
   end
+
+  describe '#update_memberships' do
+    let(:provider) { user.ldap_identity.provider }
+    let(:entry) { ldap_user_entry(user.ldap_identity.extern_uid) }
+
+    let(:person_with_memberof) do
+      entry['memberof'] = ['CN=Group1,CN=Users,DC=The dc,DC=com',
+                           'CN=Group2,CN=Builtin,DC=The dc,DC=com']
+      Gitlab::LDAP::Person.new(entry, provider)
+    end
+
+    it 'triggers a sync for all groups found in `memberof`' do
+      group_link_1 = create(:ldap_group_link, cn: 'Group1', provider: provider)
+      group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider)
+      group_ids = [group_link_1, group_link_2].map(&:group_id)
+
+      allow(access).to receive(:ldap_user).and_return(person_with_memberof)
+
+      expect(LdapGroupSyncWorker).to receive(:perform_async)
+                                       .with(group_ids, provider)
+
+      access.update_memberships
+    end
+
+    it "doesn't continue when there is no `memberOf` param" do
+      allow(access).to receive(:ldap_user)
+                         .and_return(Gitlab::LDAP::Person.new(entry, provider))
+
+      expect(LdapGroupLink).not_to receive(:where)
+      expect(LdapGroupSyncWorker).not_to receive(:perform_async)
+
+      access.update_memberships
+    end
+
+    it "doesn't trigger a sync when there are no links for the provider" do
+      _another_provider = create(:ldap_group_link,
+                                 cn: 'Group1',
+                                 provider: 'not-this-ldap')
+
+      allow(access).to receive(:ldap_user).and_return(person_with_memberof)
+
+      expect(LdapGroupSyncWorker).not_to receive(:perform_async)
+
+      access.update_memberships
+    end
+  end
 end

spec/lib/gitlab/ldap/adapter_spec.rb

@@ -5,6 +5,11 @@ describe Gitlab::LDAP::Adapter, lib: true do
 
   let(:ldap) { double(:ldap) }
   let(:adapter) { ldap_adapter('ldapmain', ldap) }
+  let(:default_user_search_attributes) {  }
+
+  def user_search_attributes(id_name)
+    [id_name, 'cn', 'mail', 'dn', 'memberof']
+  end
 
   describe '#users' do
     before do
@@ -16,7 +21,7 @@ describe Gitlab::LDAP::Adapter, lib: true do
       expect(adapter).to receive(:ldap_search) do |arg|
         expect(arg[:filter].to_s).to eq('(uid=johndoe)')
         expect(arg[:base]).to eq('dc=example,dc=com')
-        expect(arg[:attributes]).to match(%w{uid cn mail dn})
+        expect(arg[:attributes]).to match(user_search_attributes('uid'))
       end.and_return({})
 
       adapter.users('uid', 'johndoe')
@@ -26,7 +31,7 @@ describe Gitlab::LDAP::Adapter, lib: true do
       expect(adapter).to receive(:ldap_search).with(
         base: 'uid=johndoe,ou=users,dc=example,dc=com',
         scope: Net::LDAP::SearchScope_BaseObject,
-        attributes: %w{uid cn mail dn},
+        attributes: user_search_attributes('uid'),
         filter: nil
       ).and_return({})
 
@@ -63,7 +68,7 @@ describe Gitlab::LDAP::Adapter, lib: true do
     it 'uses the right uid attribute when non-default' do
       stub_ldap_config(uid: 'sAMAccountName')
       expect(adapter).to receive(:ldap_search).with(
-        hash_including(attributes: %w{sAMAccountName cn mail dn})
+        hash_including(attributes: user_search_attributes('sAMAccountName'))
       ).and_return({})
 
       adapter.users('sAMAccountName', 'johndoe')

spec/lib/gitlab/ldap/person_spec.rb

@@ -43,4 +43,51 @@ describe Gitlab::LDAP::Person do
       expect(person.email).to eq([user_principal_name])
     end
   end
+
+  describe '#memberof' do
+    it 'returns an empty array if the field was not present' do
+      person = described_class.new(entry, 'ldapmain')
+
+      expect(person.memberof).to eq([])
+    end
+
+    it 'returns the values of `memberof` if the field was present' do
+      example_memberof = ['CN=Group Policy Creator Owners,CN=Users,DC=Vosmaer,DC=com',
+                          'CN=Domain Admins,CN=Users,DC=Vosmaer,DC=com',
+                          'CN=Enterprise Admins,CN=Users,DC=Vosmaer,DC=com',
+                          'CN=Schema Admins,CN=Users,DC=Vosmaer,DC=com',
+                          'CN=Administrators,CN=Builtin,DC=Vosmaer,DC=com']
+      entry['memberof'] = example_memberof
+      person = described_class.new(entry, 'ldapmain')
+
+      expect(person.memberof).to eq(example_memberof)
+    end
+  end
+
+  describe '#cn_from_memberof' do
+    it 'gets the group cn from the memberof value' do
+      person = described_class.new(entry, 'ldapmain')
+
+      expect(person.cn_from_memberof('cN=Group Policy Creator Owners,CN=Users,DC=Vosmaer,DC=com'))
+        .to eq('Group Policy Creator Owners')
+    end
+
+    it "doesn't break when there is no CN property" do
+      person = described_class.new(entry, 'ldapmain')
+
+      expect(person.cn_from_memberof('DC=Vosmaer,DC=com'))
+        .to be_nil
+    end
+  end
+
+  describe '#group_cns' do
+    it 'returns only CNs from the memberof values' do
+      example_memberof = ['CN=Group Policy Creator Owners,CN=Users,DC=Vosmaer,DC=com',
+                          'CN=Administrators,CN=Builtin,DC=Vosmaer,DC=com']
+      entry['memberof'] = example_memberof
+      person = described_class.new(entry, 'ldapmain')
+
+      expect(person.group_cns).to eq(['Group Policy Creator Owners', 'Administrators'])
+    end
+  end
 end

spec/workers/ldap_all_groups_sync_worker_spec.rb

+require 'spec_helper'
+
+describe LdapAllGroupsSyncWorker do
+  let(:subject) { described_class.new }
+
+  before do
+    allow(Sidekiq.logger).to receive(:info)
+  end
+
+  describe '#perform' do
+    it 'syncs all groups when group_id is nil' do
+      expect(EE::Gitlab::LDAP::Sync::Groups).to receive(:execute)
+
+      subject.perform
+    end
+  end
+end

spec/workers/ldap_group_sync_worker_spec.rb

 require 'spec_helper'
 
 describe LdapGroupSyncWorker do
+  include LdapHelpers
+  let(:subject) { described_class.new }
+  let(:group) { create(:group) }
+
+  def expect_fake_proxy(provider)
+    fake = double
+    expect(EE::Gitlab::LDAP::Sync::Proxy)
+      .to receive(:open).with(provider).and_yield(fake)
+    fake
+  end
+
+  before do
+    allow(Sidekiq.logger).to receive(:info)
+  end
+
   describe '#perform' do
-    it 'syncs all groups when group_id is nil' do
-      expect(EE::Gitlab::LDAP::Sync::Groups).to receive(:execute)
+    it 'syncs a single group when group_id is present' do
+      expect(subject).to receive(:sync_groups).with([group])
 
-      described_class.new.perform
+      subject.perform(group.id)
     end
 
-    it 'syncs a single group when group_id is present' do
-      group = create(:group)
+    it 'creates a proxy for syncing a single provider' do
+      fake_proxy = expect_fake_proxy('the-provider')
+      expect(subject).to receive(:sync_groups).with([group], proxy: fake_proxy)
+
+      subject.perform(group.id, 'the-provider')
+    end
+  end
+
+  describe '#sync_groups' do
+    it 'syncs a group when it was found without a proxy' do
+      expect(subject).to receive(:sync_group).with(group, proxy: nil)
+
+      subject.sync_groups([group])
+    end
+
+    it 'syncs with an existing proxy when one was given' do
+      fake_proxy = double('proxy')
+      expect(subject).to receive(:sync_group).with(group, proxy: fake_proxy)
+
+      subject.sync_groups([group], proxy: fake_proxy)
+    end
+  end
+
+  describe '#sync_group' do
+    it 'syncs a single provider when a provider was given' do
+      proxy = EE::Gitlab::LDAP::Sync::Proxy.new('ldapmain', ldap_adapter)
 
-      expect(EE::Gitlab::LDAP::Sync::Group)
-        .to receive(:execute_all_providers).with(group)
+      expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute)
+                                                   .with(group, proxy)
 
-      described_class.new.perform(group.id)
+      subject.sync_group(group, proxy: proxy)
     end
 
-    it 'logs an error when group cannot be found' do
-      expect(EE::Gitlab::LDAP::Sync::Group).not_to receive(:execute_all_providers)
-      expect(Sidekiq.logger)
-        .to receive(:warn).with('Could not find group 9999 for LDAP group sync')
+    it 'syncs all providers when no proxy was given' do
+      expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute_all_providers)
+                                                   .with(group)
 
-      described_class.new.perform(9999)
+      subject.sync_group(group)
     end
   end
 end