Ticket 47810 - memberOf plugin not properly rejecting updates

Bug Description:  When the memberOf plugin tries to add memberOf attribute to
                  an entry during a mod-replace on a group, even though the
                  update to the user entry fails, but plugin still allows
                  the member to be added to the group.

Fix Description:  During a mod/replace check and return an error if the member
                  update fails.

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

Reviewed by: nhosoi(Thanks!)

dirsrvtests/suites/betxns/betxn_test.py

@@ -3,7 +3,7 @@
 # All rights reserved.
 #
 # License: GPL (version 3 or any later version).
-# See LICENSE for details. 
+# See LICENSE for details.
 # --- END COPYRIGHT BLOCK ---
 #
 import os
@@ -174,6 +174,67 @@ def test_betxn_attr_uniqueness(topology):
     log.info('test_betxn_attr_uniqueness: PASSED')
 
 
+def test_betxn_memberof(topology):
+    ENTRY1_DN = 'cn=group1,' + DEFAULT_SUFFIX
+    ENTRY2_DN = 'cn=group2,' + DEFAULT_SUFFIX
+    PLUGIN_DN = 'cn=' + PLUGIN_MEMBER_OF + ',cn=plugins,cn=config'
+
+    # Enable and configure memberOf plugin
+    topology.standalone.plugins.enable(name=PLUGIN_MEMBER_OF)
+    try:
+        topology.standalone.modify_s(PLUGIN_DN, [(ldap.MOD_REPLACE, 'memberofgroupattr', 'member')])
+    except ldap.LDAPError, e:
+        log.fatal('test_betxn_memberof: Failed to update config(member): error ' + e.message['desc'])
+        assert False
+
+    # Add our test entries
+    try:
+        topology.standalone.add_s(Entry((ENTRY1_DN, {'objectclass': "top groupofnames".split(),
+                                     'cn': 'group1'})))
+    except ldap.LDAPError, e:
+        log.error('test_betxn_memberof: Failed to add group1:' +
+                  ENTRY1_DN + ', error ' + e.message['desc'])
+        assert False
+
+    try:
+        topology.standalone.add_s(Entry((ENTRY2_DN, {'objectclass': "top groupofnames".split(),
+                                     'cn': 'group1'})))
+    except ldap.LDAPError, e:
+        log.error('test_betxn_memberof: Failed to add group2:' +
+                  ENTRY2_DN + ', error ' + e.message['desc'])
+        assert False
+
+    #
+    # Test mod replace
+    #
+
+    # Add group2 to group1 - it should fail with objectclass violation
+    try:
+        topology.standalone.modify_s(ENTRY1_DN, [(ldap.MOD_REPLACE, 'member', ENTRY2_DN)])
+        log.fatal('test_betxn_memberof: Group2 was incorrectly allowed to be added to group1')
+        assert False
+    except ldap.LDAPError, e:
+        log.info('test_betxn_memberof: Group2 was correctly rejected (mod replace): error ' + e.message['desc'])
+
+    #
+    # Test mod add
+    #
+
+    # Add group2 to group1 - it should fail with objectclass violation
+    try:
+        topology.standalone.modify_s(ENTRY1_DN, [(ldap.MOD_ADD, 'member', ENTRY2_DN)])
+        log.fatal('test_betxn_memberof: Group2 was incorrectly allowed to be added to group1')
+        assert False
+    except ldap.LDAPError, e:
+        log.info('test_betxn_memberof: Group2 was correctly rejected (mod add): error ' + e.message['desc'])
+
+    #
+    # Done
+    #
+
+    log.info('test_betxn_memberof: PASSED')
+
+
 def test_betxn_final(topology):
     topology.standalone.delete()
     log.info('betxn test suite PASSED')
@@ -187,6 +248,7 @@ def run_isolated():
     test_betxn_init(topo)
     test_betxt_7bit(topo)
     test_betxn_attr_uniqueness(topo)
+    test_betxn_memberof(topo)
     test_betxn_final(topo)
 
 

ldap/servers/plugins/memberof/memberof.c

@@ -2373,6 +2373,7 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 	struct slapi_entry *post_e = NULL;
 	Slapi_Attr *pre_attr = 0;
 	Slapi_Attr *post_attr = 0;
+	int rc = 0;
 	int i = 0;
 
 	slapi_pblock_get( pb, SLAPI_ENTRY_PRE_OP, &pre_e );
@@ -2449,14 +2450,14 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 				in pre, not in post, delete from entry
 				not in pre, in post, add to entry
 			*/
-			while(pre_index < pre_total || post_index < post_total)
+			while(rc == 0 && (pre_index < pre_total || post_index < post_total))
 			{
 				if(pre_index == pre_total)
 				{
 					/* add the rest of post */
 					slapi_sdn_set_normdn_byref(sdn,
 					            slapi_value_get_string(post_array[post_index]));
-					memberof_add_one(pb, config, group_sdn, sdn);
+					rc = memberof_add_one(pb, config, group_sdn, sdn);
 
 					post_index++;
 				}
@@ -2465,7 +2466,7 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 					/* delete the rest of pre */
 					slapi_sdn_set_normdn_byref(sdn,
 					            slapi_value_get_string(pre_array[pre_index]));
-					memberof_del_one(pb, config, group_sdn, sdn);
+					rc = memberof_del_one(pb, config, group_sdn, sdn);
 
 					pre_index++;
 				}
@@ -2482,7 +2483,7 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 						/* delete pre array */
 						slapi_sdn_set_normdn_byref(sdn,
 					            slapi_value_get_string(pre_array[pre_index]));
-						memberof_del_one(pb, config, group_sdn, sdn);
+						rc = memberof_del_one(pb, config, group_sdn, sdn);
 
 						pre_index++;
 					}
@@ -2491,7 +2492,7 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 						/* add post array */
 						slapi_sdn_set_normdn_byref(sdn,
 					            slapi_value_get_string(post_array[post_index]));
-						memberof_add_one(pb, config, group_sdn, sdn);
+						rc = memberof_add_one(pb, config, group_sdn, sdn);
 
 						post_index++;
 					}
@@ -2509,7 +2510,7 @@ memberof_replace_list(Slapi_PBlock *pb, MemberOfConfig *config,
 		}
 	}
 	
-	return 0;
+	return rc;
 }
 
 /* memberof_load_array()