yuqi1129 commented on code in PR #13478: URL: https://github.com/apache/gravitino/pull/13478#discussion_r4230354490
########## core/src/main/java/org/apache/gravitino/policy/expression/RestrictedRegoProgram.java: ########## @@ -0,0 +1,417 @@ +/* + * 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.expression; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import org.apache.gravitino.policy.expression.CanonicalExpression.Logical; +import org.apache.gravitino.policy.expression.CanonicalExpression.Not; +import org.apache.gravitino.policy.expression.CanonicalExpression.Operator; + +/** 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( + ExpressionValidation.isPredicate(result), "filter branch result must be Boolean"); + Preconditions.checkArgument( + ExpressionValidation.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; + } + + List<CanonicalExpression> disjuncts = new ArrayList<>(); + List<CanonicalExpression> precedingBranchMisses = new ArrayList<>(); + for (FilterBranch branch : branches) { + List<CanonicalExpression> selected = new ArrayList<>(precedingBranchMisses); + selected.add(branch.condition()); + selected.add(branch.result()); + disjuncts.add(new Logical(Operator.AND, selected)); + precedingBranchMisses.add(new Not(branch.condition())); Review Comment: Each branch appends one more `Not(condition)` that is repeated in every later disjunct, so the lowered predicate grows as O(n²) in the number of branches. For example, a filter with 63 branches of the form `true if col("cN") == 1` stays within the 256-node source limit, but `lower()` returns about 8.4k nodes (depth is only 4). Those conditions reference columns, so context binding cannot fold them, and they would end up in the Iceberg wire predicate. Since `validateProgram` already lowers the filter to check depth, could it also check the node count of the lowered predicate? It could reuse `MAX_AST_NODES` or use a separate documented resolved-predicate limit. The Limits section in the design doc should say which one applies. ########## core/src/main/java/org/apache/gravitino/policy/PolicyManager.java: ########## @@ -128,6 +129,7 @@ public PolicyEntity createPolicy( throws PolicyAlreadyExistsException { NameIdentifier metalakeIdent = NameIdentifierUtil.ofMetalake(metalake); checkMetalake(metalakeIdent, entityStore); + validateReadRestrictionContent(type, content); Review Comment: With this check, create and update now return 400 for any expression that is not a valid `restricted-rego-v1` program. However, `docs/policies.md` (lines 42-48) and `docs/open-api/policies.yaml` (`RowFilterContent` and `ColumnMaskContent`) still describe the field only as "a nonblank expression whose UTF-8 encoding must not exceed 16 KiB". The Javadoc of `PolicyContents.rowFilter` and `columnMask` says the same. Users who read only the docs have no way to write a valid expression. Could you add a short section to `docs/policies.md`? It should cover the `filter := ...` and `mask := action(...)` forms, the supported functions and operators, the limits, and a few examples. The OpenAPI descriptions should be updated to match, and `./gradlew :docs:build` run afterwards. ########## core/src/main/java/org/apache/gravitino/policy/expression/CanonicalExpression.java: ########## @@ -0,0 +1,797 @@ +/* + * 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.expression; + +import com.google.common.base.Preconditions; +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A node in an unresolved {@code restricted-rego-v1} expression tree. + * + * <p>The tree contains only the source constructs allowlisted by the read-restriction contract. + * Column and context references remain symbolic until the effective-policy resolver binds them to a + * table schema and request context. + */ +public interface CanonicalExpression { + + /** + * Validates this expression node and all of its children. + * + * @throws IllegalArgumentException if the expression is malformed or outside the allowlisted + * source profile + */ + void validate() throws IllegalArgumentException; + + /** + * Returns the source operation depth defined by {@code restricted-rego-v1}. + * + * @return expression depth + */ + int depth(); + + /** Operators supported by {@code restricted-rego-v1}. */ + enum Operator { + /** Equality comparison. */ + EQ("eq"), + /** Inequality comparison. */ + NEQ("neq"), + /** Less-than comparison. */ + LT("lt"), + /** Less-than-or-equal comparison. */ + LTE("lte"), + /** Greater-than comparison. */ + GT("gt"), + /** Greater-than-or-equal comparison. */ + GTE("gte"), + /** Literal-array membership comparison. */ + IN("in"), + /** Boolean conjunction. */ + AND("and"), + /** Boolean disjunction. */ + OR("or"), + /** Boolean negation. */ + NOT("not"); Review Comment: Nit: `Operator.NOT` is never used, because `Not` does not carry an operator. `fromSourceToken` handles only comparison tokens, which is probably why `and`/`or` look unsupported there. Also, the Javadoc says `canonicalName` is "the canonical operator name used by the resolved model", but the resolved model in the design doc uses the Iceberg names (`not-eq`, `lt-eq`, `gt-eq`), not `neq`, `lte`, `gte`. How about splitting this into `ComparisonOperator` and `LogicalOperator { AND, OR }`, removing `NOT`, and either aligning the names with Iceberg or rewording the Javadoc? ########## core/src/test/java/org/apache/gravitino/policy/expression/TestRestrictedRegoExpressionParserFacade.java: ########## @@ -0,0 +1,470 @@ +/* + * 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.expression; + +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.util.List; +import org.apache.gravitino.policy.expression.CanonicalExpression.Column; +import org.apache.gravitino.policy.expression.CanonicalExpression.Comparison; +import org.apache.gravitino.policy.expression.CanonicalExpression.GroupMembership; +import org.apache.gravitino.policy.expression.CanonicalExpression.Literal; +import org.apache.gravitino.policy.expression.CanonicalExpression.LiteralArray; +import org.apache.gravitino.policy.expression.CanonicalExpression.LiteralType; +import org.apache.gravitino.policy.expression.CanonicalExpression.Logical; +import org.apache.gravitino.policy.expression.CanonicalExpression.Not; +import org.apache.gravitino.policy.expression.CanonicalExpression.Operator; +import org.apache.gravitino.policy.expression.CanonicalExpression.SessionUser; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.RowFilter; +import org.apache.gravitino.policy.expression.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 { + + @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(Operator.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() { + Logical or = + (Logical) + parseFilterExpression( + "col(\"region\") in [\"US\", \"CA\"] or " + + "col(\"level\") >= 3 and not col(\"deleted\") == true"); + + Assertions.assertEquals(Operator.OR, or.operator()); + Assertions.assertEquals(2, or.operands().size()); + + Comparison membership = (Comparison) or.operands().get(0); + Assertions.assertEquals(Operator.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())); + + Logical and = (Logical) or.operands().get(1); + Assertions.assertEquals(Operator.AND, and.operator()); + Assertions.assertEquals(Operator.GTE, ((Comparison) and.operands().get(0)).operator()); + Not not = (Not) and.operands().get(1); + Assertions.assertEquals(Operator.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( + Operator.EQ, ((Comparison) filter.branches().get(1).condition()).operator()); + Assertions.assertEquals(false, ((Literal) filter.fallback()).value()); + Assertions.assertThrows(UnsupportedOperationException.class, () -> filter.branches().add(null)); + + Logical lowered = (Logical) filter.lower(); Review Comment: These assertions only check node types and where the `Not` nodes are. I replaced `selected.add(branch.result())` with `selected.add(fallback)` in `lower()`, so every branch returns the fallback, and all 88 tests in this class still pass. Since the nodes have structural `equals`, could we compare against the expected tree directly? For example: ```java RowFilter expected = RestrictedRegoExpressionParserFacade.parseRowFilter( "filter := (c1 and r1) or (not c1 and c2 and r2) or (not c1 and not c2 and fb)"); Assertions.assertEquals(expected.fallback(), filter.lower()); ``` ########## core/src/main/java/org/apache/gravitino/policy/expression/RestrictedRegoProgram.java: ########## @@ -0,0 +1,417 @@ +/* + * 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.expression; + +import com.google.common.base.Preconditions; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import org.apache.gravitino.policy.expression.CanonicalExpression.Logical; +import org.apache.gravitino.policy.expression.CanonicalExpression.Not; +import org.apache.gravitino.policy.expression.CanonicalExpression.Operator; + +/** 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( + ExpressionValidation.isPredicate(result), "filter branch result must be Boolean"); + Preconditions.checkArgument( + ExpressionValidation.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; + } + + List<CanonicalExpression> disjuncts = new ArrayList<>(); + List<CanonicalExpression> precedingBranchMisses = new ArrayList<>(); + for (FilterBranch branch : branches) { + List<CanonicalExpression> selected = new ArrayList<>(precedingBranchMisses); + selected.add(branch.condition()); + selected.add(branch.result()); + disjuncts.add(new Logical(Operator.AND, selected)); Review Comment: Nit: the parser now flattens `and`/`or` chains, but `lower()` still nests a source `and` chain inside the generated conjunction (`AND(NOT c, AND(a, ...))`). Because of this, save-time validation is stricter than the depth definition in the design doc. For example, `filter := true if is_group_member("g") else := <an and/or chain with source depth 7>` has lowered depth 8 under the documented definition, but it is rejected here as depth 9. Flattening same-operator operands when building `Logical` would fix it. ########## core/src/main/java/org/apache/gravitino/policy/expression/CanonicalExpression.java: ########## @@ -0,0 +1,797 @@ +/* + * 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.expression; + +import com.google.common.base.Preconditions; +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Objects; + +/** + * A node in an unresolved {@code restricted-rego-v1} expression tree. + * + * <p>The tree contains only the source constructs allowlisted by the read-restriction contract. + * Column and context references remain symbolic until the effective-policy resolver binds them to a + * table schema and request context. + */ +public interface CanonicalExpression { + + /** + * Validates this expression node and all of its children. + * + * @throws IllegalArgumentException if the expression is malformed or outside the allowlisted + * source profile + */ + void validate() throws IllegalArgumentException; + + /** + * Returns the source operation depth defined by {@code restricted-rego-v1}. + * + * @return expression depth + */ + int depth(); + + /** Operators supported by {@code restricted-rego-v1}. */ + enum Operator { + /** Equality comparison. */ + EQ("eq"), + /** Inequality comparison. */ + NEQ("neq"), + /** Less-than comparison. */ + LT("lt"), + /** Less-than-or-equal comparison. */ + LTE("lte"), + /** Greater-than comparison. */ + GT("gt"), + /** Greater-than-or-equal comparison. */ + GTE("gte"), + /** Literal-array membership comparison. */ + IN("in"), + /** Boolean conjunction. */ + AND("and"), + /** Boolean disjunction. */ + OR("or"), + /** Boolean negation. */ + NOT("not"); + + private final String canonicalName; + + Operator(String canonicalName) { + this.canonicalName = canonicalName; + } + + /** + * Parses an operator token from restricted Rego source. + * + * @param token source operator token + * @return parsed operator + */ + public static Operator fromSourceToken(String token) { + Preconditions.checkArgument(token != null && !token.isEmpty(), "operator cannot be empty"); + switch (token) { + case "==": + return EQ; + case "!=": + return NEQ; + case "<": + return LT; + case "<=": + return LTE; + case ">": + return GT; + case ">=": + return GTE; + case "in": + return IN; + default: + throw new IllegalArgumentException("Unsupported restricted-rego-v1 operator: " + token); + } + } + + /** + * Returns the canonical operator name used by the resolved model. + * + * @return canonical operator name + */ + public String canonicalName() { + return canonicalName; + } + } + + /** Source literal types supported by {@code restricted-rego-v1}. */ + enum LiteralType { + /** String literal. */ + STRING, + /** Exact base-10 numeric literal. */ + NUMBER, + /** Boolean literal. */ + BOOLEAN, + /** Null literal. */ + NULL + } + + /** A binary comparison operation. */ + final class Comparison implements CanonicalExpression { + private final Operator operator; + private final CanonicalExpression left; + private final CanonicalExpression right; + + Comparison(Operator operator, CanonicalExpression left, CanonicalExpression right) { + this.operator = operator; + this.left = left; + this.right = right; + } + + /** + * Returns the comparison operator. + * + * @return comparison operator + */ + public Operator operator() { + return operator; + } + + /** + * Returns the left operand. + * + * @return left operand + */ + public CanonicalExpression left() { + return left; + } + + /** + * Returns the right operand. + * + * @return right operand + */ + public CanonicalExpression right() { + return right; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + ExpressionValidation.isComparisonOperator(operator), + "comparison requires a comparison operator"); + Preconditions.checkArgument( + left != null && right != null, "comparison operands cannot be null"); + left.validate(); + right.validate(); + ExpressionValidation.validateComparison(operator, left, right); + } + + @Override + public int depth() { + return 1; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Comparison)) { + return false; + } + Comparison that = (Comparison) other; + return operator == that.operator + && Objects.equals(left, that.left) + && Objects.equals(right, that.right); + } + + @Override + public int hashCode() { + return Objects.hash(operator, left, right); + } + + @Override + public String toString() { + return "Comparison{" + "operator=" + operator + ", left=" + left + ", right=" + right + '}'; + } + } + + /** A Boolean negation operation. */ + final class Not implements CanonicalExpression { + private final CanonicalExpression operand; + + Not(CanonicalExpression operand) { + this.operand = operand; + } + + /** + * Returns the negated predicate. + * + * @return negated predicate + */ + public CanonicalExpression operand() { + return operand; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(operand != null, "not requires an operand"); + operand.validate(); + Preconditions.checkArgument( + ExpressionValidation.isPredicate(operand), "not operand must be a boolean predicate"); + } + + @Override + public int depth() { + return 1 + (operand == null ? 0 : operand.depth()); + } + + @Override + public boolean equals(Object other) { + return other instanceof Not && Objects.equals(operand, ((Not) other).operand); + } + + @Override + public int hashCode() { + return Objects.hash(operand); + } + + @Override + public String toString() { + return "Not{" + "operand=" + operand + '}'; + } + } + + /** An n-ary Boolean conjunction or disjunction. */ + final class Logical implements CanonicalExpression { + private final Operator operator; + private final List<CanonicalExpression> operands; + + Logical(Operator operator, List<CanonicalExpression> operands) { + this.operator = operator; + this.operands = + operands == null ? null : Collections.unmodifiableList(new ArrayList<>(operands)); + } + + /** + * Returns the logical operator. + * + * @return {@link Operator#AND} or {@link Operator#OR} + */ + public Operator operator() { + return operator; + } + + /** + * Returns the logical operands. + * + * @return immutable operands + */ + public List<CanonicalExpression> operands() { + return operands; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + operator == Operator.AND || operator == Operator.OR, + "logical expression requires AND or OR"); + Preconditions.checkArgument( + operands != null && operands.size() >= 2, + "%s requires at least two operands", + operator.canonicalName()); + for (CanonicalExpression child : operands) { + Preconditions.checkArgument( + child != null, "%s operand cannot be null", operator.canonicalName()); + child.validate(); + Preconditions.checkArgument( + ExpressionValidation.isPredicate(child), + "%s operands must be boolean predicates", + operator.canonicalName()); + } + } + + @Override + public int depth() { + int childDepth = 0; + if (operands != null) { + for (CanonicalExpression child : operands) { + if (child != null) { + childDepth = Math.max(childDepth, child.depth()); + } + } + } + return 1 + childDepth; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Logical)) { + return false; + } + Logical that = (Logical) other; + return operator == that.operator && Objects.equals(operands, that.operands); + } + + @Override + public int hashCode() { + return Objects.hash(operator, operands); + } + + @Override + public String toString() { + return "Logical{" + "operator=" + operator + ", operands=" + operands + '}'; + } + } + + /** A symbolic top-level column reference. */ + final class Column implements CanonicalExpression { + private final String name; + + Column(String name) { + this.name = name; + } + + /** + * Returns the exact decoded column name. + * + * @return column name + */ + public String name() { + return name; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(name != null && !name.isEmpty(), "column name cannot be empty"); + Preconditions.checkArgument(name.indexOf('\0') < 0, "column name cannot contain NUL"); + ExpressionValidation.validateUnicodeScalars(name, "column name"); + } + + @Override + public int depth() { + return 0; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Column)) { + return false; + } + Column that = (Column) other; + return Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return Objects.hash(name); + } + + @Override + public String toString() { + return "Column{" + "name='" + name + '\'' + '}'; + } + } + + /** A symbolic reference to the trusted effective session user. */ + final class SessionUser implements CanonicalExpression { + /** Creates a session-user reference. */ + SessionUser() {} + + @Override + public void validate() throws IllegalArgumentException {} + + @Override + public int depth() { + return 0; + } + + @Override + public boolean equals(Object other) { + return other instanceof SessionUser; + } + + @Override + public int hashCode() { + return SessionUser.class.hashCode(); + } + + @Override + public String toString() { + return "SessionUser{}"; + } + } + + /** A request-context group-membership predicate. */ + final class GroupMembership implements CanonicalExpression { + private final String group; + + GroupMembership(String group) { + this.group = group; + } + + /** + * Returns the exact decoded group name. + * + * @return group name + */ + public String group() { + return group; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(group != null && !group.isEmpty(), "group name cannot be empty"); + ExpressionValidation.validateUnicodeScalars(group, "group name"); + } + + @Override + public int depth() { + return 1; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof GroupMembership)) { + return false; + } + GroupMembership that = (GroupMembership) other; + return Objects.equals(group, that.group); + } + + @Override + public int hashCode() { + return Objects.hash(group); + } + + @Override + public String toString() { + return "GroupMembership{" + "group='" + group + '\'' + '}'; + } + } + + /** A decoded source literal. */ + final class Literal implements CanonicalExpression { + private final LiteralType literalType; + + private final Object value; + + private final boolean negativeZero; + + Literal(LiteralType literalType, Object value) { + this(literalType, value, false); + } + + Literal(LiteralType literalType, Object value, boolean negativeZero) { + this.literalType = literalType; + this.value = value; + this.negativeZero = negativeZero; + } + + /** + * Returns the source literal type. + * + * @return literal type + */ + public LiteralType literalType() { + return literalType; + } + + /** + * Returns the decoded value. + * + * @return decoded value, or {@code null} for a null literal + */ + public Object value() { + return value; + } + + /** + * Returns whether the numeric token was a negative zero. + * + * @return {@code true} only for a numeric token with a leading minus and mathematical value + * zero + */ + public boolean negativeZero() { + return negativeZero; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument(literalType != null, "literalType cannot be null"); + switch (literalType) { + case STRING: + Preconditions.checkArgument(value instanceof String, "string literal must be a string"); + Preconditions.checkArgument(!negativeZero, "string literal cannot be negative zero"); + ExpressionValidation.validateUnicodeScalars((String) value, "string literal"); + break; + case NUMBER: + Preconditions.checkArgument( + value instanceof BigDecimal, "number literal must be an exact decimal"); + Preconditions.checkArgument( + !negativeZero || ((BigDecimal) value).signum() == 0, + "negative-zero marker requires a zero value"); + break; + case BOOLEAN: + Preconditions.checkArgument(value instanceof Boolean, "boolean literal must be boolean"); + Preconditions.checkArgument(!negativeZero, "boolean literal cannot be negative zero"); + break; + case NULL: + Preconditions.checkArgument(value == null, "null literal cannot contain a value"); + Preconditions.checkArgument(!negativeZero, "null literal cannot be negative zero"); + break; + default: + throw new IllegalArgumentException("Unsupported literal type: " + literalType); + } + } + + @Override + public int depth() { + return literalType == LiteralType.BOOLEAN ? 1 : 0; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Literal)) { + return false; + } + Literal that = (Literal) other; + if (literalType != that.literalType || negativeZero != that.negativeZero) { + return false; + } + if (literalType == LiteralType.NUMBER) { + return ((BigDecimal) value).compareTo((BigDecimal) that.value) == 0; + } + return Objects.equals(value, that.value); + } + + @Override + public int hashCode() { + Object normalizedValue = + literalType == LiteralType.NUMBER ? ((BigDecimal) value).stripTrailingZeros() : value; + return Objects.hash(literalType, normalizedValue, negativeZero); + } + + @Override + public String toString() { + return "Literal{" + + "literalType=" + + literalType + + ", value=" + + value + + ", negativeZero=" + + negativeZero + + '}'; + } + } + + /** A non-empty homogeneous array of non-null source literals. */ + final class LiteralArray implements CanonicalExpression { + private final LiteralType elementType; + + private final List<Literal> values; + + LiteralArray(LiteralType elementType, List<Literal> values) { + this.elementType = elementType; + this.values = values == null ? null : Collections.unmodifiableList(new ArrayList<>(values)); + } + + /** + * Returns the common element type. + * + * @return element type + */ + public LiteralType elementType() { + return elementType; + } + + /** + * Returns the literal values. + * + * @return immutable literal values + */ + public List<Literal> values() { + return values; + } + + @Override + public void validate() throws IllegalArgumentException { + Preconditions.checkArgument( + elementType != null && elementType != LiteralType.NULL, + "array element type must be non-null"); + Preconditions.checkArgument(values != null && !values.isEmpty(), "array cannot be empty"); + for (Literal value : values) { + Preconditions.checkArgument(value != null, "array cannot contain a null node"); + value.validate(); + Preconditions.checkArgument( + value.literalType() == elementType, + "array literals must have one homogeneous non-null type"); + } + } + + @Override + public int depth() { + return 0; + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof LiteralArray)) { + return false; + } + LiteralArray that = (LiteralArray) other; + return elementType == that.elementType && Objects.equals(values, that.values); + } + + @Override + public int hashCode() { + return Objects.hash(elementType, values); + } + + @Override + public String toString() { + return "LiteralArray{" + "elementType=" + elementType + ", values=" + values + '}'; + } + } +} + +final class ExpressionValidation { + private ExpressionValidation() {} + + static boolean isPredicate(CanonicalExpression expression) { + if (expression instanceof CanonicalExpression.Comparison + || expression instanceof CanonicalExpression.Not + || expression instanceof CanonicalExpression.Logical + || expression instanceof CanonicalExpression.GroupMembership) { + return true; + } + return expression instanceof CanonicalExpression.Literal + && ((CanonicalExpression.Literal) expression).literalType() + == CanonicalExpression.LiteralType.BOOLEAN; + } + + static boolean isContextOnly(CanonicalExpression expression) { + if (expression instanceof CanonicalExpression.Column) { + return false; + } + if (expression instanceof CanonicalExpression.Comparison) { + CanonicalExpression.Comparison comparison = (CanonicalExpression.Comparison) expression; + return isContextOnly(comparison.left()) && isContextOnly(comparison.right()); + } + if (expression instanceof CanonicalExpression.Not) { + return isContextOnly(((CanonicalExpression.Not) expression).operand()); + } + if (expression instanceof CanonicalExpression.Logical) { + for (CanonicalExpression child : ((CanonicalExpression.Logical) expression).operands()) { + if (!isContextOnly(child)) { + return false; + } + } + } + return true; + } + + static boolean isComparisonOperator(CanonicalExpression.Operator operator) { + return operator == CanonicalExpression.Operator.EQ + || operator == CanonicalExpression.Operator.NEQ + || operator == CanonicalExpression.Operator.LT + || operator == CanonicalExpression.Operator.LTE + || operator == CanonicalExpression.Operator.GT + || operator == CanonicalExpression.Operator.GTE + || operator == CanonicalExpression.Operator.IN; + } + + static void validateComparison( + CanonicalExpression.Operator operator, CanonicalExpression left, CanonicalExpression right) { + if (operator == CanonicalExpression.Operator.IN) { + Preconditions.checkArgument( + right instanceof CanonicalExpression.LiteralArray, + "right operand of in must be a literal array"); + Preconditions.checkArgument( + left instanceof CanonicalExpression.Column + || left instanceof CanonicalExpression.SessionUser, + "left operand of in must be col(...) or session_user()"); + if (left instanceof CanonicalExpression.SessionUser) { + CanonicalExpression.LiteralArray array = (CanonicalExpression.LiteralArray) right; + Preconditions.checkArgument( + array.elementType() == CanonicalExpression.LiteralType.STRING, + "session_user() can only be tested against a string array"); + } + return; + } + + Preconditions.checkArgument( + !(left instanceof CanonicalExpression.LiteralArray) + && !(right instanceof CanonicalExpression.LiteralArray), + "%s does not support array operands", + operator.canonicalName()); + + boolean equality = + operator == CanonicalExpression.Operator.EQ || operator == CanonicalExpression.Operator.NEQ; + if (isColumnLiteralPair(left, right)) { + CanonicalExpression.Literal literal = + left instanceof CanonicalExpression.Literal + ? (CanonicalExpression.Literal) left + : (CanonicalExpression.Literal) right; + Preconditions.checkArgument( + literal.literalType() != CanonicalExpression.LiteralType.NULL || equality, + "null supports only == and !="); + Preconditions.checkArgument( + literal.literalType() != CanonicalExpression.LiteralType.BOOLEAN || equality, + "boolean literal supports only == and !="); + return; + } + + if (isColumnSessionUserPair(left, right)) { + Preconditions.checkArgument(equality, "session_user() supports only == and !="); + return; + } + + if (isSessionUserStringPair(left, right)) { + Preconditions.checkArgument(equality, "session_user() supports only == and !="); + return; + } + + throw new IllegalArgumentException( Review Comment: Nit: semantic errors raised from `validate()` still have no location. For example, `col("b") < true` reports only `boolean literal supports only == and !=`, and `1 == 1` reports only `Unsupported operands for eq in restricted-rego-v1`. In a policy with several branches, it is hard to find which comparison is wrong. Could the nodes keep the line and column of their start token, or could the message at least include the offending sub-expression? ########## core/src/main/java/org/apache/gravitino/policy/expression/RestrictedRegoExpressionParserFacade.java: ########## @@ -0,0 +1,750 @@ +/* + * 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.expression; + +import com.google.common.base.Preconditions; +import java.math.BigDecimal; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.policy.expression.CanonicalExpression.Comparison; +import org.apache.gravitino.policy.expression.CanonicalExpression.GroupMembership; +import org.apache.gravitino.policy.expression.CanonicalExpression.Literal; +import org.apache.gravitino.policy.expression.CanonicalExpression.LiteralArray; +import org.apache.gravitino.policy.expression.CanonicalExpression.LiteralType; +import org.apache.gravitino.policy.expression.CanonicalExpression.Logical; +import org.apache.gravitino.policy.expression.CanonicalExpression.Not; +import org.apache.gravitino.policy.expression.CanonicalExpression.Operator; +import org.apache.gravitino.policy.expression.CanonicalExpression.SessionUser; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.ColumnMask; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.FilterBranch; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.MaskAction; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.MaskBranch; +import org.apache.gravitino.policy.expression.RestrictedRegoProgram.RowFilter; + +/** Parses and validates complete programs in the {@code restricted-rego-v1} source dialect. */ +public final class RestrictedRegoExpressionParserFacade { + + private static final int MAX_SOURCE_BYTES = 16 * 1024; Review Comment: Nit: this duplicates the public `ReadRestrictionContent.MAX_SOURCE_LENGTH_BYTES` in `api`. Could we reuse that constant so the two limits cannot drift apart? -- 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]
