[pve-devel] [PATCH common] ldap: de-duplicate LDAP search for users and groups

[pve-devel] [PATCH common] ldap: de-duplicate LDAP search for users and groups

[Christoph Heiss]

For the most part, query_users() and query_groups() are identical, so
extract the common part into a separate method and let
query_{users,groups}() be simple wrappers over it.

No change in functionality.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 src/PVE/LDAP.pm | 192 ++++++++++++++++++++----------------------------
 1 file changed, 81 insertions(+), 111 deletions(-)

diff --git a/src/PVE/LDAP.pm b/src/PVE/LDAP.pm
index 342c352..a56b957 100644
--- a/src/PVE/LDAP.pm
+++ b/src/PVE/LDAP.pm
@@ -99,29 +99,41 @@ sub auth_user_dn {
     return 1;
 }
 
-sub query_users {
-    my ($ldap, $filter, $attributes, $base_dn, $classes) = @_;
+my $build_filter = sub {
+    my ($expr, $parts) = @_;
 
-    # build filter from given filter and attribute list
-    my $tmp = "(|";
-    foreach my $att (@$attributes) {
-	$tmp .= "($att=*)";
+    my $res = '(|';
+    for my $part (@$parts) {
+	$res .= &$expr($part);
     }
-    $tmp .= ")";
 
-    if ($classes) {
-	$tmp = "(&$tmp(|";
-	for my $class (@$classes) {
-	    $tmp .= "(objectclass=$class)";
-	}
-	$tmp .= "))";
-    }
+    return "$res)";
+};
 
-    if ($filter) {
-	$filter = "($filter)" if $filter !~ m/^\(.*\)$/;
-	$filter = "(&${filter}${tmp})"
+my $query_objects = sub {
+    my ($ldap, $base_dn, $custom_filter, $classes, $attributes, $search_attrs) = @_;
+
+    # build filter from given class and attribute list
+    my $class_filter = &$build_filter(sub { "(objectclass=$_[0])" }, $classes) if defined($classes);
+    my $attr_filter = &$build_filter(sub { "($_[0]=*)" }, $attributes) if defined($attributes);
+
+    # $class_filter is always defined
+    my $filter;
+    if (defined($class_filter) && defined($attr_filter)) {
+	$filter = "(&${class_filter}${attr_filter})";
     } else {
-	$filter = $tmp;
+	$filter = $class_filter || $attr_filter;
+    }
+
+    # combine custom (user-supplied) filter and internal one using AND
+    if ($custom_filter) {
+	$custom_filter = "($custom_filter)" if $custom_filter !~ m/^\(.*\)$/;
+
+	if ($filter) {
+	    $filter = "(& {${custom_filter}${filter})";
+	} else {
+	    $filter = $custom_filter;
+	}
     }
 
     my $page = Net::LDAP::Control::Paged->new(size => 900);
@@ -131,40 +143,23 @@ sub query_users {
 	scope    => "subtree",
 	filter   => $filter,
 	control  => [ $page ],
-	attrs    => [ @$attributes, 'memberOf'],
+	attrs    => $search_attrs,
     );
 
     my $cookie;
     my $err;
-    my $users = [];
+    my $objs = [];
 
     while(1) {
-
 	my $mesg = $ldap->search(@args);
 
 	# stop on error
 	if ($mesg->code)  {
-	    $err = "ldap user search error: " . $mesg->error;
+	    $err = $mesg->error;
 	    last;
 	}
 
-	#foreach my $entry ($mesg->entries) { $entry->dump; }
-	foreach my $entry ($mesg->entries) {
-	    my $user = {
-		dn => $entry->dn,
-		attributes => {},
-		groups => [$entry->get_value('memberOf')],
-	    };
-
-	    foreach my $attr (@$attributes) {
-		my $vals = [$entry->get_value($attr)];
-		if (scalar(@$vals)) {
-		    $user->{attributes}->{$attr} = $vals;
-		}
-	    }
-
-	    push @$users, $user;
-	}
+	push @$objs, $mesg->entries;
 
 	# Get cookie from paged control
 	my ($resp) = $mesg->control(LDAP_CONTROL_PAGED) or last;
@@ -181,93 +176,68 @@ sub query_users {
 	$page->cookie($cookie);
 	$page->size(0);
 	$ldap->search(@args);
-	$err = "LDAP user query unsuccessful" if !$err;
+	$err = "query unsuccessful" if !$err;
     }
 
-    die $err if $err;
+    die "$err\n" if $err;
+    return $objs;
+};
 
-    return $users;
-}
-
-sub query_groups {
-    my ($ldap, $base_dn, $classes, $filter, $group_name_attr) = @_;
+sub query_users {
+    my ($ldap, $filter, $attributes, $base_dn, $classes) = @_;
 
-    my $tmp = "(|";
-    for my $class (@$classes) {
-	$tmp .= "(objectclass=$class)";
-    }
-    $tmp .= ")";
+    my $search_attrs = [ @$attributes, 'memberOf' ];
+    my $objs = &$query_objects($ldap, $base_dn, $filter, $classes, $attributes, $search_attrs);
+
+    my @users = map {
+	my $entry = $_;
+	my $user = {
+	    dn => $entry->dn,
+	    attributes => {},
+	    groups => [$entry->get_value('memberOf')],
+	};
+
+	foreach my $attr (@$attributes) {
+	    my $vals = [$entry->get_value($attr)];
+	    if (scalar(@$vals)) {
+		$user->{attributes}->{$attr} = $vals;
+	    }
+	}
 
-    if ($filter) {
-	$filter = "($filter)" if $filter !~ m/^\(.*\)$/;
-	$filter = "(&${filter}${tmp})"
-    } else {
-	$filter = $tmp;
-    }
+	$user
+    } @$objs;
 
-    my $page = Net::LDAP::Control::Paged->new(size => 100);
+    return \@users;
+}
 
-    my $attrs = [ 'member', 'uniqueMember' ];
-    push @$attrs, $group_name_attr if $group_name_attr;
-    my @args = (
-	base     => $base_dn,
-	scope    => "subtree",
-	filter   => $filter,
-	control  => [ $page ],
-	attrs    => $attrs,
-    );
+sub query_groups {
+    my ($ldap, $base_dn, $classes, $filter, $group_name_attr) = @_;
 
-    my $cookie;
-    my $err;
-    my $groups = [];
+    my $search_attrs = [ 'member', 'uniqueMember' ];
+    push @$search_attrs, $group_name_attr if $group_name_attr;
 
-    while(1) {
+    my $objs = &$query_objects($ldap, $base_dn, $filter, $classes, undef, $search_attrs);
 
-	my $mesg = $ldap->search(@args);
+    my @groups = map {
+	my $entry = $_;
+	my $group = {
+	    dn => $entry->dn,
+	    members => [],
+	};
 
-	# stop on error
-	if ($mesg->code)  {
-	    $err = "ldap group search error: " . $mesg->error;
-	    last;
+	my $members = [$entry->get_value('member')];
+	if (!scalar(@$members)) {
+	    $members = [$entry->get_value('uniqueMember')];
 	}
-
-	foreach my $entry ( $mesg->entries ) {
-	    my $group = {
-		dn => $entry->dn,
-		members => []
-	    };
-	    my $members = [$entry->get_value('member')];
-	    if (!scalar(@$members)) {
-		$members = [$entry->get_value('uniqueMember')];
-	    }
-	    $group->{members} = $members;
-	    if ($group_name_attr && (my $name = $entry->get_value($group_name_attr))) {
-		$group->{name} = $name;
-	    }
-	    push @$groups, $group;
+	$group->{members} = $members;
+	if ($group_name_attr && (my $name = $entry->get_value($group_name_attr))) {
+	    $group->{name} = $name;
 	}
 
-	# Get cookie from paged control
-	my ($resp) = $mesg->control(LDAP_CONTROL_PAGED) or last;
-	$cookie = $resp->cookie;
-
-	last if (!defined($cookie) || !length($cookie));
-
-	# Set cookie in paged control
-	$page->cookie($cookie);
-    }
-
-    if ($cookie) {
-	# We had an abnormal exit, so let the server know we do not want any more
-	$page->cookie($cookie);
-	$page->size(0);
-	$ldap->search(@args);
-	$err = "LDAP group query unsuccessful" if !$err;
-    }
-
-    die $err if $err;
+	$group
+    } @$objs;
 
-    return $groups;
+    return \@groups;
 }
 
 1;
-- 
2.41.0