Bug 620927 - RFE: Allow multiple membership attributes for one memberof attribute in memberof plugin
Summary: RFE: Allow multiple membership attributes for one memberof attribute in memb...
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Server - memberOf Plug-in
Version: 1.2.6
Hardware: All
OS: Linux
high
medium
Target Milestone: ---
Assignee: Nathan Kinder
QA Contact: Viktor Ashirov
URL:
Whiteboard:
Depends On:
Blocks: 639035
TreeView+ depends on / blocked
 
Reported: 2010-08-03 19:16 UTC by Rob Crittenden
Modified: 2015-12-07 16:43 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-12-07 16:43:19 UTC


Attachments (Terms of Use)
Proposed Patch (32.89 KB, patch)
2010-08-17 22:43 UTC, Nathan Kinder
no flags Details | Diff
Revised patch (35.65 KB, patch)
2010-08-30 17:52 UTC, Nathan Kinder
nhosoi: review+
Details | Diff

Description Rob Crittenden 2010-08-03 19:16:17 UTC
In IPA we configure memberof to work on the member attribute. This works for the majority of our group types (for users, hosts, etc). Our configuration looks like this:

dn: cn=MemberOf Plugin,cn=plugins,cn=config
objectClass: top
objectClass: nsSlapdPlugin
objectClass: extensibleObject
cn: MemberOf Plugin
nsslapd-pluginPath: libmemberof-plugin
nsslapd-pluginInitfunc: memberof_postop_init
nsslapd-pluginType: postoperation
nsslapd-pluginEnabled: on
nsslapd-plugin-depends-on-type: database
memberofgroupattr: member
memberofattr: memberOf
nsslapd-pluginId: memberof
nsslapd-pluginVersion: 1.2.6.rc3
nsslapd-pluginVendor: 389 Project
nsslapd-pluginDescription: memberof plugin

For netgroups we define two additional member attributes: memberHost and memberUser. We would like memberOf to operate on these attributes as well. 

I would be fine to modify the plugin to allow only a single memberOf config entry with multiple memberOfGroupAttr settings.

Comment 1 Nathan Kinder 2010-08-05 20:58:22 UTC
I put some design notes about this enhancement on the 389 wiki:

http://directory.fedoraproject.org/wiki/MemberOf_Multiple_Grouping_Enhancements

Comment 3 Nathan Kinder 2010-08-17 22:43:01 UTC
Created attachment 439237 [details]
Proposed Patch

Comment 5 Noriko Hosoi 2010-08-18 17:24:38 UTC
I have some minor requests and questions...  The rest looks good.

1. 
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec8
It might not matter at all, but just want to have a confirmation... memberof_del_dn_from_groups returns the rc from the last memberof_call_foreach_dn, where the error return is ignored in the loop.  Is it okay?

393		rc = memberof_call_foreach_dn(pb, dn, groupattrs,
394			memberof_del_dn_type_callback, &data);

Same for this section:
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec10

2. 
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec5
Forgive me.  Now my eyes are contaminated by coverity... :)  It'd be nice if you could add checking NULL types here...
469		for (num_types = 0; types[num_types]; num_types++)
==>
469		for (num_types = 0; types && types[num_types]; num_types++)

Same for this section:
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2
294		for (num_groupattrs = 0; theConfig.groupattrs[num_groupattrs]; num_groupattrs++)
==>
294		for (num_groupattrs = 0; theConfig.groupattrs && theConfig.groupattrs[num_groupattrs]; num_groupattrs++)

309		for (i = 0; theConfig.groupattrs[i]; i++)
==>
309		for (i = 0; theConfig.groupattrs && theConfig.groupattrs[i]; i++)

3.
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec1
This is just an RFE.  It'd be nice if this message tells which attribute is violating the dn syntax requirement...
202		if (not_dn_syntax)
203		{
204			PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,
205				"The %s configuration attribute must be set to "
206				"an attribute defined to use the Distinguished 207				"Name syntax.",
208				MEMBEROF_GROUP_ATTR);
209		}

And can we move slapi_attr_new out from the loop to reduce the allocation count?
187			test_attr = slapi_attr_new();

4.
https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2
I wonder what happens if slapi_str2filter returns NULL for some reason (I guess it will not happen, tho...)
346		theConfig.group_filter = slapi_str2filter(filter_str);

Comment 6 Nathan Kinder 2010-08-30 17:11:37 UTC
(In reply to comment #5)
> I have some minor requests and questions...  The rest looks good.
> 
> 1. 
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec8
> It might not matter at all, but just want to have a confirmation...
> memberof_del_dn_from_groups returns the rc from the last
> memberof_call_foreach_dn, where the error return is ignored in the loop.  Is it
> okay?
> 
> 393  rc = memberof_call_foreach_dn(pb, dn, groupattrs,
> 394   memberof_del_dn_type_callback, &data);

The return value of memberof_del_dn_from_groups is never used.  I will change the return type to void for this function and remove the rc variable.

> 
> Same for this section:
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec10

I'll change this return type to void and remove the usage of rc as well.

> 
> 2. 
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof.c_sec5
> Forgive me.  Now my eyes are contaminated by coverity... :)  It'd be nice if
> you could add checking NULL types here...
> 469  for (num_types = 0; types[num_types]; num_types++)
> ==>
> 469  for (num_types = 0; types && types[num_types]; num_types++)

Sure.

> 
> Same for this section:
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2
> 294  for (num_groupattrs = 0; theConfig.groupattrs[num_groupattrs];
> num_groupattrs++)
> ==>
> 294  for (num_groupattrs = 0; theConfig.groupattrs &&
> theConfig.groupattrs[num_groupattrs]; num_groupattrs++)
> 
> 309  for (i = 0; theConfig.groupattrs[i]; i++)
> ==>
> 309  for (i = 0; theConfig.groupattrs && theConfig.groupattrs[i]; i++)

Sure.

> 
> 3.
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec1
> This is just an RFE.  It'd be nice if this message tells which attribute is
> violating the dn syntax requirement...
> 202  if (not_dn_syntax)
> 203  {
> 204   PR_snprintf(returntext, SLAPI_DSE_RETURNTEXT_SIZE,
> 205    "The %s configuration attribute must be set to "
> 206    "an attribute defined to use the Distinguished 207    "Name syntax.",
> 208    MEMBEROF_GROUP_ATTR);
> 209  }

Good idea.  We should probably do this for the MEMBEROF_ATTR check as well, even though it only works with a single value.

> 
> And can we move slapi_attr_new out from the loop to reduce the allocation
> count?
> 187   test_attr = slapi_attr_new();

I had looked into doing this initially, but slapi_attr_init() is for initializing an empty Slapi_Attr that has been created by slapi_attr_new().  The contents of an existing Slapi_Attr are not free'd first.  There is no public SLAPI function to clear out the contents of an existing Slapi_Attr for reuse.  There is a private attr_done() function which we could expose, but I thought that it wasn't worth doing for this issue since we only execute this code when the server is started or when the memberOf plug-in configuration is modified.  If you feel that it is worthwhile to make these changes, I can.

> 
> 4.
> https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2
> I wonder what happens if slapi_str2filter returns NULL for some reason (I guess
> it will not happen, tho...)
> 346  theConfig.group_filter = slapi_str2filter(filter_str);

I'm not sure how/if that could happen since we validate the attribute names used to build the filter and build the filter ourselves in the plug-in.  If it can happen, we would end up passing a NULL filter to slapi_filter_test_simple() when performing group checks in the memberOf plug-in, which would cause a server crash.

The return value of memberof_apply_config() is not checked.  It is assumed that memberof_validate_config() already caught any config problems that would cause issues, but memberof_validate_config is not responsible for building a Slapi_Filter.  I can make memberof_apply_config log an error message if it is unable to build the filter and add NULL checking in the 4 places we use the filter to perform group checks.  The plug-in will not work properly in this case as it will be unable to detect if an entry is a group, but that is better than crashing.  One would see the error message and be able to fix their memberOf configuration to get things working again.  Does this approach sound OK?

Comment 7 Noriko Hosoi 2010-08-30 17:25:29 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > 3.
> I had looked into doing this initially, but slapi_attr_init() is for
> initializing an empty Slapi_Attr that has been created by slapi_attr_new(). 
> The contents of an existing Slapi_Attr are not free'd first.  There is no
> public SLAPI function to clear out the contents of an existing Slapi_Attr for
> reuse.  There is a private attr_done() function which we could expose, but I
> thought that it wasn't worth doing for this issue since we only execute this
> code when the server is started or when the memberOf plug-in configuration is
> modified.  If you feel that it is worthwhile to make these changes, I can.

Thank you for the details.  Your analysis makes sense.  Since it's not executed very often, the current code is efficient enough.

> > 4.
> > https://bugzilla.redhat.com/attachment.cgi?id=439237&action=diff#a/ldap/servers/plugins/memberof/memberof_config.c_sec2
> > I wonder what happens if slapi_str2filter returns NULL for some reason (I guess
> > it will not happen, tho...)
> > 346  theConfig.group_filter = slapi_str2filter(filter_str);
> 
> I'm not sure how/if that could happen since we validate the attribute names
> used to build the filter and build the filter ourselves in the plug-in.  If it
> can happen, we would end up passing a NULL filter to slapi_filter_test_simple()
> when performing group checks in the memberOf plug-in, which would cause a
> server crash.
> 
> The return value of memberof_apply_config() is not checked.  It is assumed that
> memberof_validate_config() already caught any config problems that would cause
> issues, but memberof_validate_config is not responsible for building a
> Slapi_Filter.  I can make memberof_apply_config log an error message if it is
> unable to build the filter and add NULL checking in the 4 places we use the
> filter to perform group checks.  The plug-in will not work properly in this
> case as it will be unable to detect if an entry is a group, but that is better
> than crashing.  One would see the error message and be able to fix their
> memberOf configuration to get things working again.  Does this approach sound
> OK?

Yes, I think so.  Thank you!!

Comment 8 Nathan Kinder 2010-08-30 17:52:16 UTC
Created attachment 442000 [details]
Revised patch

This addresses the issues raised in Noriko's review (thanks!).

Comment 9 Nathan Kinder 2010-08-31 15:21:59 UTC
Thanks to Noriko for her reviews!  Pushed to master.

Counting objects: 23, done.
Delta compression using 2 threads.
Compressing objects: 100% (12/12), done.
Writing objects: 100% (12/12), 5.78 KiB, done.
Total 12 (delta 9), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
   8db7b8a..a2bcd81  master -> master

Comment 10 Jenny Severance 2011-06-06 17:08:07 UTC
Verified
389-ds-base-1.2.8.2-1.el6.x86_64 
Red Hat Enterprise Linux Server release 6.1 (Santiago)

# ldapsearch -D "cn=Directory Manager" -w Secret123 -b "cn=MemberOf Plugin,cn=plugins,cn=config"
# extended LDIF
#
# LDAPv3
# base <cn=MemberOf Plugin,cn=plugins,cn=config> with scope subtree
# filter: (objectclass=*)
# requesting: ALL
#

# MemberOf Plugin, plugins, config
dn: cn=MemberOf Plugin,cn=plugins,cn=config
objectClass: top
objectClass: nsSlapdPlugin
objectClass: extensibleObject
cn: MemberOf Plugin
nsslapd-pluginPath: libmemberof-plugin
nsslapd-pluginInitfunc: memberof_postop_init
nsslapd-pluginType: postoperation
nsslapd-pluginEnabled: on
nsslapd-plugin-depends-on-type: database
memberofgroupattr: member
memberofgroupattr: memberUser
memberofgroupattr: memberHost
memberofattr: memberOf
nsslapd-pluginId: memberof
nsslapd-pluginVersion: 1.2.8.2
nsslapd-pluginVendor: 389 Project
nsslapd-pluginDescription: memberof plugin

# search result
search: 2
result: 0 Success

# numResponses: 2
# numEntries: 1


Note You need to log in before you can comment on or make changes to this bug.