Add any and all operators

[emina]

Rendered

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[mwhicks1]

What about this alternative:

You evaluate the predicate on every member of the set, and collect all of the errors to be returned in a deterministic order? So in your example [1, true].all(it like "foo") you'd return the equivalent of

[(true,TypeError("Boolean found where String expected"),
 (1,TypeError("Long found where String expected"))]

The order in the list is based on a global order defined on Cedar values, e.g., that all Booleans come before all Longs.

[mwhicks1]

Alternatively, you could sort based on error type, and ignore the contents of the error message and the expression that accompanies it. I believe that matches the level of abstraction we have today when considering Cedar error messages in the model.

[emina]

I don't think that using a list is a viable alternative because it forces the specification to impose an (arbitrary) order on values and/or error types. Then, every implementation has to enforce the same order.

If we want to go this route, how about returning a set of error messages instead?

Not that this a non-trivial change because it changes the return type of the evaluator from Result<Value, Error> to Result<Value, Set<Error>>.

Another alternative is to package this error set into the QuantifierError.

[mwhicks1]

Specifying the result as a set feels like kicking the can; oftentimes sets are implemented as search trees, which require an ordering.

I feel that the loss in error specificity of the proposal is a potentially significant hit to the user experience. Without coupling errors with the actual expressions, we are still short of the best possible debugging experience.

[emina]

We can preserve the mapping: use a map from values to errors (instead of a set). That still avoids having to impose an arbitrary ordering in the specification, and allows implementations to use a HashMap if desired.

[mwhicks1]

Debug mode probably is another evaluator implementation which we have to prove/test is equivalent to the production one.

[mwhicks1]

It seems that this is still the sticking point of this RFC. To recap:

The proposal in the RFC is that any error that results from foo.any(...) or foo.all(...) is emitted from the evaluator as a QuantifierError with the location of the whole expression. The pro is a simple, efficient semantics, which admits short-circuiting when the first error occurs, and enjoys O(1) output size. The con is a lack of specific information about what element of foo was the source of the error, and what the error actually is (e.g., TypeError). @cdisselkoen and I are worried about the impact on user experience.

To overcome this con we could elect to package more details in the QuantifierError, e.g., as a map from element in foo to the associated error. Doing this adds a bit more complexity to the implementation, but not much. It eliminates the ability to do short-circuiting in evaluation once an error, and makes output size O(n) for the policy in the worst case. But these downsides are minimal, to my mind. Using a map does not impose an order on Cedar values (which would have added a lot of pain/complexity). The worst case (every element in the set produces an error) is unlikely to happen in practice, and it won't affect the performance of non-erroring evaluation. All of this is worth it, to my mind, because it values customer experience. Good errors are extremely important.

So: I propose we change the proposal to the above, and then accept it.

[cdisselkoen]

This sounds reasonable to me.

As an implementation note, the natural choice for the map from foo element to error would probably be a map from Expr to error. (Note that Expr implements Hash so it can be put in an unordered hashmap.) But I'm a little less clear on what type the keys should be if you are calling .any() or .all() on a set that originates from the entity data or Context. Expr is still one option (we could easily convert the RestrictedExprs to Expr and return them in that form), but RFC 34 proposes not keeping around the RestrictedExpr by the time we get to evaluation. At that point you'd have to have the key be Value, but those are not Hash. Values are totally ordered, so you can put them in a treemap, but then your concern above about appearing unordered is kind of a fiction. Moreover, if Values are indeed the key type, they'd have to be made public -- something which is also not currently the case but also proposed in RFC 34.

[cdisselkoen]

Perhaps an acceptable workaround to the above would be to have the key be the string representation of the Value; the string representation would be trivially total-ordered, hashable, and useful for debugging, while dodging the issues about whether Value is made public.

[emina]

I think we need to keep the size of the evaluator output constant. Letting it grow linearly with input size (policies + request + entities) means that the authorizer's output is worst-case quadratic in input size. This is undesirable even if rare.

I've revised the RFC to relax the specification of QuantifierError so that it can include more information in the error message, as long as the additional information is computed deterministically and is bounded in size.

[cdisselkoen]

I'll approve this as-is because I think it would be an improvement to the Cedar language and unlock additional use-cases. I'm still somewhat unsatisfied with an opaque QuantifierError instead of any specific, actionable feedback when a policy fails due to an error in a predicate. But I think the RFC as-written is still a positive change vs the status quo.

[emina]

I'll approve this as-is because I think it would be an improvement to the Cedar language and unlock additional use-cases. I'm still somewhat unsatisfied with an opaque QuantifierError instead of any specific, actionable feedback when a policy fails due to an error in a predicate. But I think the RFC as-written is still a positive change vs the status quo.

Agree on all counts. One thing that might help in the future is developing debugging tools for Cedar. Both @aaronjeline and @mwhicks1 have mentioned this in the past and in this conversation. This would likely need an alternative interpreter and/or interpretation mode, and we could support detailed error messages in the debugger.

[anwarmamat]

any behaves differently than||. any does not short-circuit while || does. For example. 2 > 1 || true > 1 evaluates to true, but [2, true].any(it > 1) errors.

[mwhicks1]

@emina I think @anwarmamat makes an important point here, which is glossed over in the text of the RFC: using || and && is order dependent due to short circuiting, whereas use of any and all cannot short circuit in order to give an order independent result. I'd aim to clarify this point in the RFC text throughout. (There is one place that talks about the use of contains as opposed to its encoding using any, but this is an indirect statement about the avoidance of short circuiting, not a direct one.)

[emina]

This is explained in detail in the semantics section: https://github.com/cedar-policy/rfcs/blob/any-and-all-operators/text/0021-any-and-all-operators.md#extending-the-semantics

In the case of errors, .any and .all will error when any expansion to || and && errors.

So, the meaning is "analogous" but not "equivalent".

[emina]

Added a forward pointer to the semantics section in the intro, explicitly stating that the expansion is not equivalent to the use of || and &&.

[khieta]

I had the same question as @anwarmamat here. I think the text could be more clear to callout that the behavior for any and || is different in the presence of errors.

[mwhicks1]

Looks good! Let's make it happen.

[emina]

The final comment period (FCP) for this FRC is starting now, with intent to accept the RFC.

The FCP will end 2023-11-06 at noon PT / 3pm ET / 7pm UTC. Please add comments, and especially any objections, if you have any. For more on the RFC process, see https://github.com/cedar-policy/rfcs.

[khieta]

Approving as-is, but I do have two small comments:

1. Added to the thread above.
2. What happens to containsAll and containsAny? Should we drop them from the language now that they can be implemented using all and any?

[john-h-kastner-aws]

I'm convinced that a single QuantifierError won't be good enough, but this still sound good as is.

[mwhicks1]

What happens to containsAll and containsAny? Should we drop them from the language now that they can be implemented using all and any?

I think we leave them, because their implementation can be more efficient than the corresponding implementations with .any and/or .all.

[emina]

What happens to containsAll and containsAny? Should we drop them from the language now that they can be implemented using all and any?

I think we leave them, because their implementation can be more efficient than the corresponding implementations with .any and/or .all.

And removing them would be a breaking change.

[emina]

I'm convinced that a single QuantifierError won't be good enough, but this still sound good as is.

I'm curious, why don't you think this will be sufficient? Note that QuantifierError can include additional implementation-specific information, as long as it's computed deterministically and bounded in size.

[khieta]

I think we leave them, because their implementation can be more efficient than the corresponding implementations with .any and/or .all.

Sounds good. Let's update the RFC text in case the implementer has this question down the line.

[emina]

After further investigation, we've found that even the simplified proposal for .any? / .all? is not amenable to automated reasoning. To support analysis in principle, we would have to restrict the operators even more: specifically, the expressions on the left and right hand side of the operators cannot both refer to the same variable. For practical analyzability, we believe that this restriction would need to be much tighter: the expression on the right-hand side should be a constant (literal).

It's not clear at this point that such a highly restricted version of .any? / .all? is sufficiently useful.

For that reason, I propose that we reject this RFC.

We can consider revisiting this decision in the future if there is enough demand for specialized any/all checks, such as anyLike? <pattern> / allLike? <pattern> on sets of strings.