Bug #51258 for RT-Extension-LDAPImport: Collected Patches

Hi, Just to let you know, I've successfully tested patches 01, 02 and 03 with RT 3.8.7. Here is a new version of the 03-multiple-ldap-sources.patch with two small improvements : * Added "$importer->_group(undef);" at the beginning of the while loop added by the multiple-ldap-sources.patch, so that each "site" can define a different group to import into. * Slight adjustments to the help text. Thanks, -- Jérôme Charaoui Service informatique - Collège de Maisonneuve

Enable rtldapimport to pull from more than one ldapsource. This is From: <> accomplished by specifying files that are included at runtime by require. File contents and format are exactly what is already being put in RT_SiteConfig.pm. To avoid the messy to type and maintain "hash of hashes" mess, and also to minimize the changes throughout the code, this methodology is "one site per file". Some new config variables are defined (site names) to make clear what ldapsource the output is related to. Arguably, rtldapimport's configs shouldn't be in RT_SiteConfig.pm at all since a production RT install doesn't use them - only this tool, so the option to ignore RT_SiteConfig.pm is given. Otherwise, it is the "default site". --- bin/rtldapimport | 48 ++++++++++++++++++++++++++++++++-------- lib/RT/Extension/LDAPImport.pm | 3 +++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/bin/rtldapimport b/bin/rtldapimport index 6148fb5..09900d2 100644 --- a/bin/rtldapimport +++ b/bin/rtldapimport @@ -11,13 +11,19 @@ RT::Init; use RT::Extension::LDAPImport; -my ($debug, $help, $import); +my ($debug, $help, $import, @ldap_files, $nositeconfig ); use Getopt::Long; -GetOptions ( "debug" => \$debug, "import" => \$import, "help" => \$help ); +GetOptions ( "debug" => \$debug, "import" => \$import, "ldapdir=s" => \@ldap_files,"nositeconfig" => \$nositeconfig, "help" => \$help ); if ($help) { print "$0: [--debug] [--import] [--ldapdirs=file] [--help]\n"; print " --help This usage statement.\n"; print " --import import the identities found in ldap.\n"; + print " --ldapdir file Specify file containing config directives describing\n"; + print " an LDAP directory. Useful when you have multiple\n"; + print " sources of identities feeding one RT instance.\n"; + print " --nositeconfig If set, no imports will be performed with default values in\n"; + print " RT_SiteConfig.pm. Only settings from files specified\n"; + print " with --ldapdirs will be consulted.\n"; print " --debug Enable debugging.\n"; exit 0; } @@ -26,13 +32,36 @@ if ($help) { my $importer = RT::Extension::LDAPImport->new; $importer->screendebug(1) if $debug; -if ($import) { - print "Importing... \n"; - $importer->import_users; - print "Finished importing. \n"; -} else { - print "Searching... \n"; - $importer->show_users; - print "Finished searching. \n"; +# Do search/import with values defined in RT_SiteConfig.pm +if (! $nositeconfig) { + if ($import) { + print "Importing from RT_SiteConfig.pm... \n"; + $importer->import_users; + print "Finished importing. \n"; + } else { + print "Searching from RT_SiteConfig.pm \n"; + $importer->show_users; + print "Finished searching. \n"; + } } +# Perform searches with values defined in config files +# passed in via command line. +while (@ldap_files > 0) { + require $ldap_files[0]; + shift @ldap_files; + $importer->_group(undef); + + if ($import) { + print "Importing from $RT::LDAPSite\n"; + $importer->import_users; + print "Finished importing from $RT::LDAPSite\n"; + } else { + print "Searching site $RT::LDAPSite\n"; + my $results = $importer->show_users; + print "Finished searching $RT::LDAPSite\n"; + } + +} + + diff --git a/lib/RT/Extension/LDAPImport.pm b/lib/RT/Extension/LDAPImport.pm index 4d80914..009cde0 100644 --- a/lib/RT/Extension/LDAPImport.pm +++ b/lib/RT/Extension/LDAPImport.pm @@ -471,6 +471,9 @@ sub disconnect_ldap { $ldap->unbind; $ldap->disconnect; + # This needs to be cleared or import_users will try to + # reuse a connection that's been torn down. + $self->_ldap(''); return; }

Thank you! I believe after I submitted these patches I found a minor thing or two. I'll incorporate your changes and review to see what else I should have included. I think I missed setting a variable to clear the connection object after it had been torn down in one spot. Might take me a few days to circle back to this based on my meeting schedule. -Rob 2010/3/1 Jérôme Charaoui via RT <bug-RT-Extension-LDAPImport@rt.cpan.org> Show quoted text

On Wed, Jul 14, 2010 at 11:11 AM, Kevin Falcone via RT < bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
I'm confused about what exactly you mean by that. I think my confusion stems from the following line of thought, which probably has a false assumption. This behavior is not configurable now. So to make it configurable a config option needs to be created, which is by definition not documented because it doesn't exist yet? Show quoted text
Well - that was my hack for passing a flag from a command line into a plug-in, but what about passing an argument to a function being invoked inside the plug-in? (I'm just spit-balling here.) Really - while I have a strong opinion on style (and command line options) I can roll with whatever works. Show quoted text
Um... hmm. Intriguing thought about the plugin etc dirs. Those supersede the defaults automatically, right? And I think I can articulate the difference in our perceptions here too now. You: the tool is meant to be a very simple wrapper around the RT infrastructure, which leverages the RT config system. Me: Webserver + RT infrastructure + RT Configs = web service. rtldapimport tool = identity provisioning system that enables me to source identity for the web service from ldap. It uses a running RT instance incidentally for convenience (really to manipulate users in the database). But it is not my web service, and shouldn't require me to alter files related to the configuration of my web service. Unfortunately, even if we were to support multiple file solution you Show quoted text
I'm guessing that's a fail on me then. Sorry about that. :( Thanks again for your time, Kevin. I suspect I'm coming off as being difficult and mildly irksome, for which I apologize. -Rob

On Wed, Jul 14, 2010 at 11:49 AM, Kevin Falcone via RT < bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
Okay - I think I see what you're saying. Were you intending to do that or would you like me to try to come up with something? -Rob

On Wed Jul 14 13:11:58 2010, kevin@jibsheet.com wrote: Show quoted text
that or Show quoted text
+1 for the update users setting the privileged flag, rather than just on create (only tested 0.31, but I don't see a change in 0.31_03). I am using a patched version as per attachment.

diff --git a/README b/README index 7079aee..b7c96d6 100644 --- a/README +++ b/README @@ -56,7 +56,8 @@ The LDAP attribute can also be a subroutine reference that returns either an arrayref or a list of attributes By default users are created as Unprivileged, but you can change this by -setting $LDAPCreatePrivileged to 1. +setting $LDAPCreatePrivileged to 1. If the privilege status for users is +required to be updated then also set $LDAPUpdateSetsPrivileged to 1. For more information on these see the import_users documentation in RT::Extension::LDAPImport diff --git a/lib/RT/Extension/LDAPImport.pm b/lib/RT/Extension/LDAPImport.pm index c6022d7..938456f 100644 --- a/lib/RT/Extension/LDAPImport.pm +++ b/lib/RT/Extension/LDAPImport.pm @@ -146,7 +146,8 @@ If it is an arrayref, the values will be concatenated together with a single space. By default users are created as Unprivileged, but you can change this by -setting $LDAPCreatePrivileged to 1. +setting $LDAPCreatePrivileged to 1. If the privilege status for users is +required to be updated then also set $LDAPUpdateSetsPrivileged to 1. =cut @@ -350,6 +351,7 @@ sub create_rt_user { if ($RT::LDAPUpdateUsers || $RT::LDAPUpdateOnly) { $self->_debug("$message, updating their data"); if ($args{import}) { + $user->{'Privileged'} = $RT::LDAPCreatePrivileged if $RT::LDAPUpdateSetsPrivileged; my @results = $user_obj->Update( ARGSRef => $user, AttributesRef => [keys %$user] ); $self->_debug(join("\n",@results)||'no change'); } else {