Merge branch 'group-attribute-value'

README

@@ -92,12 +92,17 @@ The search filter to apply (in this case, find all the bobs)
 A mapping of
 Attribute in RT => Attribute in LDAP
 (this has changed since version 1, which was the other way around)
- Set($LDAPGroupMapping, {Name         => 'cn',
-                         Member_Attr  => 'member'});
+ Set($LDAPGroupMapping, {Name               => 'cn',
+                         Member_Attr        => 'member',
+                         Member_Attr_Value  => 'dn' });
 
 The mapping logic is the same as the LDAPMapping.
-There is one important special-case variable, Member_Attr
-Use this to tell the importer which attribute will contain DNs of group members
+There are two important special-case keys, Member_Attr and Member_Attr_Value.
+Member_Attr tells the importer which attribute contains group members.
+Member_Attr_Value, which defaults to 'dn', specifies what kind of user values
+are in Member_Attr.  OpenLDAP, for example, often stores uid instead of dn in
+member.
+
 If you do not specify a Description attribute, it will be filled with
 'Imported from LDAP'
 

lib/RT/Extension/LDAPImport.pm

@@ -5,9 +5,10 @@ our $VERSION = '0.31';
 use warnings;
 use strict;
 use base qw(Class::Accessor);
-__PACKAGE__->mk_accessors(qw(_ldap _group screendebug _dnlist));
+__PACKAGE__->mk_accessors(qw(_ldap _group screendebug _users));
 use Carp;
 use Net::LDAP;
+use Net::LDAP::Util qw(escape_filter_value);
 use Data::Dumper;
 
 =head1 NAME
@@ -164,12 +165,11 @@ sub import_users {
     my $mapping = $RT::LDAPMapping;
     return unless $self->_check_ldap_mapping( mapping => $mapping );
 
-    $self->_dnlist({});
+    $self->_users({});
 
     my $done = 0; my $count = $results->count;
     while (my $entry = $results->shift_entry) {
-        my $user = $self->_build_object( ldap_entry => $entry, skip => qr/(?i)^CF\./, mapping => $mapping );
-        $user->{Name} ||= $user->{EmailAddress};
+        my $user = $self->_build_user_object( ldap_entry => $entry );
         unless ( $user->{Name} ) {
             $self->_warn("No Name or Emailaddress for user, skipping ".Dumper $user);
             next;
@@ -196,7 +196,7 @@ sub _import_user {
     my %args = @_;
 
     $self->_debug("Processing user $args{user}{Name}");
-    $self->_dnlist->{lc $args{ldap_entry}->dn} = $args{user}{Name};
+    $self->_cache_user( %args );
 
     $args{user} = $self->create_rt_user( %args );
     return unless $args{user};
@@ -207,6 +207,38 @@ sub _import_user {
     return 1;
 }
 
+=head2 _cache_user ldap_entry => Net::LDAP::Entry, [user => { ... }]
+
+Adds the user to a global cache which is used when importing groups later.
+
+Optionally takes a second argument which is a user data object returned by
+_build_user_object.  If not given, _cache_user will call _build_user_object
+itself.
+
+Returns the user Name.
+
+=cut
+
+sub _cache_user {
+    my $self = shift;
+    my %args = (@_);
+    my $user = $args{user} || $self->_build_user_object( ldap_entry => $args{ldap_entry} );
+
+    my $group_map       = $RT::LDAPGroupMapping           || {};
+    my $member_attr_val = $group_map->{Member_Attr_Value} || 'dn';
+    my $membership_key  = lc $member_attr_val eq 'dn'
+                            ? $args{ldap_entry}->dn
+                            : $args{ldap_entry}->get_value($member_attr_val);
+
+    # Fallback to the DN if the user record doesn't have a value
+    unless (defined $membership_key) {
+        $membership_key = $args{ldap_entry}->dn;
+        $self->_warn("User attribute '$member_attr_val' has no value for '$membership_key'; falling back to DN");
+    }
+
+    return $self->_users->{lc $membership_key} = $user->{Name};
+}
+
 sub _show_user_info {
     my $self = shift;
     my %args = @_;
@@ -253,10 +285,28 @@ sub _check_ldap_mapping {
     return 1;
 }
 
+=head2 _build_user_object
+
+Utility method which wraps _build_object to provide sane defaults for building
+users.  It also tries to ensure a Name exists in the returned object.
+
+=cut
+
+sub _build_user_object {
+    my $self = shift;
+    my $user = $self->_build_object(
+        skip    => qr/(?i)^CF\./,
+        mapping => $RT::LDAPMapping,
+        @_
+    );
+    $user->{Name} ||= $user->{EmailAddress};
+    return $user;
+}
+
 =head2 _build_object
 
 Builds up data from LDAP for importing
-Returns a hash of user data ready for RT::User::Create
+Returns a hash of user or group data ready for RT::User::Create or RT::Group::Create
 
 =cut
 
@@ -680,7 +730,7 @@ sub create_rt_group {
 
 =head3 add_group_members
 
-Iterate over the list of DNs in the Member_Attr LDAP entry.
+Iterate over the list of values in the Member_Attr LDAP entry.
 Look up the appropriate username from LDAP.
 Add those users to the group.
 Remove members of the RT Group who are no longer members
@@ -714,23 +764,24 @@ sub add_group_members {
         $self->_debug("No group in RT, would create with members:");
     }
 
-    my $dnlist = $self->_dnlist;
+    my $users = $self->_users;
     foreach my $member (@$members) {
         my $username;
-        if (exists $dnlist->{lc $member}) {
-            next unless $username = $dnlist->{lc $member};
+        if (exists $users->{lc $member}) {
+            next unless $username = $users->{lc $member};
         } else {
             my $ldap_users = $self->_run_search(
-                base   => $member,
-                filter => $RT::LDAPFilter,
+                base   => $RT::LDAPBase,
+                filter => "(&$RT::LDAPFilter($RT::LDAPGroupMapping->{Member_Attr_Value}="
+                            . escape_filter_value($member) . "))"
             );
             unless ( $ldap_users && $ldap_users->count ) {
-                $dnlist->{lc $member} = undef;
+                $users->{lc $member} = undef;
                 $self->_error("No user found for $member who should be a member of $groupname");
                 next;
             }
             my $ldap_user = $ldap_users->shift_entry;
-            $dnlist->{lc $member} = $username = $ldap_user->get_value($RT::LDAPMapping->{Name});
+            $username = $self->_cache_user( ldap_entry => $ldap_user );
         }
         if ( delete $rt_group_members{$username} ) {
             $self->_debug("\t$username\tin RT and LDAP");

t/group-import.t

@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use lib 't/lib';
-use RT::Extension::LDAPImport::Test tests => 43;
+use RT::Extension::LDAPImport::Test tests => 66;
 eval { require Net::LDAP::Server::Test; 1; } or do {
     plan skip_all => 'Unable to test without Net::Server::LDAP::Test';
 };
@@ -40,6 +40,7 @@ for ( 1 .. 4 ) {
     my $entry = {
         cn   =>  $groupname,
         members => [ map { $_->{dn} } @ldap_user_entries[($_-1),($_+3),($_+7)] ],
+        memberUid => [ map { $_->{uid} } @ldap_user_entries[($_+1),($_+3),($_+5)] ],
         objectClass => 'Group',
     };
     $ldap->add( $dn, attr => [%$entry] );
@@ -89,40 +90,55 @@ ok( $importer->import_groups() );
     is($groups->Count,0);
 }
 
-ok( $importer->import_groups( import => 1 ) );
-for my $entry (@ldap_group_entries) {
-    my $group = RT::Group->new($RT::SystemUser);
-    $group->LoadUserDefinedGroup( $entry->{cn} );
-    ok($group->Id, "Found $entry->{cn} as ".$group->Id);
-
-    my $idlist;
-    my $members = $group->MembersObj;
-    while (my $group_member = $members->Next) {
-        my $member = $group_member->MemberObj;
-        next unless $member->IsUser();
-        $idlist->{$member->Object->Id}++;
-    }
+import_group_members_ok( members => 'dn' );
+
+RT->Config->Set('LDAPGroupMapping',
+                   {Name                => 'cn',
+                    Member_Attr         => 'memberUid',
+                    Member_Attr_Value   => 'uid',
+                   });
+import_group_members_ok( memberUid => 'uid' );
+
+sub import_group_members_ok {
+    my $attr = shift;
+    my $user_attr = shift;
+
+    ok( $importer->import_groups( import => 1 ), "imported groups" );
 
-    foreach my $dn ( @{$entry->{members}} ) {
-        my ($user) = grep { $_->{dn} eq $dn } @ldap_user_entries;
-        my $rt_user = RT::User->new($RT::SystemUser);
-        my ($res,$msg) = $rt_user->Load($user->{uid});
-        unless ($res) {
-            diag("Couldn't load user $user->{uid}: $msg");
-            next;
+    for my $entry (@ldap_group_entries) {
+        my $group = RT::Group->new($RT::SystemUser);
+        $group->LoadUserDefinedGroup( $entry->{cn} );
+        ok($group->Id, "Found $entry->{cn} as ".$group->Id);
+
+        my $idlist;
+        my $members = $group->MembersObj;
+        while (my $group_member = $members->Next) {
+            my $member = $group_member->MemberObj;
+            next unless $member->IsUser();
+            $idlist->{$member->Object->Id}++;
+        }
+
+        foreach my $member ( @{$entry->{$attr}} ) {
+            my ($user) = grep { $_->{$user_attr} eq $member } @ldap_user_entries;
+            my $rt_user = RT::User->new($RT::SystemUser);
+            my ($res,$msg) = $rt_user->Load($user->{uid});
+            unless ($res) {
+                diag("Couldn't load user $user->{uid}: $msg");
+                next;
+            }
+            ok($group->HasMember($rt_user->PrincipalObj->Id),"Correctly assigned $user->{uid} to $entry->{cn}");
+            delete $idlist->{$rt_user->Id};
         }
-        ok($group->HasMember($rt_user->PrincipalObj->Id),"Correctly assigned $user->{uid} to $entry->{cn}");
-        delete $idlist->{$rt_user->Id};
+        is(keys %$idlist,0,"No dangling users");
     }
-    is(keys %$idlist,0,"No dangling users");
-}
 
-my $group = RT::Group->new($RT::SystemUser);
-$group->LoadUserDefinedGroup( "42" );
-ok( !$group->Id );
+    my $group = RT::Group->new($RT::SystemUser);
+    $group->LoadUserDefinedGroup( "42" );
+    ok( !$group->Id );
 
-$group->LoadByCols(
-    Domain => 'UserDefined',
-    Name   => "42",
-);
-ok( !$group->Id );
+    $group->LoadByCols(
+        Domain => 'UserDefined',
+        Name   => "42",
+    );
+    ok( !$group->Id );
+}