Allow typed unknown resource and principal #1391

B-Lorentz · 2024-12-23T17:50:30Z

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):

A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

I'm not sure how my change impacts 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):

Does not require updates because my change does not impact the Cedar language specification.

cedar-policy-core/src/ast/request.rs

cedar-policy/src/api.rs

cdisselkoen

Thanks for this! Very nice code. Appreciate the helpful incidental refactors too.

cedar-policy-core/src/evaluator.rs

cedar-policy/CHANGELOG.md

shaobo-he-aws · 2025-01-04T21:01:49Z

cedar-policy-core/src/evaluator.rs

                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) {


Nit: IMO, short_circuit_typed_residual should be "inlined" here, e.g., by more precise pattern matching.

I could do that.. but it would get quite a but long

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-core/src/evaluator.rs

cedar-policy/src/api.rs

shaobo-he-aws · 2025-01-04T21:09:56Z

It looks reasonable to me. That being said, this PR kinda motivates #209. Specifically, changes of PE should not impact concrete evaluation.

B-Lorentz · 2025-01-05T14:02:31Z

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?

shaobo-he-aws · 2025-01-06T17:42:50Z

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.

shaobo-he-aws · 2025-01-06T18:25:29Z

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 <[email protected]>

…ore::ast::EntityType in the public api Signed-off-by: Lőrinc Bódy <[email protected]>

Signed-off-by: Lőrinc Bódy <[email protected]>

B-Lorentz · 2025-01-06T20:51:48Z

After discussion, this PR looks reasonable to us and we will accept it once review feedback is addressed.

Thank you. However I will need some guidance on this one:
#1391 (comment)
I can't see a clean way to inline it into the top-level match due to needing to look at some private fields that are only accessible by a helper method.

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?

shaobo-he-aws · 2025-01-07T17:11:11Z

Thank you. However I will need some guidance on this one: #1391 (comment) I can't see a clean way to inline it into the top-level match due to needing to look at some private fields that are only accessible by a helper method.

I'm fine with not inlining it.

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?

Yes, it's Ok. Please submit a PR to cedar-spec to fix it.

Signed-off-by: Lőrinc Bódy <[email protected]>

cedar-policy-core/src/evaluator.rs

Signed-off-by: Lőrinc Bódy <[email protected]>

B-Lorentz force-pushed the unknowns branch from 7f78c01 to c58b340 Compare December 23, 2024 17:53

shaobo-he-aws reviewed Dec 23, 2024

View reviewed changes

cedar-policy-core/src/ast/request.rs Show resolved Hide resolved

cedar-policy/src/api.rs Outdated Show resolved Hide resolved

cedar-policy/src/api.rs Show resolved Hide resolved

shaobo-he-aws requested review from cdisselkoen, adpaco-aws and john-h-kastner-aws December 23, 2024 21:52

B-Lorentz force-pushed the unknowns branch from deaa1a0 to 5fd3fb3 Compare December 23, 2024 21:56

shaobo-he-aws mentioned this pull request Jan 2, 2025

Allow an unknown principal, resource or action to be constrained to a certain entity type #1393

Closed

2 tasks

cdisselkoen approved these changes Jan 3, 2025

View reviewed changes

cedar-policy-core/src/evaluator.rs Show resolved Hide resolved

cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved

B-Lorentz requested a review from shaobo-he-aws January 4, 2025 12:37

shaobo-he-aws reviewed Jan 4, 2025

View reviewed changes

B-Lorentz added 12 commits January 6, 2025 21:46

introduce EntityUIDEntry::UnknownOfType

c474d2e

Signed-off-by: Lőrinc Bódy <[email protected]>

modify the evaluator to consider the type annotation in 'is' expressions

4f08117

Signed-off-by: Lőrinc Bódy <[email protected]>

add public api

3b44007

Signed-off-by: Lőrinc Bódy <[email protected]>

rename the builders to unknown_*_with_type

8f96787

Signed-off-by: Lőrinc Bódy <[email protected]>

Merge typed and untyped unknown into one enum value

178dc7f

Signed-off-by: Lőrinc Bódy <[email protected]>

short-cicuit also equal and not equal

aba1beb

Signed-off-by: Lőrinc Bódy <[email protected]>

Fix protobuf feature

7ee4cee

Signed-off-by: Lőrinc Bódy <[email protected]>

fix typo

0cbbffd

Signed-off-by: Lőrinc Bódy <[email protected]>

update comment

f158ef0

Signed-off-by: Lőrinc Bódy <[email protected]>

Use the cedar_policy EntityTypeName wrapper instead of cedar_policy_c…

f6d4cc5

…ore::ast::EntityType in the public api Signed-off-by: Lőrinc Bódy <[email protected]>

remove unknown_action_with_type

708914f

Signed-off-by: Lőrinc Bódy <[email protected]>

also resolve comparing two typed unknown entites

e03daa9

Signed-off-by: Lőrinc Bódy <[email protected]>

B-Lorentz force-pushed the unknowns branch from 3ac8ad6 to e03daa9 Compare January 6, 2025 20:48

shaobo-he-aws approved these changes Jan 7, 2025

View reviewed changes

fix formatting

73d4575

Signed-off-by: Lőrinc Bódy <[email protected]>

john-h-kastner-aws reviewed Jan 8, 2025

View reviewed changes

cedar-policy-core/src/evaluator.rs Show resolved Hide resolved

add comment explaining commutativeness and short_circuit_typed_residual

b097c1d

Signed-off-by: Lőrinc Bódy <[email protected]>

john-h-kastner-aws approved these changes Jan 8, 2025

View reviewed changes

shaobo-he-aws merged commit 1129542 into cedar-policy:main Jan 8, 2025
19 checks passed

B-Lorentz mentioned this pull request Jan 9, 2025

Minor refactor to partial eval #1417

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow typed unknown resource and principal #1391

Allow typed unknown resource and principal #1391

B-Lorentz commented Dec 23, 2024 •

edited

Loading

cdisselkoen left a comment

shaobo-he-aws Jan 4, 2025

B-Lorentz Jan 5, 2025

B-Lorentz Jan 6, 2025

shaobo-he-aws commented Jan 4, 2025

B-Lorentz commented Jan 5, 2025

shaobo-he-aws commented Jan 6, 2025

shaobo-he-aws commented Jan 6, 2025

B-Lorentz commented Jan 6, 2025

shaobo-he-aws commented Jan 7, 2025

Allow typed unknown resource and principal #1391

Allow typed unknown resource and principal #1391

Conversation

B-Lorentz commented Dec 23, 2024 • edited Loading

Description of changes

Issue #, if available

Checklist for requesting a review

cdisselkoen left a comment

Choose a reason for hiding this comment

shaobo-he-aws Jan 4, 2025

Choose a reason for hiding this comment

B-Lorentz Jan 5, 2025

Choose a reason for hiding this comment

B-Lorentz Jan 6, 2025

Choose a reason for hiding this comment

shaobo-he-aws commented Jan 4, 2025

B-Lorentz commented Jan 5, 2025

shaobo-he-aws commented Jan 6, 2025

shaobo-he-aws commented Jan 6, 2025

B-Lorentz commented Jan 6, 2025

shaobo-he-aws commented Jan 7, 2025

B-Lorentz commented Dec 23, 2024 •

edited

Loading