Bug #68449 for RT-Authen-ExternalAuth: fix for enable user and LDAP update user info warnings

Tue May 24 15:24:13 2011 adaugherity [...] tamu.edu - Ticket created

Subject:	fix for enable user and LDAP update user info warnings
Date:	Tue, 24 May 2011 19:24:00 +0000
To:	"bug-RT-Authen-ExternalAuth [...] rt.cpan.org" <bug-RT-Authen-ExternalAuth [...] rt.cpan.org>
From:	"Daugherity, Andrew W" <adaugherity [...] tamu.edu>

First of all, thanks for this module, it's much nicer than the old User_Local.pm LDAP auth we had been using. I have fixed a couple bugs that generated warnings every time a user logged in. With RT 3.8 only the "Couldn't enable user" warning is logged, but RT 4.0 apparently uses a stricter warning level and also logs several "Use of uninitialized value" warnings. Here is what is logged every time a user logs in (RT 4.0.0, RT::Authen::ExternalAuth 0.09, using an eDirectory LDAP server): ==== [Thu May 19 22:01:43 2011] [info]: RT::Authen::ExternalAuth::LDAP::GetAuth External Auth OK ( eDir_LDAP ): adaugherity (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth/LDAP.pm:139) [Thu May 19 22:01:43 2011] [warning]: Couldn't enable user 93751 (/opt/rt4/sbin/../lib/RT/User.pm:1065) [Thu May 19 22:01:43 2011] [warning]: Use of uninitialized value $val in concatenation (.) or string at /opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm line 274. (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm:274) [Thu May 19 22:01:43 2011] [warning]: Use of uninitialized value $message in concatenation (.) or string at /opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm line 274. (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm:274) [Thu May 19 22:01:43 2011] [info]: User marked as ENABLED ( ADaugherity ) per External Service (, ) (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm:274) [Thu May 19 22:01:43 2011] [warning]: Use of uninitialized value in string eq at /opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth/LDAP.pm line 236. (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth/LDAP.pm:236) [Thu May 19 22:01:43 2011] [warning]: Use of uninitialized value in string eq at /opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth/LDAP.pm line 236. (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth/LDAP.pm:236) < a lot more "uninitialized value in string eq" errors, apparently one for each LDAP attribute > [Thu May 19 22:01:43 2011] [info]: RT::Authen::ExternalAuth::CanonicalizeUserInfo returning Address1: [$address], City: College Station, Country: US, EmailAddress: [$email], ExternalAuthId: ADaugherity, Gecos: [$gecos], Name: ADaugherity, Organization: Systems, RealName: Andrew Daugherity, State: TX, WorkPhone: [$phone], Zip: 77843 (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm:536) < previous line sanitized a bit; actual values are properly returned > [Thu May 19 22:01:43 2011] [info]: Successful login for ADaugherity from 10.95.0.228 (/opt/rt4/local/plugins/RT-Authen-ExternalAuth/lib/RT/Authen/ExternalAuth.pm:219) ==== There are two bugs here: 1) Calling RT::User::SetDisabled() with the current disabled state (e.g. attempting to enable an already-enabled user) fails, returning undef. This means that my ($val, $message) = $UserObj->SetDisabled(0); results in $val being undef and the subsequent $RT::Logger call using $val and $message generating the "Use of uninitialized value" warning. FYI, RT::User::SetDisabled in turn calls RT::Principal::SetDisabled(), which returns (0, "That is already the current value") in this case. 2) LDAP::CanonicalizeUserInfo() is testing $RT::LdapAttrMap->{$key}, but $RT::LdapAttrMap is not defined anywhere (in fact, this is the only occurrence of that name anywhere). A simple one-line patch fixes that: foreach my $key (keys(%{$config->{'attr_map'}})) { - if ($RT::LdapAttrMap->{$key} eq 'dn') { + if ($config->{'attr_map'}->{$key} eq 'dn') { I wasn't using dn for anything, but I tested assigning it to FreeformContactInfo in the attr_map and it works now. Also, no more uninitialized value errors every time through the loop! A patch for both issues is attached. I also fixed some nearby typos (s/principle/principal/). Thanks, Andrew Daugherity

Message body is not shown because sender requested not to inline it.

Tue May 24 17:13:44 2011 ruz [...] bestpractical.com - Correspondence added

Subject:	Re: [rt.cpan.org #68449] fix for enable user and LDAP update user info warnings
Date:	Wed, 25 May 2011 01:13:37 +0400
To:	bug-RT-Authen-ExternalAuth [...] rt.cpan.org
From:	Ruslan Zakirov <ruz [...] bestpractical.com>

On Tue, May 24, 2011 at 11:24 PM, Daugherity, Andrew W via RT <bug-RT-Authen-ExternalAuth@rt.cpan.org> wrote: [snip] Show quoted text

> 1) Calling RT::User::SetDisabled() with the current disabled state (e.g. attempting to enable an already-enabled user) fails, returning undef.

Applied slightly modified patch and pushed into the repo as dont-disable-disabled-user branch. Please test this branch. Show quoted text

> 2) LDAP::CanonicalizeUserInfo() is testing $RT::LdapAttrMap->{$key}, but $RT::LdapAttrMap is not defined anywhere (in fact, this is the only occurrence of that name anywhere).

Noticed this during development of multiple-emails branch. It's in testing and hopefuly would be part of the next release. Show quoted text

> Thanks, > Andrew Daugherity

-- Best regards, Ruslan.

Tue May 24 17:13:46 2011 The RT System itself - Status changed from 'new' to 'open'

Thu May 26 17:06:02 2011 adaugherity [...] tamu.edu - Correspondence added

Subject:	Re: [rt.cpan.org #68449] fix for enable user and LDAP update user info warnings
Date:	Thu, 26 May 2011 21:05:52 +0000
To:	"<bug-RT-Authen-ExternalAuth [...] rt.cpan.org>" <bug-RT-Authen-ExternalAuth [...] rt.cpan.org>
From:	"Daugherity, Andrew W" <adaugherity [...] tamu.edu>

On May 24, 2011, at 4:13 PM, Ruslan Zakirov via RT wrote: Show quoted text

>> 1) Calling RT::User::SetDisabled() with the current disabled state (e.g. attempting to enable an already-enabled user) fails, returning undef.

> > Applied slightly modified patch and pushed into the repo as > dont-disable-disabled-user branch. Please test this branch.

Tested, and it works the same as my patch. I tested all four possibilities: action = { disable, enable } x current status = { disabled, enabled } and it works properly. However, I don't think the additional check/error log you added will work as intended: my ($val, $message) = $UserObj->SetDisabled(0); unless ( $val ) { $RT::Logger->error("Failed to enable user ($username) per External Service: $message"); return ($updated, "Failed to enable"); } If $UserObj->SetDisabled(0) fails, it returns undef, but not any message. $message is also going to be undef, and the $RT::Logger call is going to generate a "use of uninitialized value" warning. The only way to get the actual message is to directly call $UserObj->PrincipalObj->SetDisabled, but that may not be advisable since User::SetDisabled does a HasRight check first before calling Principal::SetDisabled. This seems like a deficiency in the API -- I think it would be preferable for User::SetDisabled to pass through the error message from Principal::SetDisabled. It also seems inconsistent that if the HasRight check fails, it returns (0, 'Permission Denied'), but returns (undef) if PrincipalObj->SetDisabled fails. But, I definitely don't know the code well enough to be demanding API changes. Probably best to just remove $message from the Logger call. Also, your patch doesn't include the spelling fixes (principal). It's just comments but it's annoying to read. :-) Thanks, Andrew Daugherity Systems Analyst Division of Research, Texas A&M University

Tue Jun 28 13:52:41 2011 adaugherity [...] tamu.edu - Correspondence added

Subject:	Re: [rt.cpan.org #68449] fix for enable user and LDAP update user info warnings
Date:	Mon, 27 Jun 2011 18:21:28 +0000
To:	"<bug-RT-Authen-ExternalAuth [...] rt.cpan.org>" <bug-RT-Authen-ExternalAuth [...] rt.cpan.org>
From:	"Daugherity, Andrew W" <adaugherity [...] tamu.edu>

One more patch; this fixes two "use of uninitialized value" warnings when a new user is created. When AutoCreateNonExternalUsers is set and a new user mails in a ticket, the user is created with no password. This later gets changed to '*NO-PASSWORD*' by RT but it's still null while in ExternalAuth, and causes the Logger calls to generate this warning. There may be a better way to do this but using the ternary operator works for me. It also might be preferable to make the undefined case log something other than the empty string, e.g. 'undef', 'NULL', '<not set>', etc., but I'll leave that up to you. (Patch is against the dont-disable-disabled-user branch.) Thanks, Andrew Daugherity Systems Analyst Division of Research, Texas A&M University

Message body is not shown because sender requested not to inline it.

Mon Sep 19 14:30:42 2011 cpan [...] jibsheet.com - Correspondence added

This is now merged into master

Mon Sep 19 14:30:43 2011 cpan [...] jibsheet.com - Status changed from 'open' to 'resolved'

Mon Sep 19 19:44:26 2011 adaugherity [...] tamu.edu - Correspondence added

Subject:	Re: [rt.cpan.org #68449] fix for enable user and LDAP update user info warnings
Date:	Mon, 19 Sep 2011 23:44:16 +0000
To:	"<bug-RT-Authen-ExternalAuth [...] rt.cpan.org>" <bug-RT-Authen-ExternalAuth [...] rt.cpan.org>
From:	"Daugherity, Andrew W" <adaugherity [...] tamu.edu>

On Sep 19, 2011, at 1:30 PM, Kevin Falcone via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=68449 > > > This is now merged into master

Excellent! (I presume the plan is to eventually merge the multiple-emails branch, which obsoleted part of my patch, as well?) Checking github, I see my second patch (create-user-silence-warn.diff) was never merged into the (now-defunct) dont-disable-disabled-user branch, let alone master. Would it be possible to merge that as well? Thanks, Andrew

Mon Sep 19 19:44:27 2011 The RT System itself - Status changed from 'resolved' to 'open'

Mon Sep 19 21:22:27 2011 cpan [...] jibsheet.com - Correspondence added

Subject:	Re: [rt.cpan.org #68449] fix for enable user and LDAP update user info warnings
Date:	Mon, 19 Sep 2011 21:22:12 -0400
To:	"Daugherity, Andrew W via RT" <bug-RT-Authen-ExternalAuth [...] rt.cpan.org>
From:	Kevin Falcone <cpan [...] jibsheet.com>

On Mon, Sep 19, 2011 at 07:44:27PM -0400, Daugherity, Andrew W via RT wrote: Show quoted text

> Queue: RT-Authen-ExternalAuth > Excellent! (I presume the plan is to eventually merge the multiple-emails branch, which obsoleted part of my patch, as well?)

Yep, I'm sure we'll get some conflicts, but the warnings really needed to die. Show quoted text

> Checking github, I see my second patch (create-user-silence-warn.diff) > was never merged into the (now-defunct) dont-disable-disabled-user > branch, let alone master. Would it be possible to merge that as well?

It's actually been merged locally, I just forgot to push up. -kevin

Thu Jan 26 16:00:45 2012 tsibley [...] cpan.org - Status changed from 'open' to 'resolved'