search

apache / accumulo-access

Apache Accumulo Access Control Library
https://accumulo.apache.org
Apache License 2.0
4 stars 11 forks source link

Add javadoc about AccessExpression normalization #77

Closed dlmarion closed 2 months ago

dlmarion commented 2 months ago

I want to add a note about normalization in the javadoc, as I don't think it's well defined. However, looking at ParserEvaluator (https://github.com/apache/accumulo-access/blob/main/src/main/java/org/apache/accumulo/access/ParserEvaluator.java#L95-L109), I'm not sure that it is short-circuiting.

dlmarion commented 2 months ago

I want to add a note about normalization in the javadoc, as I don't think it's well defined. However, looking at ParserEvaluator (https://github.com/apache/accumulo-access/blob/main/src/main/java/org/apache/accumulo/access/ParserEvaluator.java#L95-L109), I'm not sure that it is short-circuiting.

I think I have resolved this question myself, it looks like the short-circuiting is happening in the OR case. It's not done by stopping processing, but it continues processing to validate the expression but swaps out the test that is used to see if the token is in the auth set to a test that just returns true. The ParserEvaluator.parseAccessExpression is called from AccessExpressionImpl and AccessEvaluatorImpl. I can see why we need to validate the expression in AccessExpressionImpl. I'm not sure we need to re-validate the expression in AccessEvaluatorImpl.

keith-turner commented 2 months ago

I'm not sure we need to re-validate the expression in AccessEvaluatorImpl.

Un-validated expression can be passed to the AccessEvaluator because it accepts String and byte[], so its good to still validate those. In the case where a AccessExpression is passed in it is doing redundant work for the short circuit case.