diff --git a/ee/changelogs/unreleased-ee/9613-ldap-sync-check-parent-group-member.yml b/ee/changelogs/unreleased-ee/9613-ldap-sync-check-parent-group-member.yml new file mode 100644 index 0000000000000000000000000000000000000000..35c73f7869f8538d2f8de5a07a3d4eb3139c1592 --- /dev/null +++ b/ee/changelogs/unreleased-ee/9613-ldap-sync-check-parent-group-member.yml @@ -0,0 +1,4 @@ +title: "LDAP group sync: check parent group membership and improve performance" +merge_request: 13435 +author: Alex Lossent +type: fixed \ No newline at end of file diff --git a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb index 81a3d45f687c07889e4d93b3993a0b46300cc337..2c8d6642b002b47de10a872fb93798ec869237ba 100644 --- a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb +++ b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb @@ -106,6 +106,19 @@ def update_permissions update_access_levels(access_levels, group_link) end + # Users in this LDAP group may already have a higher access level in a parent group. + # Currently demoting a user in a subgroup is forbidden by (Group)Member validation + # so we must propagate any higher inherited permissions unconditionally. + inherit_higher_access_levels(group, access_levels) + + logger.debug do + <<-MSG.strip_heredoc.tr("\n", ' ') + Resolved '#{group.name}' group member access, + propagating any higher access inherited from a parent group: + #{access_levels.to_hash} + MSG + end + update_existing_group_membership(group, access_levels) add_new_members(group, access_levels) end @@ -136,38 +149,60 @@ def dns_for_group_cn(group_cn) proxy.dns_for_group_cn(group_cn) end + # for all LDAP Distinguished Names in access_levels, merge access level + # with any higher permission inherited from a parent group + # rubocop: disable CodeReuse/ActiveRecord + def inherit_higher_access_levels(group, access_levels) + return unless group.parent + + # for any permission granted by an ancestor group to any DN in access_levels, + # retrieve user DN, access_level and ID of the group providing it. + # Ignore unapproved access requests. + permissions_in_ancestry = ::GroupMember.of_groups(group.ancestors) + .non_request + .with_identity_provider(provider) + .where(users: { identities: ::Identity.iwhere(extern_uid: access_levels.keys) }) + .select(:id, 'identities.extern_uid AS distinguished_name', :access_level, :source_id) + .references(:identities) + + permissions_in_ancestry.each do |member| + access_levels.set([member.distinguished_name], to: member.access_level) + end + end + # rubocop: enable CodeReuse/ActiveRecord + 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| + multiple_ldap_providers = ::Gitlab::Auth::LDAP::Config.providers.count > 1 + existing_members = select_and_preload_group_members(group) + # For each existing group member, we'll need to look up its LDAP identity in the current LDAP provider. + # It is much faster to resolve these at once than later for each member one by one. + ldap_identity_by_user_id = resolve_ldap_identities(for_users: existing_members.map(&:user)) + + existing_members.each do |member| user = member.user - identity = user.identities.select(:id, :extern_uid) - .with_provider(provider).first - member_dn = identity.extern_uid.downcase + identity = ldap_identity_by_user_id[user.id] # Skip if this is not an LDAP user with a valid `extern_uid`. - next unless member_dn.present? + next unless identity.present? && identity.extern_uid.present? + + member_dn = identity.extern_uid # 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 + if multiple_ldap_providers && user.ldap_identity.id != identity.id access_levels.delete(member_dn) next end - desired_access = access_levels[member_dn] - # Skip validations and callbacks. We have a limited set of attrs # due to the `select` lookup, and we need to be efficient. # Low risk, because the member should already be valid. member.update_column(:ldap, true) unless member.ldap? - # 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 + desired_access = access_levels[member_dn] # Check and update the access level. If `desired_access` is `nil` # we need to delete the user from the group. @@ -175,7 +210,9 @@ def update_existing_group_membership(group, access_levels) # Delete this entry from the hash now that we're acting on it access_levels.delete(member_dn) - next if member.ldap? && member.override? + # Don't do anything if the user already has the desired access level + # and respect existing overrides + next if member.access_level == desired_access || member.override? add_or_update_user_membership( user, @@ -193,8 +230,15 @@ def update_existing_group_membership(group, access_levels) def add_new_members(group, access_levels) logger.debug { "Adding new members to '#{group.name}' group" } + return unless access_levels.present? + + # Even in the absence of new members, the list of DNs to add can be consistently large + # when LDAP groups contain members who do not have a gitlab account. + # Thus we can be a lot more efficient by pre-resolving all candidate DNs into gitlab users. + gitlab_users_by_dn = resolve_users_from_normalized_dn(for_normalized_dns: access_levels.keys) + access_levels.each do |member_dn, access_level| - user = ::Gitlab::Auth::LDAP::User.find_by_uid_and_provider(member_dn, provider) + user = gitlab_users_by_dn[member_dn] if user.present? add_or_update_user_membership( @@ -248,6 +292,23 @@ def select_and_preload_group_members(group) end # rubocop: enable CodeReuse/ActiveRecord + # returns a hash user_id -> LDAP identity in current LDAP provider + def resolve_ldap_identities(for_users:) + ::Identity.for_user(for_users).with_provider(provider) + .map {|identity| [identity.user_id, identity] } + .to_h + end + + # returns a hash of normalized DN -> user for the current LDAP provider + # rubocop: disable CodeReuse/ActiveRecord + def resolve_users_from_normalized_dn(for_normalized_dns:) + ::Identity.with_provider(provider).iwhere(extern_uid: for_normalized_dns) + .preload(:user) + .map {|identity| [identity.extern_uid, identity.user] } + .to_h + end + # rubocop: enable CodeReuse/ActiveRecord + def logger Rails.logger # rubocop:disable Gitlab/RailsLogger end diff --git a/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb b/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb index 6213831df343b046d47401da3c2eaa7828445f11..604735a55073c4926c1a641884c292de2bdeefcc 100644 --- a/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb +++ b/ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb @@ -307,6 +307,152 @@ def execute end end + context 'when user inherits higher permissions from parent' do + let(:parent_group) { create(:group) } + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + before do + group.update(parent: parent_group) + parent_group.add_maintainer(user) + end + + it "adds member with the inherited higher permission" do + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::MAINTAINER) + end + + it "upgrades existing member to the inherited higher permission" do + group.add_user(user, Gitlab::Access::DEVELOPER) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::MAINTAINER) + end + + it "does not alter an ldap member that has a permission override" do + group.members.create( + user: user, + access_level: ::Gitlab::Access::OWNER, + ldap: true, + override: 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 user inherits lower permissions from parent' do + let(:parent_group) { create(:group) } + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + before do + group.update(parent: parent_group) + parent_group.add_reporter(user) + end + + it "adds member with the ldap group link's access level" do + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::DEVELOPER) + end + + it "downgrades existing member access to the ldap group link's access level" do + group.add_user(user, Gitlab::Access::MAINTAINER) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::DEVELOPER) + end + + it "does not alter an ldap member that has a permission override" do + group.members.create( + user: user, + access_level: ::Gitlab::Access::OWNER, + ldap: true, + override: 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 user has a pending access request in a parent group' do + let(:parent_group) { create(:group, :access_requestable) } + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + let(:access_requester) { parent_group.request_access(user) } + before do + group.update(parent: parent_group) + parent_group.add_owner(create(:user)) + end + + it 'does not propagate the access level of the pending access request' do + group.members.create( + user: user, + access_level: ::Gitlab::Access::DEVELOPER, + ldap: true + ) + access_requester.update(access_level: ::Gitlab::Access::MAINTAINER) + + sync_group.update_permissions + + expect(parent_group.requesters.find_by(id: access_requester.id).access_level) + .to eq(::Gitlab::Access::MAINTAINER) + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::DEVELOPER) + end + end + + context 'when user inherits permissions from parent and user is no longer in LDAP group' do + let(:parent_group) { create(:group) } + let(:ldap_group1) { ldap_group_entry(user_dn('other_user')) } + before do + group.update(parent: parent_group) + parent_group.add_maintainer(user) + end + + it "removes existing member" do + group.add_user(user, Gitlab::Access::MAINTAINER) + + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id)).to be_nil + end + end + + context 'when permissions are inherited from a complex ancestry' do + let(:ldap_group1) { ldap_group_entry(user_dn(user.username)) } + let(:group1) { create(:group) } + let(:group2) { create(:group) } + let(:group3) { create(:group) } + before do + group1.add_reporter(user) + + group2.update(parent: group1) + group2.add_maintainer(user) + + group3.update(parent: group2) + # no specific permission for user in group3 + + group.update(parent: group3) + end + + it "applies the permission inherited from the closest ancestor when it's higher" do + sync_group.update_permissions + + expect(group.members.find_by(user_id: user.id).access_level) + .to eq(::Gitlab::Access::MAINTAINER) + end + end + context 'when the extern_uid and group member DNs have different case' do let(:user1) { create(:user) } let(:user2) { create(:user) }