Commit 195a66ba authored by Drew Blessing's avatar Drew Blessing

Perform LDAP group sync on sign in only for new users

Syncing a user's groups from LDAP every time they sign in,
including on Git operations, is causing performance issues for
some customers. The benefit of sync on sign in is primarily
for new users so this change restricts it as such. After initial
sign in LDAP group sync will be handled by the hourly (default)
sync and the on-demand group membership sync. Fixes #7352
parent 372b4654
...@@ -63,10 +63,10 @@ be created on first sign in. ...@@ -63,10 +63,10 @@ be created on first sign in.
## Group Sync ## Group Sync
If your LDAP supports the `memberof` property, GitLab will add the user to any If your LDAP supports the `memberof` property, when the user signs in for the
new groups they might be added to when the user logs in. That way they don't need first time GitLab will trigger a sync for groups the user should be a member of.
to wait for the hourly sync to be granted access to the groups that they are in That way they don't need to wait for the hourly sync to be granted
in LDAP. access to their groups and projects.
In GitLab Premium, we can also add a GitLab group to sync with one or multiple LDAP groups or we can In GitLab Premium, we can also add a GitLab group to sync with one or multiple LDAP groups or we can
also add a filter. The filter must comply with the syntax defined in [RFC 2254](https://tools.ietf.org/search/rfc2254). also add a filter. The filter must comply with the syntax defined in [RFC 2254](https://tools.ietf.org/search/rfc2254).
......
---
title: Perform LDAP group sync on sign in only for new users
merge_request:
author:
type: changed
...@@ -120,6 +120,10 @@ module EE ...@@ -120,6 +120,10 @@ module EE
def update_memberships def update_memberships
return if ldap_user.nil? || ldap_user.group_cns.empty? return if ldap_user.nil? || ldap_user.group_cns.empty?
# Only update membership for new users. After this, regular
# LDAP group sync will take care of things.
return unless user.created_at > 5.minutes.ago && !user.last_credential_check_at
group_ids = ::LdapGroupLink.where(cn: ldap_user.group_cns, provider: provider) group_ids = ::LdapGroupLink.where(cn: ldap_user.group_cns, provider: provider)
.distinct(:group_id) .distinct(:group_id)
.pluck(:group_id) .pluck(:group_id)
......
...@@ -83,7 +83,7 @@ describe Gitlab::Auth::LDAP::Access do ...@@ -83,7 +83,7 @@ describe Gitlab::Auth::LDAP::Access do
stub_ldap_person_find_by_dn(entry, provider) stub_ldap_person_find_by_dn(entry, provider)
end end
it 'triggers a sync for all groups found in `memberof`' do it 'triggers a sync for all groups found in `memberof` for new users' do
group_link_1 = create(:ldap_group_link, cn: 'Group1', provider: provider) group_link_1 = create(:ldap_group_link, cn: 'Group1', provider: provider)
group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider) group_link_2 = create(:ldap_group_link, cn: 'Group2', provider: provider)
group_ids = [group_link_1, group_link_2].map(&:group_id) group_ids = [group_link_1, group_link_2].map(&:group_id)
...@@ -113,6 +113,17 @@ describe Gitlab::Auth::LDAP::Access do ...@@ -113,6 +113,17 @@ describe Gitlab::Auth::LDAP::Access do
access.update_user access.update_user
end end
it 'does not performs the membership update for existing users' do
user.created_at = Time.now - 10.minutes
user.last_credential_check_at = Time.now
user.save
expect(LdapGroupLink).not_to receive(:where)
expect(LdapGroupSyncWorker).not_to receive(:perform_async)
access.update_user
end
end end
it "doesn't continue when there is no `memberOf` param" do it "doesn't continue when there is no `memberOf` param" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment