Implement RFC 55 #983

aaronjeline · 2024-06-13T20:17:30Z

Description of changes

(partially) Implements RFC 55
This does not implement the backwards compatibility functions, saving that for a second PR as this one is already large.

Issue #, if available

N/A

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).

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

Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

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

Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

aaronjeline · 2024-06-14T13:45:29Z

Note: one integration test will need change

khieta

Added a couple comments -- more will come later. I've decided to work on this review in parts.

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

cedar-policy/src/ffi/is_authorized.rs

cedar-policy/src/api.rs

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

cedar-policy/CHANGELOG.md

khieta

A few more comments. I'll wait until Monday to look at the rest of the validator code 😅 Great to see all this special casing for unspecified entities disappear!

cedar-policy-validator/src/coreschema.rs

cedar-policy-validator/src/human_schema/err.rs

cedar-policy-validator/src/human_schema/test.rs

cedar-policy/CHANGELOG.md

cedar-policy/tests/public_interface.rs

khieta · 2024-06-14T19:31:08Z

cedar-policy/tests/public_interface.rs

-        None,
-        Some(resource.clone()),
+        principal.clone(),
+        r#"__cedar::"Default""#.parse().unwrap(),


Is this going to work with RFC52?

Why wouldn't it?

It uses __cedar?

Did we decide the interactions of those RFCs? I.e., whether __cedar::Default will be one of the allowed uses of the __cedar namespace (like __cedar::Long)?

It has to be or we need to change the RFC. I was assuming it was because otherwise that part of the RFC doesn't make sense in the context of 52

The __cedar syntax if only mentioned in the Alternatives section of RFC 55, not in the main proposal. But the intention in the alternatives was that __cedar::Default would be builtin the same way as __cedar::Long.

(Regardless, I think it should be removed from the tests here since we decided not to support it as part of RFC 55.)

khieta · 2024-06-14T19:34:06Z

cedar-policy/tests/public_interface.rs

@@ -219,11 +219,11 @@ fn authorize_custom_request() -> Result<(), Box<dyn Error>> {
        Decision::Allow
    );

-    // Requesting with an unspecified principal or resource will return Deny (but not fail)
+    // Requesting with default principal or resource will return Deny (but not fail)


This test also makes it seem like __cedar::"Default" is something special. Are these tests really testing something interesting? I suspect they could be removed in favor of existing tests.

Eh, I think it's nice to have a confirmation our migration suggestion is breaking anything

khieta

Final batch of comments from me! I'll approve once my comments are addressed & you have a draft of the corresponding cedar-spec PR to fix DRT.

cedar-policy-validator/src/human_schema/to_json_schema.rs

cedar-policy-validator/src/human_schema/err.rs

cedar-policy-validator/src/rbac.rs

cedar-policy-validator/src/typecheck.rs

aaronjeline · 2024-06-17T15:14:10Z

Draft pr for cedar-integration-tests: cedar-policy/cedar-integration-tests#6

cedar-policy-cli/src/lib.rs

john-h-kastner-aws · 2024-06-17T16:55:33Z

cedar-policy-validator/src/human_schema/fmt.rs

Tagging @shaobo-he-aws to review the changes to this file and other human-schema related files.

john-h-kastner-aws · 2024-06-17T16:56:45Z

cedar-policy-validator/src/human_schema/grammar.lalrpop

@@ -21,17 +21,17 @@ use cedar_policy_core::parser::{Node, Loc, unescape::to_unescaped_string, cst::R
 use cedar_policy_core::ast::Id;
 use smol_str::SmolStr;
 use smol_str::ToSmolStr;
-use crate::human_schema::ast::{ 


Might be nice to split this white space fix into a separate PR.

Uh I think my editor did that automatically

john-h-kastner-aws · 2024-06-17T17:31:41Z

cedar-policy/tests/public_interface.rs

-        None,
-        Some(resource.clone()),
+        principal.clone(),
+        r#"__cedar::"Default""#.parse().unwrap(),


It uses __cedar?

john-h-kastner-aws · 2024-06-17T17:38:30Z

cedar-policy-validator/src/typecheck/test/unspecified_entity.rs

We should fold this in with the other schema parsing tests. It's doesn't test typechecking at all anymore

There will be tests that do when I add the backwards compat methods

cedar-policy/CHANGELOG.md

cdisselkoen

Love this PR and RFC, from the implementation perspective. So many of these changes make things simpler.

cedar-policy-cli/src/lib.rs

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

cedar-policy-core/src/est/expr.rs

cedar-policy-core/src/est/scope_constraints.rs

cedar-policy-validator/src/schema/namespace_def.rs

cedar-policy/src/ffi/is_authorized.rs

cdisselkoen · 2024-06-17T19:48:12Z

cedar-policy/tests/public_interface.rs

-        None,
-        Some(resource.clone()),
+        principal.clone(),
+        r#"__cedar::"Default""#.parse().unwrap(),


Did we decide the interactions of those RFCs? I.e., whether __cedar::Default will be one of the allowed uses of the __cedar namespace (like __cedar::Long)?

aaronjeline · 2024-06-18T14:56:24Z

DRT PR: cedar-policy/cedar-spec#366

Co-authored-by: Kesha Hietala <khieta@amazon.com> Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Co-authored-by: Craig Disselkoen <cdiss@amazon.com> Signed-off-by: Aaron Eline <aeline+github@amazon.com>

khieta · 2024-06-18T18:03:31Z

Only remaining comment from me is that you're using __cedar::Default in some tests. This will not be supported with the implementation of RFC52 -- these can be added as tests when implementing that RFC. I don't think they should be added here.

shaobo-he-aws

LGTM

shaobo-he-aws · 2024-06-18T18:37:03Z

Only remaining comment from me is that you're using __cedar::Default in some tests. This will not be supported with the implementation of RFC52 -- these can be added as tests when implementing that RFC. I don't think they should be added here.

I think we can keep these tests. I'll add them to my PR implementing RFC 52 when I sync it with main.

Signed-off-by: Aaron Eline <aeline+github@amazon.com>

aaronjeline force-pushed the aeline/rfc55 branch 2 times, most recently from 029038f to 3536e78 Compare June 14, 2024 13:44

aaronjeline requested review from john-h-kastner-aws and khieta June 14, 2024 13:44

khieta reviewed Jun 14, 2024

View reviewed changes

khieta mentioned this pull request Jun 14, 2024

more small edits to the JSON FFI #985

Merged

3 tasks

khieta reviewed Jun 14, 2024

View reviewed changes

khieta reviewed Jun 17, 2024

View reviewed changes

khieta mentioned this pull request Jun 17, 2024

Replace internal ::new() with From/Into for EntityUid and EntityTypeName #993

Merged

3 tasks

john-h-kastner-aws reviewed Jun 17, 2024

View reviewed changes

cdisselkoen approved these changes Jun 17, 2024

View reviewed changes

cdisselkoen mentioned this pull request Jun 17, 2024

[4.0] Consistent public error types in the Rust API #745

Closed

2 tasks

aaronjeline force-pushed the aeline/rfc55 branch from 810c5ad to c9cef68 Compare June 18, 2024 17:43

Removing unspecified

1c91167

Co-authored-by: Kesha Hietala <khieta@amazon.com> Co-authored-by: John Kastner <130772734+john-h-kastner-aws@users.noreply.github.com> Co-authored-by: Craig Disselkoen <cdiss@amazon.com> Signed-off-by: Aaron Eline <aeline+github@amazon.com>

aaronjeline force-pushed the aeline/rfc55 branch from c9cef68 to 1c91167 Compare June 18, 2024 17:54

aaronjeline requested review from khieta and john-h-kastner-aws June 18, 2024 17:54

shaobo-he-aws approved these changes Jun 18, 2024

View reviewed changes

Removing __cedar::"Default" from tests

0c0ca3c

Signed-off-by: Aaron Eline <aeline+github@amazon.com>

shaobo-he-aws approved these changes Jun 18, 2024

View reviewed changes

aaronjeline merged commit afd99f2 into main Jun 19, 2024
10 of 17 checks passed

aaronjeline deleted the aeline/rfc55 branch June 20, 2024 13:39

This was referenced Jun 20, 2024

cedar#745 for RequestValidationError #1010

Merged

Bunch more comments and docstrings in cedar-policy-validator #1003

Merged

introduce RawName wrapper type #1016

Merged

john-h-kastner-aws mentioned this pull request Jul 23, 2024

Fix benchmark; better CI for benchmarks; comments #1088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 55 #983

Implement RFC 55 #983

aaronjeline commented Jun 13, 2024 •

edited

Loading

aaronjeline commented Jun 14, 2024

khieta left a comment

khieta left a comment

khieta Jun 14, 2024

aaronjeline Jun 17, 2024

john-h-kastner-aws Jun 17, 2024

cdisselkoen Jun 17, 2024

aaronjeline Jun 17, 2024

khieta Jun 18, 2024

khieta Jun 18, 2024

khieta Jun 14, 2024

aaronjeline Jun 17, 2024

khieta left a comment

aaronjeline commented Jun 17, 2024

john-h-kastner-aws Jun 17, 2024

john-h-kastner-aws Jun 17, 2024

aaronjeline Jun 17, 2024

john-h-kastner-aws Jun 17, 2024

john-h-kastner-aws Jun 17, 2024

aaronjeline Jun 18, 2024

cdisselkoen left a comment

cdisselkoen Jun 17, 2024

aaronjeline commented Jun 18, 2024

khieta commented Jun 18, 2024

shaobo-he-aws left a comment

shaobo-he-aws commented Jun 18, 2024

Implement RFC 55 #983

Implement RFC 55 #983

Conversation

aaronjeline commented Jun 13, 2024 • edited Loading

Description of changes

Issue #, if available

Checklist for requesting a review

aaronjeline commented Jun 14, 2024

khieta left a comment

Choose a reason for hiding this comment

khieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khieta left a comment

Choose a reason for hiding this comment

aaronjeline commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdisselkoen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronjeline commented Jun 18, 2024

khieta commented Jun 18, 2024

shaobo-he-aws left a comment

Choose a reason for hiding this comment

shaobo-he-aws commented Jun 18, 2024

aaronjeline commented Jun 13, 2024 •

edited

Loading