Allow typed unknown resource and principal #1391
Conversation
shaobo-he-aws
requested review from
cdisselkoen,
adpaco-aws and
john-h-kastner-aws
| 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
There was a problem hiding this comment.
I tried and promptly rediscovered why I made it a separate method: because expr_kind in Expr is private, and can only be accessed trough the expr_kind() method.
Therefore I can't easily match it precisely in a single match expression (I could call expr_kind in a match guard, but then I would need to extract it twice, once with matches! in the guard, and once in the match body (since you can't have an if-let match guard).
Unless you want me to make the expr_kind field public (or public(crate)), I think we are better of with this solution.
cedar-policy/src/api.rs
Outdated
| /// | ||
| /// 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.
There was a problem hiding this comment.
It feels to me this is more of an useful convention than a fundamental property that will be forever true.
But if you say so, I can implement the check, or even remove this method, as it's dramatically less useful than unknown typed resource and principal
|
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? |
There's no need to hold off this PR as mine is postponed due to technical issues. I'll discuss this PR with other team members. |
|
After discussion, this PR looks reasonable to us and we will accept it once review feedback is addressed. |
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
…ore::ast::EntityType in the public api Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Signed-off-by: Lőrinc Bódy <lorinc.body@devrev.ai>
Thank you. However I will need some guidance on this one: Also, it seems my semver-breaking the AST broke the DRT build. Is that okay? I will just make a followup PR to the repo to fix it? |
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):