From: Alan T. DeKok Date: Thu, 14 Dec 2023 13:53:08 +0000 (-0500) Subject: make edits return fail when aborting the edit changes X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8b8feb65129f04ce3f5e3be75242a4373c284c4c;p=thirdparty%2Ffreeradius-server.git make edits return fail when aborting the edit changes make transactions default to "fail=1", which is much more useful than bailing (for now). remove auto-grouping of edits in a "group", and require the use of "transaction" Clean up the tests so that they pass under the new framework update documentation to match --- diff --git a/doc/antora/modules/reference/nav.adoc b/doc/antora/modules/reference/nav.adoc index 0291272a1f8..25df91df3e8 100644 --- a/doc/antora/modules/reference/nav.adoc +++ b/doc/antora/modules/reference/nav.adoc @@ -29,6 +29,7 @@ **** xref:unlang/subrequest.adoc[subrequest] **** xref:unlang/switch.adoc[switch] **** xref:unlang/timeout.adoc[timeout] +**** xref:unlang/transaction.adoc[transaction] **** xref:unlang/update.adoc[update] *** xref:unlang/module.adoc[Modules] diff --git a/doc/antora/modules/reference/pages/unlang/group.adoc b/doc/antora/modules/reference/pages/unlang/group.adoc index a4b6141e4b2..fb92488c0b5 100644 --- a/doc/antora/modules/reference/pages/unlang/group.adoc +++ b/doc/antora/modules/reference/pages/unlang/group.adoc @@ -37,12 +37,5 @@ group { The last entry in a `group` section can also be an xref:unlang/actions.adoc[actions] subsection. -== Grouping Edits - -The `group` keyword can also be used to group multiple -xref:unlang/edit.adoc[edit] instructions. When edit instructions are -grouped, then the edits are "atomic". That is, either all of the -edits succeed, or none of them do. - // Copyright (C) 2021 Network RADIUS SAS. Licenced under CC-by-NC 4.0. // This documentation was developed by Network RADIUS SAS. diff --git a/src/lib/unlang/compile.c b/src/lib/unlang/compile.c index b3af9d0f815..8fc3fb42304 100644 --- a/src/lib/unlang/compile.c +++ b/src/lib/unlang/compile.c @@ -94,7 +94,6 @@ typedef struct { rlm_components_t component; char const *section_name1; char const *section_name2; - bool all_edits; unlang_actions_t actions; tmpl_rules_t const *rules; } unlang_compile_t; @@ -1443,13 +1442,10 @@ static int unlang_fixup_edit(map_t *map, void *ctx) } /** Compile one edit section. - * - * Edits which are adjacent to one another are automatically merged - * into one larger edit transaction. */ -static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang_ctx, unlang_t **prev, CONF_SECTION *cs) +static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_SECTION *cs) { - unlang_edit_t *edit, *edit_free; + unlang_edit_t *edit; unlang_t *c, *out = UNLANG_IGNORE; map_t *map; char const *name; @@ -1484,29 +1480,20 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang t_rules.attr.allow_unknown = true; RULES_VERIFY(&t_rules); - c = *prev; - edit = edit_free = NULL; - - if (c && (c->type == UNLANG_TYPE_EDIT)) { - edit = unlang_generic_to_edit(c); - - } else { - edit = talloc_zero(parent, unlang_edit_t); - if (!edit) return NULL; + edit = talloc_zero(parent, unlang_edit_t); + if (!edit) return NULL; - c = out = unlang_edit_to_generic(edit); - c->parent = parent; - c->next = NULL; - c->name = cf_section_name1(cs); - c->debug_name = c->name; - c->type = UNLANG_TYPE_EDIT; - c->ci = CF_TO_ITEM(cs); + c = out = unlang_edit_to_generic(edit); + c->parent = parent; + c->next = NULL; + c->name = cf_section_name1(cs); + c->debug_name = c->name; + c->type = UNLANG_TYPE_EDIT; + c->ci = CF_TO_ITEM(cs); - map_list_init(&edit->maps); - edit_free = edit; + map_list_init(&edit->maps); - compile_action_defaults(c, unlang_ctx); - } + compile_action_defaults(c, unlang_ctx); /* * Allocate the map and initialize it. @@ -1522,7 +1509,7 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang if (slen <= 0) { cf_log_err(cs, "Failed parsing list reference %s - %s", name, fr_strerror()); fail: - talloc_free(edit_free); + talloc_free(edit); return NULL; } @@ -1614,18 +1601,15 @@ static unlang_t *compile_edit_section(unlang_t *parent, unlang_compile_t *unlang map_list_insert_tail(&edit->maps, map); - *prev = c; return out; } /** Compile one edit pair * - * Edits which are adjacent to one another are automatically merged - * into one larger edit transaction. */ -static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ctx, unlang_t **prev, CONF_PAIR *cp) +static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_PAIR *cp) { - unlang_edit_t *edit, *edit_free; + unlang_edit_t *edit; unlang_t *c = NULL, *out = UNLANG_IGNORE; map_t *map; int num; @@ -1640,32 +1624,20 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct t_rules.attr.allow_unknown = true; RULES_VERIFY(&t_rules); - /* - * Auto-merge pairs only if they're all in a group. - */ - if (unlang_ctx->all_edits) c = *prev; - edit = edit_free = NULL; - - if (c && (c->type == UNLANG_TYPE_EDIT)) { - edit = unlang_generic_to_edit(c); + edit = talloc_zero(parent, unlang_edit_t); + if (!edit) return NULL; - } else { - edit = talloc_zero(parent, unlang_edit_t); - if (!edit) return NULL; - - c = out = unlang_edit_to_generic(edit); - c->parent = parent; - c->next = NULL; - c->name = cf_pair_attr(cp); - c->debug_name = c->name; - c->type = UNLANG_TYPE_EDIT; - c->ci = CF_TO_ITEM(cp); + c = out = unlang_edit_to_generic(edit); + c->parent = parent; + c->next = NULL; + c->name = cf_pair_attr(cp); + c->debug_name = c->name; + c->type = UNLANG_TYPE_EDIT; + c->ci = CF_TO_ITEM(cp); - map_list_init(&edit->maps); - edit_free = edit; + map_list_init(&edit->maps); - compile_action_defaults(c, unlang_ctx); - } + compile_action_defaults(c, unlang_ctx); op = cf_pair_operator(cp); if ((op == T_OP_CMP_TRUE) || (op == T_OP_CMP_FALSE)) { @@ -1679,7 +1651,7 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct */ if (map_afrom_cp(edit, &map, map_list_tail(&edit->maps), cp, &t_rules, NULL, true) < 0) { fail: - talloc_free(edit_free); + talloc_free(edit); return NULL; } @@ -1711,7 +1683,6 @@ static unlang_t *compile_edit_pair(unlang_t *parent, unlang_compile_t *unlang_ct map_list_insert_tail(&edit->maps, map); - *prev = c; return out; } @@ -2189,7 +2160,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct unlang_t *c, *single; bool was_if = false; char const *skip_else = NULL; - unlang_t *edit = NULL; unlang_compile_t *unlang_ctx; unlang_compile_t unlang_ctx2; tmpl_rules_t t_rules; @@ -2234,7 +2204,7 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct * In-line attribute editing. */ if (*name == '&') { - single = compile_edit_section(c, unlang_ctx, &edit, subcs); + single = compile_edit_section(c, unlang_ctx, subcs); if (!single) { talloc_free(c); return NULL; @@ -2243,11 +2213,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct goto add_child; } - /* - * Only merge edits if they're all in a group { ... } - */ - if (!unlang_ctx->all_edits) edit = NULL; - if (strcmp(name, "actions") == 0) { if (!compile_action_subsection(c, g->cs, subcs)) { talloc_free(c); @@ -2316,7 +2281,7 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct * In-line attribute editing is not supported. */ if (*attr == '&') { - single = compile_edit_pair(c, unlang_ctx, &edit, cp); + single = compile_edit_pair(c, unlang_ctx, cp); if (!single) { talloc_free(c); return NULL; @@ -2325,8 +2290,6 @@ static unlang_t *compile_children(unlang_group_t *g, unlang_compile_t *unlang_ct goto add_child; } - edit = NULL; /* no longer doing implicit merging of edits */ - /* * Variable definition. */ @@ -2493,10 +2456,6 @@ static unlang_t *compile_section(unlang_t *parent, unlang_compile_t *unlang_ctx, static unlang_t *compile_group(unlang_t *parent, unlang_compile_t *unlang_ctx, CONF_SECTION *cs) { - CONF_ITEM *ci = NULL; - bool all_edits = true; - unlang_compile_t unlang_ctx2; - static unlang_ext_t const group = { .type = UNLANG_TYPE_GROUP, .len = sizeof(unlang_group_t), @@ -2505,44 +2464,6 @@ static unlang_t *compile_group(unlang_t *parent, unlang_compile_t *unlang_ctx, C if (!cf_item_next(cs, NULL)) return UNLANG_IGNORE; - if (!unlang_ctx->all_edits) { - while ((ci = cf_item_next(cs, ci)) != NULL) { - char const *name; - - if (cf_item_is_section(ci)) { - CONF_SECTION *subcs; - - subcs = cf_item_to_section(ci); - name = cf_section_name1(subcs); - - } else if (cf_item_is_pair(ci)) { - CONF_PAIR *cp; - - cp = cf_item_to_pair(ci); - name = cf_pair_attr(cp); - - } else { - continue; - } - - if (*name != '&') { - all_edits = false; - break; - } - } - - /* - * The parent wasn't all edits, but we are. Set - * the "all edits" flag, and recurse. - */ - if (all_edits) { - compile_copy_context(&unlang_ctx2, unlang_ctx, unlang_ctx->component); - unlang_ctx2.all_edits = true; - - return compile_section(parent, &unlang_ctx2, cs, &group); - } - } - return compile_section(parent, unlang_ctx, cs, &group); } @@ -2564,10 +2485,8 @@ static fr_table_num_sorted_t transaction_keywords[] = { static int transaction_keywords_len = NUM_ELEMENTS(transaction_keywords); /** Limit the operations which can appear in a transaction. - * - * We probably want this to be */ -static bool transaction_ok(CONF_SECTION *cs, bool *all_edits) +static bool transaction_ok(CONF_SECTION *cs) { CONF_ITEM *ci = NULL; @@ -2589,9 +2508,7 @@ static bool transaction_ok(CONF_SECTION *cs, bool *all_edits) if (fr_table_value_by_str(transaction_keywords, name, -1) < 0) goto fail; - *all_edits = false; - - if (!transaction_ok(subcs, all_edits)) return false; + if (!transaction_ok(subcs)) return false; continue; @@ -2607,7 +2524,6 @@ static bool transaction_ok(CONF_SECTION *cs, bool *all_edits) * Allow rcodes via the "always" module. */ if (fr_table_value_by_str(mod_rcode_table, name, -1) >= 0) { - *all_edits = false; continue; } @@ -2631,7 +2547,6 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_ { unlang_group_t *g; unlang_t *c; - bool all_edits = true; unlang_compile_t unlang_ctx2; static unlang_ext_t const transaction = { @@ -2644,8 +2559,7 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_ * The transaction is empty, ignore it. */ if (!cf_item_next(cs, NULL)) return UNLANG_IGNORE; - - if (!transaction_ok(cs, &all_edits)) return NULL; + if (!transaction_ok(cs)) return NULL; /* * Any failure is return, not continue. @@ -2657,8 +2571,6 @@ static unlang_t *compile_transaction(unlang_t *parent, unlang_compile_t *unlang_ unlang_ctx2.actions.actions[RLM_MODULE_INVALID] = MOD_ACTION_RETURN; unlang_ctx2.actions.actions[RLM_MODULE_DISALLOW] = MOD_ACTION_RETURN; - unlang_ctx2.all_edits = all_edits; - /* * We always create a group, even if the section is empty. */ diff --git a/src/lib/unlang/edit.c b/src/lib/unlang/edit.c index a296cffd0c9..72fe938c298 100644 --- a/src/lib/unlang/edit.c +++ b/src/lib/unlang/edit.c @@ -1546,7 +1546,7 @@ static unlang_action_t process_edit(rlm_rcode_t *p_result, request_t *request, u if (state->ours) fr_edit_list_abort(state->el); TALLOC_FREE(frame->state); repeatable_clear(frame); - *p_result = RLM_MODULE_NOOP; + *p_result = RLM_MODULE_FAIL; return UNLANG_ACTION_CALCULATE_RESULT; } diff --git a/src/tests/keywords/edit-group b/src/tests/keywords/edit-group index a5b93f048dc..37f1d2c4bd1 100644 --- a/src/tests/keywords/edit-group +++ b/src/tests/keywords/edit-group @@ -6,7 +6,7 @@ # ALl of these edits are grouped. So if one of them fails, all of # them are rolled back. # -group { +transaction { &Tmp-String-0 := "foo" &Tmp-String-1 -= "bar" # fails, no existing Tmp-String-1 &Tmp-String-2 := "bar" @@ -27,7 +27,7 @@ if (&Tmp-String-2) { # # All of these succeed individually, so all of them should succeed. # -group { +transaction { &Tmp-String-0 := "foo" &Tmp-String-1 := "yup" &Tmp-String-2 := "bar" diff --git a/src/tests/keywords/ethernet b/src/tests/keywords/ethernet index c09bbc716d6..296a5a44f0c 100644 --- a/src/tests/keywords/ethernet +++ b/src/tests/keywords/ethernet @@ -22,8 +22,10 @@ if (!(&Tmp-Ethernet-0[2] == 00:11:22:33:44:56)) { # invalid assignment # this will silently fail with a no Module-Failure-Message. -&request += { - &Tmp-Ethernet-0 = %{Tmp-Ethernet-1[42]} +transaction { + &request += { + &Tmp-Ethernet-0 = %{Tmp-Ethernet-1[42]} + } } if (!(%{request.Tmp-Ethernet-0[#]} == 3)) { diff --git a/src/tests/keywords/immutable b/src/tests/keywords/immutable index aa269413196..82a5c522fa2 100644 --- a/src/tests/keywords/immutable +++ b/src/tests/keywords/immutable @@ -24,9 +24,9 @@ if !(&NAS-Port == 1813) { # # Try to edit it. The value shouldn't change. # -# @todo - we don't have run-time exceptions... maybe this should return "fail"? -# -&NAS-Port += 1 +transaction { + &NAS-Port += 1 +} if !(&NAS-Port == 1813) { test_fail } diff --git a/src/tests/keywords/policy.conf b/src/tests/keywords/policy.conf index 4a6c9c844d2..84516c140a5 100644 --- a/src/tests/keywords/policy.conf +++ b/src/tests/keywords/policy.conf @@ -47,7 +47,9 @@ with.dots { # Set the test to successful, but only if there are no failures. # success { - &parent.reply.Result-Status = "success" +# if (&parent.reply) { + &parent.reply.Result-Status = "success" +# } &reply.Result-Status = "success" ok