Skip to content

Commit 5d1a668

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
Add support for configuring custom expression-level validators in CelPolicyCompiler
PiperOrigin-RevId: 815866955
1 parent 1fc8922 commit 5d1a668

File tree

5 files changed

+177
-3
lines changed

5 files changed

+177
-3
lines changed

‎policy/src/main/java/dev/cel/policy/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ java_library(
137137
],
138138
deps = [
139139
":compiler",
140+
"//validator:ast_validator",
140141
"@maven//:com_google_errorprone_error_prone_annotations",
141142
],
142143
)

‎policy/src/main/java/dev/cel/policy/CelPolicyCompilerBuilder.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
import com.google.errorprone.annotations.CanIgnoreReturnValue;
1818
import com.google.errorprone.annotations.CheckReturnValue;
19+
import dev.cel.validator.CelAstValidator;
1920

2021
/** Interface for building an instance of {@link CelPolicyCompiler} */
2122
public interface CelPolicyCompilerBuilder {
@@ -38,6 +39,24 @@ public interface CelPolicyCompilerBuilder {
3839
@CanIgnoreReturnValue
3940
CelPolicyCompilerBuilder setAstDepthLimit(int iterationLimit);
4041

42+
/**
43+
* Adds one or more {@link CelAstValidators} to the compiler. These apply per CEL expression in
44+
* the policy.
45+
*/
46+
@CanIgnoreReturnValue
47+
CelPolicyCompilerBuilder addValidators(Iterable<? extends CelAstValidator> validators);
48+
49+
/**
50+
* Adds one or more {@link CelAstValidators} to the compiler. These apply per CEL expression in
51+
* the policy.
52+
*/
53+
@CanIgnoreReturnValue
54+
CelPolicyCompilerBuilder addValidators(CelAstValidator... validators);
55+
56+
/** Removes any custom validators from the compiler builder. */
57+
@CanIgnoreReturnValue
58+
CelPolicyCompilerBuilder clearValidators();
59+
4160
@CheckReturnValue
4261
CelPolicyCompiler build();
4362
}

‎policy/src/main/java/dev/cel/policy/CelPolicyCompilerImpl.java‎

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ final class CelPolicyCompilerImpl implements CelPolicyCompiler {
6464
private final String variablesPrefix;
6565
private final int iterationLimit;
6666
private final Optional<CelAstValidator> astDepthValidator;
67+
private final Optional<CelValidator> validator;
6768

6869
@Override
6970
public CelCompiledRule compileRule(CelPolicy policy) throws CelPolicyValidationException {
@@ -194,6 +195,10 @@ private CelCompiledRule compileRuleImpl(
194195
CelType outputType = SimpleType.DYN;
195196
try {
196197
varAst = localCel.compile(expression.value()).getAst();
198+
if (this.validator.isPresent()) {
199+
CelValidationResult result = this.validator.get().validate(varAst);
200+
varAst = result.getAst();
201+
}
197202
outputType = varAst.getResultType();
198203
} catch (CelValidationException e) {
199204
compilerContext.addIssue(expression.id(), e.getErrors());
@@ -212,6 +217,10 @@ private CelCompiledRule compileRuleImpl(
212217
CelAbstractSyntaxTree conditionAst;
213218
try {
214219
conditionAst = localCel.compile(match.condition().value()).getAst();
220+
if (this.validator.isPresent()) {
221+
CelValidationResult result = this.validator.get().validate(conditionAst);
222+
conditionAst = result.getAst();
223+
}
215224
if (!conditionAst.getResultType().equals(SimpleType.BOOL)) {
216225
compilerContext.addIssue(
217226
match.condition().id(),
@@ -229,6 +238,10 @@ private CelCompiledRule compileRuleImpl(
229238
ValueString output = match.result().output();
230239
try {
231240
outputAst = localCel.compile(output.value()).getAst();
241+
if (this.validator.isPresent()) {
242+
CelValidationResult result = this.validator.get().validate(outputAst);
243+
outputAst = result.getAst();
244+
}
232245
} catch (CelValidationException e) {
233246
compilerContext.addIssue(output.id(), e.getErrors());
234247
continue;
@@ -340,10 +353,12 @@ static final class Builder implements CelPolicyCompilerBuilder {
340353
private String variablesPrefix;
341354
private int iterationLimit;
342355
private Optional<CelAstValidator> astDepthLimitValidator;
356+
private ArrayList<CelAstValidator> validators;
343357

344358
private Builder(Cel cel) {
345359
this.cel = cel;
346360
this.astDepthLimitValidator = Optional.of(AstDepthLimitValidator.DEFAULT);
361+
this.validators = new ArrayList<>();
347362
}
348363

349364
@Override
@@ -360,6 +375,26 @@ public Builder setIterationLimit(int iterationLimit) {
360375
return this;
361376
}
362377

378+
@Override
379+
@CanIgnoreReturnValue
380+
public Builder addValidators(Iterable<? extends CelAstValidator> validators) {
381+
validators.forEach(this.validators::add);
382+
return this;
383+
}
384+
385+
@Override
386+
@CanIgnoreReturnValue
387+
public Builder addValidators(CelAstValidator... validators) {
388+
return addValidators(Arrays.asList(validators));
389+
}
390+
391+
@Override
392+
@CanIgnoreReturnValue
393+
public Builder clearValidators() {
394+
this.validators.clear();
395+
return this;
396+
}
397+
363398
@Override
364399
@CanIgnoreReturnValue
365400
public CelPolicyCompilerBuilder setAstDepthLimit(int astDepthLimit) {
@@ -374,7 +409,7 @@ public CelPolicyCompilerBuilder setAstDepthLimit(int astDepthLimit) {
374409
@Override
375410
public CelPolicyCompiler build() {
376411
return new CelPolicyCompilerImpl(
377-
cel, this.variablesPrefix, this.iterationLimit, astDepthLimitValidator);
412+
cel, this.variablesPrefix, this.iterationLimit, astDepthLimitValidator, validators);
378413
}
379414
}
380415

@@ -388,10 +423,20 @@ private CelPolicyCompilerImpl(
388423
Cel cel,
389424
String variablesPrefix,
390425
int iterationLimit,
391-
Optional<CelAstValidator> astDepthValidator) {
426+
Optional<CelAstValidator> astDepthValidator,
427+
List<CelAstValidator> additionalValidators) {
392428
this.cel = checkNotNull(cel);
393429
this.variablesPrefix = checkNotNull(variablesPrefix);
394430
this.iterationLimit = iterationLimit;
395431
this.astDepthValidator = astDepthValidator;
432+
if (additionalValidators.isEmpty()) {
433+
this.validator = Optional.empty();
434+
} else {
435+
this.validator =
436+
Optional.of(
437+
CelValidatorFactory.standardCelValidatorBuilder(cel)
438+
.addAstValidators(additionalValidators)
439+
.build());
440+
}
396441
}
397442
}

‎policy/src/test/java/dev/cel/policy/BUILD.bazel‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,11 @@ java_library(
1717
"//bundle:environment_yaml_parser",
1818
"//common:cel_ast",
1919
"//common:options",
20+
"//common/ast",
2021
"//common/formats:value_string",
2122
"//common/internal",
23+
"//common/navigation",
24+
"//common/navigation:common",
2225
"//common/resources/testdata/proto3:standalone_global_enum_java_proto",
2326
"//common/types",
2427
"//compiler",
@@ -35,8 +38,8 @@ java_library(
3538
"//policy:validation_exception",
3639
"//runtime",
3740
"//runtime:function_binding",
38-
"//runtime:late_function_binding",
3941
"//testing/protos:single_file_java_proto",
42+
"//validator:ast_validator",
4043
"@cel_spec//proto/cel/expr/conformance/proto3:test_all_types_java_proto",
4144
"@maven//:com_google_guava_guava",
4245
"@maven//:com_google_testparameterinjector_test_parameter_injector",

‎policy/src/test/java/dev/cel/policy/CelPolicyCompilerImplTest.java‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@
3131
import dev.cel.bundle.CelFactory;
3232
import dev.cel.common.CelAbstractSyntaxTree;
3333
import dev.cel.common.CelOptions;
34+
import dev.cel.common.ast.CelConstant;
35+
import dev.cel.common.ast.CelExpr.ExprKind;
36+
import dev.cel.common.navigation.CelNavigableAst;
37+
import dev.cel.common.navigation.CelNavigableExpr;
38+
import dev.cel.common.navigation.TraversalOrder;
3439
import dev.cel.common.types.OptionalType;
3540
import dev.cel.common.types.SimpleType;
3641
import dev.cel.expr.conformance.proto3.TestAllTypes;
@@ -47,6 +52,8 @@
4752
import dev.cel.runtime.CelLateFunctionBindings;
4853
import dev.cel.testing.testdata.SingleFileProto.SingleFile;
4954
import dev.cel.testing.testdata.proto3.StandaloneGlobalEnum;
55+
import dev.cel.validator.CelAstValidator;
56+
import dev.cel.validator.CelAstValidator.IssuesFactory;
5057
import java.io.IOException;
5158
import java.util.Map;
5259
import java.util.Optional;
@@ -265,6 +272,105 @@ public void evaluateYamlPolicy_nestedRuleProducesOptionalOutput() throws Excepti
265272
assertThat(evalResult).hasValue(Optional.of(true));
266273
}
267274

275+
static final class NoFooLiteralsValidator implements CelAstValidator {
276+
private static boolean isFooLiteral(CelNavigableExpr node) {
277+
return node.getKind().equals(ExprKind.Kind.CONSTANT)
278+
&& node.expr().constant().getKind().equals(CelConstant.Kind.STRING_VALUE)
279+
&& node.expr().constant().stringValue().equals("foo");
280+
}
281+
282+
@Override
283+
public void validate(CelNavigableAst navigableAst, Cel cel, IssuesFactory issuesFactory) {
284+
navigableAst
285+
.getRoot()
286+
.descendants(TraversalOrder.POST_ORDER)
287+
.filter(NoFooLiteralsValidator::isFooLiteral)
288+
.forEach(node -> issuesFactory.addError(node.id(), "'foo' is a forbidden literal"));
289+
}
290+
}
291+
292+
@Test
293+
public void evaluateYamlPolicy_validatorReportsErrors() throws Exception {
294+
Cel cel = newCel();
295+
String policySource =
296+
"name: nested_rule_with_forbidden_literal\n"
297+
+ "rule:\n"
298+
+ " variables:\n"
299+
+ " - name: 'foo'\n"
300+
+ " expression: \"(true) ? 'bar' : 'foo'\"\n"
301+
+ " match:\n"
302+
+ " - condition: |\n"
303+
+ " variables.foo in ['foo', 'bar', 'foo']\n"
304+
+ " output: >\n"
305+
+ " 'foo' == variables.foo\n";
306+
CelPolicy policy = POLICY_PARSER.parse(policySource);
307+
CelPolicyValidationException e =
308+
assertThrows(
309+
CelPolicyValidationException.class,
310+
() ->
311+
CelPolicyCompilerFactory.newPolicyCompiler(cel)
312+
.addValidators(new NoFooLiteralsValidator())
313+
.build()
314+
.compile(policy));
315+
316+
assertThat(e)
317+
.hasMessageThat()
318+
.contains(
319+
"ERROR: <input>:5:37: 'foo' is a forbidden literal\n"
320+
+ " | expression: \"(true) ? 'bar' : 'foo'\"\n"
321+
+ " | ....................................^");
322+
assertThat(e)
323+
.hasMessageThat()
324+
.contains(
325+
"ERROR: <input>:8:27: 'foo' is a forbidden literal\n"
326+
+ " | variables.foo in ['foo', 'bar', 'foo']\n"
327+
+ " | ..........................^");
328+
assertThat(e)
329+
.hasMessageThat()
330+
.contains(
331+
"ERROR: <input>:8:41: 'foo' is a forbidden literal\n"
332+
+ " | variables.foo in ['foo', 'bar', 'foo']\n"
333+
+ " | ........................................^");
334+
}
335+
336+
// If the condition fails to validate, then the compiler doesn't attempt to compile or validate
337+
// the output, so second test case for coverage.
338+
@Test
339+
public void evaluateYamlPolicy_validatorReportsOutput() throws Exception {
340+
Cel cel = newCel();
341+
String policySource =
342+
"name: nested_rule_with_forbidden_literal\n"
343+
+ "rule:\n"
344+
+ " variables:\n"
345+
+ " - name: 'foo'\n"
346+
+ " expression: \"(true) ? 'bar' : 'foo'\"\n"
347+
+ " match:\n"
348+
+ " - output: >\n"
349+
+ " 'foo' == variables.foo\n";
350+
CelPolicy policy = POLICY_PARSER.parse(policySource);
351+
CelPolicyValidationException e =
352+
assertThrows(
353+
CelPolicyValidationException.class,
354+
() ->
355+
CelPolicyCompilerFactory.newPolicyCompiler(cel)
356+
.addValidators(new NoFooLiteralsValidator())
357+
.build()
358+
.compile(policy));
359+
360+
assertThat(e)
361+
.hasMessageThat()
362+
.contains(
363+
"ERROR: <input>:5:37: 'foo' is a forbidden literal\n"
364+
+ " | expression: \"(true) ? 'bar' : 'foo'\"\n"
365+
+ " | ....................................^");
366+
assertThat(e)
367+
.hasMessageThat()
368+
.contains(
369+
"ERROR: <input>:8:9: 'foo' is a forbidden literal\n"
370+
+ " | 'foo' == variables.foo\n"
371+
+ " | ........^");
372+
}
373+
268374
@Test
269375
public void evaluateYamlPolicy_lateBoundFunction() throws Exception {
270376
String configSource =

0 commit comments

Comments
 (0)