jerryshao commented on code in PR #13478: URL: https://github.com/apache/gravitino/pull/13478#discussion_r4248024023
########## core/src/main/java/org/apache/gravitino/policy/rego/RestrictedRegoProgram.java: ########## @@ -0,0 +1,410 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** A parsed, unresolved program in the {@code restricted-rego-v1} source dialect. */ +public interface RestrictedRegoProgram { + + /** + * Returns the rule type declared by this program. + * + * @return rule type + */ + RuleType ruleType(); + + /** + * Validates the rule and every expression it contains. + * + * @throws IllegalArgumentException if the rule is malformed + */ + void validate() throws IllegalArgumentException; + + /** Rule heads supported by {@code restricted-rego-v1}. */ + enum RuleType { + /** A row-filter rule. */ + FILTER, + /** A column-mask rule. */ + MASK + } + + /** Logical column-mask actions supported by the first restricted profile. */ + enum MaskAction { + /** Mask alphanumeric characters. */ + MASK_ALPHANUM("mask-alphanum"), + /** Replace the value with the action's fixed value. */ + MASK_TO_FIXED_VALUE("mask-to-fixed-value"), + /** Replace the value with null. */ + REPLACE_WITH_NULL("replace-with-null"), + /** Preserve only the first four characters. */ + SHOW_FIRST_4("show-first-4"), + /** Preserve only the last four characters. */ + SHOW_LAST_4("show-last-4"), + /** Truncate a temporal value to its year. */ + TRUNCATE_TO_YEAR("truncate-to-year"), + /** Truncate a temporal value to its month. */ + TRUNCATE_TO_MONTH("truncate-to-month"), + /** Apply a global SHA-256 mask. */ + SHA_256_GLOBAL("sha-256-global"), + /** Apply a query-local SHA-256 mask. */ + SHA_256_QUERY_LOCAL("sha-256-query-local"); + + private final String value; + + MaskAction(String value) { + this.value = value; + } + + /** + * Parses a mask-action value. + * + * @param value source action value + * @return parsed action + * @throws IllegalArgumentException if the action is not allowlisted + */ + public static MaskAction fromValue(String value) { + Preconditions.checkArgument(value != null && !value.isEmpty(), "mask action cannot be empty"); + for (MaskAction action : values()) { + if (action.value.equals(value)) { + return action; + } + } + throw new IllegalArgumentException("Unsupported restricted-rego-v1 mask action: " + value); + } + + /** + * Returns the source action value. + * + * @return source action value + */ + public String value() { + return value; + } + } + + /** One guarded result in a conditional row-filter rule. */ + final class FilterBranch { + private final CanonicalExpression result; + private final CanonicalExpression condition; + + FilterBranch(CanonicalExpression result, CanonicalExpression condition) { + this.result = result; + this.condition = condition; + } + + /** + * Returns the Boolean result selected by this branch. + * + * @return unresolved Boolean result + */ + public CanonicalExpression result() { + return result; + } + + /** + * Returns this branch's Boolean condition. + * + * @return unresolved Boolean condition + */ + public CanonicalExpression condition() { + return condition; + } + + /** + * Validates the branch. + * + * @throws IllegalArgumentException if its result or condition is not Boolean + */ + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(result != null, "filter branch result cannot be null"); + Preconditions.checkArgument(condition != null, "filter branch condition cannot be null"); + result.validate(); + condition.validate(); + Preconditions.checkArgument( + ExpressionValidator.isPredicate(result), "filter branch result must be Boolean"); + Preconditions.checkArgument( + ExpressionValidator.isPredicate(condition), "filter branch condition must be Boolean"); + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof FilterBranch)) { + return false; + } + FilterBranch that = (FilterBranch) other; + return Objects.equals(result, that.result) && Objects.equals(condition, that.condition); + } + + @Override + public int hashCode() { + return Objects.hash(result, condition); + } + + @Override + public String toString() { + return "FilterBranch{" + "result=" + result + ", condition=" + condition + '}'; + } + } + + /** A complete {@code filter := ...} row-filter rule. */ + final class RowFilter implements RestrictedRegoProgram { + private final List<FilterBranch> branches; + private final CanonicalExpression fallback; + + RowFilter(List<FilterBranch> branches, CanonicalExpression fallback) { + this.branches = + branches == null ? null : Collections.unmodifiableList(new ArrayList<>(branches)); + this.fallback = fallback; + } + + @Override + public RuleType ruleType() { + return RuleType.FILTER; + } + + /** + * Returns the guarded results in source order. + * + * @return immutable branches, or an empty list for an unconditional rule + */ + public List<FilterBranch> branches() { + return branches; + } + + /** + * Returns the unconditional result or final {@code else} result. + * + * @return unresolved Boolean fallback + */ + public CanonicalExpression fallback() { + return fallback; + } + + /** + * Returns whether this rule contains at least one conditional branch. + * + * @return {@code true} for a conditional rule + */ + public boolean isConditional() { + return branches != null && !branches.isEmpty(); + } + + /** + * Lowers the ordered branches to one Boolean predicate with first-match semantics. + * + * <p>The returned predicate remains unresolved. Context and column binding can be applied to it + * by a later resolver. + * + * @return lowered unresolved predicate + */ + public CanonicalExpression lower() { + validate(); + if (branches.isEmpty()) { + return fallback; + } + + CanonicalExpression lowered = fallback; + for (int index = branches.size() - 1; index >= 0; index--) { + FilterBranch branch = branches.get(index); + CanonicalExpression selected = + LogicalExpression.of(LogicalOperator.AND, List.of(branch.condition(), branch.result())); + CanonicalExpression remaining = + LogicalExpression.of( + LogicalOperator.AND, List.of(Not.negate(branch.condition()), lowered)); + lowered = LogicalExpression.of(LogicalOperator.OR, List.of(selected, remaining)); Review Comment: [Question] With the lowered checks removed, nothing bounds the generated predicate - is the implied bound worth asserting somewhere? The rewrite is right and the budget argument holds: each condition appears once in `selected` and once negated in `remaining`, so lowering is linear. But the explicit ceilings added in the previous round are gone, and the ceiling is now implicit in the source limits. Measured on this head with `filter := true` + N copies of ` if true else := true`: 10 branches give lowered depth 22, 50 give 102, 127 give depth 256 and about 900 nodes (a 42 KB `toString`), and 200 branches are rejected - but by the 256-node source limit, not by anything about the lowered form. For arrays the doubling is what the docs promise: a 256-element array in a branch condition yields 512 occurrences after lowering. So the real bound on what the resolver will serialize is "whatever 256 source nodes lower to", around depth 256 and 900 nodes. That is safe for the recursive `validate()` / `depth()` / `toString()` walks here, and no input I tried produced a `StackOverflowError`. The question is whether the next stage - canonicalization and Iceberg wire serialization, design-docs/tag-based-read-restrictions-for-iceberg-rest.md:500-510 - is equally happy with a 256-deep tree, since the save path no longer rejects one. A single assertion in `lower()` on the lowered depth, or a test that pins the worst case the source limits allow, would make the implicit contract explicit without reintroducing the branch-dependent budget this round removed. Verified by: ran the branch-count series above through a standalone build of this package and printed `lower().depth()` and the node count for each; the 127/200 boundary and the 512-element array result reproduce exactly. ########## design-docs/tag-based-read-restrictions-for-iceberg-rest.md: ########## @@ -397,16 +404,18 @@ is lowered to: ```text (condition1 and result1) -or (not condition1 and condition2 and result2) -or (not condition1 and not condition2 and fallback) +or (not condition1 and ( + (condition2 and result2) + or (not condition2 and fallback) +)) ``` Request-context-only conditions are folded before the final predicate is built, but every branch must still parse and type-check. For a column mask, all conditions are context-only: the resolver evaluates them in order and selects the first action whose condition is true, or the final fallback action. The conditional assignment, `if`, and `else` nodes never appear in the Iceberg wire expression or projection. This preserves the first-matching-branch semantics of Rego's `else` -chain while producing the one closed Boolean predicate or one column action required by Iceberg. +The nested form grows linearly rather than repeating every earlier condition in every later branch. Review Comment: [Nit] This edit truncated the preceding sentence. Line 417 still ends mid-clause - it stops at "This preserves the first-matching-branch semantics of Rego's else" - because the replaced line was its continuation (`chain while producing the one closed Boolean predicate or one column action required by Iceberg.`). The paragraph now reads as two sentences run together, and the statement about producing one closed Boolean predicate or one column action, which is the point of the section, is gone. Restoring `chain while producing the one closed Boolean predicate or one column action required by Iceberg.` and putting the new sentence after it as its own line fixes both. Verified by: read lines 407-420 of the file on this head, and the hunk `@@ -409 +418 @@` of `git diff origin/main...HEAD` for that file. ########## docs/policies.md: ########## @@ -47,6 +47,120 @@ The row-filter and column-mask policy types can be created and associated with t does not enforce them yet. Until enforcement is available, these policy types do not restrict data access. +### Read-Restriction Expressions + +Row-filter and column-mask expressions use `restricted-rego-v1`, a small, fail-closed Rego-like +syntax owned by Gravitino. Each expression is one complete rule, not an arbitrary Rego module. A +row filter starts with `filter :=` and produces a Boolean predicate. A column mask starts with +`mask := action("...")` and produces an allowlisted masking action. + +The complete grammar is: + +```text +program := filterRule | maskRule +filterRule := unconditionalFilter | conditionalFilter +unconditionalFilter := "filter" ":=" expr +conditionalFilter := "filter" ":=" expr "if" expr filterElse* filterFallback +filterElse := "else" ":=" expr "if" expr +filterFallback := "else" ":=" expr +maskRule := unconditionalMask | conditionalMask +unconditionalMask := "mask" ":=" maskAction +conditionalMask := "mask" ":=" maskAction "if" contextExpr maskElse* maskFallback +maskElse := "else" ":=" maskAction "if" contextExpr +maskFallback := "else" ":=" maskAction +maskAction := "action" "(" string ")" +contextExpr := expr +expr := orExpr +orExpr := andExpr ("or" andExpr)* +andExpr := notExpr ("and" notExpr)* +notExpr := "not" notExpr | compareExpr +compareExpr := primary (("==" | "!=" | "<" | "<=" | ">" | ">=" | "in") primary)? +primary := colRef | sessionUser | groupMember | literal | array | "(" expr ")" +colRef := "col" "(" string ")" +sessionUser := "session_user" "(" ")" +groupMember := "is_group_member" "(" string ")" +literal := string | number | boolean | null +array := "[" literal ("," literal)* "]" +boolean := "true" | "false" +null := "null" +number := "-"? ("0" | nonZeroDigit digit*) ("." digit+)? +digit := "0" | nonZeroDigit +nonZeroDigit := "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9" +``` + +For example, a row-filter program can select the first matching predicate: + +```restricted-rego-v1 +filter := col("region") == "US" if is_group_member("auditors") +else := col("owner") == session_user() +``` + +This column-mask program selects a masking action in the same way: + +```restricted-rego-v1 +mask := action("show-last-4") if is_group_member("support") +else := action("replace-with-null") +``` + +Conditional branches are evaluated from left to right. The first true condition selects its result, +and every conditional rule must end with an unconditional `else`. Row-filter conditions and results +must be Boolean. Column-mask conditions may use only request context; they cannot reference columns. + +The syntax supports these constructs: + +- `col("name")`, `session_user()`, and `is_group_member("group")`; +- JSON string literals, exact decimal numbers, `true`, `false`, `null`, and homogeneous non-null + literal arrays; +- comparisons `==`, `!=`, `<`, `<=`, `>`, `>=`, and `in`; and +- Boolean operators `and`, `or`, and `not`. + +Keywords and built-in function names are case-sensitive. For example, only lowercase `in` is an +operator; `In`, `IN`, and `iN` are invalid. + +Strings use JSON double-quoted syntax and are decoded exactly once after the enclosing HTTP JSON is +decoded. Raw control characters, NUL, isolated Unicode surrogates, single-quoted strings, and invalid +JSON escapes are rejected. Gravitino does not normalize, trim, or case-fold decoded column names, +group names, or values. Numbers are exact decimals; exponent notation, a leading `+`, leading zeroes, +and a trailing decimal point are invalid. + +Supported operand shapes are: + +| Form | Operators | Requirements | +| --- | --- | --- | +| Boolean predicates | `and`, `or`, `not` | Every operand must be Boolean. | +| Column and non-null literal | `==`, `!=`, `<`, `<=`, `>`, `>=` | Either operand order; types must be compatible. Boolean literals support only `==` and `!=`. | +| String column and `session_user()` | `==`, `!=` | Either operand order. | +| `session_user()` and string literal | `==`, `!=` | Either operand order. | +| Column and `null` | `==`, `!=` | Either operand order. | +| Column and literal array | `in` | Column on the left; array must be non-empty and homogeneous. | +| `session_user()` and string array | `in` | `session_user()` on the left; array must be non-empty. | +| `is_group_member("group")` | none | Produces a Boolean request-context predicate. | + +Column-to-column and literal-to-literal comparisons, ordering on `session_user()`, null array +elements, nested arrays, and comparisons on `is_group_member(...)` are invalid. + +Column-mask actions are `mask-alphanum`, `mask-to-fixed-value`, `replace-with-null`, `show-first-4`, +`show-last-4`, `truncate-to-year`, `truncate-to-month`, `sha-256-global`, and +`sha-256-query-local`. + +Validation applies these limits when a policy is created or its content is updated: + +- 16 KiB of UTF-8 source; +- operation depth 8 for the source AST; +- 256 nodes for the source AST; +- 4 KiB of UTF-8 for each decoded string literal; and +- 256 bytes for each numeric literal; and +- 256 array elements in total in the source. Review Comment: [Nit] Two user-visible rejections are missing from the Limits list. The list no longer mentions the per-array cap, and it never mentioned parenthesis nesting, but both are enforced and both produce their own error: - `RestrictedRegoSourceParser.parseLiteralArray` rejects the 257th element of a single array with `array literal must not exceed 256 elements` - a different message and a different code path from the total-elements check this list does document; - `parsePrimary` rejects a 257th nested parenthesis with `parenthesis nesting must not exceed 256`, which nothing in either document mentions. The per-array cap is invisible today only because it equals the total; if the total is ever raised, the list becomes wrong rather than incomplete. One bullet each would close it. Separately, the list now has `; and` on both the fourth and fifth bullets. Verified by: ran a 257-element array and 256 vs 257 nested parentheses through a standalone build of the parser and recorded the two messages above; `RestrictedRegoLimits.java:8,11` holds both constants. ########## core/src/test/java/org/apache/gravitino/policy/rego/TestRestrictedRegoExpressionParserFacade.java: ########## @@ -0,0 +1,705 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.policy.rego; + +import java.io.IOException; +import java.io.InputStream; +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RowFilter; +import org.apache.gravitino.policy.rego.RestrictedRegoProgram.RuleType; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +/** Tests for {@link RestrictedRegoExpressionParserFacade}. */ +public class TestRestrictedRegoExpressionParserFacade { + + private static final String GOLDEN_CASES_RESOURCE = "/restricted-rego-v1-cases.txt"; + private static final String GOLDEN_FILE = + "core/src/test/resources/restricted-rego-v1-cases.golden"; + private static final String UPDATE_GOLDEN_ENV = "GRAVITINO_UPDATE_RESTRICTED_REGO_GOLDEN"; + private static final String REPOSITORY_ROOT_PROPERTY = "gravitino.repositoryRoot"; + private static final String GOLDEN_LICENSE_HEADER = + "# Licensed to the Apache Software Foundation (ASF) under one or more\n" + + "# contributor license agreements. See the NOTICE file distributed with\n" + + "# this work for additional information regarding copyright ownership.\n" + + "# The ASF licenses this file to You under the Apache License, Version 2.0\n" + + "# (the \"License\"); you may not use this file except in compliance with\n" + + "# the License. You may obtain a copy of the License at\n" + + "#\n" + + "# http://www.apache.org/licenses/LICENSE-2.0\n"; + + "#\n" + + "# Unless required by applicable law or agreed to in writing, software\n" + + "# distributed under the License is distributed on an \"AS IS\" BASIS,\n" + + "# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n" + + "# See the License for the specific language governing permissions and\n" + + "# limitations under the License.\n"; + + @Test + void testParsesTypedRules() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := col(\"owner\") == session_user()"); + Assertions.assertEquals(RuleType.FILTER, filter.ruleType()); + Assertions.assertFalse(filter.isConditional()); + Assertions.assertInstanceOf(Comparison.class, filter.fallback()); + + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask("mask := action(\"show-last-4\")"); + Assertions.assertEquals(RuleType.MASK, mask.ruleType()); + Assertions.assertFalse(mask.isConditional()); + Assertions.assertEquals(MaskAction.SHOW_LAST_4, mask.fallback()); + + Assertions.assertInstanceOf( + RowFilter.class, RestrictedRegoExpressionParserFacade.parse("filter := true")); + Assertions.assertInstanceOf( + ColumnMask.class, + RestrictedRegoExpressionParserFacade.parse("mask := action(\"mask-alphanum\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter("mask := action(\"show-last-4\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RestrictedRegoExpressionParserFacade.parseColumnMask("filter := true")); + } + + @Test + void testParsesContextReferences() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"owner\") == session_user()"); + + Assertions.assertEquals(ComparisonOperator.EQ, comparison.operator()); + Assertions.assertEquals("owner", ((Column) comparison.left()).name()); + Assertions.assertInstanceOf(SessionUser.class, comparison.right()); + + GroupMembership membership = + (GroupMembership) parseFilterExpression("is_group_member(\"auditors\")"); + Assertions.assertEquals("auditors", membership.group()); + } + + @Test + void testParsesPrecedenceAndLiteralArray() { + LogicalExpression or = + (LogicalExpression) + parseFilterExpression( + "col(\"region\") in [\"US\", \"CA\"] or " + + "col(\"level\") >= 3 and not col(\"deleted\") == true"); + + Assertions.assertEquals(LogicalOperator.OR, or.operator()); + Assertions.assertEquals(2, or.operands().size()); + + Comparison membership = (Comparison) or.operands().get(0); + Assertions.assertEquals(ComparisonOperator.IN, membership.operator()); + LiteralArray values = (LiteralArray) membership.right(); + Assertions.assertEquals(LiteralType.STRING, values.elementType()); + Assertions.assertEquals( + List.of("US", "CA"), + List.of(values.values().get(0).value(), values.values().get(1).value())); + + LogicalExpression and = (LogicalExpression) or.operands().get(1); + Assertions.assertEquals(LogicalOperator.AND, and.operator()); + Assertions.assertEquals( + ComparisonOperator.GTE, ((Comparison) and.operands().get(0)).operator()); + Not not = (Not) and.operands().get(1); + Assertions.assertEquals(ComparisonOperator.EQ, ((Comparison) not.operand()).operator()); + } + + @Test + void testParsesConditionalRowFilterAndLowersFirstMatch() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := col(\"tenant\") == session_user() if is_group_member(\"member\") " + + "else := col(\"public\") == true if session_user() == \"guest\" " + + "else := false"); + + Assertions.assertTrue(filter.isConditional()); + Assertions.assertEquals(2, filter.branches().size()); + Assertions.assertInstanceOf(GroupMembership.class, filter.branches().get(0).condition()); + Assertions.assertEquals( + ComparisonOperator.EQ, ((Comparison) filter.branches().get(1).condition()).operator()); + Assertions.assertEquals(false, ((Literal) filter.fallback()).value()); + Assertions.assertThrows(UnsupportedOperationException.class, () -> filter.branches().add(null)); + + CanonicalExpression expected = + parseFilterExpression( + "(is_group_member(\"member\") and col(\"tenant\") == session_user()) or " + + "(not is_group_member(\"member\") and " + + "((session_user() == \"guest\" and col(\"public\") == true) or " + + "(not session_user() == \"guest\" and false)))"); + Assertions.assertEquals(expected, filter.lower()); + Assertions.assertEquals(6, filter.lower().depth()); + } + + @Test + void testParsesConditionalColumnMask() { + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask( + "mask := action(\"show-last-4\") if is_group_member(\"support\") " + + "else := action(\"sha-256-query-local\") if session_user() == \"service\" " + + "else := action(\"replace-with-null\")"); + + Assertions.assertTrue(mask.isConditional()); + Assertions.assertEquals(2, mask.branches().size()); + Assertions.assertEquals(MaskAction.SHOW_LAST_4, mask.branches().get(0).action()); + Assertions.assertEquals(MaskAction.SHA_256_QUERY_LOCAL, mask.branches().get(1).action()); + Assertions.assertEquals(MaskAction.REPLACE_WITH_NULL, mask.fallback()); + Assertions.assertInstanceOf(GroupMembership.class, mask.branches().get(0).condition()); + } + + @ParameterizedTest + @ValueSource( + strings = { + "mask-alphanum", + "mask-to-fixed-value", + "replace-with-null", + "show-first-4", + "show-last-4", + "truncate-to-year", + "truncate-to-month", + "sha-256-global", + "sha-256-query-local" + }) + void testAcceptsAllMaskActions(String action) { + ColumnMask mask = + RestrictedRegoExpressionParserFacade.parseColumnMask("mask := action(\"" + action + "\")"); + Assertions.assertEquals(action, mask.fallback().value()); + } + + @Test + void testRetainsExactNumbersAndNegativeZero() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"ratio\") == -0.00"); + Literal literal = (Literal) comparison.right(); + + Assertions.assertEquals(new BigDecimal("0.00"), literal.value()); + Assertions.assertTrue(literal.negativeZero()); + + Comparison positiveZero = (Comparison) parseFilterExpression("col(\"ratio\") == 0.00"); + Assertions.assertNotEquals(literal, positiveZero.right()); + + Comparison one = (Comparison) parseFilterExpression("col(\"x\") == 1"); + Comparison onePointZero = (Comparison) parseFilterExpression("col(\"x\") == 1.00"); + Assertions.assertEquals(one.right(), onePointZero.right()); + } + + @Test + void testDecodesJsonStringsWithoutNormalizing() { + Comparison comparison = (Comparison) parseFilterExpression("col(\"a\\\"b.c\") == \"a'b\\n\""); + + Assertions.assertEquals("a\"b.c", ((Column) comparison.left()).name()); + Assertions.assertEquals("a'b\n", ((Literal) comparison.right()).value()); + + String escapedPair = "\\" + "uD83D" + "\\" + "uDE00"; + Comparison unicode = + (Comparison) parseFilterExpression("col(\"emoji\") == \"" + escapedPair + "\""); + Assertions.assertEquals("😀", ((Literal) unicode.right()).value()); + } + + @Test + void testSourceDepthLimit() { + CanonicalExpression depthEight = + parseFilterExpression("not not not not not not not col(\"active\") == true"); + Assertions.assertEquals(8, depthEight.depth()); + + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("not not not not not not not not col(\"active\") == true")); + } + + @Test + void testFlattensAssociativeBooleanChains() { + LogicalExpression and = + (LogicalExpression) + parseFilterExpression( + "true and true and true and true and true and true and true and true and true"); + Assertions.assertEquals(LogicalOperator.AND, and.operator()); + Assertions.assertEquals(9, and.operands().size()); + Assertions.assertEquals(2, and.depth()); + } + + @Test + void testDoesNotApplySourceDepthLimitToGeneratedPredicate() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if not not not not not not not col(\"active\") == true " + + "else := false"); + + Assertions.assertTrue(filter.lower().depth() > 8); + } + + @Test + void testFoldsDoubleNegationIntroducedByLowering() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if not is_group_member(\"g\") else := false"); + + CanonicalExpression expected = + parseFilterExpression( + "(not is_group_member(\"g\") and true) or " + "(is_group_member(\"g\") and false)"); + Assertions.assertEquals(expected, filter.lower()); + } + + @Test + void testLowersConditionalBranchesWithLinearNodeGrowth() { + RowFilter filter = + RestrictedRegoExpressionParserFacade.parseRowFilter(conditionalFilterWithBranches(24)); + + Assertions.assertEquals(265, countExpressionNodes(filter.lower())); + } + + @Test + void testSemanticErrorsDescribeOperandKindsWithoutValues() { + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, () -> parseFilterExpression("col(\"b\") < true")); + + Assertions.assertTrue(error.getMessage().contains("boolean literal supports only == and !=")); + Assertions.assertTrue(error.getMessage().contains("operator '<'")); + Assertions.assertTrue(error.getMessage().contains("left=column")); + Assertions.assertTrue(error.getMessage().contains("right=literal(BOOLEAN)")); + + IllegalArgumentException largeComparisonError = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("session_user() in " + literalArray(256))); + Assertions.assertTrue(largeComparisonError.getMessage().length() < 512); + Assertions.assertFalse(largeComparisonError.getMessage().contains("value=255")); + } + + @Test + void testRejectsInvalidUnicodeScalarsAndNul() { + String isolatedSurrogate = "\\" + "uD800"; + String nul = "\\" + "u0000"; + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + isolatedSurrogate + "\") == \"x\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"x\") == \"" + isolatedSurrogate + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + nul + "\") == \"x\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("is_group_member(\"" + nul + "\")")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"x\") == \"" + nul + "\"")); + } + + @Test + void testRejectsNonAsciiUnicodeEscapeDigits() { + List<String> invalidEscapes = + List.of( + unicodeEscape(0x0660, 0x0660, 0x0664, 0x0661), + unicodeEscape(0xFF10, 0xFF10, 0xFF14, 0xFF11), + unicodeEscape(0x0966, 0x0966, 0x096A, 0x0967), + unicodeEscape('0', '0', 0x0664, 0x0661)); + + for (String invalidEscape : invalidEscapes) { + IllegalArgumentException error = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"" + invalidEscape + "\") == \"x\"")); + Assertions.assertTrue(error.getMessage().contains("invalid Unicode escape")); + } + } + + @Test + void testResourceLimits() { + String sourcePrefix = "filter := true"; + String maximumSource = + sourcePrefix + " ".repeat(16 * 1024 - sourcePrefix.getBytes(StandardCharsets.UTF_8).length); + Assertions.assertDoesNotThrow( + () -> RestrictedRegoExpressionParserFacade.parseRowFilter(maximumSource)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RestrictedRegoExpressionParserFacade.parseRowFilter(maximumSource + " ")); + + String maximumString = "a".repeat(4 * 1024); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == \"" + maximumString + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == \"" + maximumString + "a\"")); + + String maximumUnicodeString = "😀".repeat(1024); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == \"" + maximumUnicodeString + "\"")); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == \"" + maximumUnicodeString + "😀\"")); + + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") in " + literalArray(256))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") in " + literalArray(257))); + + Assertions.assertDoesNotThrow( + () -> + parseFilterExpression( + "col(\"left\") in " + + literalArray(128) + + " or col(\"right\") in " + + literalArray(128))); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + parseFilterExpression( + "col(\"left\") in " + + literalArray(128) + + " or col(\"right\") in " + + literalArray(129))); + + String maximumNumber = "1" + "0".repeat(255); + Assertions.assertDoesNotThrow( + () -> parseFilterExpression("col(\"value\") == " + maximumNumber)); + Assertions.assertThrows( + IllegalArgumentException.class, + () -> parseFilterExpression("col(\"value\") == " + maximumNumber + "0")); + + Assertions.assertDoesNotThrow(() -> parseFilterExpression(balancedAndExpression(0, 85))); + Assertions.assertThrows( + IllegalArgumentException.class, () -> parseFilterExpression(balancedAndExpression(0, 86))); + } + + @Test + void testDoesNotApplySourceArrayLimitToGeneratedPredicate() { + RowFilter filter = + Assertions.assertDoesNotThrow( + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := true if col(\"value\") in " + + literalArray(256) + + " else := true if is_group_member(\"fallback\") else := false")); + + Assertions.assertEquals(512, countArrayElements(filter.lower())); + } + + @Test + void testRestrictedRegoGoldenFile() throws IOException { + String actual = renderGoldenResults(); + Path goldenFile = repositoryRoot().resolve(GOLDEN_FILE); + if (Boolean.parseBoolean(System.getenv(UPDATE_GOLDEN_ENV))) { + Files.writeString(goldenFile, actual, StandardCharsets.UTF_8); + } + + String expected = Files.readString(goldenFile, StandardCharsets.UTF_8); + Assertions.assertEquals( + expected, + actual, + "Golden output changed. Review the parser behavior and regenerate with " + + UPDATE_GOLDEN_ENV + + "=true if the change is intentional."); + } + + @Test + void testParsesDocumentedPrograms() throws IOException { + List<String> programs = documentedPrograms(repositoryRoot().resolve("docs/policies.md")); + + Assertions.assertFalse(programs.isEmpty()); + for (String program : programs) { + Assertions.assertDoesNotThrow(() -> RestrictedRegoExpressionParserFacade.parse(program)); + } + } + + @ParameterizedTest + @ValueSource( + strings = { + "col(\"owner\") == session_user()", + "session_user() != col(\"owner\")", + "session_user() == \"alice\"", + "\"alice\" != session_user()", + "session_user() in [\"alice\", \"bob\"]", + "is_group_member(\"auditors\")", + "col(\"region\") in [\"US\", \"CA\"]", + "not col(\"deleted\") == true", + "col(\"level\") >= 3 and col(\"region\") == \"US\"", + "col(\"nullable\") == null", + "null != col(\"nullable\")", + "true", + "false" + }) + void testAcceptsAllowlistedExpressions(String expression) { + Assertions.assertDoesNotThrow(() -> parseFilterExpression(expression)); + } + + @ParameterizedTest + @ValueSource( + strings = { + "col(\"owner\")", + "session_user()", + "\"alice\"", + "1", + "null", + "[\"US\"]", + "is_group_member(\"\")", + "is_group_member(\"auditors\") == true", + "col(\"left\") == col(\"right\")", + "1 == 1", + "col(\"active\") > true", + "false <= col(\"active\")", + "session_user() < \"bob\"", + "session_user() == true", + "session_user() == session_user()", + "col(\"region\") in []", + "col(\"region\") in [\"US\", 1]", + "col(\"region\") in [\"US\", null]", + "col(\"region\") == [\"US\"]", + "col(\"level\") + 1 > 3", + "col(\"level\") == 1e3", + "col(\"level\") == +1", + "col(\"level\") == 01", + "col(\"a\") < 2 < 3", + "notcol(\"active\") == true", + "trueand false", + "col(\"level\") == 1and col(\"region\") == \"US\"", + "session_userx() == \"alice\"", + "col(\"owner\") == \"alice\" trailing", + "identity(\"user\") == col(\"owner\")", + "input.user == \"alice\"", + "col(\"x\") == \"unterminated", + "col(\"x\") == \"line\nbreak\"" + }) + void testRejectsUnsupportedOrMalformedExpressions(String expression) { + Assertions.assertThrows( + IllegalArgumentException.class, () -> parseFilterExpression(expression)); + } + + @ParameterizedTest + @ValueSource( + strings = { + "", + " ", + "true", + "result := true", + "package restrictions", + "import data.rules", + "filter := true filter := false", + "filter := true { true }", + "filter := true # comment", + "filter := true if is_group_member(\"a\")", + "filter := true if true else := false if false", + "filter := true then false else := false", + "filter := \"not-boolean\"", + "mask := \"show-last-4\"", + "mask := action(\"unknown\")", + "mask := action(\"show-last-4\") if true", + "mask := action(\"show-last-4\") if col(\"region\") == \"US\" else := action(\"mask-alphanum\")", + "mask := action(\"show-last-4\") if true else := action(\"mask-alphanum\") if col(\"x\") == 1 else := action(\"replace-with-null\")", + "mask := true", + "filter := action(\"show-last-4\")" + }) + void testRejectsUnsupportedOrMalformedPrograms(String source) { + Assertions.assertThrows( + IllegalArgumentException.class, () -> RestrictedRegoExpressionParserFacade.parse(source)); + } + + @Test + void testReportsSpecificOneBasedSyntaxLocations() { + IllegalArgumentException identifierError = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter("filter := region == \"US\"")); + Assertions.assertTrue(identifierError.getMessage().contains("line 1, column 11")); + Assertions.assertTrue(identifierError.getMessage().contains("unsupported identifier region")); + + IllegalArgumentException characterError = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> RestrictedRegoExpressionParserFacade.parseRowFilter("filter := @")); + Assertions.assertTrue(characterError.getMessage().contains("line 1, column 11")); + + IllegalArgumentException leadingZeroError = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter("filter := col(\"x\") == 01")); + Assertions.assertTrue(leadingZeroError.getMessage().contains("leading zeros")); + + IllegalArgumentException chainedComparisonError = + Assertions.assertThrows( + IllegalArgumentException.class, + () -> + RestrictedRegoExpressionParserFacade.parseRowFilter( + "filter := col(\"x\") == 1 == true")); + Assertions.assertTrue( + chainedComparisonError.getMessage().contains("chained comparisons are not supported")); + } + + private static String renderGoldenResults() throws IOException { + StringBuilder result = new StringBuilder(GOLDEN_LICENSE_HEADER); + List<String> lines; + try (InputStream input = + TestRestrictedRegoExpressionParserFacade.class.getResourceAsStream(GOLDEN_CASES_RESOURCE)) { + Assertions.assertNotNull(input, "restricted Rego golden cases"); + lines = new String(input.readAllBytes(), StandardCharsets.UTF_8).lines().toList(); + } + + for (String line : lines) { + if (line.isBlank() || line.startsWith("#")) { + continue; + } + String[] fields = line.split("\\t", 2); + Assertions.assertEquals(2, fields.length, "Invalid restricted Rego golden case: " + line); + String name = fields[0]; + String source = fields[1]; + result.append('\n').append("=== ").append(name).append(" ===\n"); + result.append("source: ").append(escapeGoldenValue(source)).append('\n'); + try { + RestrictedRegoProgram program = RestrictedRegoExpressionParserFacade.parse(source); + result.append("result: ").append(escapeGoldenValue(program.toString())).append('\n'); + if (program instanceof RowFilter && ((RowFilter) program).isConditional()) { + result + .append("lowered: ") + .append(escapeGoldenValue(((RowFilter) program).lower().toString())) + .append('\n'); + } + } catch (IllegalArgumentException exception) { + result.append("error: ").append(escapeGoldenValue(exception.getMessage())).append('\n'); + } + } + return result.toString(); + } + + private static String escapeGoldenValue(String value) { + return value + .replace("\\", "\\\\") + .replace("\r", "\\r") + .replace("\n", "\\n") + .replace("\t", "\\t"); + } + + private static Path repositoryRoot() { + String repositoryRoot = System.getProperty(REPOSITORY_ROOT_PROPERTY); + if (repositoryRoot == null) { + throw new IllegalStateException("Missing test system property " + REPOSITORY_ROOT_PROPERTY); Review Comment: [Nit] The whole test class now fails outside Gradle, not just the docs test. `gravitino.repositoryRoot` is set only by `core/build.gradle.kts:109`, so running this class from an IDE, or under any other `Test` task, throws `IllegalStateException: Missing test system property gravitino.repositoryRoot`. Before 3ad6e25 the parent-walk found the root in both cases. Replacing a declared input with a system property was the right call for the drift problem - the walk did not make `docs/policies.md` an input - but the two are independent: the `inputs.file` declaration at line 108 is what fixes staleness, and the property only locates the file. Keeping the walk as a fallback when the property is absent gets both: `System.getProperty(...)` first, parent-walk second, the `IllegalStateException` only if neither works. Verified by: grepped both build files for `gravitino.repositoryRoot` - `core/build.gradle.kts:109` is the only setter - and read `repositoryRoot()` at lines 597-603 against the version at 48cd060. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
