Allow typed unknown resource and principal #1391
Conversation
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
shaobo-he-aws
requested review from
cdisselkoen,
adpaco-aws and
john-h-kastner-aws
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
Signed-off-by: Lőrinc Bódy <[email protected]>
…ore::ast::EntityType in the public api Signed-off-by: Lőrinc Bódy <[email protected]>
| let (arg1, arg2) = match ( | ||
| self.partial_interpret(arg1, slots)?, | ||
| self.partial_interpret(arg2, slots)?, | ||
| ) { | ||
| (PartialValue::Value(v1), PartialValue::Value(v2)) => (v1, v2), | ||
| (PartialValue::Value(v1), PartialValue::Residual(e2)) => { | ||
| return Ok(PartialValue::Residual(Expr::binary_app(*op, v1.into(), e2))) | ||
| if let Some(val) = self.short_circuit_typed_residual(&v1, &e2, *op) { |
There was a problem hiding this comment.
Nit: IMO, short_circuit_typed_residual should be "inlined" here, e.g., by more precise pattern matching.
There was a problem hiding this comment.
I could do that.. but it would get quite a but long
| @@ -901,6 +918,35 @@ impl<'e> Evaluator<'e> { | |||
| PartialValue::Residual(r) => Ok(Either::Right(r)), | |||
| } | |||
| } | |||
|
|
|||
| fn short_circuit_typed_residual( | |||
There was a problem hiding this comment.
Maybe we can do better here: Two unknowns with different types are not equal.
There was a problem hiding this comment.
Makes sense, that would be consistent
| /// | ||
| /// This information is taken into account when evaluating 'is', '==' and '!=' expressions. | ||
| #[must_use] | ||
| pub fn unknown_action_with_type(self, action_type: EntityTypeName) -> Self { |
There was a problem hiding this comment.
Should we be more conservative here: not all EntityTypeName are allowed; only those with basename Action.
There was a problem hiding this comment.
Well, setting a concrete action just above also doesn't have such a check.
|
It looks reasonable to me. That being said, this PR kinda motivates #209. Specifically, changes of PE should not impact concrete evaluation. |
Should we just hold off this PR, and then rebase it on top of yours? |
Description of changes
Allows the caller to create a request with an unknown principal or resource, that is still known to belong to a certain type.
I had to modify the evaluator a little bit, to take the type annotation into account for 'is' expressions. I know this is enroaching on the open rfc of https://github.com/cedar-policy/rfcs/pull/83/files
but I found it to be very useful for a 'query construction' usecase, similar to the one outlined here:
https://cedarland.blog/usage/partial-evaluation/content.html#what-can-alice-access
Without this, the only way to remove from the residual policies which are clearly excluded by the scope is to construct a full dummy entity ID of the right type, and then insert into the entity store an entry with that id.
But this requires all the fields of the entity to be explicitly set to 'unknown', and makes the evaluator assume the entity has no parents (since the parents of a concrete entity can't be unknown atm)
Issue #, if available
#1393
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)(I guess partial-eval is not part of the formal spec, right)
I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):