Bug #78573 for RT-Authen-ExternalAuth: [PATCH] Added "scope" as a configurable option.

Wed Jul 25 15:48:49 2012 valmiky.arquissandas [...] ist.utl.pt - Ticket created

Subject:	[PATCH] Added "scope" as a configurable option.
Date:	Wed, 25 Jul 2012 20:48:21 +0100
To:	bug-RT-Authen-ExternalAuth [...] rt.cpan.org
From:	Valmiky Arquissandas <valmiky.arquissandas [...] ist.utl.pt>

The attached patch allows the scope of the LDAP group search to be configured, instead of being hard-coded to "base". It is useful to change the scope to "sub" when there are nested groups. The default option has been kept as "base".

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

Download signature.asc
application/pgp-signature 554b

Message body not shown because it is not plain text.

Thu Jul 26 12:26:37 2012 tsibley [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #78573] [PATCH] Added "scope" as a configurable option.
Date:	Thu, 26 Jul 2012 09:26:02 -0700
To:	bug-RT-Authen-ExternalAuth [...] rt.cpan.org
From:	Thomas Sibley <tsibley [...] cpan.org>

Hi Valmiky, On 07/25/2012 12:48 PM, Valmiky Arquissandas via RT wrote: Show quoted text

> Transaction: Ticket created by valmiky.arquissandas@ist.utl.pt > Queue: RT-Authen-ExternalAuth > Subject: [PATCH] Added "scope" as a configurable option. > > The attached patch allows the scope of the LDAP group search to be > configured, instead of being hard-coded to "base". > It is useful to change the scope to "sub" when there are nested groups. > > The default option has been kept as "base".

Thanks for the patch! There's a few changes I'd like to see to make it a better patch before applying it. 1) Take your description in the ticket above ("The attached patch..." and "The default option...") and include it in the commit message. 2) Rename the option to group_scope (and move it next to the other group options) since it only affects the group check. 3) Default it using $config->{'group_scope'} || 'base' in your my $group_scope line. etc/RT_SiteConfig.pm in the extension is just an example, not a set of defaults. Ideally there'd also be tests for this, if you're up for writing them. Thomas

Thu Jul 26 12:26:39 2012 The RT System itself - Status changed from 'new' to 'open'

Thu Jul 26 14:10:24 2012 valmiky.arquissandas [...] ist.utl.pt - Correspondence added

Subject:	Re: [rt.cpan.org #78573] [PATCH] Added "scope" as a configurable option.
Date:	Thu, 26 Jul 2012 19:09:54 +0100
To:	bug-RT-Authen-ExternalAuth [...] rt.cpan.org
From:	Valmiky Arquissandas <valmiky.arquissandas [...] ist.utl.pt>

Hi Thomas, On 26-07-2012 17:26, Thomas Sibley via RT wrote: Show quoted text

> 1) Take your description in the ticket above ("The attached patch..." > and "The default option...") and include it in the commit message.

Done. Show quoted text

> 2) Rename the option to group_scope (and move it next to the other group > options) since it only affects the group check.

Done. Show quoted text

> 3) Default it using > > $config->{'group_scope'} || 'base' > > in your my $group_scope line. etc/RT_SiteConfig.pm in the extension is > just an example, not a set of defaults.

Done. I initially thought that users would read etc/RT_SiteConfig.pm and keep the option unchanged, but forgot that it would break any upgrades (and isn't a default at all, indeed). Show quoted text

> Ideally there'd also be tests for this, if you're up for writing them.

I would, if I had any experience on RT's test infrastructure. For now, I'll keep the patch with the functionality, and perhaps do the tests when I learn how to do them. Valmiky

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

Download signature.asc
application/pgp-signature 554b

Message body not shown because it is not plain text.

Fri Jul 27 13:10:11 2012 tsibley [...] cpan.org - Taken

Fri Jul 27 17:56:50 2012 tsibley [...] cpan.org - Correspondence added

Thanks, applied.

Fri Jul 27 17:56:55 2012 tsibley [...] cpan.org - Status changed from 'open' to 'resolved'