Ticket 47622 - Automember betxnpreoperation - transaction not aborted when group entry does not exist

Bug Description:  If the group defined in the automember plugin does not exist, than any add operation
                  that should trigger an update, succeeds even though the automember update failed.

Fix Description:  Return an error if a automember post operation update fails - previously we always
                  returned success.

                  Updated plugin_call_func() to check the result of betxn postop plugins.

                  Also added return text to the result message when a betxn plugin fails.  This is
                  useful for clients to explain why the operation failed.

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

Jenkins: passed
Valgrind: passed
Coverity: passed

Reviewed by: rmeggins(Thanks!)

ldap/servers/plugins/automember/automember.c

@@ -103,8 +103,8 @@ static struct automemberRegexRule *automember_parse_regex_rule(char *rule_string
 static void automember_free_regex_rule(struct automemberRegexRule *rule);
 static int automember_parse_grouping_attr(char *value, char **grouping_attr,
     char **grouping_value);
-static void automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileDesc *ldif_fd);
-static void automember_add_member_value(Slapi_Entry *member_e, const char *group_dn,
+static int automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileDesc *ldif_fd);
+static int automember_add_member_value(Slapi_Entry *member_e, const char *group_dn,
     char *grouping_attr, char *grouping_value, PRFileDesc *ldif_fd);
 const char *fetch_attr(Slapi_Entry *e, const char *attrname, const char *default_val);
 
@@ -1401,7 +1401,7 @@ automember_parse_grouping_attr(char *value, char **grouping_attr, char **groupin
  * Determines which target groups need to be updated according to
  * the rules in config, then performs the updates.
  */
-static void
+static int
 automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileDesc *ldif_fd)
 {
     PRCList *rule = NULL;
@@ -1412,10 +1412,11 @@ automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileD
     Slapi_DN *last = NULL;
     PRCList *curr_exclusion = NULL;
     char **vals = NULL;
+    int rc = 0;
     int i = 0;
 
     if (!config || !e) {
-        return;
+        return -1;
     }
 
     slapi_log_error(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,
@@ -1555,15 +1556,23 @@ automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileD
     if (PR_CLIST_IS_EMPTY(&targets)) {
         /* Add to each default group. */
         for (i = 0; config->default_groups && config->default_groups[i]; i++) {
-            automember_add_member_value(e, config->default_groups[i],
-                                        config->grouping_attr, config->grouping_value, ldif_fd);
+            if(automember_add_member_value(e, config->default_groups[i], config->grouping_attr,
+                                           config->grouping_value, ldif_fd))
+            {
+                rc = SLAPI_PLUGIN_FAILURE;
+                goto out;
+            }
         }
     } else {
         /* Update the target groups. */
         dnitem = (struct automemberDNListItem *)PR_LIST_HEAD(&targets);
         while ((PRCList *)dnitem != &targets) {
-            automember_add_member_value(e, slapi_sdn_get_dn(dnitem->dn),
-                                        config->grouping_attr, config->grouping_value, ldif_fd);
+            if(automember_add_member_value(e, slapi_sdn_get_dn(dnitem->dn),config->grouping_attr,
+                                           config->grouping_value, ldif_fd))
+            {
+                rc = SLAPI_PLUGIN_FAILURE;
+                goto out;
+            }
             dnitem = (struct automemberDNListItem *)PR_NEXT_LINK((PRCList *)dnitem);
         }
     }
@@ -1582,6 +1591,9 @@ automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileD
             slapi_ch_free((void**)&dnitem);
     }
 
+out:
+
+    return rc;
 }
 
 /*
@@ -1589,7 +1601,7 @@ automember_update_membership(struct configEntry *config, Slapi_Entry *e, PRFileD
  *
  * Adds a member entry to a group.
  */
-static void
+static int
 automember_add_member_value(Slapi_Entry *member_e, const char *group_dn, char *grouping_attr,
                             char *grouping_value, PRFileDesc *ldif_fd)
 {
@@ -1600,6 +1612,7 @@ automember_add_member_value(Slapi_Entry *member_e, const char *group_dn, char *g
     char *vals[2];
     char *member_value = NULL;
     int freeit = 0;
+    int rc = 0;
 
     /* If grouping_value is dn, we need to fetch the dn instead. */
     if (slapi_attr_type_cmp(grouping_value, "dn", SLAPI_TYPE_CMP_EXACT) == 0) {
@@ -1649,6 +1662,7 @@ automember_add_member_value(Slapi_Entry *member_e, const char *group_dn, char *g
                             "a \"%s\" value to group \"%s\" (%s).\n", 
                             member_value, grouping_attr, group_dn,
                             ldap_err2string(result));
+            rc = result;
         }
     } else {
         slapi_log_error(SLAPI_LOG_FATAL, AUTOMEMBER_PLUGIN_SUBSYSTEM,
@@ -1662,8 +1676,9 @@ out:
     if (freeit) {
         slapi_ch_free_string(&member_value);
     }
-
     slapi_pblock_destroy(mod_pb);
+
+    return rc;
 }
 
 
@@ -1833,6 +1848,7 @@ automember_add_post_op(Slapi_PBlock *pb)
     Slapi_DN *sdn = NULL;
     struct configEntry *config = NULL;
     PRCList *list = NULL;
+    int rc = SLAPI_PLUGIN_SUCCESS;
 
     slapi_log_error(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,
                     "--> automember_add_post_op\n");
@@ -1848,8 +1864,9 @@ automember_add_post_op(Slapi_PBlock *pb)
         }
     } else {
         slapi_log_error(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,
-                        "automember_add_post_op: Error "
-                        "retrieving dn\n");
+                        "automember_add_post_op: Error retrieving dn\n");
+
+        rc = SLAPI_PLUGIN_FAILURE;
         goto bail;
     }
 
@@ -1863,12 +1880,11 @@ automember_add_post_op(Slapi_PBlock *pb)
 
     if (e) {
         /* If the entry is a tombstone, just bail. */
-        Slapi_Value *tombstone =
-                        slapi_value_new_string(SLAPI_ATTR_VALUE_TOMBSTONE);
-        int rc = slapi_entry_attr_has_syntax_value(e, SLAPI_ATTR_OBJECTCLASS,
-                                                   tombstone);
+        Slapi_Value *tombstone = slapi_value_new_string(SLAPI_ATTR_VALUE_TOMBSTONE);
+        int is_tombstone = slapi_entry_attr_has_syntax_value(e, SLAPI_ATTR_OBJECTCLASS,
+                                                             tombstone);
         slapi_value_free(&tombstone);
-        if (rc) {
+        if (is_tombstone) {
             return SLAPI_PLUGIN_SUCCESS;
         }
 
@@ -1891,7 +1907,10 @@ automember_add_post_op(Slapi_PBlock *pb)
                 if (slapi_dn_issuffix(slapi_sdn_get_dn(sdn), config->scope) &&
                     (slapi_filter_test_simple(e, config->filter) == 0)) {
                     /* Find out what membership changes are needed and make them. */
-                    automember_update_membership(config, e, NULL);
+                    if(automember_update_membership(config, e, NULL)){
+                        rc = SLAPI_PLUGIN_FAILURE;
+                        break;
+                    }
                 }
 
                 list = PR_NEXT_LINK(list);
@@ -1904,11 +1923,21 @@ automember_add_post_op(Slapi_PBlock *pb)
                         "automember_add_post_op: Error "
                         "retrieving post-op entry %s\n", slapi_sdn_get_dn(sdn));
     }
+
 bail:
     slapi_log_error(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,
-                    "<-- automember_add_post_op\n");
+                    "<-- automember_add_post_op (%d)\n", rc);
 
-    return SLAPI_PLUGIN_SUCCESS;
+    if(rc){
+        char errtxt[SLAPI_DSE_RETURNTEXT_SIZE];
+        int result = LDAP_UNWILLING_TO_PERFORM;
+
+        PR_snprintf(errtxt, SLAPI_DSE_RETURNTEXT_SIZE, "Automember Plugin update unexpectedly failed.\n");
+        slapi_pblock_set(pb, SLAPI_RESULT_CODE, &result);
+        slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, &errtxt);
+    }
+
+    return rc;
 }
 
 /*
@@ -2216,7 +2245,11 @@ void automember_rebuild_task_thread(void *arg){
                 if (slapi_dn_issuffix(slapi_entry_get_dn(entries[i]), config->scope) &&
                     (slapi_filter_test_simple(entries[i], config->filter) == 0))
                 {
-                    automember_update_membership(config, entries[i], NULL);
+                    if(automember_update_membership(config, entries[i], NULL)){
+                        result = SLAPI_PLUGIN_FAILURE;
+                        automember_config_unlock();
+                        goto out;
+                    }
                 }
                 list = PR_NEXT_LINK(list);
             }
@@ -2417,7 +2450,7 @@ void automember_export_task_thread(void *arg){
         /* make sure the plugin is still up, as this loop could run for awhile */
         if (!g_plugin_started) {
             automember_config_unlock();
-            result = -1;
+            result = SLAPI_DSE_CALLBACK_ERROR;
             goto out;
         }
         if (!PR_CLIST_IS_EMPTY(g_automember_config)) {
@@ -2427,7 +2460,11 @@ void automember_export_task_thread(void *arg){
                 if (slapi_dn_issuffix(slapi_sdn_get_dn(td->base_dn), config->scope) &&
                     (slapi_filter_test_simple(entries[i], config->filter) == 0))
                 { 
-                    automember_update_membership(config, entries[i], ldif_fd);
+                    if(automember_update_membership(config, entries[i], ldif_fd)){
+                        result = SLAPI_DSE_CALLBACK_ERROR;
+                        automember_config_unlock();
+                        goto out;
+                    }
                 }
                 list = PR_NEXT_LINK(list);
             }
@@ -2627,7 +2664,13 @@ void automember_map_task_thread(void *arg){
                     if (slapi_dn_issuffix(slapi_entry_get_dn_const(e), config->scope) &&
                         (slapi_filter_test_simple(e, config->filter) == 0))
                     {
-                        automember_update_membership(config, e, ldif_fd_out);
+                        if(automember_update_membership(config, e, ldif_fd_out)){
+                            result = SLAPI_DSE_CALLBACK_ERROR;
+                            slapi_entry_free(e);
+                            slapi_ch_free_string(&entrystr);
+                            automember_config_unlock();
+                            goto out;
+                        }
                     }
                     list = PR_NEXT_LINK(list);
                 }
@@ -2638,7 +2681,7 @@ void automember_map_task_thread(void *arg){
             slapi_task_log_notice(task, "Automember map task, skipping invalid entry.");
             slapi_task_log_status(task, "Automember map task, skipping invalid entry.");
         }
-        slapi_ch_free((void **)&entrystr);
+        slapi_ch_free_string(&entrystr);
     }
     automember_config_unlock();
 
@@ -2671,6 +2714,7 @@ automember_modrdn_post_op(Slapi_PBlock *pb)
     Slapi_DN *new_sdn = NULL;
     struct configEntry *config = NULL;
     PRCList *list = NULL;
+    int rc = SLAPI_PLUGIN_SUCCESS;
 
     slapi_log_error(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,
                     "--> automember_modrdn_post_op\n");
@@ -2691,7 +2735,7 @@ automember_modrdn_post_op(Slapi_PBlock *pb)
         slapi_log_error(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,
                         "automember_modrdn_post_op: Error "
                         "retrieving post-op entry\n");
-        return SLAPI_PLUGIN_SUCCESS;
+        return SLAPI_PLUGIN_FAILURE;
     }
 
     if ((old_sdn = automember_get_sdn(pb))) {
@@ -2702,7 +2746,7 @@ automember_modrdn_post_op(Slapi_PBlock *pb)
         slapi_log_error(SLAPI_LOG_PLUGIN, AUTOMEMBER_PLUGIN_SUBSYSTEM,
                         "automember_modrdn_post_op: Error "
                         "retrieving dn\n");
-        return SLAPI_PLUGIN_SUCCESS;
+        return SLAPI_PLUGIN_FAILURE;
     }
 
     /* If replication, just bail. */
@@ -2730,7 +2774,10 @@ automember_modrdn_post_op(Slapi_PBlock *pb)
             if (slapi_dn_issuffix(slapi_sdn_get_dn(new_sdn), config->scope) &&
                 (slapi_filter_test_simple(post_e, config->filter) == 0)) {
                 /* Find out what membership changes are needed and make them. */
-                automember_update_membership(config, post_e, NULL);
+                if(automember_update_membership(config, post_e, NULL)){
+                    rc = SLAPI_PLUGIN_FAILURE;
+                    break;
+                }
             }
 
             list = PR_NEXT_LINK(list);
@@ -2739,9 +2786,19 @@ automember_modrdn_post_op(Slapi_PBlock *pb)
 
     automember_config_unlock();
 
+    if(rc){
+        char errtxt[SLAPI_DSE_RETURNTEXT_SIZE];
+        int result = LDAP_UNWILLING_TO_PERFORM;
+
+        PR_snprintf(errtxt, SLAPI_DSE_RETURNTEXT_SIZE, "Automember Plugin update unexpectedly failed.  "
+                    "Please see the server errors log for more information.\n");
+        slapi_pblock_set(pb, SLAPI_RESULT_CODE, &result);
+        slapi_pblock_set(pb, SLAPI_PB_RESULT_TEXT, &errtxt);
+    }
+
     slapi_log_error(SLAPI_LOG_TRACE, AUTOMEMBER_PLUGIN_SUBSYSTEM,
-                    "<-- automember_modrdn_post_op\n");
+                    "<-- automember_modrdn_post_op (%d)\n", rc);
 
-    return SLAPI_PLUGIN_SUCCESS;
+    return rc;
 }
 

ldap/servers/slapd/back-ldbm/ldbm_add.c

@@ -357,7 +357,7 @@ ldbm_back_add( Slapi_PBlock *pb )
 						/* make sure opreturn is set for the postop plugins */
 						slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &rc);
 					}
-
+					slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 					goto error_return;
 				}
 				/*
@@ -795,6 +795,7 @@ ldbm_back_add( Slapi_PBlock *pb )
 			if (!opreturn) {
 				slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
 			}
+			slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 			goto error_return;
 		}
 
@@ -1046,6 +1047,7 @@ ldbm_back_add( Slapi_PBlock *pb )
 		if (!opreturn) {
 			slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
 		}
+		slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 		goto error_return;
 	}
 

ldap/servers/slapd/back-ldbm/ldbm_delete.c

@@ -325,6 +325,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 					if (!opreturn) {
 						slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &rc );
 					}
+					slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 					goto error_return;
 				}
 				/* the flag could be set in a preop plugin (e.g., USN) */
@@ -354,6 +355,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 									  ldap_result_code ?
 									  &ldap_result_code : &retval );
 				}
+				slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 				goto error_return;
 			}
 
@@ -603,6 +605,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 					                  ldap_result_code ?
 					                  &ldap_result_code : &retval );
 				}
+				slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 				goto error_return;
 			}
 		}
@@ -633,6 +636,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 					                  &ldap_result_code : &rc );
 				}
 				/* retval is -1 */
+				slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 				goto error_return;
 			}
 			slapi_pblock_set( pb, SLAPI_DELETE_BEPREOP_ENTRY, orig_entry );
@@ -1105,6 +1109,7 @@ ldbm_back_delete( Slapi_PBlock *pb )
 		if (!opreturn) {
 			slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, &retval );
 		}
+		slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 		goto error_return;
 	}
 

ldap/servers/slapd/back-ldbm/ldbm_modify.c

@@ -623,6 +623,7 @@ ldbm_back_modify( Slapi_PBlock *pb )
 			if (!opreturn) {
 				slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
 			}
+			slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 			goto error_return;
 		}
 
@@ -793,6 +794,7 @@ ldbm_back_modify( Slapi_PBlock *pb )
 		if (!opreturn) {
 			slapi_pblock_set(pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval);
 		}
+		slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
 		goto error_return;
 	}
 

ldap/servers/slapd/back-ldbm/ldbm_modrdn.c

@@ -466,6 +466,7 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
                     if (!opreturn) {
                         slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &rc );
                     }
+                    slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
                     goto error_return;
                 }
                 /*
@@ -890,6 +891,7 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
             if (!opreturn) {
                 slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval );
             }
+            slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
             goto error_return;
         }
 
@@ -1130,6 +1132,7 @@ ldbm_back_modrdn( Slapi_PBlock *pb )
         if (!opreturn) {
             slapi_pblock_set( pb, SLAPI_PLUGIN_OPRETURN, ldap_result_code ? &ldap_result_code : &retval );
         }
+        slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &ldap_result_message);
         goto error_return;
     }
 

ldap/servers/slapd/plugin.c

@@ -1476,7 +1476,8 @@ plugin_call_func (struct slapdplugin *list, int operation, Slapi_PBlock *pb, int
 				}
 				else if (SLAPI_PLUGIN_BEPREOPERATION == list->plg_type ||
 				         SLAPI_PLUGIN_BETXNPREOPERATION == list->plg_type ||
-				         SLAPI_PLUGIN_BEPOSTOPERATION == list->plg_type)
+				         SLAPI_PLUGIN_BEPOSTOPERATION == list->plg_type ||
+				         SLAPI_PLUGIN_BETXNPOSTOPERATION == list->plg_type )
 				{
 					/* 
 					 * respect fatal error SLAPI_PLUGIN_FAILURE (-1);