Bug #48562 for Net-LDAP-Class: Possible bug or just user error with contacts in groups

Sun Aug 09 14:03:46 2009 PJNEWMAN [...] cpan.org - Ticket created

Subject:

Possible bug or just user error with contacts in groups

Hi, I don't know if this is user error on my part due to not understanding enough about AD or your classes, a bug/foible due to the potentially odd way we use AD or something else. Anyway I'm trying to recursively find all users in a group and any of its subgroups and I hit a few issues with it failing when calling fetch_secondary_users with a "can't find user" message, after a bit of checking of AD, this always seems to occur when there is a contact, as opposed to a proper user in the group, and its always the contact referred to in the error message. Changing line 141 in Net/LDAP/Class/Group/AD.pm from if ($user) { to if (defined($user)) { Has cured my issue, although doing a straight print of the $user or $user->sAMAccountName throws an error about it being undefined, which makes sense I think, they won't have an account name if they are only a contact rather than a full user. If this is in fact operating as expected, can you suggest a way for me to list all secondary users and just catch these croaks and carry on. On a related note, and this one is undoubtedly more due to me abusing your class and not knowing an easy way to distinguish a user from a group, but if I try and do a fetch_primary_users or (by inclusion) a users on a user instead of a group, it throws an error about "Use of uninitialized value in concatenation (.) or string at Net/LDAP/Class/Group/AD.pm line 105, <DATA> line 408.". I was able to fix that by changing line 105-110 in Net/LDAP/Class/Group/AD.pm from my @users = $user_class->find( scope => 'sub', filter => "(primaryGroupID=$pgt)", ldap => $self->ldap, base_dn => $self->base_dn, ); to the rather hacky my @users = undef; if (defined($pgt)) { my @users = $user_class->find( scope => 'sub', filter => "(primaryGroupID=$pgt)", ldap => $self->ldap, base_dn => $self->base_dn, ); } Although its just occurred to me that some testing of the existence of sAMAccountName or similar may help to differentiate users from groups and contacts. Thanks, PN

Mon Aug 10 23:01:34 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Mon, 10 Aug 2009 22:01:12 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/9/09 1:03 PM: Show quoted text

> Sun Aug 09 14:03:46 2009: Request 48562 was acted upon. > Transaction: Ticket created by PJNEWMAN > Queue: Net-LDAP-Class > Subject: Possible bug or just user error with contacts in groups > Broken in: 0.18 > Severity: Normal > Owner: Nobody > Requestors: PJNEWMAN@cpan.org > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > > Hi, > > I don't know if this is user error on my part due to not understanding > enough about AD or your classes, a bug/foible due to the potentially > odd way we use AD or something else.

Looks like a combination of both. :) Show quoted text

> > Anyway I'm trying to recursively find all users in a group and any of > its subgroups and I hit a few issues with it failing when calling > fetch_secondary_users with a "can't find user" message, after a bit of > checking of AD, this always seems to occur when there is a contact, as > opposed to a proper user in the group, and its always the contact > referred to in the error message.

I don't use AD myself, or at least, when I have been an AD user it has been as a non-admin, so I'm unfamiliar with the "contact" category. However: Show quoted text

> > Changing line 141 in Net/LDAP/Class/Group/AD.pm from > if ($user) { > to > if (defined($user)) { >

seems like a reasonable change. I've committed that to svn and it'll be in the next (0.19) cpan release. Show quoted text

> Has cured my issue, although doing a straight print of the $user or > $user->sAMAccountName throws an error about it being undefined, which > makes sense I think, they won't have an account name if they are only a > contact rather than a full user. > > If this is in fact operating as expected, can you suggest a way for me > to list all secondary users and just catch these croaks and carry on.

have you tried the standard eval approach? eval { print $user; }; if ($@) { # handle the error } Show quoted text

> > On a related note, and this one is undoubtedly more due to me abusing > your class and not knowing an easy way to distinguish a user from a > group, but if I try and do a fetch_primary_users or (by inclusion) a > users on a user instead of a group, it throws an error about "Use of > uninitialized value in concatenation (.) or string at > Net/LDAP/Class/Group/AD.pm line 105, <DATA> line 408.". I was able to > fix that by changing line 105-110 in Net/LDAP/Class/Group/AD.pm from > my @users = $user_class->find( > scope => 'sub', > filter => "(primaryGroupID=$pgt)", > ldap => $self->ldap, > base_dn => $self->base_dn, > ); > to the rather hacky > my @users = undef; > if (defined($pgt)) { > my @users = $user_class->find( > scope => 'sub', > filter > => "(primaryGroupID=$pgt)", > ldap => $self->ldap, > base_dn => $self->base_dn, > ); > } > > Although its just occurred to me that some testing of the existence of > sAMAccountName or similar may help to differentiate users from groups > and contacts.

if ( ! $user->isa('Net::LDAP::Class::User') ) { die "I thought it was a user, but it wasn't!"; } Does that help? I just convenience functions in the base Net::LDAP::Class like this: sub isa_user { return shift->isa('Net::LDAP::Class::User'); } That'll be in 0.19 too. I can't seem to upload 0.19 to pause at the moment; when I can, I'll close this ticket. Thanks for the report. -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Mon Aug 10 23:01:36 2009 The RT System itself - Status changed from 'new' to 'open'

Mon Aug 17 21:47:11 2009 karman [...] cpan.org - Correspondence added

0.19 available on CPAN.

Mon Aug 17 21:47:13 2009 karman [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Aug 18 07:49:01 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Mon Aug 17 21:47:11 2009, KARMAN wrote: Show quoted text

> 0.19 available on CPAN.

Thanks for that. The contact's are just Outlook style contacts from what I can tell, we just use them to send email to external addresses by adding them into DLists. Hence they obviously don't have an account/login name. I think I'd tried eval, but because it was croaking I didn't ever get the array because it died before it could return it, but I may be misremembering that. Thanks for the isa tip too, that looks very promising. A few more minor issues that get thrown if you try and request a group that doesn't exist, obviously a slightly silly thing to be doing, but it would seem nice if it just returned undef or something rather than throwing errors. Adding the following or an equivalent at line 105: $pgt = "" unless (defined($pgt)); Making 133 onwards: for my $dn (@members) { if (defined($dn)) { my ($cn) = ( $dn =~ m/^cn=([^,]+),/i ); $dn =~ s/$/\\(/g; $dn =~ s/$/\\)/g; my $user = $user_class->new( distinguishedName => $dn, ldap => $self->ldap, base_dn => $self->base_dn, )->read; if (defined $user) { # see https://rt.cpan.org/Ticket/Display.html?id=48562 push( @users, $user ); } else { croak "can't find user $cn ($dn) via LDAP"; } } } More generally, I don't know if you're aware, but LDAP/Microsoft's implementation, places a limit on the number of items you can return, so your code currently fails to retrieve any members for large groups. I've got some code working using Net::LDAP::Control::Paged, but I'm not quite sure where in Net::LDAP::Class that would need to go to even start thinking about writing a patch. I'm guessing its NLC::read, is that correct? PN

Tue Aug 18 07:49:02 2009 The RT System itself - Status changed from 'resolved' to 'open'

Tue Aug 18 09:25:59 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Tue, 18 Aug 2009 08:25:14 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 08/18/2009 06:49 AM: Show quoted text

> Queue: Net-LDAP-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > On Mon Aug 17 21:47:11 2009, KARMAN wrote:

>> 0.19 available on CPAN.

> > Thanks for that. > > The contact's are just Outlook style contacts from what I can tell, we > just use them to send email to external addresses by adding them into > DLists. Hence they obviously don't have an account/login name. > > I think I'd tried eval, but because it was croaking I didn't ever get > the array because it died before it could return it, but I may be > misremembering that. > > Thanks for the isa tip too, that looks very promising. > > A few more minor issues that get thrown if you try and request a group > that doesn't exist, obviously a slightly silly thing to be doing, but > it would seem nice if it just returned undef or something rather than > throwing errors.

hm. read() should return undef if not found. The code below fetches users that LDAP says belong to a group, which seems to not apply to what you're describing? Show quoted text

> > Adding the following or an equivalent at line 105: > $pgt = "" unless (defined($pgt)); > > Making 133 onwards: > > for my $dn (@members) { > if (defined($dn)) { > my ($cn) = ( $dn =~ m/^cn=([^,]+),/i ); > $dn =~ s/$/\\(/g; > $dn =~ s/$/\\)/g; > my $user = $user_class->new( > distinguishedName => > $dn, > ldap => > $self->ldap, > base_dn => > $self->base_dn, > )->read; > if (defined $user) { # see > https://rt.cpan.org/Ticket/Display.html?id=48562 > push( @users, $user ); > } > else { > croak "can't find user > $cn ($dn) via LDAP"; > } > } > } > > More generally, I don't know if you're aware, but LDAP/Microsoft's > implementation, places a limit on the number of items you can return, > so your code currently fails to retrieve any members for large groups. > I've got some code working using Net::LDAP::Control::Paged, but I'm not > quite sure where in Net::LDAP::Class that would need to go to even > start thinking about writing a patch. I'm guessing its NLC::read, is > that correct? >

Yes, the act_on_all() method in the base NLC class uses the Control::Paged feature. Perhaps NLC should be modified to use iterators instead of arrays for related objects. I use something similar for Rose::DB::Object. I'd like to not break the existing functionality though. Maybe add new methods to explicity ask for users_iterator() or groups_iterator() if you are expecting large numbers? What do you think of that idea? -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Tue Aug 18 11:45:20 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Tue Aug 18 06:25:59 2009, peter@peknet.com wrote: Show quoted text

> hm. read() should return undef if not found. The code below fetches > users that LDAP says belong to a group, which seems to not apply to

what Show quoted text

> you're describing?

Well that explains/resolves that. I was just doing a: my $group = MyLDAPGroup->new(cn => $CN, ldap => $ldap); and not trying or testing a read, because it doesn't seem to need one to work, I assume because it reads anyway if its not been read. Hence that function was causing me issues, anyway I've modified my code in respect to that and its behaving fine now. Show quoted text

> > Yes, the act_on_all() method in the base NLC class uses the > Control::Paged feature. > > Perhaps NLC should be modified to use iterators instead of arrays for > related objects. I use something similar for Rose::DB::Object. I'd

like Show quoted text

> to not break the existing functionality though. Maybe add new methods

to Show quoted text

> explicity ask for users_iterator() or groups_iterator() if you are > expecting large numbers? > > What do you think of that idea? >

I've got Control::Paged working on the secondary users as part of NLC::Group::AD, and it doesn't break existing functionality, because if you query a group that's too big at the moment then you don't get any users back, so I've currently got the following working on my system (currently overriding the existing sub as part of my local class): sub fetch_secondary_users { my $self = shift; $self->read; # make sure we have latest ldap_entry for member my @members = $self->member; my $user_class = $self->user_class; my @users; if (@members == 0) { my $page = Net::LDAP::Control::Paged->new( size => 1000 ) or die $!; @args = ( base => $base, filter => "memberOf=" . $self- Show quoted text

>ldap_entry->{attrs}->{distinguishedname}[0],

control => [ $page ], attrs => "member", ); my $cookie; while(1) { # Perform search my $mesg = $self->ldap->search( @args ); foreach my $entry ( $mesg->entries ) { push(@members, $entry->get_value ( "distinguishedname" )); } # Only continue on LDAP_SUCCESS $mesg->code and last; # Get cookie from paged control my($resp) = $mesg->control( LDAP_CONTROL_PAGED ) or last; $cookie = $resp->cookie or last; # Set cookie in paged control $page->cookie($cookie); } if ($cookie) { $page->cookie($cookie); $page->size(0); $self->ldap->search( @args ); } } for my $dn (@members) { my ($cn) = ( $dn =~ m/^cn=([^,]+),/i ); $dn =~ s/$/\\(/g; $dn =~ s/$/\\)/g; my $user = $user_class->new( distinguishedName => $dn, ldap => $self->ldap, base_dn => $self->base_dn, )->read; if (defined $user) { # see https://rt.cpan.org/Ticket/Display.html?id=48562 push( @users, $user ); } else { croak "can't find user $cn ($dn) via LDAP"; } } return wantarray ? @users : \@users; } It does seem rather slow though, 24 seconds for 1600 odd machines in one group, so I wouldn't be surprised if I'm getting all the data on the first pass and then needlessly stuffing it into the array, to keep simple compatibility with your existing code. Then presumably your code pulls all the data down again for each user/machine to actually create the object. I was going to try and look into this, but I'm sure you know it far better than me! :) Basically can you pass a Net::LDAP::Entry into something and use that as the base of a user class? If you do go down the integration route, it might be worth making the page size option a global variable when you initialise everything and pass in the LDAP variable. Regarding my isa issues, this is likely to be user error, but I'm currently doing the following to find out if an item being returned back from a request for "users" is a user or actually a subgroup: $objclass = undef; for ($user->objectClass) { $objclass-> {$_} = 1;} if (defined($objclass->{group})) { #I'm a subgroup } else { #I'm a user or similar } Is there a better way to do this, I'm guessing so, but I'm not sure what it is. Thanks again, apologies in advance for any granny/egg sucking moments and hope a few of my comments/lines of code are of use. PN

Tue Aug 18 12:53:10 2009 PJNEWMAN [...] cpan.org - Correspondence added

Another little one, perhaps its bad Perl on my part, but I've changed the croak in NLC::Group::AD::fetch_secondary_users to a carp, from what I can see you can still catch that with some methods if you want, but we've somehow ended up with someone's name as "\,re" in AD, whcih unsurprisingly causes issues, "can't find user \", presumably because of some escaping and its all getting a bit confusing, anyway that's all fair enough, but I'd still quite like, and it seems quite sensible to return, the rest of the users in the group and a warning, rather than just dieing. Obviously this can be overwritten in my subclass, and is indeed what I'm doing for the moment, but it seems like it might be a worthwhile change in general.

Tue Aug 18 22:15:40 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Tue, 18 Aug 2009 21:14:49 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/18/09 10:45 AM: Show quoted text

> Queue: Net-LDAP-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > On Tue Aug 18 06:25:59 2009, peter@peknet.com wrote:

>> hm. read() should return undef if not found. The code below fetches >> users that LDAP says belong to a group, which seems to not apply to

> what

>> you're describing?

> > Well that explains/resolves that. I was just doing a: > my $group = MyLDAPGroup->new(cn => $CN, ldap => $ldap); > > and not trying or testing a read, because it doesn't seem to need one > to work, I assume because it reads anyway if its not been read. Hence > that function was causing me issues, anyway I've modified my code in > respect to that and its behaving fine now. >

>> Yes, the act_on_all() method in the base NLC class uses the >> Control::Paged feature. >> >> Perhaps NLC should be modified to use iterators instead of arrays for >> related objects. I use something similar for Rose::DB::Object. I'd

> like

>> to not break the existing functionality though. Maybe add new methods

> to

>> explicity ask for users_iterator() or groups_iterator() if you are >> expecting large numbers? >> >> What do you think of that idea? >>

> > I've got Control::Paged working on the secondary users as part of > NLC::Group::AD, and it doesn't break existing functionality, because if > you query a group that's too big at the moment then you don't get any > users back, so I've currently got the following working on my system > (currently overriding the existing sub as part of my local class): > > sub fetch_secondary_users { > my $self = shift; > > $self->read; # make sure we have latest > ldap_entry for member > > my @members = $self->member; > my $user_class = $self->user_class; > my @users; > > if (@members == 0) { > my $page = Net::LDAP::Control::Paged->new( size > => 1000 ) or die $!; > > @args = ( base => $base, > filter => "memberOf=" . $self-

>> ldap_entry->{attrs}->{distinguishedname}[0],

> control => [ $page ], > attrs => "member", > ); > > my $cookie; > while(1) { > # Perform search > my $mesg = $self->ldap->search( @args ); > > foreach my $entry ( $mesg->entries ) { > push(@members, $entry->get_value > ( "distinguishedname" )); > } > > # Only continue on LDAP_SUCCESS > $mesg->code and last; > > # Get cookie from paged control > my($resp) = $mesg->control( > LDAP_CONTROL_PAGED ) or last; > $cookie = $resp->cookie or last; > > # Set cookie in paged control > $page->cookie($cookie); > } > > if ($cookie) { > $page->cookie($cookie); > $page->size(0); > $self->ldap->search( @args ); > } > > } > > for my $dn (@members) { > my ($cn) = ( $dn =~ m/^cn=([^,]+),/i ); > $dn =~ s/$/\\(/g; > $dn =~ s/$/\\)/g; > my $user = $user_class->new( > distinguishedName => > $dn, > ldap => > $self->ldap, > base_dn => > $self->base_dn, > )->read; > if (defined $user) { # see > https://rt.cpan.org/Ticket/Display.html?id=48562 > push( @users, $user ); > } > else { > croak "can't find user > $cn ($dn) via LDAP"; > } > } > > return wantarray ? @users : \@users; > } > > It does seem rather slow though, 24 seconds for 1600 odd machines in > one group, so I wouldn't be surprised if I'm getting all the data on > the first pass and then needlessly stuffing it into the array, to keep > simple compatibility with your existing code. Then presumably your code > pulls all the data down again for each user/machine to actually create > the object. I was going to try and look into this, but I'm sure you > know it far better than me! :)

well, if you've got something working for you atm, that's far better than anything I can drum up off the top of my head. :) Show quoted text

> Basically can you pass a > Net::LDAP::Entry into something and use that as the base of a user > class?

well, you can, but not sure if you really want to: my $user = MyClass->new( ldap => $ldap, ldap_entry => $entry ); that saves a subsequent read() call. That's in fact what act_on_all() does. Have a look at that code. The idea of an iterator is that you could do: my $groups = $user->groups_iterator({ size => 1000 }); while (my $group = $groups->next) { # do something with $group } and it is far more efficient for large datasets that stuffing them all in an array at once. I've put that on my (ever-growing) todo list. Show quoted text

> > If you do go down the integration route, it might be worth making the > page size option a global variable when you initialise everything and > pass in the LDAP variable.

probably not a global var, but like act_on_all(), a method param with a sane default. Show quoted text

> > Regarding my isa issues, this is likely to be user error, but I'm > currently doing the following to find out if an item being returned > back from a request for "users" is a user or actually a subgroup: > > $objclass = undef; > for ($user->objectClass) { $objclass-> > {$_} = 1;} > if (defined($objclass->{group})) { > #I'm a subgroup > } else { > #I'm a user or similar > } > > Is there a better way to do this, I'm guessing so, but I'm not sure > what it is.

your code got a little mangled in the mail, but if you can trust your objectClass metadata, it seems ok to me. -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Tue Aug 18 22:18:02 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Tue, 18 Aug 2009 21:17:44 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/18/09 11:53 AM: Show quoted text

> Queue: Net-LDAP-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > Another little one, perhaps its bad Perl on my part, but I've changed > the croak in NLC::Group::AD::fetch_secondary_users to a carp, from what > I can see you can still catch that with some methods if you want, but > we've somehow ended up with someone's name as "\,re" in AD, whcih > unsurprisingly causes issues, "can't find user \", presumably because > of some escaping and its all getting a bit confusing, anyway that's all > fair enough, but I'd still quite like, and it seems quite sensible to > return, the rest of the users in the group and a warning, rather than > just dieing. > > Obviously this can be overwritten in my subclass, and is indeed what > I'm doing for the moment, but it seems like it might be a worthwhile > change in general.

My credo for exception handling comes from something Marvin Humphrey said to me once: "catastrophic failure is a good thing." Or, die early and often. I view carp() as a very good debugging tool, but for production code I croak if something is amiss. A warning to stderr can very easily turn into noise that just gets ignored and that can hide something very wrong, either in the code or (in this case) in the LDAP system itself. If you don't want death, use eval(). That's what it's there for. It's the perl version of try/catch, which is a very useful idiom. -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Sat Aug 22 10:19:35 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Tue Aug 18 22:15:40 2009, peter@peknet.com wrote: Show quoted text

> well, if you've got something working for you atm, that's far better > than > anything I can drum up off the top of my head. :)

In which case feel free to include it, and indeed I'd probably almost urge it. My reasoning being, if you currently search for a big group, the class tells you that it has no members, which is actually incorrect. Adding my code means you do get the correct results every time, albeit it takes a while longer if the group needs paging. I can see the benefits of iterators, although in my case I'm generally just doing dirty data processing, so the speed or memory implications of loading it into an array first aren't too much of an issue. But as I mentioned above, I'd feel getting accurate results out first is a more important bug fix than the enhancement of iterators. Show quoted text

> > Basically can you pass a > > Net::LDAP::Entry into something and use that as the base of a user > > class?

> > well, you can, but not sure if you really want to: > > my $user = MyClass->new( ldap => $ldap, ldap_entry => $entry ); > > that saves a subsequent read() call. That's in fact what act_on_all() > does. Have > a look at that code.

I'm guessing that because it removes the subsequent read call it should hopefully speed it up quite a bit. Currently for my group of 1600 items, its requesting the group twice (to get the full set), then all 1600 items twice, which obviously isn't too efficient. Anyway I'll try and give it a go and see if it makes a noticeable speedup, although given the variable time my script seems to normally take, it may be hard to tell. Show quoted text

> probably not a global var, but like act_on_all(), a method param with > a sane > default.

Fair point, I guess you could pass it into users or fetch_secondary_members. Show quoted text

> your code got a little mangled in the mail, but if you can trust your > objectClass metadata, it seems ok to me. >

It seems to be okay so far. Presumably its this data, or is it something else, but AD somehow knows whether to give something the icon for a group, contact, computer or user etc. On Tue Aug 18 22:18:02 2009, peter@peknet.com wrote: Show quoted text

> I view carp() as a very good debugging tool, but for production code I > croak if > something is amiss. A warning to stderr can very easily turn into > noise that > just gets ignored and that can hide something very wrong, either in > the code or > (in this case) in the LDAP system itself. > > If you don't want death, use eval(). That's what it's there for. It's > the perl > version of try/catch, which is a very useful idiom. >

My issue/concern is I'm looking at using this in combination with NTLM authentication, to grab a list of users in a group who will be authorised, therefore I'd agree that the error in LDAP is probably not a good thing, but returning no results at all is even more awkward. Because the croak is within the for loop, unless I'm mistaken, the eval would indeed catch the error, but I'm still lacking an array of the other users then, whereas if it carped I could catch it and either choose to die or just carry on and print a message. I can't see a way with the current code to catch this error and still get a list of all the users in a group.

Sat Aug 22 11:50:01 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Sat Aug 22 10:19:35 2009, PJNEWMAN wrote: Show quoted text

> I'm guessing that because it removes the subsequent read call it should > hopefully speed it up quite a bit. Currently for my group of 1600 items, > its requesting the group twice (to get the full set), then all 1600 > items twice, which obviously isn't too efficient. Anyway I'll try and > give it a go and see if it makes a noticeable speedup, although given > the variable time my script seems to normally take, it may be hard to

tell. Okay, I've just switched to loading the existing returned entry and it seems to have speeded things up quite nicely, from 1m20-30s down to 20-40s passing the existing LDAP entry in, which I guess makes sense if its having to do a unique query for each member, compared to the more efficient results that are presumably returned when you do a search. New code follows: sub fetch_secondary_users { my $self = shift; $self->read; # make sure we have latest ldap_entry for member my @members = $self->member; my $user_class = $self->user_class; my @users; if (@members > 0) { for my $dn (@members) { my ($cn) = ( $dn =~ m/^cn=([^,]+),/i ); $dn =~ s/$/\\(/g; $dn =~ s/$/\\)/g; my $user = $user_class->new( distinguishedName => $dn, ldap => $self->ldap, base_dn => $self->base_dn, )->read; if (defined $user) { # see https://rt.cpan.org/Ticket/Display.html?id=48562 push( @users, $user ); } else { carp "can't find user $cn ($dn) via LDAP"; } } } else { my $page = Net::LDAP::Control::Paged->new( size => 1000 ) or die $!; @args = ( base => $base, filter => "memberOf=" . $self->ldap_entry->{attrs}->{distinguishedname}[0], control => [ $page ], attrs => "member", ); my $cookie; while(1) { # Perform search my $mesg = $self->ldap->search( @args ); foreach my $entry ( $mesg->entries ) { my $user = $user_class->new( ldap => $self->ldap, ldap_entry => $entry ); if (defined $user) { # see https://rt.cpan.org/Ticket/Display.html?id=48562 push( @users, $user ); } } # Only continue on LDAP_SUCCESS $mesg->code and last; # Get cookie from paged control my($resp) = $mesg->control( LDAP_CONTROL_PAGED ) or last; $cookie = $resp->cookie or last; # Set cookie in paged control $page->cookie($cookie); } if ($cookie) { $page->cookie($cookie); $page->size(0); $self->ldap->search( @args ); } } return wantarray ? @users : \@users; }

Sun Aug 23 02:03:35 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Sun, 23 Aug 2009 01:03:17 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/22/09 9:19 AM: Show quoted text

> Queue: Net-LDAP-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > On Tue Aug 18 22:15:40 2009, peter@peknet.com wrote:

>> well, if you've got something working for you atm, that's far better >> than >> anything I can drum up off the top of my head. :)

> > In which case feel free to include it, and indeed I'd probably almost > urge it. My reasoning being, if you currently search for a big group, > the class tells you that it has no members, which is actually incorrect. > Adding my code means you do get the correct results every time, albeit > it takes a while longer if the group needs paging. > > I can see the benefits of iterators, although in my case I'm generally > just doing dirty data processing, so the speed or memory implications of > loading it into an array first aren't too much of an issue. But as I > mentioned above, I'd feel getting accurate results out first is a more > important bug fix than the enhancement of iterators.

The Iterator technique is going to be a better overall solution because it uses the cleaner filter query (checking for memberOf rather than trying to parse out the dn). I have checked a first pass at this feature into svn here: https://svn.msi.umn.edu/sw/perl/Net-LDAP-Class/trunk It implements some of your code in a more generalized way, offering users_iterator(), primary_users_iterator() and secondary_users_iterator() methods on AD::Group. I still need to add similar methods for POSIX::Group and both User classes. But you'll be able to see the technique at work right now for your case. Please check it out and see if it works for you. It should be pretty fast, since it injects the ldap_entry directly just as your latest optimization does. If you try and run the tests on the svn trunk, you'll need the latest version of the Net::LDAP::Server::Test package, also at: https://svn.msi.umn.edu/sw/perl/Net-LDAP-Server-Test/trunk which now has support for the Net::LDAP::Control::Paged feature. Show quoted text

>> If you don't want death, use eval(). That's what it's there for. It's >> the perl >> version of try/catch, which is a very useful idiom. >>

> > My issue/concern is I'm looking at using this in combination with NTLM > authentication, to grab a list of users in a group who will be > authorised, therefore I'd agree that the error in LDAP is probably not a > good thing, but returning no results at all is even more awkward. > > Because the croak is within the for loop, unless I'm mistaken, the eval > would indeed catch the error, but I'm still lacking an array of the > other users then, whereas if it carped I could catch it and either > choose to die or just carry on and print a message. I can't see a way > with the current code to catch this error and still get a list of all > the users in a group.

I see your point here. The new Iterator technique avoids this issue altogether, since it's not trying to reconcile possible differences between what $group reports as its members and what LDAP reports for each of those members. The bug really lies in the brittle-ness of trying to parse the $dn value with a regex. The Iterator avoids that issue altogether since it uses a different attribute to filter on. -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Sun Aug 23 13:42:36 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Sun Aug 23 02:03:35 2009, peter@peknet.com wrote: Show quoted text

> The Iterator technique is going to be a better overall solution > because it uses > the cleaner filter query (checking for memberOf rather than trying to > parse out > the dn). > > I have checked a first pass at this feature into svn here: > > https://svn.msi.umn.edu/sw/perl/Net-LDAP-Class/trunk > > It implements some of your code in a more generalized way, offering > users_iterator(), primary_users_iterator() and > secondary_users_iterator() > methods on AD::Group. I still need to add similar methods for > POSIX::Group and > both User classes. But you'll be able to see the technique at work > right now for > your case. > > Please check it out and see if it works for you. It should be pretty > fast, since > it injects the ldap_entry directly just as your latest optimization > does. > > If you try and run the tests on the svn trunk, you'll need the latest > version of > the Net::LDAP::Server::Test package, also at: > > https://svn.msi.umn.edu/sw/perl/Net-LDAP-Server-Test/trunk > > which now has support for the Net::LDAP::Control::Paged feature. >

Thanks for doing this. Unfortunately I can't get it to play ball, which I think I've tracked down to line 121 in NLC::Iterator. If I leave it as is "if ( !$cookie ) {" the iterator only ever returns undef, and hence no results, if I change it to "if (!defined($cookie)) {" I get the results, but $self->{_exhausted} never seems to get set, so I just keep getting the results forever. If it helps, I think I ran your new module through all the tests in the end, and I think it passed them all. Show quoted text

> I see your point here. The new Iterator technique avoids this issue > altogether, > since it's not trying to reconcile possible differences between what > $group > reports as its members and what LDAP reports for each of those > members. The bug > really lies in the brittle-ness of trying to parse the $dn value with > a regex. > The Iterator avoids that issue altogether since it uses a different > attribute to > filter on. >

The Iterator method sounds great in that respect, when its working. Interestingly my new code (that uses the returned LDAP entry from the search) actually seemed to be slower when working on groups smaller than the 1000 limit, I haven't looked into it properly yet, but I wonder if its still unneccesarily processing it twice or something, or again it might just be the variability in our AD infrastructure. Incidentally, its my understanding that the AD administrators can set the return limit, but I believe 1000 is the default/most common, rather than the 500 currently in your code. Although I guess that may be in there for performance reasons.

Mon Aug 24 00:21:32 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Sun, 23 Aug 2009 23:21:12 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/23/09 12:42 PM: Show quoted text

> Thanks for doing this. Unfortunately I can't get it to play ball, which > I think I've tracked down to line 121 in NLC::Iterator. > > If I leave it as is "if ( !$cookie ) {" the iterator only ever returns > undef, and hence no results, if I change it to "if (!defined($cookie)) > {" I get the results, but $self->{_exhausted} never seems to get set, > so I just keep getting the results forever. >

Thanks for testing. I've refactored a bit to make the cookie handling a little cleaner internally. You'll want to svn up on both NLC and NLST. See if the issue remains? I added a DESTROY() in the Iterator class to double check that is_exhausted() is true and call finish() if it isn't. That check is raising the warning against my AD server, though I'm still not sure why the Iterator isn't returning all the entries. Just an FYI in case you see that warning too. Show quoted text

> If it helps, I think I ran your new module through all the tests in the > end, and I think it passed them all. >

that's good. thanks. Show quoted text

>> I see your point here. The new Iterator technique avoids this issue >> altogether, >> since it's not trying to reconcile possible differences between what >> $group >> reports as its members and what LDAP reports for each of those >> members. The bug >> really lies in the brittle-ness of trying to parse the $dn value with >> a regex. >> The Iterator avoids that issue altogether since it uses a different >> attribute to >> filter on. >>

> > The Iterator method sounds great in that respect, when its working. > > Interestingly my new code (that uses the returned LDAP entry from the > search) actually seemed to be slower when working on groups smaller > than the 1000 limit, I haven't looked into it properly yet, but I > wonder if its still unneccesarily processing it twice or something, or > again it might just be the variability in our AD infrastructure. > > Incidentally, its my understanding that the AD administrators can set > the return limit, but I believe 1000 is the default/most common, rather > than the 500 currently in your code. Although I guess that may be in > there for performance reasons.

I made it smaller than the default just for testing the feature. No reason it couldn't be bumped up to 1000 I suppose. -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Mon Aug 24 00:59:18 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Sun, 23 Aug 2009 23:59:01 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

peter@peknet.com via RT wrote on 8/23/09 11:21 PM: Show quoted text

> I added a DESTROY() in the Iterator class to double check that is_exhausted() is > true and call finish() if it isn't. That check is raising the warning against my > AD server, though I'm still not sure why the Iterator isn't returning all the > entries. Just an FYI in case you see that warning too. >

Fixed in r568: https://trac.msi.umn.edu/trac/sw/changeset/568 -- Peter Karman . http://peknet.com/ . peter@peknet.com gpg key: 37D2 DAA6 3A13 D415 4295 3A69 448F E556 374A 34D9

Wed Aug 26 15:41:21 2009 PJNEWMAN [...] cpan.org - Correspondence added

On Mon Aug 24 00:21:32 2009, peter@peknet.com wrote: Show quoted text

> Thanks for testing.

That seems to be working fine now thanks, it actually returns more results than my code! Although I'm not sure if that's a bug in my code or in the LDAP results parsing stuff you mentioned before. I'd still suggest including my code or something similar in get_secondary_users, or if not a warning in the documentation, given its currently effectively broken for large groups. Obviously I'm biased though. :) The iterator seems to match the execution time of the other method anyway, again within the variations of our AD setup. I'll just await this being rolled out into the live version then and I'll promise not to reopen this ticket!

Wed Aug 26 23:41:04 2009 peter [...] peknet.com - Correspondence added

Subject:	Re: [rt.cpan.org #48562] Possible bug or just user error with contacts in groups
Date:	Wed, 26 Aug 2009 22:40:44 -0500
To:	bug-Net-LDAP-Class [...] rt.cpan.org
From:	Peter Karman <peter [...] peknet.com>

Peter Newman via RT wrote on 8/26/09 2:41 PM: Show quoted text

> Queue: Net-LDAP-Class > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=48562 > > > On Mon Aug 24 00:21:32 2009, peter@peknet.com wrote:

>> Thanks for testing.

> That seems to be working fine now thanks, it actually returns more > results than my code! Although I'm not sure if that's a bug in my code > or in the LDAP results parsing stuff you mentioned before.

thanks for testing. interesting that it returns more results? are any of them duplicates? Show quoted text

> > I'd still suggest including my code or something similar in > get_secondary_users, or if not a warning in the documentation, given > its currently effectively broken for large groups. Obviously I'm biased > though. :)

I've added lots of docs/warnings about using iterators instead of arrays and when you might choose either. I also changed the fetch_secondary_users() code to just wrap around secondary_users_iterator(), so your particular bug should not manifest. Show quoted text

> > The iterator seems to match the execution time of the other method > anyway, again within the variations of our AD setup. > > I'll just await this being rolled out into the live version then and > I'll promise not to reopen this ticket!

0.21 uploaded to pause! thanks for your persistence on this. -- Peter Karman . http://peknet.com/ . peter@peknet.com

Wed Aug 26 23:42:00 2009 karman [...] cpan.org - Correspondence added

0.21 should fix these bugs.

Wed Aug 26 23:42:02 2009 karman [...] cpan.org - Status changed from 'open' to 'resolved'

Thu Aug 27 11:52:00 2009 PJNEWMAN [...] cpan.org - Correspondence added

Okay I said I wouldn't reopen it, but I wonder if my reply will automatically reopen it, anyway, some answers. On Wed Aug 26 23:41:04 2009, peter@peknet.com wrote: Show quoted text

> > thanks for testing. > > interesting that it returns more results? are any of them duplicates?

Not duplicates in the bad sense, they were repeated multiple times, but only because of our AD tree and the fact the main group I'm testing is a major parent of many others, although their members didn't appear at all before. There is one group with an ampersand, although others with ampersands got through fine before, although I note its pre 2000 name is the same as its normal name and particularly long, so that might be why its failing. Also one group where there is a user with the same name and slightly different capitalisation, although the pre 2000 names don't match. Anyway that's just as likely to be a bug in my code as the main Net::LDAP code or your old code. Show quoted text

> I've added lots of docs/warnings about using iterators instead of > arrays and > when you might choose either. I also changed the > fetch_secondary_users() code to > just wrap around secondary_users_iterator(), so your particular bug > should not > manifest.

Excellent, that sounds like the most sensible resolution to the problem. Show quoted text

> thanks for your persistence on this.

Thanks for yours, you're the one who's done all the hard work!

Thu Aug 27 11:52:01 2009 The RT System itself - Status changed from 'resolved' to 'open'

Thu Aug 27 11:55:09 2009 karman [...] cpan.org - Status changed from 'open' to 'resolved'