[PATCH] Add ipa_hbac_support_srchost option to IPA provider

[Jan Zelený]

don't fetch all host groups if this option is set to false
https://fedorahosted.org/sssd/ticket/1078

Jan

[Stephen Gallagher]

On Fri, 2011-11-18 at 16:13 +0100, Jan Zeleny wrote:
>  don't fetch all host groups if this option is set to false
>  https://fedorahosted.org/sssd/ticket/1078 
Nack.

I don't like setting the srchost element to NULL and checking for that.
Technically, we're violating the HBAC design by omitting the srchost
here. I'd rather that our solution be to set srchost to
HBAC_CATEGORY_ALL instead of special-casing NULL.

You're missing a comma in:
static struct sdap_attr_map hostgroup_map[] = {
...
    {"ipa_id", IPA_UNIQUE_ID IPA_UNIQUE_ID, NULL}
};

If you're going to use an sdap_attr_map, it's probably better to do the
memberOf->originalMemberOf and member->orig_member conversion in the
attribute map instead of calls to replace_attribute_name in the _done()
functions.

In ipa_hbac_host_info_done(), don't allocate the hostgroup_filter unless
we're doing the full lookup. Move it into the support_srchost if block.

[Jan Zelený]

>  On Fri, 2011-11-18 at 16:13 +0100, Jan Zeleny wrote:
>  > don't fetch all host groups if this option is set to false
>  > https://fedorahosted.org/sssd/ticket/1078
>  
>  Nack.
>  
>  I don't like setting the srchost element to NULL and checking for that.
>  Technically, we're violating the HBAC design by omitting the srchost
>  here. I'd rather that our solution be to set srchost to
>  HBAC_CATEGORY_ALL instead of special-casing NULL.
>  
>  You're missing a comma in:
>  static struct sdap_attr_map hostgroup_map[] = {
>  ...
>      {"ipa_id", IPA_UNIQUE_ID IPA_UNIQUE_ID, NULL}
>  };
>  
>  If you're going to use an sdap_attr_map, it's probably better to do the
>  memberOf->originalMemberOf and member->orig_member conversion in the
>  attribute map instead of calls to replace_attribute_name in the _done()
>  functions.
>  
>  In ipa_hbac_host_info_done(), don't allocate the hostgroup_filter unless
>  we're doing the full lookup. Move it into the support_srchost if block. 
I'm sending the new patch in attachment, all issues are addressed.

Thanks
Jan

[Stephen Gallagher]

On Wed, 2011-11-23 at 16:23 +0100, Jan Zelený wrote:
>  > On Fri, 2011-11-18 at 16:13 +0100, Jan Zeleny wrote:
>  > > don't fetch all host groups if this option is set to false
>  > > https://fedorahosted.org/sssd/ticket/1078
>  > 
>  > Nack. 
Nack again.

>  > 
>  > I don't like setting the srchost element to NULL and checking for that.
>  > Technically, we're violating the HBAC design by omitting the srchost
>  > here. I'd rather that our solution be to set srchost to
>  > HBAC_CATEGORY_ALL instead of special-casing NULL.
>  >  
This is fine.

>  > You're missing a comma in:
>  > static struct sdap_attr_map hostgroup_map[] = {
>  > ...
>  >     {"ipa_id", IPA_UNIQUE_ID IPA_UNIQUE_ID, NULL}
>  > };
>  >  
You didn't fix the missing comma.

>  > If you're going to use an sdap_attr_map, it's probably
> better to do the
>  > memberOf->originalMemberOf and member->orig_member conversion in the
>  > attribute map instead of calls to replace_attribute_name in the _done()
>  > functions.
>  >  

You didn't remove the replace_attribute_name() calls.

>  > In ipa_hbac_host_info_done(), don't allocate the
> hostgroup_filter unless
>  > we're doing the full lookup. Move it into the support_srchost if block.
>   

>  I'm sending the new patch in attachment, all issues are
> addressed. 
Not quite :)

[Jan Zelený]

>  On Wed, 2011-11-23 at 16:23 +0100, Jan Zelený wrote:
>  > > On Fri, 2011-11-18 at 16:13 +0100, Jan Zeleny wrote:
>  > > > don't fetch all host groups if this option is set to false
>  > > > https://fedorahosted.org/sssd/ticket/1078
>  > > 
>  > > Nack.
>  
>  Nack again.
>  
>  > > I don't like setting the srchost element to NULL and checking for that.
>  > > Technically, we're violating the HBAC design by omitting the srchost
>  > > here. I'd rather that our solution be to set srchost to
>  > > HBAC_CATEGORY_ALL instead of special-casing NULL.
>  
>  This is fine.
>  
>  > > You're missing a comma in:
>  > > static struct sdap_attr_map hostgroup_map[] = {
>  > > ...
>  > > 
>  > >     {"ipa_id", IPA_UNIQUE_ID IPA_UNIQUE_ID, NULL}
>  > > 
>  > > };
>  
>  You didn't fix the missing comma. 
Sorry, I didn't notice the missing comma between the two IPA_UNIQUE_ID. Fixed 
now.

>  
>  > > If you're going to use an sdap_attr_map, it's probably better to do
> the
>  > > memberOf->originalMemberOf and member->orig_member conversion in the
>  > > attribute map instead of calls to replace_attribute_name in the _done()
>  > > functions.
>  
>  You didn't remove the replace_attribute_name() calls. 
Here I misunderstood what did you originally meant. I thought you meant to use 
the map conversion for deref query only. Now I don't understand how could I 
thought that.

>  
>  > > In ipa_hbac_host_info_done(), don't allocate the hostgroup_filter
>  > > unless we're doing the full lookup. Move it into the support_srchost
>  > > if block.
>  > 
>  > I'm sending the new patch in attachment, all issues are addressed.
>  
>  Not quite :) 
Perhaps now? ;)

Thanks
Jan

[Stephen Gallagher]

On Thu, 2011-11-24 at 10:35 +0100, Jan Zelený wrote:
>  > On Wed, 2011-11-23 at 16:23 +0100, Jan Zelený wrote:
>  > > > On Fri, 2011-11-18 at 16:13 +0100, Jan Zeleny wrote:
>  > > > > don't fetch all host groups if this option is set to false
>  > > > > https://fedorahosted.org/sssd/ticket/1078
>  > > > 
>  > > > Nack.
>  > 
>  > Nack again.
>  > 
>  > > > I don't like setting the srchost element to NULL and checking for
> that.
>  > > > Technically, we're violating the HBAC design by omitting the srchost
>  > > > here. I'd rather that our solution be to set srchost to
>  > > > HBAC_CATEGORY_ALL instead of special-casing NULL.
>  > 
>  > This is fine.
>  > 
>  > > > You're missing a comma in:
>  > > > static struct sdap_attr_map hostgroup_map[] = {
>  > > > ...
>  > > > 
>  > > >     {"ipa_id", IPA_UNIQUE_ID IPA_UNIQUE_ID, NULL}
>  > > > 
>  > > > };
>  > 
>  > You didn't fix the missing comma.
>  
>  Sorry, I didn't notice the missing comma between the two IPA_UNIQUE_ID. Fixed 
>  now.
>  
>  > 
>  > > > If you're going to use an sdap_attr_map, it's probably better to
> do the
>  > > > memberOf->originalMemberOf and member->orig_member conversion in
> the
>  > > > attribute map instead of calls to replace_attribute_name in the _done()
>  > > > functions.
>  > 
>  > You didn't remove the replace_attribute_name() calls.
>  
>  Here I misunderstood what did you originally meant. I thought you meant to use 
>  the map conversion for deref query only. Now I don't understand how could I 
>  thought that.
>  
>  > 
>  > > > In ipa_hbac_host_info_done(), don't allocate the hostgroup_filter
>  > > > unless we're doing the full lookup. Move it into the support_srchost
>  > > > if block.
>  > > 
>  > > I'm sending the new patch in attachment, all issues are addressed.
>  > 
>  > Not quite :)
>  
>  Perhaps now? ;) 
Ack and pushed to master. (I sent an email to this effect yesterday, but
it got lost in the mail, apparently).