diff --git a/CHANGELOG-EE b/CHANGELOG-EE index 68a30c73d99af0d61ede806f91e1ddcd78f29fae..a34574a3b476d76faa806e47a6dbb08b4c0a4be5 100644 --- a/CHANGELOG-EE +++ b/CHANGELOG-EE @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.7.0 (unreleased) - Update GitLab Pages to 0.2.1: support user-defined 404 pages + - Refactor group sync to pull access level logic to its own class. !306 v 8.6.3 (unreleased) - Exit ElasticIndexerWorker's job happily if record cannot be found. !311 diff --git a/lib/gitlab/ldap/access_levels.rb b/lib/gitlab/ldap/access_levels.rb new file mode 100644 index 0000000000000000000000000000000000000000..234c13bf46fec0dc25bf921ca689df93db5053fa --- /dev/null +++ b/lib/gitlab/ldap/access_levels.rb @@ -0,0 +1,17 @@ +module Gitlab + module LDAP + # Create a hash map of member DNs to access levels. The highest + # access level is retained in cases where `set` is called multiple times + # for the same DN. + class AccessLevels < Hash + def set(dns, to:) + dns.each do |dn| + current = self[dn] + + # Keep the higher of the access values. + self[dn] = to if current.nil? || to > current + end + end + end + end +end diff --git a/lib/gitlab/ldap/group_sync.rb b/lib/gitlab/ldap/group_sync.rb index 903c97b81abffb4fb85048da58407d82647babe2..38207d7f077a3eff8d41beafac8c5566f09cbc55 100644 --- a/lib/gitlab/ldap/group_sync.rb +++ b/lib/gitlab/ldap/group_sync.rb @@ -64,21 +64,19 @@ def sync_groups next unless lease.try_obtain logger.debug { "Syncing '#{group.name}' group" } - access_hash = {} # Only iterate over group links for the current provider group.ldap_group_links.with_provider(provider).each do |group_link| if member_dns = dns_for_group_cn(group_link.cn) - members_to_access_hash( - access_hash, member_dns, group_link.group_access - ) - - logger.debug { "Resolved '#{group.name}' group member access: #{access_hash}" } + access_levels.set(member_dns, to: group_link.group_access) + logger.debug do + "Resolved '#{group.name}' group member access: #{access_levels.to_hash}" + end end end - update_existing_group_membership(group, access_hash) - add_new_members(group, access_hash) + update_existing_group_membership(group) + add_new_members(group) group.update(last_ldap_sync_at: Time.now) @@ -120,18 +118,6 @@ def sync_admin_users end end - def members_to_access_hash(access_hash, member_dns, group_access) - member_dns.each do |member_dn| - current_access = access_hash[member_dn] - - # Keep the higher of the access values. - if current_access.nil? || group_access > current_access - access_hash[member_dn] = group_access - end - end - access_hash - end - private # Cache LDAP group member DNs so we don't query LDAP groups more than once. @@ -154,6 +140,10 @@ def config @config ||= Gitlab::LDAP::Config.new(provider) end + def access_levels + @access_levels ||= Gitlab::LDAP::AccessLevels.new + end + def group_base config.group_base end @@ -213,7 +203,7 @@ def update_identity(dn, uid) identity.save end - def update_existing_group_membership(group, access_hash) + def update_existing_group_membership(group) logger.debug { "Updating existing membership for '#{group.name}' group" } select_and_preload_group_members(group).each do |member| @@ -229,15 +219,15 @@ def update_existing_group_membership(group, access_hash) # of two LDAP groups from different providers linked to the same # GitLab group. This is not ideal, but preserves existing behavior. if user.ldap_identity.id != identity.id - access_hash.delete(member_dn) + access_levels.delete(member_dn) next end - desired_access = access_hash[member_dn] + desired_access = access_levels[member_dn] # Don't do anything if the user already has the desired access level if member.access_level == desired_access - access_hash.delete(member_dn) + access_levels.delete(member_dn) next end @@ -247,7 +237,7 @@ def update_existing_group_membership(group, access_hash) add_or_update_user_membership(user, group, desired_access) # Delete this entry from the hash now that we've acted on it - access_hash.delete(member_dn) + access_levels.delete(member_dn) elsif group.last_owner?(user) warn_cannot_remove_last_owner(user, group) else @@ -256,10 +246,10 @@ def update_existing_group_membership(group, access_hash) end end - def add_new_members(group, access_hash) + def add_new_members(group) logger.debug { "Adding new members to '#{group.name}' group" } - access_hash.each do |member_dn, access_level| + access_levels.each do |member_dn, access_level| user = Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider) if user.present? diff --git a/spec/lib/gitlab/ldap/access_levels_spec.rb b/spec/lib/gitlab/ldap/access_levels_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..5fea40a21aeeea5293f05d072916d59629451a88 --- /dev/null +++ b/spec/lib/gitlab/ldap/access_levels_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' + +describe Gitlab::LDAP::AccessLevels, lib: true do + describe '#set' do + let(:access_levels) { Gitlab::LDAP::AccessLevels.new } + let(:dns) do + %w( + uid=johndoe,ou=users,dc=example,dc=com + uid=janedoe,ou=users,dc=example,dc=com + ) + end + subject { access_levels } + + context 'when access_levels is empty' do + before { access_levels.set(dns, to: Gitlab::Access::DEVELOPER) } + + it do + is_expected + .to eq({ + 'uid=janedoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER, + 'uid=johndoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER + }) + end + end + + context 'when access_hash has existing entries' do + let(:developer_dns) do + %w{ + uid=janedoe,ou=users,dc=example,dc=com + uid=jamesdoe,ou=users,dc=example,dc=com + } + end + let(:master_dns) do + %w{ + uid=johndoe,ou=users,dc=example,dc=com + uid=janedoe,ou=users,dc=example,dc=com + } + end + before do + access_levels.set(master_dns, to: Gitlab::Access::MASTER) + access_levels.set(developer_dns, to: Gitlab::Access::DEVELOPER) + end + + it 'keeps the higher of all access values' do + is_expected + .to eq({ + 'uid=janedoe,ou=users,dc=example,dc=com' => Gitlab::Access::MASTER, + 'uid=johndoe,ou=users,dc=example,dc=com' => Gitlab::Access::MASTER, + 'uid=jamesdoe,ou=users,dc=example,dc=com' => Gitlab::Access::DEVELOPER + }) + end + end + end +end diff --git a/spec/lib/gitlab/ldap/group_sync_spec.rb b/spec/lib/gitlab/ldap/group_sync_spec.rb index 121b76eefe2abaa6dea73e44ee99dde01e52edf3..0e2a8c754cb582a7e03ecb7c33acffe26ffb7b2e 100644 --- a/spec/lib/gitlab/ldap/group_sync_spec.rb +++ b/spec/lib/gitlab/ldap/group_sync_spec.rb @@ -498,47 +498,4 @@ .not_to change { User.admins.where(id: user3.id).any? } end end - - describe '#members_to_access_hash' do - let(:group_access) { Gitlab::Access::DEVELOPER } - let(:member_dns) do - %w( - uid=johndoe,ou=users,dc=example,dc=com - uid=janedoe,ou=users,dc=example,dc=com - ) - end - - subject { group_sync.members_to_access_hash(access_hash, member_dns, group_access) } - - context 'when access_hash is empty' do - let(:access_hash) { Hash.new } - - it do - is_expected - .to eq({ - 'uid=janedoe,ou=users,dc=example,dc=com' => 30, - 'uid=johndoe,ou=users,dc=example,dc=com' => 30 - }) - end - end - - context 'when access_hash has existing entries' do - let(:access_hash) do - { - 'uid=janedoe,ou=users,dc=example,dc=com' => 40, - 'uid=johndoe,ou=users,dc=example,dc=com' => 20, - 'uid=jamesdoe,ou=users,dc=example,dc=com' => 40, - } - end - - it 'keeps the higher of all access values' do - is_expected - .to eq({ - 'uid=janedoe,ou=users,dc=example,dc=com' => 40, - 'uid=johndoe,ou=users,dc=example,dc=com' => 30, - 'uid=jamesdoe,ou=users,dc=example,dc=com' => 40 - }) - end - end - end end