From 63c719f395923d98751c80ba9325c059455c45a6 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 18 Feb 2016 19:47:49 +0100 Subject: [PATCH 1/6] Update LDAP group links asynchronously --- app/workers/ldap_group_links_worker.rb | 10 ++++++++++ app/workers/ldap_sync_worker.rb | 2 +- lib/gitlab/ldap/access.rb | 14 ++++++++++---- 3 files changed, 21 insertions(+), 5 deletions(-) create mode 100644 app/workers/ldap_group_links_worker.rb diff --git a/app/workers/ldap_group_links_worker.rb b/app/workers/ldap_group_links_worker.rb new file mode 100644 index 00000000000000..c9e32a82c382bb --- /dev/null +++ b/app/workers/ldap_group_links_worker.rb @@ -0,0 +1,10 @@ +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.username})" + access = Gitlab::LDAP::Access.new(user) + access.update_ldap_group_links + end +end \ No newline at end of file diff --git a/app/workers/ldap_sync_worker.rb b/app/workers/ldap_sync_worker.rb index df71217ef033b8..e0c4f0ef46f985 100644 --- a/app/workers/ldap_sync_worker.rb +++ b/app/workers/ldap_sync_worker.rb @@ -8,7 +8,7 @@ def perform Rails.logger.info "Performing daily LDAP sync task." User.ldap.find_each(batch_size: 100).each do |ldap_user| Rails.logger.debug "Syncing user #{ldap_user.username}, #{ldap_user.email}" - Gitlab::LDAP::Access.allowed?(ldap_user) + Gitlab::LDAP::Access.allowed?(ldap_user, update_groups: true) end end end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index eb924a92b4ff45..105f4c2a756bbc 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -14,11 +14,11 @@ def self.open(user, &block) end end - def self.allowed?(user) + def self.allowed?(user, options={}) self.open(user) do |access| # Whether user is allowed, or not, we should update # permissions to keep things clean - access.update_permissions + access.update_permissions(options) if access.allowed? access.update_user user.last_credential_check_at = Time.now @@ -75,8 +75,14 @@ def update_user update_kerberos_identity if import_kerberos_identities? end - def update_permissions - update_ldap_group_links if group_base.present? + def update_permissions(options) + if group_base.present? + if options[:update_groups] + update_ldap_group_links + else + LdapGroupLinksWorker.perform_async(user.id) + end + end update_admin_status if admin_group.present? end -- GitLab From f74cb73aa0f7acfbbb89ca71d274811b1391794a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 19 Feb 2016 16:02:52 +0100 Subject: [PATCH 2/6] Newlines, newlines --- app/workers/ldap_group_links_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/ldap_group_links_worker.rb b/app/workers/ldap_group_links_worker.rb index c9e32a82c382bb..ff5fe589c85c7e 100644 --- a/app/workers/ldap_group_links_worker.rb +++ b/app/workers/ldap_group_links_worker.rb @@ -7,4 +7,4 @@ def perform(user_id) access = Gitlab::LDAP::Access.new(user) access.update_ldap_group_links end -end \ No newline at end of file +end -- GitLab From 6703354f153a3111d7735ce9f3fd1e3253452d47 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 19 Feb 2016 18:30:35 +0100 Subject: [PATCH 3/6] Fix tests and other small improvements --- app/workers/ldap_group_links_worker.rb | 2 +- app/workers/ldap_sync_worker.rb | 4 +++- lib/gitlab/ldap/access.rb | 2 +- spec/lib/gitlab/ldap/access_spec.rb | 19 ++++++++++++++++--- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/app/workers/ldap_group_links_worker.rb b/app/workers/ldap_group_links_worker.rb index ff5fe589c85c7e..a685da86be022c 100644 --- a/app/workers/ldap_group_links_worker.rb +++ b/app/workers/ldap_group_links_worker.rb @@ -3,7 +3,7 @@ class LdapGroupLinksWorker def perform(user_id) user = User.find(user_id) - logger.info "Updating LDAP group memberships for user #{user.id} (#{user.username})" + logger.info "Updating LDAP group memberships for user #{user.id} (#{user.email})" access = Gitlab::LDAP::Access.new(user) access.update_ldap_group_links end diff --git a/app/workers/ldap_sync_worker.rb b/app/workers/ldap_sync_worker.rb index e0c4f0ef46f985..a03011a63da815 100644 --- a/app/workers/ldap_sync_worker.rb +++ b/app/workers/ldap_sync_worker.rb @@ -8,7 +8,9 @@ def perform Rails.logger.info "Performing daily LDAP sync task." User.ldap.find_each(batch_size: 100).each do |ldap_user| Rails.logger.debug "Syncing user #{ldap_user.username}, #{ldap_user.email}" - Gitlab::LDAP::Access.allowed?(ldap_user, update_groups: true) + # Use the 'update_ldap_group_links_synchronously' option to avoid creating a ton + # of new Sidekiq jobs all at once. + Gitlab::LDAP::Access.allowed?(ldap_user, update_ldap_group_links_synchronously: true) end end end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 105f4c2a756bbc..8fe73ce1d80ae6 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -77,7 +77,7 @@ def update_user def update_permissions(options) if group_base.present? - if options[:update_groups] + if options[:update_ldap_group_links_synchronously] update_ldap_group_links else LdapGroupLinksWorker.perform_async(user.id) diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 0174700ddec6ba..09db17816a8202 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -117,18 +117,19 @@ end describe '#update_permissions' do - subject { access.update_permissions } + subject { access.update_permissions(options) } + let(:options) { Hash.new } it 'does update group permissions with a group base configured' do allow(access).to receive_messages(group_base: 'my-group-base') - expect(access).to receive(:update_ldap_group_links) + expect(LdapGroupLinksWorker).to receive(:perform_async).with(user.id) subject end it 'does not update group permissions without a group base configured' do allow(access).to receive_messages(group_base: '') - expect(access).not_to receive(:update_ldap_group_links) + expect(LdapGroupLinksWorker).not_to receive(:perform_async) subject end @@ -146,6 +147,18 @@ subject end + + context 'when synchronously updating group permissions' do + let(:options) { { update_ldap_group_links_synchronously: true } } + + it 'updates group permissions directly' do + allow(access).to receive_messages(group_base: 'my-group-base') + expect(LdapGroupLinksWorker).not_to receive(:perform_async) + expect(access).to receive(:update_ldap_group_links) + + subject + end + end end describe :update_kerberos_identity do -- GitLab From 5700ee72454b9366eaa54ee13637f4839830d39c Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 24 Feb 2016 14:44:35 +0100 Subject: [PATCH 4/6] Remove "let options" --- spec/lib/gitlab/ldap/access_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 09db17816a8202..b979a5de5b4279 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -117,8 +117,7 @@ end describe '#update_permissions' do - subject { access.update_permissions(options) } - let(:options) { Hash.new } + subject { access.update_permissions({}) } it 'does update group permissions with a group base configured' do allow(access).to receive_messages(group_base: 'my-group-base') @@ -149,7 +148,7 @@ end context 'when synchronously updating group permissions' do - let(:options) { { update_ldap_group_links_synchronously: true } } + subject { access.update_permissions(update_ldap_group_links_synchronously: true) } it 'updates group permissions directly' do allow(access).to receive_messages(group_base: 'my-group-base') -- GitLab From e378646ad4c07667e10a4121ea424e2ec9271167 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 24 Feb 2016 14:47:56 +0100 Subject: [PATCH 5/6] Also spec what should _not_ happen --- spec/lib/gitlab/ldap/access_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index b979a5de5b4279..3d226404a8b8e5 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -121,6 +121,7 @@ it 'does update group permissions with a group base configured' do allow(access).to receive_messages(group_base: 'my-group-base') + expect(access).not_to receive(:update_ldap_group_links) expect(LdapGroupLinksWorker).to receive(:perform_async).with(user.id) subject @@ -128,6 +129,7 @@ it 'does not update group permissions without a group base configured' do allow(access).to receive_messages(group_base: '') + expect(access).not_to receive(:update_ldap_group_links) expect(LdapGroupLinksWorker).not_to receive(:perform_async) subject -- GitLab From 200fb63e92949851467a232b2d42d3e29d142261 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 24 Feb 2016 18:39:07 +0100 Subject: [PATCH 6/6] No subject --- spec/lib/gitlab/ldap/access_spec.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb index 3d226404a8b8e5..371d42bf57f766 100644 --- a/spec/lib/gitlab/ldap/access_spec.rb +++ b/spec/lib/gitlab/ldap/access_spec.rb @@ -150,14 +150,12 @@ end context 'when synchronously updating group permissions' do - subject { access.update_permissions(update_ldap_group_links_synchronously: true) } - it 'updates group permissions directly' do allow(access).to receive_messages(group_base: 'my-group-base') expect(LdapGroupLinksWorker).not_to receive(:perform_async) expect(access).to receive(:update_ldap_group_links) - subject + access.update_permissions(update_ldap_group_links_synchronously: true) end end end -- GitLab