Ticket 47963 - memberof skip nested groups breaks the plugin

Bug Description:  The previous patch broke the memberOf plugin - it
                  basically wouldn't do anything.  The skip was being
                  done too early.

Fix Description:  Move the "recursion skip" to the appropriate location.

https://fedorahosted.org/389/ticket/47963

Reviewed by: nhosoi(Thanks!)

dirsrvtests/tickets/ticket47963_test.py

@@ -0,0 +1,191 @@
+import os
+import sys
+import time
+import ldap
+import logging
+import pytest
+from lib389 import DirSrv, Entry, tools, tasks
+from lib389.tools import DirSrvTools
+from lib389._constants import *
+from lib389.properties import *
+from lib389.tasks import *
+
+logging.getLogger(__name__).setLevel(logging.DEBUG)
+log = logging.getLogger(__name__)
+
+installation1_prefix = None
+
+
+class TopologyStandalone(object):
+    def __init__(self, standalone):
+        standalone.open()
+        self.standalone = standalone
+
+
[email protected](scope="module")
+def topology(request):
+    global installation1_prefix
+    if installation1_prefix:
+        args_instance[SER_DEPLOYED_DIR] = installation1_prefix
+
+    # Creating standalone instance ...
+    standalone = DirSrv(verbose=False)
+    args_instance[SER_HOST] = HOST_STANDALONE
+    args_instance[SER_PORT] = PORT_STANDALONE
+    args_instance[SER_SERVERID_PROP] = SERVERID_STANDALONE
+    args_instance[SER_CREATION_SUFFIX] = DEFAULT_SUFFIX
+    args_standalone = args_instance.copy()
+    standalone.allocate(args_standalone)
+    instance_standalone = standalone.exists()
+    if instance_standalone:
+        standalone.delete()
+    standalone.create()
+    standalone.open()
+
+    # Clear out the tmp dir
+    standalone.clearTmpDir(__file__)
+
+    return TopologyStandalone(standalone)
+
+
+def test_ticket47963(topology):
+    '''
+    Test that the memberOf plugin works correctly after setting:
+
+        memberofskipnested: on
+
+    '''
+    PLUGIN_DN = 'cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config'
+    USER_DN = 'uid=test_user,' + DEFAULT_SUFFIX
+    GROUP_DN1 = 'cn=group1,' + DEFAULT_SUFFIX
+    GROUP_DN2 = 'cn=group2,' + DEFAULT_SUFFIX
+    GROUP_DN3 = 'cn=group3,' + DEFAULT_SUFFIX
+
+    #
+    # Enable the plugin and configure the skiop nest attribute, then restart the server
+    #
+    topology.standalone.plugins.enable(name=PLUGIN_MEMBER_OF)
+    try:
+        topology.standalone.modify_s(PLUGIN_DN, [(ldap.MOD_REPLACE, 'memberofskipnested', 'on')])
+    except ldap.LDAPError, e:
+        log.error('test_automember: Failed to modify config entry: error ' + e.message['desc'])
+        assert False
+
+    topology.standalone.restart(timeout=10)
+
+    #
+    # Add our groups, users, memberships, etc
+    #
+    try:
+        topology.standalone.add_s(Entry((USER_DN, {
+                          'objectclass': 'top extensibleObject'.split(),
+                          'uid': 'test_user'
+                          })))
+    except ldap.LDAPError, e:
+        log.error('Failed to add teset user: error ' + e.message['desc'])
+        assert False
+
+    try:
+        topology.standalone.add_s(Entry((GROUP_DN1, {
+                          'objectclass': 'top groupOfNames groupOfUniqueNames extensibleObject'.split(),
+                          'cn': 'group1',
+                          'member': USER_DN
+                          })))
+    except ldap.LDAPError, e:
+        log.error('Failed to add group1: error ' + e.message['desc'])
+        assert False
+
+    try:
+        topology.standalone.add_s(Entry((GROUP_DN2, {
+                          'objectclass': 'top groupOfNames groupOfUniqueNames extensibleObject'.split(),
+                          'cn': 'group2',
+                          'member': USER_DN
+                          })))
+    except ldap.LDAPError, e:
+        log.error('Failed to add group2: error ' + e.message['desc'])
+        assert False
+
+    # Add group with no member(yet)
+    try:
+        topology.standalone.add_s(Entry((GROUP_DN3, {
+                          'objectclass': 'top groupOfNames groupOfUniqueNames extensibleObject'.split(),
+                          'cn': 'group'
+                          })))
+    except ldap.LDAPError, e:
+        log.error('Failed to add group3: error ' + e.message['desc'])
+        assert False
+    time.sleep(1)
+
+    #
+    # Test we have the correct memberOf values in the user entry
+    #
+    try:
+        member_filter = ('(&(memberOf=' + GROUP_DN1 + ')(memberOf=' + GROUP_DN2 + '))')
+        entries = topology.standalone.search_s(USER_DN, ldap.SCOPE_BASE, member_filter)
+        if not entries:
+            log.fatal('User is missing expected memberOf attrs')
+            assert False
+    except ldap.LDAPError, e:
+        log.fatal('Search for user1 failed: ' + e.message['desc'])
+        assert False
+
+    # Add the user to the group
+    try:
+        topology.standalone.modify_s(GROUP_DN3, [(ldap.MOD_ADD, 'member', USER_DN)])
+    except ldap.LDAPError, e:
+        log.error('Failed to member to group: error ' + e.message['desc'])
+        assert False
+    time.sleep(1)
+
+    # Check that the test user is a "memberOf" all three groups
+    try:
+        member_filter = ('(&(memberOf=' + GROUP_DN1 + ')(memberOf=' + GROUP_DN2 +
+                        ')(memberOf=' + GROUP_DN3 + '))')
+        entries = topology.standalone.search_s(USER_DN, ldap.SCOPE_BASE, member_filter)
+        if not entries:
+            log.fatal('User is missing expected memberOf attrs')
+            assert False
+    except ldap.LDAPError, e:
+        log.fatal('Search for user1 failed: ' + e.message['desc'])
+        assert False
+
+    #
+    # Delete group2, and check memberOf values in the user entry
+    #
+    try:
+        topology.standalone.delete_s(GROUP_DN2)
+    except ldap.LDAPError, e:
+        log.error('Failed to delete test group2: ' + e.message['desc'])
+        assert False
+    time.sleep(1)
+
+    try:
+        member_filter = ('(&(memberOf=' + GROUP_DN1 + ')(memberOf=' + GROUP_DN3 + '))')
+        entries = topology.standalone.search_s(USER_DN, ldap.SCOPE_BASE, member_filter)
+        if not entries:
+            log.fatal('User incorrect memberOf attrs')
+            assert False
+    except ldap.LDAPError, e:
+        log.fatal('Search for user1 failed: ' + e.message['desc'])
+        assert False
+
+    log.info('Test complete')
+
+
+def test_ticket47963_final(topology):
+    topology.standalone.delete()
+    log.info('Testcase PASSED')
+
+
+def run_isolated():
+    global installation1_prefix
+    installation1_prefix = None
+
+    topo = topology(True)
+    test_ticket47963(topo)
+    test_ticket47963_final(topo)
+
+
+if __name__ == '__main__':
+    run_isolated()
+

ldap/servers/plugins/memberof/memberof.c

@@ -691,7 +691,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,
 	char *cookie = NULL;
 	int all_backends = memberof_config_get_all_backends();
 	Slapi_DN *entry_scope = memberof_config_get_entry_scope();
-        Slapi_DN *entry_scope_exclude_subtree = memberof_config_get_entry_scope_exclude_subtree();
+	Slapi_DN *entry_scope_exclude_subtree = memberof_config_get_entry_scope_exclude_subtree();
 	int types_name_len = 0;
 	int num_types = 0;
 	int dn_len = slapi_sdn_get_ndn_len(sdn);
@@ -703,7 +703,7 @@ memberof_call_foreach_dn(Slapi_PBlock *pb, Slapi_DN *sdn,
 		return (rc);
 	}
         
-        if (entry_scope_exclude_subtree && slapi_sdn_issuffix(sdn, entry_scope_exclude_subtree)) {
+	if (entry_scope_exclude_subtree && slapi_sdn_issuffix(sdn, entry_scope_exclude_subtree)) {
 		return (rc);
 	}
 
@@ -2024,7 +2024,7 @@ Slapi_ValueSet *
 memberof_get_groups(MemberOfConfig *config, Slapi_DN *member_sdn)
 {
 	Slapi_ValueSet *groupvals = slapi_valueset_new();
-        Slapi_ValueSet *group_norm_vals = slapi_valueset_new();
+	Slapi_ValueSet *group_norm_vals = slapi_valueset_new();
 	Slapi_Value *memberdn_val = 
 	                      slapi_value_new_string(slapi_sdn_get_ndn(member_sdn));
 	slapi_value_set_flags(memberdn_val, SLAPI_ATTR_FLAG_NORMALIZED_CIS);
@@ -2034,7 +2034,7 @@ memberof_get_groups(MemberOfConfig *config, Slapi_DN *member_sdn)
 	memberof_get_groups_r(config, member_sdn, &data);
 
 	slapi_value_free(&memberdn_val);
-        slapi_valueset_free(group_norm_vals);
+	slapi_valueset_free(group_norm_vals);
 
 	return groupvals;
 }
@@ -2057,12 +2057,13 @@ int memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
 {
 	Slapi_DN *group_sdn = slapi_entry_get_sdn(e);
 	char *group_ndn = slapi_entry_get_ndn(e);
-        char *group_dn = slapi_entry_get_dn(e);
+	char *group_dn = slapi_entry_get_dn(e);
 	Slapi_Value *group_ndn_val = 0;
-        Slapi_Value *group_dn_val = 0;
+	Slapi_Value *group_dn_val = 0;
 	Slapi_ValueSet *groupvals = *((memberof_get_groups_data*)callback_data)->groupvals;
-        Slapi_ValueSet *group_norm_vals = *((memberof_get_groups_data*)callback_data)->group_norm_vals;
-        Slapi_DN *entry_scope_exclude_subtree = memberof_config_get_entry_scope_exclude_subtree();
+	Slapi_ValueSet *group_norm_vals = *((memberof_get_groups_data*)callback_data)->group_norm_vals;
+	Slapi_DN *entry_scope_exclude_subtree = memberof_config_get_entry_scope_exclude_subtree();
+	MemberOfConfig *config = ((memberof_get_groups_data*)callback_data)->config;
 	int rc = 0;
 
 	if(slapi_is_shutting_down()){
@@ -2116,18 +2117,19 @@ int memberof_get_groups_callback(Slapi_Entry *e, void *callback_data)
 		goto bail;
 	}
 
-        /* if the group does not belong to an excluded subtree, adds it to the valueset */
-        if (!(entry_scope_exclude_subtree && slapi_sdn_issuffix(group_sdn, entry_scope_exclude_subtree))) {
-                /* Push group_dn_val into the valueset.  This memory is now owned
-                 * by the valueset. */
-                group_dn_val = slapi_value_new_string(group_dn);
-                slapi_valueset_add_value_ext(groupvals, group_dn_val, SLAPI_VALUE_FLAG_PASSIN);
-                slapi_valueset_add_value_ext(group_norm_vals, group_ndn_val, SLAPI_VALUE_FLAG_PASSIN);
-        }
-        
-	/* now recurse to find parent groups of e */
-	memberof_get_groups_r(((memberof_get_groups_data*)callback_data)->config,
-		group_sdn, callback_data);
+	/* if the group does not belong to an excluded subtree, adds it to the valueset */
+	if (!(entry_scope_exclude_subtree && slapi_sdn_issuffix(group_sdn, entry_scope_exclude_subtree))) {
+			/* Push group_dn_val into the valueset.  This memory is now owned
+			 * by the valueset. */
+			group_dn_val = slapi_value_new_string(group_dn);
+			slapi_valueset_add_value_ext(groupvals, group_dn_val, SLAPI_VALUE_FLAG_PASSIN);
+			slapi_valueset_add_value_ext(group_norm_vals, group_ndn_val, SLAPI_VALUE_FLAG_PASSIN);
+	}
+	if(!config->skip_nested || config->fixup_task){
+		/* now recurse to find parent groups of e */
+		memberof_get_groups_r(((memberof_get_groups_data*)callback_data)->config,
+			group_sdn, callback_data);
+	}
 
 bail:
 	return rc;
@@ -2844,10 +2846,7 @@ int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data)
 	memberof_del_dn_data del_data = {0, config->memberof_attr};
 	Slapi_ValueSet *groups = 0;
 
-	if(!config->skip_nested || config->fixup_task){
-		/* get a list of all of the groups this user belongs to */
-		groups = memberof_get_groups(config, sdn);
-	}
+	groups = memberof_get_groups(config, sdn);
 
 	/* If we found some groups, replace the existing memberOf attribute
 	 * with the found values.  */