From: Alan T. DeKok <aland@freeradius.org>
Date: Thu, 2 Dec 2021 21:43:25 +0000 (-0500)
Subject: update conditional evaluator to return true/false
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5cb090addd3c2784a927a9446e883bbe156572fb;p=thirdparty%2Ffreeradius-server.git

update conditional evaluator to return true/false

there is no error.  "attribute not found" is "not match"
---

diff --git a/src/lib/server/cond_eval.c b/src/lib/server/cond_eval.c
index 878f848fcdc..674572e533d 100644
--- a/src/lib/server/cond_eval.c
+++ b/src/lib/server/cond_eval.c
@@ -514,7 +514,7 @@ static int cond_realize_attr(request_t *request, fr_value_box_t **realized, fr_v
 	return 0;
 }
 
-static int cond_compare_attrs(request_t *request, fr_value_box_t *lhs, map_t const *map)
+static bool cond_compare_attrs(request_t *request, fr_value_box_t *lhs, map_t const *map)
 {
 	int	       		rcode;
 	fr_pair_t		*vp;
@@ -547,10 +547,10 @@ static int cond_compare_attrs(request_t *request, fr_value_box_t *lhs, map_t con
 	}
 
 	tmpl_pair_cursor_clear(&cc);
-	return rcode;
+	return (rcode == 1);
 }
 
-static int cond_compare_virtual(request_t *request, map_t const *map)
+static bool cond_compare_virtual(request_t *request, map_t const *map)
 {
 	int	       		rcode;
 	fr_pair_t		*virt, *vp;
@@ -589,7 +589,7 @@ static int cond_compare_virtual(request_t *request, map_t const *map)
 	}
 
 	tmpl_pair_cursor_clear(&cc);
-	return rcode;
+	return (rcode == 1);
 }
 
 /** Evaluate a map
@@ -599,11 +599,10 @@ static int cond_compare_virtual(request_t *request, map_t const *map)
  * @param[in] async_lhs the asynchronously evaluated value box, for XLAT and EXEC
  * @param[in] async_rhs the asynchronously evaluated value box, for XLAT and EXEC
  * @return
- *	- -1 on failure.
- *	- 0 for "no match".
- *	- 1 for "match".
+ *	- false for no match (including "not found")
+ *	- true for match
  */
-static int cond_eval_map(request_t *request, fr_cond_t const *c,
+static bool cond_eval_map(request_t *request, fr_cond_t const *c,
 			 fr_value_box_t *async_lhs, fr_value_box_t *async_rhs)
 {
 	int		rcode = 0;
@@ -637,7 +636,7 @@ static int cond_eval_map(request_t *request, fr_cond_t const *c,
 	 */
 	if (cond_realize_tmpl(request, &lhs, &lhs_free, map->lhs, map->rhs, async_lhs) < 0) {
 		fr_strerror_const("Failed evaluating left side of condition");
-		return -1;
+		return false;
 	}
 
 	/*
@@ -645,7 +644,7 @@ static int cond_eval_map(request_t *request, fr_cond_t const *c,
 	 */
 	if (cond_realize_tmpl(request, &rhs, &rhs_free, map->rhs, map->lhs, async_rhs) < 0) {
 		fr_strerror_const("Failed evaluating right side of condition");
-		return -1;
+		return false;
 	}
 
 	/*
@@ -666,7 +665,7 @@ static int cond_eval_map(request_t *request, fr_cond_t const *c,
 			if (slen <= 0) {
 				REMARKER(rhs->vb_strvalue, -slen, "%s", fr_strerror());
 				EVAL_DEBUG("FAIL %d", __LINE__);
-				return -1;
+				return false;
 			}
 			preg = preg_free;
 		}
@@ -707,7 +706,7 @@ static int cond_eval_map(request_t *request, fr_cond_t const *c,
 
 		if (map->op == T_OP_REG_EQ) {
 			fr_strerror_const("Virtual attributes cannot be used with regular expressions");
-			return -1;
+			return false;
 		}
 
 		/*
@@ -726,7 +725,7 @@ static int cond_eval_map(request_t *request, fr_cond_t const *c,
 		 */
 		if (!rhs) {
 			fr_strerror_const("Invalid comparison for virtual attribute");
-			return -1;
+			return false;
 		}
 
 		MEM(vp = fr_pair_afrom_da(request->request_ctx, tmpl_da(map->lhs)));
@@ -839,7 +838,7 @@ done:
 	 *	request data, in which case we don't free it.
 	 */
 	if (preg) talloc_free(preg_free);
-	return rcode;
+	return (rcode == 1);
 }
 
 
@@ -849,12 +848,10 @@ done:
  * @param[in] modreturn the previous module return code
  * @param[in] c the condition to evaluate
  * @return
- *	- -1 on failure.
- *	- -2 on attribute not found.
- *	- 0 for "no match".
- *	- 1 for "match".
+ *	- false for "no match", including "not found"
+ *	- true for "match"
  */
-int cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c)
+bool cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c)
 {
 	int rcode = -1;
 
@@ -892,13 +889,13 @@ int cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c)
 			break;
 		default:
 			EVAL_DEBUG("FAIL %d", __LINE__);
-			return -1;
+			return false;
 		}
 
 		/*
-		 *	Errors cause failures.
+		 *	Errors are "no match".
 		 */
-		if (rcode < 0) return rcode;
+		if (rcode < 0) return false;
 
 		if (c->negate) rcode = !rcode;
 
@@ -912,7 +909,7 @@ int cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c)
 		while (!c->next) {
 return_to_parent:
 			c = c->parent;
-			if (!c) return rcode;
+			if (!c) goto done;
 		}
 
 		/*
@@ -938,10 +935,11 @@ return_to_parent:
 		}
 	}
 
+done:
 	if (rcode < 0) {
 		EVAL_DEBUG("FAIL %d", __LINE__);
 	}
-	return rcode;
+	return (rcode == 1);
 }
 
 /** Asynchronous evaluation of conditions.
@@ -1112,7 +1110,7 @@ return_to_parent:
 /** Evaluate a map as if it is a condition.
  *
  */
-int fr_cond_eval_map(request_t *request, map_t const *map)
+bool fr_cond_eval_map(request_t *request, map_t const *map)
 {
 	fr_cond_t cond;
 
diff --git a/src/lib/server/cond_eval.h b/src/lib/server/cond_eval.h
index 1c4c7eecf14..f754249e13c 100644
--- a/src/lib/server/cond_eval.h
+++ b/src/lib/server/cond_eval.h
@@ -39,7 +39,7 @@ typedef struct fr_cond_s fr_cond_t;
 
 void	cond_debug(fr_cond_t const *cond);
 
-int	cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c);
+bool	cond_eval(request_t *request, rlm_rcode_t modreturn, fr_cond_t const *c);
 
 typedef struct {
 	TALLOC_CTX	*ctx;		//!< for intermediate value boxes
@@ -65,7 +65,7 @@ typedef struct {
 
 int cond_eval_async(request_t *request, fr_cond_async_t *a);
 
-int fr_cond_eval_map(request_t *request, map_t const *map);
+bool fr_cond_eval_map(request_t *request, map_t const *map);
 
 #ifdef __cplusplus
 }
diff --git a/src/lib/server/cond_tokenize.c b/src/lib/server/cond_tokenize.c
index 6eb506a220b..4ece2a3763e 100644
--- a/src/lib/server/cond_tokenize.c
+++ b/src/lib/server/cond_tokenize.c
@@ -637,15 +637,14 @@ static int cond_normalise(TALLOC_CTX *ctx, fr_token_t lhs_type, fr_cond_t **c_ou
 		 */
 		if (tmpl_is_data(c->data.map->lhs) &&
 		    tmpl_is_data(c->data.map->rhs)) {
-			int rcode;
+			bool rcode;
 
 			fr_assert(c->next == NULL);
 
 			rcode = cond_eval(NULL, RLM_MODULE_NOOP, c);
-			fr_assert(rcode >= 0);
 			TALLOC_FREE(c->data.map);
 
-			if (rcode > 0) {
+			if (rcode) {
 				c->type = COND_TYPE_TRUE;
 			} else {
 				c->type = COND_TYPE_FALSE;
diff --git a/src/lib/unlang/condition.c b/src/lib/unlang/condition.c
index 014652d08e1..9298328232f 100644
--- a/src/lib/unlang/condition.c
+++ b/src/lib/unlang/condition.c
@@ -29,32 +29,15 @@ RCSID("$Id$")
 
 static unlang_action_t unlang_if(rlm_rcode_t *p_result, request_t *request, unlang_stack_frame_t *frame)
 {
-	int			condition;
 	unlang_group_t		*g = unlang_generic_to_group(frame->instruction);
 	unlang_cond_t		*gext = unlang_group_to_cond(g);
 
 	fr_assert(gext->cond != NULL);
 
-	condition = cond_eval(request, *p_result, gext->cond);
-	if (condition < 0) {
-		switch (condition) {
-		case -2:
-			REDEBUG("Condition evaluation failed because a referenced attribute "
-				"was not found in the request");
-			break;
-		default:
-		case -1:
-			REDEBUG("Condition evaluation failed because the value of an operand "
-				"could not be determined - %s", fr_strerror());
-			break;
-		}
-		condition = 0;
-	}
-
 	/*
 	 *	Didn't pass.  Remember that.
 	 */
-	if (!condition) {
+	if (!cond_eval(request, *p_result, gext->cond)) {
 		RDEBUG2("...");
 		return UNLANG_ACTION_EXECUTE_NEXT;
 	}
diff --git a/src/modules/rlm_files/rlm_files.c b/src/modules/rlm_files/rlm_files.c
index 89c0487647c..f3522fa6496 100644
--- a/src/modules/rlm_files/rlm_files.c
+++ b/src/modules/rlm_files/rlm_files.c
@@ -477,7 +477,6 @@ redo:
 		 *	Realize the map to a list of VPs
 		 */
 		while ((map = fr_dlist_next(&pl->check, map))) {
-			int rcode;
 			fr_pair_list_t tmp_list;
 
 			/*
@@ -505,13 +504,7 @@ redo:
 				 *	Evaluate the map, including regexes.
 				 */
 			default:
-				rcode = fr_cond_eval_map(request, map);
-				if (rcode < 0) {
-					RPWARN("Failed evaluating check item, skipping entry");
-					break;
-				}
-
-				if (rcode == 0) match = false;
+				if (!fr_cond_eval_map(request, map)) match = false;
 				break;
 			}
 
diff --git a/src/tests/keywords/notfound b/src/tests/keywords/notfound
new file mode 100644
index 00000000000..e617fe2d844
--- /dev/null
+++ b/src/tests/keywords/notfound
@@ -0,0 +1,25 @@
+#
+#  PRE: if update
+#
+
+#
+#  This doesn't exist, so  the condition shouldn't match.
+#
+if (&Tmp-String-0 == "foo") {
+	test_fail
+}
+else {
+	ok
+}
+
+#
+# Since the middle part doesn't match, "NOT" the middle part SHOULD match.
+#
+if (!(&Tmp-String-0 == "foo")) {
+	ok
+}
+else {
+	test_fail
+}
+
+success