Check parent group membership in LDAP group sync

And improve sync performance by using batch queries
instead of individual queries per user.

ee/changelogs/unreleased-ee/9613-ldap-sync-check-parent-group-member.yml

+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

ee/lib/ee/gitlab/auth/ldap/sync/group.rb

@@ -106,6 +106,19 @@ module EE
                 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 @@ module EE
               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 @@ module EE
                   # 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 @@ module EE
             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 @@ module EE
             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

ee/spec/lib/ee/gitlab/auth/ldap/sync/group_spec.rb

@@ -307,6 +307,152 @@ describe EE::Gitlab::Auth::LDAP::Sync::Group do
         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) }