From 3d5128644c0cb14a601375431388b9bd0d4d040a Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Mon, 18 Jul 2016 14:06:29 -0600 Subject: [PATCH] Refactor group sync in to a more modular layout --- lib/ee/gitlab/ldap/sync/admin_users.rb | 61 ++ lib/ee/gitlab/ldap/sync/external_users.rb | 63 ++ lib/ee/gitlab/ldap/sync/group.rb | 153 ++++ lib/ee/gitlab/ldap/sync/groups.rb | 91 +++ lib/ee/gitlab/ldap/sync/proxy.rb | 132 ++++ spec/factories/groups.rb | 17 + spec/factories/users.rb | 5 + spec/lib/ee/gitlab/ldap/group_sync_spec.rb | 707 ------------------ .../ee/gitlab/ldap/sync/admin_users_spec.rb | 53 ++ .../gitlab/ldap/sync/external_users_spec.rb | 55 ++ spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 174 +++++ spec/lib/ee/gitlab/ldap/sync/groups_spec.rb | 96 +++ spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb | 178 +++++ spec/support/ldap_helpers.rb | 110 +++ 14 files changed, 1188 insertions(+), 707 deletions(-) create mode 100644 lib/ee/gitlab/ldap/sync/admin_users.rb create mode 100644 lib/ee/gitlab/ldap/sync/external_users.rb create mode 100644 lib/ee/gitlab/ldap/sync/group.rb create mode 100644 lib/ee/gitlab/ldap/sync/groups.rb create mode 100644 lib/ee/gitlab/ldap/sync/proxy.rb delete mode 100644 spec/lib/ee/gitlab/ldap/group_sync_spec.rb create mode 100644 spec/lib/ee/gitlab/ldap/sync/admin_users_spec.rb create mode 100644 spec/lib/ee/gitlab/ldap/sync/external_users_spec.rb create mode 100644 spec/lib/ee/gitlab/ldap/sync/group_spec.rb create mode 100644 spec/lib/ee/gitlab/ldap/sync/groups_spec.rb create mode 100644 spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb create mode 100644 spec/support/ldap_helpers.rb diff --git a/lib/ee/gitlab/ldap/sync/admin_users.rb b/lib/ee/gitlab/ldap/sync/admin_users.rb new file mode 100644 index 00000000000000..e84867b0817ad9 --- /dev/null +++ b/lib/ee/gitlab/ldap/sync/admin_users.rb @@ -0,0 +1,61 @@ +module EE + module Gitlab + module LDAP + module Sync + class AdminUsers + attr_reader :provider, :proxy + + def self.execute(proxy) + self.new(proxy).update_permissions + end + + def initialize(proxy) + @provider = proxy.provider + @proxy = proxy + end + + def update_permissions + return if admin_group.empty? + + admin_group_member_dns = proxy.dns_for_group_cn(admin_group) + current_admin_users = ::User.admins.with_provider(provider) + verified_admin_users = [] + + # Verify existing admin users and add new ones. + admin_group_member_dns.each do |member_dn| + user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider) + + if user.present? + user.admin = true + user.save + verified_admin_users << user + else + Rails.logger.debug do + <<-MSG.strip_heredoc.tr("\n", ' ') + #{self.class.name}: User with DN `#{member_dn}` should have admin + access but there is no user in GitLab with that identity. + Membership will be updated once the user signs in for the first time. + MSG + end + end + end + + # Revoke the unverified admins. + current_admin_users.each do |user| + unless verified_admin_users.include?(user) + user.admin = false + user.save + end + end + end + + private + + def admin_group + proxy.adapter.config.admin_group + end + end + end + end + end +end diff --git a/lib/ee/gitlab/ldap/sync/external_users.rb b/lib/ee/gitlab/ldap/sync/external_users.rb new file mode 100644 index 00000000000000..bca5e2566b7552 --- /dev/null +++ b/lib/ee/gitlab/ldap/sync/external_users.rb @@ -0,0 +1,63 @@ +module EE + module Gitlab + module LDAP + module Sync + class ExternalUsers + attr_reader :provider, :proxy + + def self.execute(proxy) + self.new(proxy).update_permissions + end + + def initialize(proxy) + @provider = proxy.provider + @proxy = proxy + end + + def update_permissions + return unless external_groups.any? + + current_external_users = ::User.external.with_provider(provider) + verified_external_users = [] + + external_groups.each do |group| + group_dns = proxy.dns_for_group_cn(group) + + group_dns.each do |member_dn| + user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider) + + if user.present? + user.external = true + user.save + verified_external_users << user + else + Rails.logger.debug do + <<-MSG.strip_heredoc.tr("\n", ' ') + #{self.class.name}: User with DN `#{member_dn}` should be marked as + external but there is no user in GitLab with that identity. + Membership will be updated once the user signs in for the first time. + MSG + end + end + end + end + + # Restore normal access to users no longer found in the external groups + current_external_users.each do |user| + unless verified_external_users.include?(user) + user.external = false + user.save + end + end + end + + private + + def external_groups + proxy.adapter.config.external_groups + end + end + end + end + end +end diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb new file mode 100644 index 00000000000000..551ae3b9af0946 --- /dev/null +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -0,0 +1,153 @@ +module EE + module Gitlab + module LDAP + module Sync + class Group + attr_reader :provider, :group, :proxy + + def self.execute(group, proxy) + self.new(group, proxy).update_permissions + end + + def initialize(group, proxy) + @provider = proxy.provider + @group = group + @proxy = proxy + end + + def update_permissions + lease = ::Gitlab::ExclusiveLease.new( + "ldap_group_sync:#{provider}:#{group.id}", + timeout: 3600 + ) + return unless lease.try_obtain + + logger.debug { "Syncing '#{group.name}' group" } + + access_levels = AccessLevels.new + # 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) + 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_levels) + add_new_members(group, access_levels) + + group.update(last_ldap_sync_at: Time.now) + + logger.debug { "Finished syncing '#{group.name}' group" } + end + + private + + def dns_for_group_cn(group_cn) + proxy.dns_for_group_cn(group_cn) + end + + def dn_for_uid(uid) + proxy.dn_for_uid(uid) + end + + def update_existing_group_membership(group, access_levels) + logger.debug { "Updating existing membership for '#{group.name}' group" } + + select_and_preload_group_members(group).each do |member| + user = member.user + identity = user.identities.select(:id, :extern_uid) + .with_provider(provider).first + member_dn = identity.extern_uid + + # Skip if this is not an LDAP user with a valid `extern_uid`. + next unless member_dn.present? + + # Prevent shifting group membership, in case where user is a member + # 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_levels.delete(member_dn) + next + end + + 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_levels.delete(member_dn) + next + end + + # Check and update the access level. If `desired_access` is `nil` + # we need to delete the user from the group. + if desired_access.present? + add_or_update_user_membership(user, group, desired_access) + + # Delete this entry from the hash now that we've acted on it + access_levels.delete(member_dn) + elsif group.last_owner?(user) + warn_cannot_remove_last_owner(user, group) + else + group.users.delete(user) + end + end + end + + def add_new_members(group, access_levels) + logger.debug { "Adding new members to '#{group.name}' group" } + + access_levels.each do |member_dn, access_level| + user = ::Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider) + + if user.present? + add_or_update_user_membership(user, group, access_level) + else + logger.debug do + <<-MSG.strip_heredoc.tr("\n", ' ') + #{self.class.name}: User with DN `#{member_dn}` should have access + to '#{group.name}' group but there is no user in GitLab with that + identity. Membership will be updated once the user signs in for + the first time. + MSG + end + end + end + end + + def add_or_update_user_membership(user, group, access) + # Prevent the last owner of a group from being demoted + if access < ::Gitlab::Access::OWNER && group.last_owner?(user) + warn_cannot_remove_last_owner(user, group) + else + # If you pass the user object, instead of just user ID, + # it saves an extra user database query. + group.add_users([user], access, skip_notification: true) + end + end + + def warn_cannot_remove_last_owner(user, group) + logger.warn do + <<-MSG.strip_heredoc.tr("\n", ' ') + #{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 + MSG + end + end + + def select_and_preload_group_members(group) + group.members.select_access_level_and_user + .with_identity_provider(provider).preload(:user) + end + + def logger + Rails.logger + end + end + end + end + end +end diff --git a/lib/ee/gitlab/ldap/sync/groups.rb b/lib/ee/gitlab/ldap/sync/groups.rb new file mode 100644 index 00000000000000..6c8211395c9725 --- /dev/null +++ b/lib/ee/gitlab/ldap/sync/groups.rb @@ -0,0 +1,91 @@ +module EE + module Gitlab + module LDAP + module Sync + class Groups + attr_reader :provider, :proxy + + def self.execute + # Shuffle providers to prevent a scenario where sync fails after a time + # and only the first provider or two get synced. This shuffles the order + # so subsequent syncs should eventually get to all providers. Obviously + # we should avoid failure, but this is an additional safeguard. + ::Gitlab::LDAP::Config.providers.shuffle.each do |provider| + Sync::Proxy.open(provider) do |proxy| + group_sync = self.new(proxy) + group_sync.update_permissions + end + end + + true + end + + def initialize(proxy) + @provider = proxy.provider + @proxy = proxy + end + + def update_permissions + unless config.group_base.present? + logger.debug { "No `group_base` configured for '#{provider}' provider. Skipping" } + return nil + end + + logger.debug { "Performing LDAP group sync for '#{provider}' provider" } + sync_groups + logger.debug { "Finished LDAP group sync for '#{provider}' provider" } + + if config.admin_group.present? + logger.debug { "Syncing admin users for '#{provider}' provider" } + sync_admin_users + logger.debug { "Finished syncing admin users for '#{provider}' provider" } + else + logger.debug { "No `admin_group` configured for '#{provider}' provider. Skipping" } + end + + if config.external_groups.empty? + logger.debug { "No `external_groups` configured for '#{provider}' provider. Skipping" } + else + logger.debug { "Syncing external users for '#{provider}' provider" } + sync_external_users + logger.debug { "Finished syncing external users for '#{provider}' provider" } + end + + nil + end + + private + + def sync_groups + groups_where_group_links_with_provider_ordered.each do |group| + Sync::Group.execute(group, proxy) + end + end + + def sync_admin_users + Sync::AdminUsers.execute(proxy) + end + + def sync_external_users + Sync::ExternalUsers.execute(proxy) + end + + def groups_where_group_links_with_provider_ordered + ::Group.where_group_links_with_provider(provider) + .preload(:ldap_group_links) + .reorder('last_ldap_sync_at ASC, namespaces.id ASC') + .distinct + end + + def config + proxy.adapter.config + end + + def logger + Rails.logger + end + end + end + end + end +end diff --git a/lib/ee/gitlab/ldap/sync/proxy.rb b/lib/ee/gitlab/ldap/sync/proxy.rb new file mode 100644 index 00000000000000..f096122877b804 --- /dev/null +++ b/lib/ee/gitlab/ldap/sync/proxy.rb @@ -0,0 +1,132 @@ +require 'net/ldap/dn' + +module EE + module Gitlab + module LDAP + module Sync + class Proxy + attr_reader :provider, :adapter + + # Open a connection and run all queries through it. + # It's more efficient than the default of opening/closing per LDAP query. + def self.open(provider, &block) + ::Gitlab::LDAP::Adapter.open(provider) do |adapter| + block.call(self.new(provider, adapter)) + end + end + + def initialize(provider, adapter) + @adapter = adapter + @provider = provider + end + + # Cache LDAP group member DNs so we don't query LDAP groups more than once. + def dns_for_group_cn(group_cn) + @dns_for_group_cn ||= Hash.new { |h, k| h[k] = ldap_group_member_dns(k) } + @dns_for_group_cn[group_cn] + end + + # Cache user DN so we don't generate excess queries to map UID to DN + def dn_for_uid(uid) + @dn_for_uid ||= Hash.new { |h, k| h[k] = member_uid_to_dn(k) } + @dn_for_uid[uid] + end + + private + + def ldap_group_member_dns(ldap_group_cn) + ldap_group = LDAP::Group.find_by_cn(ldap_group_cn, adapter) + unless ldap_group.present? + logger.warn { "Cannot find LDAP group with CN '#{ldap_group_cn}'. Skipping" } + return [] + end + + member_dns = ldap_group.member_dns + if member_dns.empty? + # Group must be empty + return [] unless ldap_group.memberuid? + + members = ldap_group.member_uids + member_dns = members.map { |uid| dn_for_uid(uid) } + end + + # Various lookups in this method could return `nil` values. + # Compact the array to remove those entries + member_dns.compact! + + ensure_full_dns!(member_dns) + + logger.debug { "Members in '#{ldap_group.name}' LDAP group: #{member_dns}" } + + # Various lookups in this method could return `nil` values. + # Compact the array to remove those entries + member_dns + end + + # At least one customer reported that their LDAP `member` values contain + # only `uid=username` and not the full DN. This method allows us to + # account for that. See gitlab-ee#442 + def ensure_full_dns!(dns) + dns.map! do |dn| + begin + parsed_dn = Net::LDAP::DN.new(dn).to_a + rescue RuntimeError => e + # Net::LDAP raises a generic RuntimeError. Bad library! Bad! + logger.error { "Found malformed DN: '#{dn}'. Skipping. #{e.message}" } + next + end + + # If there is more than one key/value set we must have a full DN, + # or at least the probability is higher. + if parsed_dn.count > 2 + dn + elsif parsed_dn[0] == 'uid' + dn_for_uid(parsed_dn[1]) + else + logger.warn { "Found potentially malformed/incomplete DN: '#{dn}'" } + dn + end + end + + # Remove `nil` values generated by the rescue above. + dns.compact! + end + + def member_uid_to_dn(uid) + identity = Identity.find_by(provider: provider, secondary_extern_uid: uid) + + if identity.present? + # Use the DN on record in GitLab when it's available + identity.extern_uid + else + ldap_user = ::Gitlab::LDAP::Person.find_by_uid(uid, adapter) + + # Can't find a matching user + return nil unless ldap_user.present? + + # Update user identity so we don't have to go through this again + update_identity(ldap_user.dn, uid) + + ldap_user.dn + end + end + + def update_identity(dn, uid) + identity = + Identity.find_by(provider: provider, extern_uid: dn) + + # User may not exist in GitLab yet. Skip. + return unless identity.present? + + identity.secondary_extern_uid = uid + identity.save + end + + def logger + Rails.logger + end + end + end + end + end +end diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index 2d47a6f6c4cb0d..4d22a263ba3183 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -15,5 +15,22 @@ trait :private do visibility_level Gitlab::VisibilityLevel::PRIVATE end + + factory :group_with_ldap_group_link do + transient do + cn 'group1' + group_access Gitlab::Access::GUEST + provider 'ldapmain' + end + + after(:create) do |group, evaluator| + group.ldap_group_links << create( + :ldap_group_link, + cn: evaluator.cn, + group_access: evaluator.group_access, + provider: evaluator.provider + ) + end + end end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index c6f7869516e207..67843ed8dd9f41 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -14,6 +14,10 @@ admin true end + trait :external do + external true + end + trait :two_factor do two_factor_via_otp end @@ -51,5 +55,6 @@ end factory :admin, traits: [:admin] + factory :external_user, traits: [:external] end end diff --git a/spec/lib/ee/gitlab/ldap/group_sync_spec.rb b/spec/lib/ee/gitlab/ldap/group_sync_spec.rb deleted file mode 100644 index 6665ab932b1ff7..00000000000000 --- a/spec/lib/ee/gitlab/ldap/group_sync_spec.rb +++ /dev/null @@ -1,707 +0,0 @@ -require 'spec_helper' - -describe EE::Gitlab::LDAP::GroupSync, lib: true do - let(:group_sync) { described_class.new('ldapmain') } - let(:config) { double(:config, active_directory: false) } - let(:adapter) { double(:adapter, config: config) } - subject { group_sync } - - before do - allow_any_instance_of(::Gitlab::ExclusiveLease) - .to receive(:try_obtain).and_return(true) - end - - describe '#update_permissions' do - before do - allow(group_sync) - .to receive_messages(sync_groups: true, sync_admin_users: true) - end - after { group_sync.update_permissions } - - context 'when group_base is present but admin_group is not' do - before do - allow(group_sync) - .to receive_messages(group_base: 'my-group-base', admin_group: nil) - end - - it { is_expected.to receive(:sync_groups) } - it { is_expected.not_to receive(:sync_admin_users) } - end - - context 'when admin_group is present but group_base is not' do - before do - allow(group_sync) - .to receive_messages(group_base: nil, admin_group: 'my-admin-group') - end - - it { is_expected.to receive(:sync_admin_users) } - it { is_expected.not_to receive(:sync_groups) } - end - end - - describe '#sync_groups' do - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:group1) { create(:group) } - let(:group2) { create(:group) } - let(:ldap_group1) do - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - description: LDAP Group 1 - gidnumber: 42 - uniqueMember: uid=#{user1.username},ou=users,dc=example,dc=com - uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - end - - context 'with all functionality against one LDAP group type' do - before do - allow_any_instance_of(EE::Gitlab::LDAP::Group) - .to receive(:adapter).and_return(adapter) - - user1.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - ) - user2.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user2.username},ou=users,dc=example,dc=com" - ) - - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('ldap_group1', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(ldap_group1)) - - group1.ldap_group_links.create( - cn: 'ldap_group1', - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapmain' - ) - group2.ldap_group_links.create( - cn: 'ldap_group1', - group_access: ::Gitlab::Access::OWNER, - provider: 'ldapmain' - ) - end - - context 'with basic add/update actions' do - before do - # Pre-populate the group with some users - group1.add_users([user1.id], - ::Gitlab::Access::MASTER, skip_notification: true) - group2.add_users([user2.id], - ::Gitlab::Access::DEVELOPER, skip_notification: true) - end - - it 'adds new members' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user2.id).any? } - .from(false).to(true) - end - - it 'downgrades existing member access' do - expect { group_sync.sync_groups } - .to change { - group1.members.where( - user_id: user1.id, - access_level: ::Gitlab::Access::DEVELOPER - ).any? - }.from(false).to(true) - end - - it 'upgrades existing member access' do - expect { group_sync.sync_groups } - .to change { - group2.members.where( - user_id: user2.id, - access_level: ::Gitlab::Access::OWNER - ).any? - }.from(false).to(true) - end - - it 'does not send a notification email' do - expect { group_sync.sync_groups } - .not_to change { ActionMailer::Base.deliveries } - end - end - - context 'when existing user is no longer in LDAP group' do - let(:user_without_group) { create(:user) } - before do - user_without_group.identities - .create(provider: group_sync.provider, - extern_uid: "uid=johndoe,ou=users,dc=example,dc=com" ) - group1.add_users([user_without_group.id], - ::Gitlab::Access::MASTER, skip_notification: true) - group2.add_users([user_without_group.id], - ::Gitlab::Access::OWNER, skip_notification: true) - end - - it 'removes the user from the group' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user_without_group.id).any? } - .from(true).to(false) - end - - it 'refuses to delete the last owner' do - expect { group_sync.sync_groups } - .not_to change { group2.members.where(user_id: user_without_group.id).any? } - end - end - - context 'when user is the last owner' do - before do - group1.ldap_group_links.create( - cn: 'ldap_group1', - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapmain' - ) - - group1.add_users([user1.id, user2.id], - ::Gitlab::Access::OWNER, skip_notification: true) - end - - it 'downgrades one user but not the other' do - group_sync.sync_groups - - expect(group1.members.pluck(:access_level).sort) - .to eq([::Gitlab::Access::DEVELOPER, ::Gitlab::Access::OWNER]) - end - - context 'when user is a member of two groups from different providers' do - let(:config) { double(:config, active_directory: false, provider: 'ldapsecondary') } - let(:adapter) { double(:adapter, config: config) } - let(:secondary_group_sync) do - described_class.new('ldapsecondary', adapter) - end - let(:ldap_secondary_group1) do - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_secondary_group1,ou=groups,dc=example,dc=com - cn: ldap_secondary_group1 - description: LDAP Group 1 - gidnumber: 42 - uniqueMember: uid=#{user1.username},ou=users,dc=example,dc=com - uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - end - let(:user_w_multiple_ids) { create(:user) } - - before do - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('ldap_group1', any_args) - .and_return(EE::Gitlab::LDAP::Group.new(ldap_group1)) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('ldap_secondary_group1', any_args) - .and_return(EE::Gitlab::LDAP::Group.new(ldap_secondary_group1)) - user_w_multiple_ids.identities.create( - [ - { - provider: 'ldapsecondary', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - }, - { - provider: 'ldapprimary', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - } - ] - ) - group1.ldap_group_links.create( - cn: 'ldap_group1', - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapprimary' - ) - group1.ldap_group_links.create( - cn: 'ldap_secondary_group1', - group_access: ::Gitlab::Access::OWNER, - provider: 'ldapsecondary' - ) - group1.add_users([user_w_multiple_ids.id], - ::Gitlab::Access::DEVELOPER, skip_notification: true) - end - - it 'does not change user permissions for secondary group link' do - expect { secondary_group_sync.sync_groups } - .not_to change { - group1.members.where( - user_id: user_w_multiple_ids.id, - access_level: ::Gitlab::Access::OWNER - ).any? - } - end - end - end - - context 'when access level spillover could happen' do - it 'does not erroneously add users' do - ldap_group2 = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group2,ou=groups,dc=example,dc=com - cn: ldap_group2 - description: LDAP Group 2 - gidnumber: 55 - uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - - allow_any_instance_of(EE::Gitlab::LDAP::Group) - .to receive(:adapter).and_return(adapter) - - user1.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - ) - user2.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user2.username},ou=users,dc=example,dc=com" - ) - - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('ldap_group1', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(ldap_group1)) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('ldap_group2', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(ldap_group2)) - - group1.members.destroy_all - group1.ldap_group_links.destroy_all - group1.ldap_group_links.create( - cn: 'ldap_group1', - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapmain' - ) - group2.members.destroy_all - group2.ldap_group_links.destroy_all - group2.ldap_group_links.create( - cn: 'ldap_group2', - group_access: ::Gitlab::Access::MASTER, - provider: 'ldapmain' - ) - - group_sync.sync_groups - - expect(group1.members.pluck(:user_id).sort).to eq([user1.id, user2.id].sort) - expect(group1.members.pluck(:access_level).uniq).to eq([::Gitlab::Access::DEVELOPER]) - expect(group2.members.pluck(:user_id)).to eq([user2.id]) - expect(group2.members.pluck(:access_level).uniq).to eq([::Gitlab::Access::MASTER]) - end - end - end - - # Test that membership can be resolved for all different type of LDAP groups - context 'with different LDAP group types' do - let(:secondary_extern_uid) { nil } - - before do - allow_any_instance_of(EE::Gitlab::LDAP::Group) - .to receive(:adapter).and_return(adapter) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with(ldap_group.cn, any_args) - .and_return(ldap_group) - user1.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com", - secondary_extern_uid: secondary_extern_uid - ) - group1.ldap_group_links.create( - cn: ldap_group.cn, - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapmain' - ) - end - - # GroupOfNames - OpenLDAP - context 'with groupOfNames style LDAP group' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - description: LDAP Group 1 - member: uid=#{user1.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - ) - end - - it 'adds the user to the group' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user1.id).any? } - .from(false).to(true) - end - end - - # posixGroup - Apple Open Directory - context 'with posixGroup style LDAP group' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - description: LDAP Group 1 - memberuid: #{user1.username} - objectclass: top - objectclass: posixGroup - EOS - ) - end - let(:ldap_user) do - ::Gitlab::LDAP::Person.new( - Net::LDAP::Entry.from_single_ldif_string( - "dn: uid=#{user1.username},ou=users,dc=example,dc=com" - ), - 'ldapmain' - ) - end - - before do - allow(::Gitlab::LDAP::Person) - .to receive(:find_by_uid) - .with(user1.username, any_args) - .and_return(ldap_user) - end - - it 'adds the user to the group' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user1.id).any? } - .from(false).to(true) - end - - it 'expects ::Gitlab::LDAP::Person to be called' do - expect(::Gitlab::LDAP::Person).to receive(:find_by_uid) - - group_sync.sync_groups - end - - it do - expect { group_sync.sync_groups } - .to change { - user1.identities.find_by( - provider: group_sync.provider, - extern_uid: ldap_user.dn - ).secondary_extern_uid - }.from(nil).to(user1.username) - end - - context 'when the uid is stored in the database' do - let(:secondary_extern_uid) { user1.username } - - it 'expects ::Gitlab::LDAP::Person will not be called' do - expect(::Gitlab::LDAP::Person) - .not_to receive(:find_by_uid) - .with(user1.username, any_args) - - group_sync.sync_groups - end - end - - context 'when a DN for UID is requesting multiple times' do - let(:secondary_extern_uid) { user1.username } - - before do - # Group 1 link was created above. Create another here. - group2.ldap_group_links.create( - cn: ldap_group.cn, - group_access: ::Gitlab::Access::DEVELOPER, - provider: 'ldapmain' - ) - end - - it 'expects the identity will be retrieved from the database once' do - expect(Identity).to receive(:find_by) - .with( - provider: 'ldapmain', - secondary_extern_uid: secondary_extern_uid - ).once.and_call_original - - group_sync.sync_groups - end - - it 'expects ::Gitlab::LDAP::Person will not be called' do - expect(::Gitlab::LDAP::Person) - .not_to receive(:find_by_uid) - .with(user1.username, any_args) - - group_sync.sync_groups - end - end - end - - context 'with groupOfUniqueNames style LDAP group' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - description: LDAP Group 1 - uniquemember: uid=#{user1.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfUniqueNames - EOS - ) - end - - it 'adds the user to the group' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user1.id).any? } - .from(false).to(true) - end - end - - context 'with an empty LDAP group' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - description: LDAP Group 1 - objectclass: top - objectclass: groupOfUniqueNames - EOS - ) - end - - it 'does nothing, without failure' do - expect { group_sync.sync_groups } - .not_to change { group1.members.count } - end - end - - # See gitlab-ee#442 and comment in GroupSync#ensure_full_dns! - context 'with uid=username member format' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - member: uid=#{user1.username} - description: LDAP Group 1 - objectclass: top - objectclass: groupOfUniqueNames - EOS - ) - end - let(:ldap_user) do - ::Gitlab::LDAP::Person.new( - Net::LDAP::Entry.from_single_ldif_string( - "dn: uid=#{user1.username},ou=users,dc=example,dc=com" - ), - 'ldapmain' - ) - end - - before do - allow(::Gitlab::LDAP::Person) - .to receive(:find_by_uid) - .with(user1.username, any_args) - .and_return(ldap_user) - end - - it 'adds the user to the group' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user1.id).any? } - .from(false).to(true) - end - - it 'expects ::Gitlab::LDAP::Person to be called' do - expect(::Gitlab::LDAP::Person).to receive(:find_by_uid) - - group_sync.sync_groups - end - - it do - expect { group_sync.sync_groups } - .to change { - user1.identities.find_by( - provider: group_sync.provider, - extern_uid: ldap_user.dn - ).secondary_extern_uid - }.from(nil).to(user1.username) - end - end - - context 'with invalid DNs in the LDAP group' do - let(:ldap_group) do - EE::Gitlab::LDAP::Group.new( - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=ldap_group1,ou=groups,dc=example,dc=com - cn: ldap_group1 - member: - member: uid=#{user1.username},ou=users,dc=example,dc=com - member: foo, bar - description: LDAP Group 1 - objectclass: top - objectclass: groupOfUniqueNames - EOS - ) - end - - # Check that the blank member and malformed member logged an error - it 'expects two errors to be logged' do - expect(Rails.logger).to receive(:error) do |&block| - expect(block.call).to match /^Found malformed DN:/ - end.twice - - group_sync.sync_groups - end - - it 'expects the valid user to be added' do - expect { group_sync.sync_groups } - .to change { group1.members.where(user_id: user1.id).any? } - .from(false).to(true) - end - end - end - end - - describe '#sync_admin_users' do - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:user3) { create(:user) } - - let(:admin_group) do - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=admin_group,ou=groups,dc=example,dc=com - cn: admin_group - description: Admin Group - gidnumber: 42 - uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - end - - before do - user1.update_attribute(:admin, true) - user3.update_attribute(:admin, true) - - allow_any_instance_of(EE::Gitlab::LDAP::Group) - .to receive(:adapter).and_return(adapter) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn).with(admin_group.cn, any_args) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('admin_group', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(admin_group)) - - user1.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - ) - user2.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user2.username},ou=users,dc=example,dc=com" - ) - - allow(group_sync).to receive_messages(admin_group: 'admin_group') - end - - it 'adds new admin users' do - expect { group_sync.sync_admin_users } - .to change { User.admins.where(id: user2.id).any? }.from(false).to(true) - end - - it 'removes users that are not in the LDAP group' do - expect { group_sync.sync_admin_users } - .to change { User.admins.where(id: user1.id).any? }.from(true).to(false) - end - - it 'leaves admins that do not have the LDAP provider' do - expect { group_sync.sync_admin_users } - .not_to change { User.admins.where(id: user3.id).any? } - end - end - - describe '#sync_external_users' do - let(:user1) { create(:user) } - let(:user2) { create(:user) } - let(:user3) { create(:user) } - let(:user4) { create(:user) } - - let(:external_group1) do - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=external_group1,ou=groups,dc=example,dc=com - cn: external_group1 - description: External Group 1 - gidnumber: 42 - uniqueMember: uid=#{user1.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - end - - let(:external_group2) do - Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) - dn: cn=external_group2,ou=groups,dc=example,dc=com - cn: external_group2 - description: External Group 2 - gidnumber: 42 - uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com - objectclass: top - objectclass: groupOfNames - EOS - end - - before do - user3.update_attribute(:external, true) - user4.update_attribute(:external, true) - - allow_any_instance_of(EE::Gitlab::LDAP::Group) - .to receive(:adapter).and_return(adapter) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('external_group1', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(external_group1)) - allow(EE::Gitlab::LDAP::Group) - .to receive(:find_by_cn) - .with('external_group2', kind_of(::Gitlab::LDAP::Adapter)) - .and_return(EE::Gitlab::LDAP::Group.new(external_group2)) - - user1.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com" - ) - user2.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user2.username},ou=users,dc=example,dc=com" - ) - user4.identities.create( - provider: 'ldapmain', - extern_uid: "uid=#{user4.username},ou=users,dc=example,dc=com" - ) - - allow(group_sync).to receive_messages(external_groups: %w(external_group1 external_group2)) - end - - it 'adds user1 as external user' do - expect { group_sync.sync_external_users } - .to change { User.external.where(id: user1.id).any? }.from(false).to(true) - end - - it 'adds user2 as external user' do - expect { group_sync.sync_external_users } - .to change { User.external.where(id: user2.id).any? }.from(false).to(true) - end - - it 'removes users that are not in the LDAP group' do - expect { group_sync.sync_external_users } - .to change { User.external.where(id: user4.id).any? }.from(true).to(false) - end - - it 'leaves as external users that do not have the LDAP provider' do - expect { group_sync.sync_external_users } - .not_to change { User.external.where(id: user3.id).any? } - end - end -end diff --git a/spec/lib/ee/gitlab/ldap/sync/admin_users_spec.rb b/spec/lib/ee/gitlab/ldap/sync/admin_users_spec.rb new file mode 100644 index 00000000000000..130e87efce51dd --- /dev/null +++ b/spec/lib/ee/gitlab/ldap/sync/admin_users_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe EE::Gitlab::LDAP::Sync::AdminUsers, lib: true do + include LdapHelpers + + describe '#update_permissions' do + let(:sync_admin) { described_class.new(proxy(adapter)) } + + let(:user) { create(:user) } + + let(:admin_group) do + ldap_group_entry(user_dn(user.username), cn: 'admin_group') + end + + before do + stub_ldap_config(admin_group: 'admin_group', active_directory: false) + stub_ldap_group_find_by_cn('admin_group', admin_group, adapter) + end + + it 'adds user as admin' do + create(:identity, user: user, extern_uid: user_dn(user.username)) + + expect { sync_admin.update_permissions } + .to change { user.reload.admin? }.from(false).to(true) + end + + it 'removes users that are not in the LDAP group' do + admin = create(:admin) + create(:identity, user: admin, extern_uid: user_dn(admin.username)) + + expect { sync_admin.update_permissions } + .to change { admin.reload.admin? }.from(true).to(false) + end + + it 'leaves admin users that do not have the LDAP provider' do + admin = create(:admin) + + expect { sync_admin.update_permissions } + .not_to change { admin.reload.admin? } + end + + it 'leaves admin users that have a different provider identity' do + admin = create(:admin) + create(:identity, + user: admin, + provider: 'ldapsecondary', + extern_uid: user_dn(admin.username)) + + expect { sync_admin.update_permissions } + .not_to change { admin.reload.admin? } + end + end +end diff --git a/spec/lib/ee/gitlab/ldap/sync/external_users_spec.rb b/spec/lib/ee/gitlab/ldap/sync/external_users_spec.rb new file mode 100644 index 00000000000000..35e376332ca701 --- /dev/null +++ b/spec/lib/ee/gitlab/ldap/sync/external_users_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe EE::Gitlab::LDAP::Sync::ExternalUsers, lib: true do + include LdapHelpers + + describe '#update_permissions' do + let(:sync_external) { described_class.new(proxy(adapter)) } + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:external_group1) { ldap_group_entry(user_dn(user1.username), cn: 'external_group1') } + let(:external_group2) { ldap_group_entry(user_dn(user2.username), cn: 'external_group2') } + + before do + stub_ldap_config( + external_groups: %w(external_group1 external_group2), + active_directory: false + ) + stub_ldap_group_find_by_cn('external_group1', external_group1, adapter) + stub_ldap_group_find_by_cn('external_group2', external_group2, adapter) + end + + it 'adds users from both external LDAP groups as external users' do + create(:identity, user: user1, extern_uid: user_dn(user1.username)) + create(:identity, user: user2, extern_uid: user_dn(user2.username)) + + sync_external.update_permissions + + expect(user1.reload.external?).to be true + expect(user2.reload.external?).to be true + end + + it 'removes users that are not in the LDAP group' do + user = create(:external_user) + create(:identity, user: user, extern_uid: user_dn(user.username)) + + expect { sync_external.update_permissions } + .to change { user.reload.external? }.from(true).to(false) + end + + it 'leaves external users that do not have the LDAP provider' do + user = create(:external_user) + + expect { sync_external.update_permissions } + .not_to change { user.reload.external? } + end + + it 'leaves external users that have a different provider identity' do + user = create(:external_user) + create(:identity, user: user, provider: 'ldapsecondary', extern_uid: user_dn(user.username)) + + expect { sync_external.update_permissions } + .not_to change { user.reload.external? } + end + end +end diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb new file mode 100644 index 00000000000000..2e86ead0b5ec30 --- /dev/null +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -0,0 +1,174 @@ +require 'spec_helper' + +describe EE::Gitlab::LDAP::Sync::Group, lib: true do + include LdapHelpers + + let(:sync_group) { described_class.new(group, proxy(adapter)) } + let(:user) { create(:user) } + + before do + allow_any_instance_of(::Gitlab::ExclusiveLease) + .to receive(:try_obtain).and_return(true) + + create(:identity, user: user, extern_uid: user_dn(user.username)) + + stub_ldap_config(active_directory: false) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter) + end + + describe '#update_permissions' do + let(:group) do + create(:group_with_ldap_group_link, + cn: 'ldap_group1', + group_access: ::Gitlab::Access::DEVELOPER) + end + + context 'with all functionality against one LDAP group type' do + context 'with basic add/update actions' do + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + + it 'adds new members' do + sync_group.update_permissions + + expect(group.members.pluck(:user_id)).to include(user.id) + end + + it 'downgrades existing member access' do + # Create user with higher access + group.add_users([user], + ::Gitlab::Access::MASTER, skip_notification: true) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::DEVELOPER) + end + + it 'upgrades existing member access' do + # Create user with lower access + group.add_users([user], + ::Gitlab::Access::GUEST, skip_notification: true) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::DEVELOPER) + end + end + + context 'when existing user is no longer in LDAP group' do + let(:ldap_group1) do + ldap_group_entry(user_dn('some_user')) + end + + it 'removes the user from the group' do + group.add_users([user], + Gitlab::Access::MASTER, skip_notification: true) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id)).to be_nil + end + + it 'refuses to delete the last owner' do + group.add_users([user], + Gitlab::Access::OWNER, skip_notification: true) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::OWNER) + end + end + + context 'when the user is the last owner' do + let(:user1) { create(:user) } + let(:user2) { create(:user) } + let(:ldap_group1) do + ldap_group_entry(%W(#{user_dn(user1.username)} #{user_dn(user2.username)})) + end + + it 'downgrades one user but not the other' do + create(:identity, user: user1, extern_uid: user_dn(user1.username)) + create(:identity, user: user2, extern_uid: user_dn(user2.username)) + group.add_users([user1, user2], + Gitlab::Access::OWNER, skip_notification: true) + + sync_group.update_permissions + + expect(group.members.pluck(:access_level).sort) + .to eq([::Gitlab::Access::DEVELOPER, ::Gitlab::Access::OWNER]) + end + end + end + + # Test that membership can be resolved for all different type of LDAP groups + context 'with different LDAP group types' do + # GroupOfNames - OpenLDAP + context 'with groupOfNames style LDAP group' do + let(:ldap_group1) do + ldap_group_entry( + user_dn(user.username), + objectclass: 'groupOfNames', + member_attr: 'uniqueMember' + ) + end + + it 'adds the user to the group' do + sync_group.update_permissions + + expect(group.members.pluck(:user_id)).to include(user.id) + end + end + + # posixGroup - Apple Open Directory + context 'with posixGroup style LDAP group' do + let(:ldap_group1) do + ldap_group_entry( + user.username, + objectclass: 'posixGroup', + member_attr: 'memberUid' + ) + end + let(:ldap_user) do + ldap_user_entry(user.username) + end + + it 'adds the user to the group' do + stub_ldap_person_find_by_uid(user.username, ldap_user) + + sync_group.update_permissions + + expect(group.members.pluck(:user_id)).to include(user.id) + end + end + + context 'with groupOfUniqueNames style LDAP group' do + let(:ldap_group1) do + ldap_group_entry( + user_dn(user.username), + objectclass: 'groupOfUniqueNames', + member_attr: 'uniqueMember' + ) + end + + it 'adds the user to the group' do + sync_group.update_permissions + + expect(group.members.pluck(:user_id)).to include(user.id) + end + end + + context 'with an empty LDAP group' do + let(:ldap_group1) do + ldap_group_entry(nil) + end + + it 'does nothing, without failure' do + expect { sync_group.update_permissions } + .not_to change { group.members.count } + end + end + end + end +end diff --git a/spec/lib/ee/gitlab/ldap/sync/groups_spec.rb b/spec/lib/ee/gitlab/ldap/sync/groups_spec.rb new file mode 100644 index 00000000000000..66978cda22c26e --- /dev/null +++ b/spec/lib/ee/gitlab/ldap/sync/groups_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +describe EE::Gitlab::LDAP::Sync::Groups, lib: true do + include LdapHelpers + + let(:group_sync) { described_class.new(proxy(adapter)) } + + describe '#update_permissions' do + before do + allow(EE::Gitlab::LDAP::Sync::Group).to receive(:execute) + allow(EE::Gitlab::LDAP::Sync::AdminUsers).to receive(:execute) + allow(EE::Gitlab::LDAP::Sync::ExternalUsers).to receive(:execute) + + 2.times { create(:group_with_ldap_group_link) } + end + + after { group_sync.update_permissions } + + context 'when group_base is not present' do + before { stub_ldap_config(group_base: nil) } + + it 'does not call EE::Gitlab::LDAP::Sync::Group#execute' do + expect(EE::Gitlab::LDAP::Sync::Group).not_to receive(:execute) + end + + it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute) + end + + it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute) + end + end + + context 'when group_base is present' do + + context 'and admin_group and external_groups are not present' do + before { stub_ldap_config(group_base: 'dc=example,dc=com') } + + it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do + expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice + end + + it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute) + end + + it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute) + end + end + + context 'and admin_group is present' do + before do + stub_ldap_config( + group_base: 'dc=example,dc=com', + admin_group: 'my-admin-group' + ) + end + + it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do + expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice + end + + it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::AdminUsers).to receive(:execute).once + end + + it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::ExternalUsers).not_to receive(:execute) + end + end + + context 'and external_groups is present' do + before do + stub_ldap_config( + group_base: 'dc=example,dc=com', + external_groups: %w(external_group) + ) + end + + it 'should call EE::Gitlab::LDAP::Sync::Group#execute' do + expect(EE::Gitlab::LDAP::Sync::Group).to receive(:execute).twice + end + + it 'does not call EE::Gitlab::LDAP::Sync::AdminUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::AdminUsers).not_to receive(:execute) + end + + it 'does not call EE::Gitlab::LDAP::Sync::ExternalUsers#execute' do + expect(EE::Gitlab::LDAP::Sync::ExternalUsers).to receive(:execute).once + end + end + end + end +end diff --git a/spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb b/spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb new file mode 100644 index 00000000000000..cb2b3913b253be --- /dev/null +++ b/spec/lib/ee/gitlab/ldap/sync/proxy_spec.rb @@ -0,0 +1,178 @@ +require 'spec_helper' +require 'net/ldap/dn' + +describe EE::Gitlab::LDAP::Sync::Proxy, lib: true do + include LdapHelpers + + let(:sync_proxy) { EE::Gitlab::LDAP::Sync::Proxy.new('ldapmain', adapter) } + + before do + stub_ldap_config(active_directory: false) + end + + describe '#dns_for_group_cn' do + it 'returns an empty array when LDAP group cannot be found' do + stub_ldap_group_find_by_cn('ldap_group1', nil, adapter) + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to eq([]) + end + + it 'returns an empty array when LDAP group has no members' do + ldap_group = ldap_group_entry(nil) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to eq([]) + end + + context 'with a valid LDAP group that contains members' do + # Create some random usernames and DNs + let(:usernames) { (1..4).map { FFaker::Internet.user_name } } + let(:dns) { usernames.map { |u| user_dn(u) } } + + it 'returns member DNs' do + ldap_group = ldap_group_entry(dns) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns) + end + + it 'returns cached results after the first lookup' do + ldap_group = ldap_group_entry(dns) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + # Do the first lookup to build the cache + sync_proxy.dns_for_group_cn('ldap_group1') + + expect(sync_proxy).not_to receive(:ldap_group_member_dns) + expect(EE::Gitlab::LDAP::Group).not_to receive(:find_by_cn) + + sync_proxy.dns_for_group_cn('ldap_group1') + end + + # posixGroup - Apple Open Directory + it 'returns member DNs for posixGroup' do + ldap_group = ldap_group_entry( + usernames, + objectclass: 'posixGroup', + member_attr: 'memberUid' + ) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + usernames.each do |username| + stub_ldap_person_find_by_uid(username, ldap_user_entry(username)) + end + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns) + end + + it 'returns member DNs when member value is in uid= format' do + ldap_group = ldap_group_entry(usernames.map { |u| "uid=#{u}" }) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + usernames.each do |username| + stub_ldap_person_find_by_uid(username, ldap_user_entry(username)) + end + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns) + end + + it 'returns valid DNs while gracefully skipping malformed DNs' do + mixed_dns = dns.dup << 'invalid_dn' + ldap_group = ldap_group_entry(mixed_dns) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns) + end + + it 'returns valid DNs while gracefully handling empty entries' do + mixed_dns = dns.dup << '' + ldap_group = ldap_group_entry(mixed_dns) + stub_ldap_group_find_by_cn('ldap_group1', ldap_group, adapter) + + expect(sync_proxy.dns_for_group_cn('ldap_group1')).to match_array(dns) + end + end + end + + describe '#dn_for_uid' do + it 'returns nil when no user is found' do + stub_ldap_person_find_by_uid('john_doe', nil) + + expect(sync_proxy.dn_for_uid('john_doe')).to be_nil + end + + context 'when secondary_extern_uid is not stored in the database' do + before do + ldap_user_entry = ldap_user_entry('jane_doe') + stub_ldap_person_find_by_uid('jane_doe', ldap_user_entry, adapter) + end + + it 'returns the user DN' do + expect(sync_proxy.dn_for_uid('jane_doe')) + .to eq('uid=jane_doe,ou=users,dc=example,dc=com') + end + + it 'retrieves the user from LDAP' do + expect(::Gitlab::LDAP::Person).to receive(:find_by_uid) + + sync_proxy.dn_for_uid('jane_doe') + end + + it 'returns cached results after the first lookup' do + sync_proxy.dn_for_uid('jane_doe') + + expect(sync_proxy).not_to receive(:member_uid_to_dn) + expect(Identity).not_to receive(:find_by) + expect(::Gitlab::LDAP::Person).not_to receive(:find_by_uid) + + sync_proxy.dn_for_uid('jane_doe') + end + + it 'saves the secondary_extern_uid' do + user = create(:user) + create(:identity, user: user, extern_uid: user_dn(user.username)) + ldap_user_entry = ldap_user_entry(user.username) + stub_ldap_person_find_by_uid(user.username, ldap_user_entry, adapter) + + expect { sync_proxy.dn_for_uid(user.username) } + .to change { + user.identities.first.secondary_extern_uid + }.from(nil).to(user.username) + end + + it 'is graceful when no user with LDAP identity is found' do + # Create a user with no LDAP identity + user = create(:user) + ldap_user_entry = ldap_user_entry(user.username) + stub_ldap_person_find_by_uid(user.username, ldap_user_entry, adapter) + + expect { sync_proxy.dn_for_uid(user.username) }.not_to raise_error + end + end + + context 'when secondary_extern_uid is stored in the database' do + let(:user) { create(:user) } + + before do + create( + :identity, + user: user, + extern_uid: user_dn(user.username), + secondary_extern_uid: user.username + ) + end + + after { sync_proxy.dn_for_uid(user.username) } + + it 'does not query LDAP' do + expect(::Gitlab::LDAP::Person).not_to receive(:find_by_uid) + end + + it 'retrieves the DN from the identity' do + expect(Identity) + .to receive(:find_by) + .with( + provider: sync_proxy.provider, + secondary_extern_uid: user.username + ).once.and_call_original + end + end + end +end diff --git a/spec/support/ldap_helpers.rb b/spec/support/ldap_helpers.rb new file mode 100644 index 00000000000000..c49544acd1781f --- /dev/null +++ b/spec/support/ldap_helpers.rb @@ -0,0 +1,110 @@ +module LdapHelpers + def adapter(provider = 'ldapmain') + ::Gitlab::LDAP::Adapter.new(provider, double(:ldap)) + end + + def proxy(adapter, provider = 'ldapmain') + EE::Gitlab::LDAP::Sync::Proxy.new(provider, adapter) + end + + def user_dn(uid) + "uid=#{uid},ou=users,dc=example,dc=com" + end + + # Accepts a hash of Gitlab::LDAP::Config keys and values. + # + # Example: + # stub_ldap_config( + # group_base: 'ou=groups,dc=example,dc=com', + # admin_group: 'my-admin-group' + # ) + def stub_ldap_config(messages) + messages.each do |config, value| + allow_any_instance_of(::Gitlab::LDAP::Config) + .to receive(config.to_sym).and_return(value) + end + end + + # Stub an LDAP person search and provide the return entry. Specify `nil` for + # `entry` to simulate when an LDAP person is not found + # + # Example: + # adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap)) + # ldap_user_entry = ldap_user_entry('john_doe') + # + # stub_ldap_person_find_by_uid('john_doe', ldap_user_entry, adapter) + def stub_ldap_person_find_by_uid(uid, entry, provider = 'ldapmain') + return_value = if entry.present? + ::Gitlab::LDAP::Person.new(entry, provider) + else + nil + end + + allow(::Gitlab::LDAP::Person) + .to receive(:find_by_uid).with(uid, any_args).and_return(return_value) + end + + # Stub an LDAP group search and provide the return entry. Specify `nil` for + # `entry` to simulate when an LDAP group is not found + # + # Example: + # adapter = ::Gitlab::LDAP::Adapter.new('ldapmain', double(:ldap)) + # ldap_group1 = ldap_group_entry('uid=user,ou=users,dc=example,dc=com') + # + # stub_ldap_group_find_by_cn('ldap_group1', ldap_group1, adapter) + def stub_ldap_group_find_by_cn(cn, entry, adapter = nil) + return_value = if entry.present? + EE::Gitlab::LDAP::Group.new(entry, adapter) + else + nil + end + + allow(EE::Gitlab::LDAP::Group) + .to receive(:find_by_cn) + .with(cn, kind_of(::Gitlab::LDAP::Adapter)).and_return(return_value) + end + + # Create a simple LDAP user entry. + def ldap_user_entry(uid) + Net::LDAP::Entry.from_single_ldif_string("dn: #{user_dn(uid)}") + end + + # Create an LDAP group entry with any number of members. By default, creates + # a groupOfNames style entry. Change the style by specifying the object class + # and member attribute name. The last example below shows how to specify a + # posixGroup (Apple Open Directory) entry. `members` can be nil to create + # an empty group. + # + # Example: + # ldap_group_entry('uid=user,ou=users,dc=example,dc=com') + # + # ldap_group_entry( + # 'uid=user1,ou=users,dc=example,dc=com', + # 'uid=user2,ou=users,dc=example,dc=com' + # ) + # + # ldap_group_entry( + # [ 'user1', 'user2' ], + # cn: 'my_group' + # objectclass: 'posixGroup', + # member_attr: 'memberUid' + # ) + def ldap_group_entry( + members, + cn: 'ldap_group1', + objectclass: 'groupOfNames', + member_attr: 'uniqueMember' + ) + entry = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc) + dn: cn=#{cn},ou=groups,dc=example,dc=com + cn: #{cn} + description: LDAP Group #{cn} + objectclass: top + objectclass: #{objectclass} + EOS + + members = [members].flatten + entry[member_attr] = members if members.any? + entry + end +end -- GitLab