On Mon, Sep 21, 2015 at 02:49:53PM +0200, Petr Cech wrote:
On 09/21/2015 01:27 PM, Sumit Bose wrote:
>>Hello Sumit,
>>>
>>>there is next version of patch addresing your comments.
>>>
>>>I didn't use statement like
>>># state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
>>>because we don't have any for originalMemberOf. I'm not sure if I
added
>we have,
>
>state->ipa_opts->host_map[IPA_AT_HOST_MEMBER_OF].sys_name
>
>>>originalMemberOf version of statement above wheter it is not more like the
>>>second version of patch. If I understood the problem right, we wouldn't
have
>>>saved both kind of memberOf in our DB.
>yes, we do not save it in the cache, but I think I was confused myself
>when I mentioned the cache earlier in the thread. I thought we used this
>data from the cache to create the netgroup data, but the IPA provider
>already saves netgroup triples to the cache which makes sense because
>otherwise the responder must have knowledge about how netgroups are
>stored on the server. So no kind of cached memberOf data is used to
>create the netgroup entries, sorry for guiding you in the wrong
>direction here.
>
>However, the server side names aren't used as well. Since the most
>common use case for the LDAP data we read from the server is to write
>them into the cache and to make further processing more easy the
>attribute names are translated automatically into the internal names
>based on the various struct sdap_attr_map maps. Based on those
>translated names the netgroups evaluation is done. As you can see in
>e.g. src/providers/ipa/ipa_opts.h ""memberOf" of a user entry is
>translated into "SYSDB_MEMBEROF" and ""memberOf" of a host
entry is
>translated into "SYSDB_ORIG_MEMBEROF". I hope this helps to make it more
>clear.
>
Hello Sumit,
thank you for your patience. There is fixed patch attached. I used the
predefined mapping.
>Nevertheless the patch looks good and works as expected. I only found an
>issues when there are duplicated entries dues to memberships in multiple
>groups. I had to add
>
>diff --git a/src/providers/ipa/ipa_netgroups.c
>b/src/providers/ipa/ipa_netgroups.c
>index 2c39257..bc89006 100644
>--- a/src/providers/ipa/ipa_netgroups.c
>+++ b/src/providers/ipa/ipa_netgroups.c
>@@ -121,9 +121,9 @@ static errno_t ipa_save_netgroup(TALLOC_CTX
>*mem_ctx,
> }
> } else {
> for(c = 0; c < el->num_values; c++) {
>- ret = sysdb_attrs_add_string(netgroup_attrs,
>- SYSDB_NETGROUP_TRIPLE,
>- (const
> char*)el->values[c].data);
I saw another 2 occurrences of calling 'sysdb_attrs_add_string()' in for
loop. Maybe there should be 'safe' variant too.
The 'safe' variant comes with a cost and should be only used when really
needed. I think the other cases are safe (famous last words :-).
The patches are looking good and fixed the netgroups issues with group
and nested group memberships. CI passes
http://sssd-ci.duckdns.org/logs/job/27/43/summary.html except the debian
test but this is an unrelated known issue, so ACK.
Thank you for your patience as well.
bye,
Sumit
Regards
Petr
>+ ret = sysdb_attrs_add_string_safe(netgroup_attrs,
>+ SYSDB_NETGROUP_TRIPLE,
>+ (const char*)el->values[c].data);
> if (ret) {
> goto fail;
> }
>
>to make it work in my tests.
>
>bye,
>Sumit
>
>>>
>>>Regards
>>>Petr