From 4557348a899d0698cd0ca7f6fe91453d1a39c358 Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Mon, 27 May 2019 17:43:40 +0200 Subject: [PATCH 1/3] Check parent group membership in LDAP group sync And improve sync performance by using batch queries instead of individual queries per user. --- ...13-ldap-sync-check-parent-group-member.yml | 4 + ee/lib/ee/gitlab/auth/ldap/sync/group.rb | 92 +++++++++-- .../ee/gitlab/auth/ldap/sync/group_spec.rb | 146 ++++++++++++++++++ 3 files changed, 227 insertions(+), 15 deletions(-) create mode 100644 ee/changelogs/unreleased-ee/9613-ldap-sync-check-parent-group-member.yml 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 00000000000000..35c73f7869f853 --- /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 81a3d45f687c07..42d7bb7e7c1d3b 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. + propagate_inherited_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,61 @@ 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 permission inherited from a parent group + # rubocop: disable CodeReuse/ActiveRecord + def propagate_inherited_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(identities: { extern_uid: access_levels.keys }) + .joins(user: :identities) + .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 +211,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 +231,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 +293,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).where(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 6213831df343b0..95daec967bfe56 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 acces 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 acces 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) } -- GitLab From 4a6b78b96ce2356bc6551a7291cc26124decd703 Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Tue, 27 Aug 2019 11:44:35 +0200 Subject: [PATCH 2/3] Extern_uid lookups should be case sensitive Also apply other suggested small changes --- ee/lib/ee/gitlab/auth/ldap/sync/group.rb | 11 +++++------ ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb | 4 ++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb index 42d7bb7e7c1d3b..8569c2a228cc3d 100644 --- a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb +++ b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb @@ -109,7 +109,7 @@ def update_permissions # 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. - propagate_inherited_access_levels(group, access_levels) + inherit_higher_access_levels(group, access_levels) logger.debug do <<-MSG.strip_heredoc.tr("\n", ' ') @@ -150,9 +150,9 @@ def dns_for_group_cn(group_cn) end # for all LDAP Distinguished Names in access_levels, merge access level - # with any permission inherited from a parent group + # with any higher permission inherited from a parent group # rubocop: disable CodeReuse/ActiveRecord - def propagate_inherited_access_levels(group, access_levels) + 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, @@ -161,8 +161,7 @@ def propagate_inherited_access_levels(group, access_levels) permissions_in_ancestry = ::GroupMember.of_groups(group.ancestors) .non_request .with_identity_provider(provider) - .where(identities: { extern_uid: access_levels.keys }) - .joins(user: :identities) + .where(users: { identities: Identity.iwhere(extern_uid: access_levels.keys) }) .select(:id, 'identities.extern_uid AS distinguished_name', :access_level, :source_id) .references(:identities) @@ -303,7 +302,7 @@ def resolve_ldap_identities(for_users:) # 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).where(extern_uid: for_normalized_dns) + ::Identity.with_provider(provider).iwhere(extern_uid: for_normalized_dns) .preload(:user) .map {|identity| [identity.extern_uid, identity.user] } .to_h 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 95daec967bfe56..604735a55073c4 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 @@ -354,14 +354,14 @@ def execute parent_group.add_reporter(user) end - it "adds member with the ldap group link's acces level" do + 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 acces level" do + 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 -- GitLab From 3c974a80d86e98dd90a11c0d6da57f0d83e7444a Mon Sep 17 00:00:00 2001 From: Alex Lossent Date: Tue, 27 Aug 2019 15:00:34 +0200 Subject: [PATCH 3/3] Fix identity scope --- ee/lib/ee/gitlab/auth/ldap/sync/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb index 8569c2a228cc3d..2c8d6642b002b4 100644 --- a/ee/lib/ee/gitlab/auth/ldap/sync/group.rb +++ b/ee/lib/ee/gitlab/auth/ldap/sync/group.rb @@ -161,7 +161,7 @@ def inherit_higher_access_levels(group, access_levels) permissions_in_ancestry = ::GroupMember.of_groups(group.ancestors) .non_request .with_identity_provider(provider) - .where(users: { identities: Identity.iwhere(extern_uid: access_levels.keys) }) + .where(users: { identities: ::Identity.iwhere(extern_uid: access_levels.keys) }) .select(:id, 'identities.extern_uid AS distinguished_name', :access_level, :source_id) .references(:identities) -- GitLab