Ticket 49072 - validate memberof fixup task args

Bug Description:  If an invalid base dn, or invalid filter was provided
                  in the task there was no way to tell thathte task
                  actually failed.

Fix Description:  Log an error, and properly update the task status/exit
                  code when an error occurs.

                  Added CI test (also fixed some issues in the dynamic
                  plugins test suite).

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

Reviewed by: nhosoi(Thanks!)

dirsrvtests/tests/suites/dynamic-plugins/plugin_tests.py

@@ -96,6 +96,7 @@ def test_dependency(inst, plugin):
 ################################################################################
 def wait_for_task(conn, task_dn):
     finished = False
+    exitcode = 0
     count = 0
     while count < 60:
         try:
@@ -105,6 +106,7 @@ def wait_for_task(conn, task_dn):
                 assert False
             if task_entry[0].hasAttr('nstaskexitcode'):
                 # task is done
+                exitcode = task_entry[0].nsTaskExitCode
                 finished = True
                 break
         except ldap.LDAPError as e:
@@ -117,6 +119,8 @@ def wait_for_task(conn, task_dn):
         log.fatal('wait_for_task: Task (%s) did not complete!' % task_dn)
         assert False
 
+    return exitcode
+
 
 ################################################################################
 #
@@ -1416,9 +1420,82 @@ def test_memberof(inst, args=None):
         log.fatal('test_memberof: Search for user1 failed: ' + e.message['desc'])
         assert False
 
-    # Enable the plugin, and run the task
+    # Enable memberof plugin
     inst.plugins.enable(name=PLUGIN_MEMBER_OF)
 
+    #############################################################
+    # Test memberOf fixup arg validation:  Test the DN and filter
+    #############################################################
+
+    #
+    # Test bad/nonexistant DN
+    #
+    TASK_DN = 'cn=task-' + str(int(time.time())) + ',' + DN_MBO_TASK
+    try:
+        inst.add_s(Entry((TASK_DN, {
+                          'objectclass': 'top extensibleObject'.split(),
+                          'basedn': DEFAULT_SUFFIX + "bad",
+                          'filter': 'objectclass=top'})))
+    except ldap.LDAPError as e:
+        log.fatal('test_memberof: Failed to add task(bad dn): error ' +
+                  e.message['desc'])
+        assert False
+
+    exitcode = wait_for_task(inst, TASK_DN)
+    if exitcode == "0":
+        # We should an error
+        log.fatal('test_memberof: Task with invalid DN still reported success')
+        assert False
+
+    #
+    # Test invalid DN syntax
+    #
+    TASK_DN = 'cn=task-' + str(int(time.time())) + ',' + DN_MBO_TASK
+    try:
+        inst.add_s(Entry((TASK_DN, {
+                          'objectclass': 'top extensibleObject'.split(),
+                          'basedn': "bad",
+                          'filter': 'objectclass=top'})))
+    except ldap.LDAPError as e:
+        log.fatal('test_memberof: Failed to add task(invalid dn syntax): ' +
+                  e.message['desc'])
+        assert False
+
+    exitcode = wait_for_task(inst, TASK_DN)
+    if exitcode == "0":
+        # We should an error
+        log.fatal('test_memberof: Task with invalid DN syntax still reported' +
+                  ' success')
+        assert False
+
+    #
+    # Test bad filter (missing closing parenthesis)
+    #
+    TASK_DN = 'cn=task-' + str(int(time.time())) + ',' + DN_MBO_TASK
+    try:
+        inst.add_s(Entry((TASK_DN, {
+                          'objectclass': 'top extensibleObject'.split(),
+                          'basedn': DEFAULT_SUFFIX,
+                          'filter': '(objectclass=top'})))
+    except ldap.LDAPError as e:
+        log.fatal('test_memberof: Failed to add task(bad filter: error ' +
+                  e.message['desc'])
+        assert False
+
+    exitcode = wait_for_task(inst, TASK_DN)
+    if exitcode == "0":
+        # We should an error
+        log.fatal('test_memberof: Task with invalid filter still reported ' +
+                  'success')
+        assert False
+
+    ####################################################
+    # Test fixup works
+    ####################################################
+
+    #
+    # Run the task and validate that it worked
+    #
     TASK_DN = 'cn=task-' + str(int(time.time())) + ',' + DN_MBO_TASK
     try:
         inst.add_s(Entry((TASK_DN, {

dirsrvtests/tests/suites/dynamic-plugins/stress_tests.py

@@ -88,8 +88,9 @@ class DelUsers(threading.Thread):
             try:
                 conn.delete_s(USER_DN)
             except ldap.LDAPError as e:
-                log.fatal('DeleteUsers: failed to delete (' + USER_DN + ') error: ' + e.message['desc'])
-                assert False
+                if e == ldap.UNAVAILABLE or e == ldap.SERVER_DOWN:
+                    log.fatal('DeleteUsers: failed to delete (' + USER_DN + ') error: ' + e.message['desc'])
+                    assert False
 
             idx += 1
 
@@ -115,11 +116,10 @@ class AddUsers(threading.Thread):
                 conn.add_s(Entry((GROUP_DN,
                     {'objectclass': 'top groupOfNames groupOfUniqueNames extensibleObject'.split(),
                      'uid': 'user' + str(idx)})))
-            except ldap.ALREADY_EXISTS:
-                pass
             except ldap.LDAPError as e:
-                log.fatal('AddUsers: failed to add group (' + USER_DN + ') error: ' + e.message['desc'])
-                assert False
+                if e == ldap.UNAVAILABLE or e == ldap.SERVER_DOWN:
+                    log.fatal('AddUsers: failed to add group (' + USER_DN + ') error: ' + e.message['desc'])
+                    assert False
 
         log.info('AddUsers - Adding ' + str(NUM_USERS) + ' entries (' + self.rdnval + ')...')
 
@@ -129,16 +129,18 @@ class AddUsers(threading.Thread):
                 conn.add_s(Entry((USER_DN, {'objectclass': 'top extensibleObject'.split(),
                            'uid': 'user' + str(idx)})))
             except ldap.LDAPError as e:
-                log.fatal('AddUsers: failed to add (' + USER_DN + ') error: ' + e.message['desc'])
-                assert False
+                if e == ldap.UNAVAILABLE or e == ldap.SERVER_DOWN:
+                    log.fatal('AddUsers: failed to add (' + USER_DN + ') error: ' + e.message['desc'])
+                    assert False
 
             if self.addToGroup:
                 # Add the user to the group
                 try:
                     conn.modify_s(GROUP_DN, [(ldap.MOD_ADD, 'uniquemember', USER_DN)])
                 except ldap.LDAPError as e:
-                    log.fatal('AddUsers: Failed to add user' + USER_DN + ' to group: error ' + e.message['desc'])
-                    assert False
+                    if e == ldap.UNAVAILABLE or e == ldap.SERVER_DOWN:
+                        log.fatal('AddUsers: Failed to add user' + USER_DN + ' to group: error ' + e.message['desc'])
+                        assert False
 
             idx += 1
 

dirsrvtests/tests/suites/dynamic-plugins/test_dynamic_plugins.py

@@ -250,7 +250,8 @@ def test_dynamic_plugins(topology_st):
 
             except:
                 log.info('Stress test failed!')
-                repl_fail(replica_inst)
+                if replication_run:
+                    repl_fail(replica_inst)
 
             stress_count += 1
             log.info('####################################################################')

ldap/servers/plugins/memberof/memberof.c

@@ -68,6 +68,13 @@ typedef struct _memberof_get_groups_data
         Slapi_ValueSet **group_norm_vals;
 } memberof_get_groups_data;
 
+typedef struct _task_data
+{
+	char *dn;
+	char *bind_dn;
+	char *filter_str;
+} task_data;
+
 /*** function prototypes ***/
 
 /* exported functions */
@@ -145,7 +152,7 @@ static void memberof_task_destructor(Slapi_Task *task);
 static const char *fetch_attr(Slapi_Entry *e, const char *attrname,
                                               const char *default_val);
 static void memberof_fixup_task_thread(void *arg);
-static int memberof_fix_memberof(MemberOfConfig *config, char *dn, char *filter_str);
+static int memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td);
 static int memberof_fix_memberof_callback(Slapi_Entry *e, void *callback_data);
 static int memberof_entry_in_scope(MemberOfConfig *config, Slapi_DN *sdn);
 static int memberof_add_objectclass(char *auto_add_oc, const char *dn);
@@ -2641,13 +2648,6 @@ void memberof_unlock()
 	}
 }
 
-typedef struct _task_data
-{
-	char *dn;
-	char *bind_dn;
-	char *filter_str;
-} task_data;
-
 void memberof_fixup_task_thread(void *arg)
 {
 	MemberOfConfig configCopy = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
@@ -2661,7 +2661,7 @@ void memberof_fixup_task_thread(void *arg)
 	}
 	slapi_task_inc_refcount(task);
 	slapi_log_err(SLAPI_LOG_PLUGIN, MEMBEROF_PLUGIN_SUBSYSTEM,
-	                "memberof_fixup_task_thread - refcount incremented.\n" );
+		"memberof_fixup_task_thread - refcount incremented.\n" );
 	/* Fetch our task data from the task */
 	td = (task_data *)slapi_task_get_data(task);
 
@@ -2672,7 +2672,8 @@ void memberof_fixup_task_thread(void *arg)
 	slapi_task_log_notice(task, "Memberof task starts (arg: %s) ...\n", 
 	                      td->filter_str);
 	slapi_log_err(SLAPI_LOG_INFO, MEMBEROF_PLUGIN_SUBSYSTEM,
-	                "memberof_fixup_task_thread - Memberof task starts (arg: %s) ...\n", td->filter_str);
+		"memberof_fixup_task_thread - Memberof task starts (filter: \"%s\") ...\n",
+		td->filter_str);
 
 	/* We need to get the config lock first.  Trying to get the
 	 * config lock after we already hold the op lock can cause
@@ -2687,7 +2688,7 @@ void memberof_fixup_task_thread(void *arg)
 
 	if (usetxn) {
 		Slapi_DN *sdn = slapi_sdn_new_dn_byref(td->dn);
-		Slapi_Backend *be = slapi_be_select(sdn);
+		Slapi_Backend *be = slapi_be_select_exact(sdn);
 		slapi_sdn_free(&sdn);
 		if (be) {
 			fixup_pb = slapi_pblock_new();
@@ -2695,12 +2696,17 @@ void memberof_fixup_task_thread(void *arg)
 			rc = slapi_back_transaction_begin(fixup_pb);
 			if (rc) {
 				slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
-				  "memberof_fixup_task_thread - Failed to start transaction\n");
+					"memberof_fixup_task_thread - Failed to start transaction\n");
+				goto done;
 			}
 		} else {
 			slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
-			  "memberof_fixup_task_thread - Failed to get be backend from %s\n",
-			  td->dn);
+				"memberof_fixup_task_thread - Failed to get be backend from (%s)\n",
+				td->dn);
+			slapi_task_log_notice(task, "Memberof task - Failed to get be backend from (%s)\n",
+				td->dn);
+			rc = -1;
+			goto done;
 		}
 	}
 
@@ -2708,13 +2714,14 @@ void memberof_fixup_task_thread(void *arg)
 	memberof_lock();
 
 	/* do real work */
-	rc = memberof_fix_memberof(&configCopy, td->dn, td->filter_str);
+	rc = memberof_fix_memberof(&configCopy, task, td);
  
 	/* release the memberOf operation lock */
 	memberof_unlock();
 
+done:
 	if (usetxn && fixup_pb) {
-		if (rc) { /* failes */
+		if (rc) { /* failed */
 			slapi_back_transaction_abort(fixup_pb);
 		} else {
 			slapi_back_transaction_commit(fixup_pb);
@@ -2726,8 +2733,6 @@ void memberof_fixup_task_thread(void *arg)
 	slapi_task_log_notice(task, "Memberof task finished.");
 	slapi_task_log_status(task, "Memberof task finished.");
 	slapi_task_inc_progress(task);
-	slapi_log_err(SLAPI_LOG_INFO, MEMBEROF_PLUGIN_SUBSYSTEM,
-	                "memberof_fixup_task_thread - Memberof task finished (arg: %s) ...\n", td->filter_str);
 
 	/* this will queue the destruction of the task */
 	slapi_task_finish(task, rc);
@@ -2845,13 +2850,13 @@ memberof_task_destructor(Slapi_Task *task)
 		"memberof_task_destructor <--\n" );
 }
 
-int memberof_fix_memberof(MemberOfConfig *config, char *dn, char *filter_str)
+int memberof_fix_memberof(MemberOfConfig *config, Slapi_Task *task, task_data *td)
 {
 	int rc = 0;
 	Slapi_PBlock *search_pb = slapi_pblock_new();
 
-	slapi_search_internal_set_pb(search_pb, dn,
-		LDAP_SCOPE_SUBTREE, filter_str, 0, 0,
+	slapi_search_internal_set_pb(search_pb, td->dn,
+		LDAP_SCOPE_SUBTREE, td->filter_str, 0, 0,
 		0, 0,
 		memberof_get_plugin_id(),
 		0);	
@@ -2860,6 +2865,16 @@ int memberof_fix_memberof(MemberOfConfig *config, char *dn, char *filter_str)
 		config,
 		0, memberof_fix_memberof_callback,
 		0);
+	if (rc){
+		char *errmsg;
+		int result;
+
+		slapi_pblock_get(search_pb, SLAPI_PLUGIN_INTOP_RESULT, &result);
+		errmsg = ldap_err2string(result);
+		slapi_log_err(SLAPI_LOG_ERR, MEMBEROF_PLUGIN_SUBSYSTEM,
+			"memberof_fix_memberof - Failed (%s)\n", errmsg );
+		slapi_task_log_notice(task, "Memberof task failed (%s)\n", errmsg );
+	}
 
 	slapi_pblock_destroy(search_pb);
 

ldap/servers/slapd/plugin_internal_op.c

@@ -726,7 +726,7 @@ search_internal_callback_pb (Slapi_PBlock *pb, void *callback_data,
 	if (ifstr == NULL || (scope != LDAP_SCOPE_BASE && scope != LDAP_SCOPE_ONELEVEL 
         && scope != LDAP_SCOPE_SUBTREE))
     {
-        opresult =  LDAP_PARAM_ERROR;
+        opresult = LDAP_PARAM_ERROR;
         slapi_pblock_set(pb, SLAPI_PLUGIN_INTOP_RESULT, &opresult);
         return -1;
     }
@@ -743,19 +743,19 @@ search_internal_callback_pb (Slapi_PBlock *pb, void *callback_data,
     op->o_search_referral_handler = internal_ref_entry_callback;
 	
     filter = slapi_str2filter((fstr = slapi_ch_strdup(ifstr)));
-    if(scope == LDAP_SCOPE_BASE) {
-        filter->f_flags |= (SLAPI_FILTER_LDAPSUBENTRY |
-                            SLAPI_FILTER_TOMBSTONE | SLAPI_FILTER_RUV);
+    if (NULL == filter) {
+        int result = LDAP_FILTER_ERROR;
+        send_ldap_result(pb, result, NULL, NULL, 0, NULL);
+        slapi_pblock_set(pb, SLAPI_PLUGIN_INTOP_RESULT, &result);
+        rc = -1;
+        goto done;
     }
 
-    if (NULL == filter) 
-	{
-    	send_ldap_result(pb, LDAP_FILTER_ERROR, NULL, NULL, 0, NULL);
-		rc = -1;
-    	goto done;
+    if (scope == LDAP_SCOPE_BASE) {
+        filter->f_flags |= (SLAPI_FILTER_LDAPSUBENTRY |
+                            SLAPI_FILTER_TOMBSTONE | SLAPI_FILTER_RUV);
     }
     filter_normalize(filter);
-
     slapi_pblock_set(pb, SLAPI_SEARCH_FILTER, filter);
 	slapi_pblock_set(pb, SLAPI_REQCONTROLS, controls);
 
@@ -783,11 +783,8 @@ search_internal_callback_pb (Slapi_PBlock *pb, void *callback_data,
     slapi_pblock_get(pb, SLAPI_SEARCH_FILTER, &filter);
 
 done:
-    slapi_ch_free((void **) & fstr);
-    if (filter != NULL) 
-    {
-        slapi_filter_free(filter, 1 /* recurse */);
-    }
+    slapi_ch_free_string(&fstr);
+    slapi_filter_free(filter, 1 /* recurse */);
     slapi_pblock_get(pb, SLAPI_SEARCH_ATTRS, &tmp_attrs);
     slapi_ch_array_free(tmp_attrs);
     slapi_pblock_set(pb, SLAPI_SEARCH_ATTRS, NULL);