Skip Menu |
 

This queue is for tickets about the RT-Extension-LDAPImport CPAN distribution.

Report information
The Basics
Id: 51258
Status: open
Priority: 0/
Queue: RT-Extension-LDAPImport

People
Owner: Nobody in particular
Requestors: kiwiroy [...] gmail.com
nesius [...] gmail.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Fwd: Undelivered Mail Returned to Sender
Date: Mon, 9 Nov 2009 22:18:43 -0600
To: bug-rt-extension-ldapimport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download series
application/octet-stream 185b

Message body not shown because it is not plain text.

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

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

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

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

Download (untitled) / with headers
text/plain 2.3k
Sending to the right place this time (I hope). I'm guessing the original email address from Jesse had an extra dash in it. -Rob Show quoted text
---------- Forwarded message ---------- From: Mail Delivery System <MAILER-DAEMON@bestpractical.com> Date: Mon, Nov 9, 2009 at 5:49 PM Subject: Undelivered Mail Returned to Sender To: nesius@gmail.com This is the mail system at host diesel.bestpractical.com. I'm sorry to have to inform you that your message could not be delivered to one or more recipients. It's attached below. For further assistance, please send mail to postmaster. If you do so, please include this problem report. You can delete your own text from the attached returned message. The mail system <cpan-bug+rt-extension-ldap-import@diesel.bestpractical.com<cpan-bug%2Brt-extension-ldap-import@diesel.bestpractical.com>> (expanded from <bug-rt-extension-ldap-import@rt.cpan.org>): temporary failure. Command output: bayes: cannot write to /opt/filter//.spamassassin/bayes_journal, bayes db update ignored: Permission denied RT server error. The RT server which handled your email did not behave as expected. It said: temporary failure - RT couldn't find the queue: rt-extension-ldap-import Final-Recipient: rfc822; cpan-bug+rt-extension-ldap-import@diesel.bestpractical.com<cpan-bug%2Brt-extension-ldap-import@diesel.bestpractical.com> Original-Recipient: rfc822;bug-rt-extension-ldap-import@rt.cpan.org<rfc822%3Bbug-rt-extension-ldap-import@rt.cpan.org> Action: failed Status: 4.3.0 Diagnostic-Code: x-unix; bayes: cannot write to /opt/filter//.spamassassin/bayes_journal, bayes db update ignored: Permission denied RT server error. The RT server which handled your email did not behave as expected. It said: temporary failure - RT couldn't find the queue: rt-extension-ldap-import
---------- Forwarded message ---------- From: Robert Nesius <nesius@gmail.com> To: bug-rt-extension-ldap-import@rt.cpan.org Date: Wed, 4 Nov 2009 17:05:09 -0600 Subject: Patches for rt-extension-ldap-import Enclosed. -Rob --------------------- These look fairly reasonable. Kevin is nose-down on client work, but asked me to thank you for them. If you can resend the same message to bug-rt-extension-ldap-import@rt.cpan.org, that will get them properly tracked so he can't escape them when he comes up for air. Best, Jesse
Subject: [rt.cpan.org #51258] Minor adjustments
Date: Mon, 1 Mar 2010 13:47:10 -0500
To: <bug-rt-extension-ldapimport [...] rt.cpan.org>
From: Jérôme Charaoui <jcharaoui [...] cmaisonneuve.qc.ca>
Download (untitled) / with headers
text/plain 479b
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; }
Subject: Re: [rt.cpan.org #51258] Minor adjustments
Date: Mon, 1 Mar 2010 16:36:51 -0600
To: bug-RT-Extension-LDAPImport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download (untitled) / with headers
text/plain 1016b
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
> <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > 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 > >
RT-Send-CC: jcharaoui [...] cmaisonneuve.qc.ca
Download (untitled) / with headers
text/plain 1.1k
Hi Robert Just to run down what did and didn't make it into the repo: There is now a usage statement By default, it runs in debugging mode rather than importing I fixed your 'undef values from ldap' in a cleaner way - d545deed9e59fcd9acd6dfb49f082fee744d9f27 I won't be folding in the ldapfiles part. For now, that can be done with RT_SITE_CONFIG=/tmp/file rtldapimport and a config system that better resembles RT-Authen-ExternalAuth and the array of hashes of configs is the way to go There is now a config to only update existing users, I don't believe that you patched that in, but looking through your code made me think about it I've skipped the flag for 'update users' from the command line since there is already a config for that Making all users privileged isn't something most people seem to want, generally only 10% of the imported users are privileged. I could see some 'extra attributes' that you could specify in the config coming in for users that fit a certain filter and I feel that would be a lot cleaner than a command line flag + undocumented config option that makes everyone privileged You can have a look at github if you'd like to see the current state. -kevin
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 10:53:56 -0500
To: bug-RT-Extension-LDAPImport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download (untitled) / with headers
text/plain 3.2k
I think that's all good stuff - thanks for spending time with this Kevin. While "most people" seem to only want 10% of users to be privileged, I found the divide between privileged and un-privileged users to just be too jarring. Every time I tried to use the RT model of "most people unprivileged" that would manifest an unpleasant side-effect with respect to how I envisioned people using the system. So I have everyone privileged by default and in a group named "employees", and this tool runs out of cron every night and pulls new users in seamlessly. My operating model may be different than the norm but I think the tool should support it. This next paragraph is a bit subjective, so take it as my opinion and something to consider. I really believe that a command line tool should use the command line for it's interface to control it's behavior. If a file is going to assist, then the command-line should define the file. Using environment variables to define the file - yeah it works. But in 12 years of administrating services in VERY large (world-wide) computing environments, that methodology has proven to be problematic. I also don't like the idea of changing my site config file for RT - the production service - to test and implement a tool. Also - by having the config options for each source in its own file, I can easily test just that source. One time I did have a source that suddenly stopped working. It was very handy to use the tool to only work against that single source without altering my rt site config file, or making a copy of my RT site config and then having to merge changes back into the production config. etc... As for multiple files for multiple sources... an array of hashes is fine in the end. Thanks again for the time you put in on this. I'm happy to see the RT community benefit from these improvements. -Rob On Tue, Jul 13, 2010 at 3:59 PM, Kevin Falcone via RT < bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > Hi Robert > > Just to run down what did and didn't make it into the repo: > > There is now a usage statement > By default, it runs in debugging mode rather than importing > I fixed your 'undef values from ldap' in a cleaner way - > d545deed9e59fcd9acd6dfb49f082fee744d9f27 > > I won't be folding in the ldapfiles part. For now, that can be done with > RT_SITE_CONFIG=/tmp/file rtldapimport > and a config system that better resembles RT-Authen-ExternalAuth and the > array of hashes > of configs is the way to go > > There is now a config to only update existing users, I don't believe that > you patched that in, > but looking through your code made me think about it > > I've skipped the flag for 'update users' from the command line since there > is already a config > for that > > Making all users privileged isn't something most people seem to want, > generally only 10% of > the imported users are privileged. I could see some 'extra attributes' > that you could specify > in the config coming in for users that fit a certain filter and I feel that > would be a lot cleaner > than a command line flag + undocumented config option that makes everyone > privileged > > You can have a look at github if you'd like to see the current state. > > -kevin >
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 12:11:35 -0400
To: Robert Nesius via RT <bug-RT-Extension-LDAPImport [...] rt.cpan.org>
From: Kevin Falcone <kevin [...] jibsheet.com>
Download (untitled) / with headers
text/plain 3.8k
On Wed, Jul 14, 2010 at 11:54:06AM -0400, Robert Nesius via RT wrote: Show quoted text
> While "most people" seem to only want 10% of users to be privileged, I found > the divide between privileged and un-privileged users to just be too > jarring. Every time I tried to use the RT model of "most people > unprivileged" that would manifest an unpleasant side-effect with respect to > how I envisioned people using the system. So I have everyone privileged by > default and in a group named "employees", and this tool runs out of cron > every night and pulls new users in seamlessly. My operating model may be > different than the norm but I think the tool should support it.
As I mentioned, making it configurable makes sense. Making it be a command line argument that sets an undocumented config option does not make sense. Show quoted text
> This next paragraph is a bit subjective, so take it as my opinion and > something to consider. I really believe that a command line tool should use > the command line for it's interface to control it's behavior. If a file is > going to assist, then the command-line should define the file. Using > environment variables to define the file - yeah it works. But in 12 years > of administrating services in VERY large (world-wide) computing > environments, that methodology has proven to be problematic. I also don't > like the idea of changing my site config file for RT - the production > service - to test and implement a tool. Also - by having the config options > for each source in its own file, I can easily test just that source. One > time I did have a source that suddenly stopped working. It was very handy > to use the tool to only work against that single source without altering my > rt site config file, or making a copy of my RT site config and then having > to merge changes back into the production config. etc...
In this case, the tool is meant to be a very simple wrapper around the RT infrastructure, which leverages the RT config system. If you really need a different config file for the tool, you can do that using plugin etc/ dirs. Unfortunately, even if we were to support multiple file solution you proposed, the patch provided couldn't have been included as written -kevin Show quoted text
> As for multiple files for multiple sources... an array of hashes is fine in > the end. Thanks again for the time you put in on this. I'm happy to see > the RT community benefit from these improvements. > > -Rob > > > > > > On Tue, Jul 13, 2010 at 3:59 PM, Kevin Falcone via RT < > bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: >
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > > > Hi Robert > > > > Just to run down what did and didn't make it into the repo: > > > > There is now a usage statement > > By default, it runs in debugging mode rather than importing > > I fixed your 'undef values from ldap' in a cleaner way - > > d545deed9e59fcd9acd6dfb49f082fee744d9f27 > > > > I won't be folding in the ldapfiles part. For now, that can be done with > > RT_SITE_CONFIG=/tmp/file rtldapimport > > and a config system that better resembles RT-Authen-ExternalAuth and the > > array of hashes > > of configs is the way to go > > > > There is now a config to only update existing users, I don't believe that > > you patched that in, > > but looking through your code made me think about it > > > > I've skipped the flag for 'update users' from the command line since there > > is already a config > > for that > > > > Making all users privileged isn't something most people seem to want, > > generally only 10% of > > the imported users are privileged. I could see some 'extra attributes' > > that you could specify > > in the config coming in for users that fit a certain filter and I feel that > > would be a lot cleaner > > than a command line flag + undocumented config option that makes everyone > > privileged > > > > You can have a look at github if you'd like to see the current state. > > > > -kevin > >
>
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 11:46:15 -0500
To: bug-RT-Extension-LDAPImport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download (untitled) / with headers
text/plain 3.9k
On Wed, Jul 14, 2010 at 11:11 AM, Kevin Falcone via RT < bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > On Wed, Jul 14, 2010 at 11:54:06AM -0400, Robert Nesius via RT wrote:
> > While "most people" seem to only want 10% of users to be privileged, I
> found
> > the divide between privileged and un-privileged users to just be too > > jarring. Every time I tried to use the RT model of "most people > > unprivileged" that would manifest an unpleasant side-effect with respect
> to
> > how I envisioned people using the system. So I have everyone privileged
> by
> > default and in a group named "employees", and this tool runs out of cron > > every night and pulls new users in seamlessly. My operating model may be > > different than the norm but I think the tool should support it.
> > As I mentioned, making it configurable makes sense. >
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
> Making it be a command line argument that sets an undocumented config > option does not make sense. >
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
> > This next paragraph is a bit subjective, so take it as my opinion and
> > something to consider. I really believe that a command line tool should
> use
> > the command line for it's interface to control it's behavior. If a file
> is
> > going to assist, then the command-line should define the file. Using > > environment variables to define the file - yeah it works. But in 12
> years
> > of administrating services in VERY large (world-wide) computing > > environments, that methodology has proven to be problematic. I also
> don't
> > like the idea of changing my site config file for RT - the production > > service - to test and implement a tool. Also - by having the config
> options
> > for each source in its own file, I can easily test just that source. One > > time I did have a source that suddenly stopped working. It was very
> handy
> > to use the tool to only work against that single source without altering
> my
> > rt site config file, or making a copy of my RT site config and then
> having
> > to merge changes back into the production config. etc...
> > In this case, the tool is meant to be a very simple wrapper around the > RT infrastructure, which leverages the RT config system. If you > really need a different config file for the tool, you can do that > using plugin etc/ dirs. >
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
> proposed, the patch provided couldn't have been included as written >
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
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 12:49:39 -0400
To: Robert Nesius via RT <bug-RT-Extension-LDAPImport [...] rt.cpan.org>
From: Kevin Falcone <kevin [...] jibsheet.com>
Download (untitled) / with headers
text/plain 540b
On Wed, Jul 14, 2010 at 12:46:25PM -0400, Robert Nesius via RT wrote: Show quoted text
> > As I mentioned, making it configurable makes sense. > >
> > I'm confused about what exactly you mean by that.
Allowing users to indicate that imported users should be Privileged makes sense. Your implementation using an undocumented config variable set from a command line option is not code that I would include. I assume if the config was rewritten to support multiple sources that adding a 'make these users privileged' flag to that would make sense. -kevin
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 12:06:01 -0500
To: bug-RT-Extension-LDAPImport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download (untitled) / with headers
text/plain 859b
On Wed, Jul 14, 2010 at 11:49 AM, Kevin Falcone via RT < bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > On Wed, Jul 14, 2010 at 12:46:25PM -0400, Robert Nesius via RT wrote:
> > > As I mentioned, making it configurable makes sense. > > >
> > > > I'm confused about what exactly you mean by that.
> > Allowing users to indicate that imported users should be Privileged > makes sense. > > Your implementation using an undocumented config variable set from a > command line option is not code that I would include. > > I assume if the config was rewritten to support multiple sources that > adding a 'make these users privileged' flag to that would make sense > >
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
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 14 Jul 2010 13:11:47 -0400
To: Robert Nesius via RT <bug-RT-Extension-LDAPImport [...] rt.cpan.org>
From: Kevin Falcone <kevin [...] jibsheet.com>
Download (untitled) / with headers
text/plain 1014b
On Wed, Jul 14, 2010 at 01:06:09PM -0400, Robert Nesius via RT wrote: Show quoted text
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > > > On Wed, Jul 14, 2010 at 12:46:25PM -0400, Robert Nesius via RT wrote:
> > > > As I mentioned, making it configurable makes sense. > > > >
> > > > > > I'm confused about what exactly you mean by that.
> > > > Allowing users to indicate that imported users should be Privileged > > makes sense. > > > > Your implementation using an undocumented config variable set from a > > command line option is not code that I would include. > > > > I assume if the config was rewritten to support multiple sources that > > adding a 'make these users privileged' flag to that would make sense > > > >
> 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?
My current work will include some configuration related to Privileged users, but it isn't clear that it will be a clear-cut grouping like you want. -kevin
From: kiwiroy [...] gmail.com
Download (untitled) / with headers
text/plain 1.2k
On Wed Jul 14 13:11:58 2010, kevin@jibsheet.com wrote: Show quoted text
> On Wed, Jul 14, 2010 at 01:06:09PM -0400, Robert Nesius via RT wrote:
> > > <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > > > > > On Wed, Jul 14, 2010 at 12:46:25PM -0400, Robert Nesius via RT wrote:
> > > > > As I mentioned, making it configurable makes sense. > > > > >
> > > > > > > > I'm confused about what exactly you mean by that.
> > > > > > Allowing users to indicate that imported users should be Privileged > > > makes sense. > > > > > > Your implementation using an undocumented config variable set from a > > > command line option is not code that I would include. > > > > > > I assume if the config was rewritten to support multiple sources that > > > adding a 'make these users privileged' flag to that would make sense > > > > > >
> > Okay - I think I see what you're saying. Were you intending to do
that or Show quoted text
> > would you like me to try to come up with something?
> > My current work will include some configuration related to Privileged > users, but it isn't clear that it will be a clear-cut grouping like > you want. > > -kevin
+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.
Subject: rt-extension-ldapimport_update-priv.patch
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 {
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 2 Nov 2011 09:44:18 -0500
To: bug-RT-Extension-LDAPImport [...] rt.cpan.org
From: Robert Nesius <nesius [...] gmail.com>
Download (untitled) / with headers
text/plain 1.2k
On Wed, Nov 2, 2011 at 1:57 AM, https://www.google.com/accounts/o8/id?id=AItOawm6mqN_GWI3FdLYzSWUXoFefrtzwr-SnZkvia RT <bug-RT-Extension-LDAPImport@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 > > > +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. > >
I found it a bit of irony to see your email today. I just started seriously assessing a 3.8.6 -> 4.x migration and was reviewing this ticket last night and debating how to move forward. Since I submitted the patches that Kevin chose from for augmenting rtldapimport, I'm using my patched version which has many of the command-line flags Kevin turned into config variables. I'd prefer to run over whatever Kevin is providing even if the idea of having multiple RT_SiteConfig.pm files is required to support multiple ldap sources. Your timely note and patch may well save me some time - thanks! Kevin - I'm afraid I never quite grokked the implication of the plugin's etc dirs from your earlier email in this thread. If you could spare a moment to clarify that, I'd really appreciate it. Thanks much, -Rob
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 2 Nov 2011 18:54:52 -0400
To: Robert Nesius via RT <bug-RT-Extension-LDAPImport [...] rt.cpan.org>
From: Kevin Falcone <falcone [...] bestpractical.com>
Download (untitled) / with headers
text/plain 789b
On Wed, Nov 02, 2011 at 10:44:29AM -0400, Robert Nesius via RT wrote: Show quoted text
> Kevin - I'm afraid I never quite grokked the implication of the plugin's > etc dirs from your earlier email in this thread. If you could spare a > moment to clarify that, I'd really appreciate it.
I believe the gist of it was that your previous patchset only contained command line changes that set internal config variables and never documented any of them or made them available to users to configure permanently. New configuration variables should be documented and discrete, like the patch provided in the previous mail on this ticket which I expect to apply. You can also have more config files than just RT_Config.pm and RT_SiteConfig.pm if you need to store configuration data closer to the tool. -kevin
Subject: Re: [rt.cpan.org #51258] Collected Patches
Date: Wed, 2 Nov 2011 19:04:30 -0400
To: "https://www.google.com/accounts/o8/id?id=AItOawm6mqN_GWI3FdLYzSWUXoFefrtzwr-SnZk via RT" <bug-RT-Extension-LDAPImport [...] rt.cpan.org>
From: Kevin Falcone <falcone [...] bestpractical.com>
Download (untitled) / with headers
text/plain 789b
On Wed, Nov 02, 2011 at 02:57:02AM -0400, https://www.google.com/accounts/o8/id?id=AItOawm6mqN_GWI3FdLYzSWUXoFefrtzwr-SnZk via RT wrote: Show quoted text
> Queue: RT-Extension-LDAPImport > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=51258 >
> > My current work will include some configuration related to Privileged > > users, but it isn't clear that it will be a clear-cut grouping like > > you want. > >
> +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.
The change mentioned was LDAPCreatePrivileged which was included in 0.31. I've pushed your changes as a branch with some extra changes on top to look at before releasing a 0.32. -kevin
From: Robert Nesius
Download (untitled) / with headers
text/plain 1.4k
On Wed Nov 02 18:54:54 2011, falcone@bestpractical.com wrote: Show quoted text
> On Wed, Nov 02, 2011 at 10:44:29AM -0400, Robert Nesius via RT wrote:
> > Kevin - I'm afraid I never quite grokked the implication of the plugin's > > etc dirs from your earlier email in this thread. If you could spare a > > moment to clarify that, I'd really appreciate it.
> > I believe the gist of it was that your previous patchset only > contained command line changes that set internal config variables and > never documented any of them or made them available to users to > configure permanently.
I remember the command line setting a variable inside the module, but that variable only made sense in the context of the command-line interface I was offering at the time. Show quoted text
> > New configuration variables should be documented and discrete, like
> the patch provided in the previous mail on this ticket which I expect > to apply.
Please forgive what I expect is a beginner-question - does this basically boil down to documenting the variable in the README? Show quoted text
> > You can also have more config files than just RT_Config.pm and > RT_SiteConfig.pm if you need to store configuration data closer to > the tool.
I think my patch was doing that, though perhaps I could have done it a little more elegantly. Are you opposed to a command line interface similar to what I initially submitted? I think there's value there, and it was very useful while testing my configs, but I'll ultimately defer to you on that point. Thanks much, -Rob


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.