On Fri, Sep 18, 2015 at 02:33:03PM +0200, Petr Cech wrote:
On 09/14/2015 01:53 PM, Sumit Bose wrote:
Hi Petr,
thank you for your patience. Both versions of your patches work as expected and fixes the issues with group and nested group memberships.
After testing a reading the current sources I think the best solution might be a slight variant of your first patch where you check for both memberOf and originalMemberOf but instead of always checking both I would suggest to pass down the attribute to check by adding a new member to struct extract_state and set it with the help of a new attribute to extract_members(). Since extract_members() is called individually for users and hosts in ipa_netgr_process_all() you can always pass the right attribute.Btw. instead of using SYSDB_ORIG_MEMBEROF and SYSDB_MEMBEROF explicitly you might want to use the configured mapping which e.g. is available in ipa_netgr_process_all() as
state->ipa_opts->id->user_map[SDAP_AT_USER_MEMBEROF].sys_name
bye, Sumit
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.
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); + 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