RFC 53 implementation #1377

shaobo-he-aws · 2024-12-16T21:39:45Z

Description of changes

Issue #, if available

Checklist for requesting a review

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

A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new 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):

Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-docs. PRs should be targeted at a staging-X.Y branch, not main.)

Signed-off-by: Shaobo He <shaobohe@amazon.com>

cdisselkoen

Scanned through this, looks like a reasonable high-level approach to me

cedar-policy-validator/src/json_schema.rs

cdisselkoen · 2025-01-02T16:34:25Z

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

-pub struct ValidatorEntityType {
-    /// The name of the entity type.
-    pub(crate) name: EntityType,
-
-    /// The set of entity types that can be members of this entity type. When
-    /// this structure is initially constructed, the field will contain direct
-    /// children, but it will be updated to contain the closure of all
-    /// descendants before it is used in any validation.
-    pub descendants: HashSet<EntityType>,
-
-    /// The attributes associated with this entity.
-    pub(crate) attributes: Attributes,
-
-    /// Indicates that this entity type may have additional attributes
-    /// other than the declared attributes that may be accessed under partial
-    /// schema validation. We do not know if they are present, and do not know
-    /// their type when they are present. Attempting to access an undeclared
-    /// attribute under standard validation is an error regardless of this flag.
-    pub(crate) open_attributes: OpenTag,
-
-    /// Tag type for this entity type. `None` indicates that entities of this
-    /// type are not allowed to have tags.
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub(crate) tags: Option<Type>,
+pub enum ValidatorEntityType {
+    Common {
+        /// The name of the entity type.
+        name: EntityType,
+
+        /// The set of entity types that can be members of this entity type. When
+        /// this structure is initially constructed, the field will contain direct
+        /// children, but it will be updated to contain the closure of all
+        /// descendants before it is used in any validation.
+        descendants: HashSet<EntityType>,
+
+        /// The attributes associated with this entity.
+        attributes: Attributes,
+
+        /// Indicates that this entity type may have additional attributes
+        /// other than the declared attributes that may be accessed under partial
+        /// schema validation. We do not know if they are present, and do not know
+        /// their type when they are present. Attempting to access an undeclared
+        /// attribute under standard validation is an error regardless of this flag.
+        open_attributes: OpenTag,
+
+        /// Tag type for this entity type. `None` indicates that entities of this
+        /// type are not allowed to have tags.
+        #[serde(skip_serializing_if = "Option::is_none")]
+        tags: Option<Type>,
+    },


This change makes the fields pub instead of pub(crate), this is probably fine, just drawing attention to this in case someone thinks they shouldn't be pub

Signed-off-by: Shaobo He <shaobohe@amazon.com>

john-h-kastner-aws

Looks pretty good

cedar-policy-core/src/entities/json/schema.rs

cedar-policy-validator/src/cedar_schema/ast.rs

cedar-policy-validator/src/coreschema.rs

cedar-policy-validator/src/diagnostics/validation_errors.rs

cedar-policy-validator/src/json_schema.rs

john-h-kastner-aws · 2025-01-08T15:01:46Z

cedar-policy-validator/src/lib.rs

@@ -202,6 +202,7 @@ impl Validator {
        } else {
            Some(
                self.validate_entity_types(p)
+                    .chain(self.validate_enum_entity(p))


Noting that this will mean enum entity validation doesn't short-circuit, matching the current behavior for validating entity types and actions. This is a good design decision IMO, but might mean we run into some differences between the Rust and Lean impls in DRT, so I'll just leave a comment here so we don't forget that it's expected

I was thinking if we should implement enum entity validity checking in the type checker. What's your thought on this?

I wouldn't do that in this PR. Could consider moving it along with the entity-type check later

cedar-policy-validator/src/rbac.rs

Signed-off-by: Shaobo He <shaobohe@amazon.com>

This reverts commit 77ec089.

Signed-off-by: Shaobo He <shaobohe@amazon.com>

john-h-kastner-aws

Looks great

cedar-policy/CHANGELOG.md

cedar-policy-core/src/entities.rs

cedar-policy-core/src/entities/conformance.rs

cedar-policy-validator/src/coreschema.rs

adpaco-aws · 2025-02-05T16:36:44Z

cedar-policy-validator/src/json_schema.rs

+        D: serde::Deserializer<'de>,
+    {
+        // A "real" option that does not accept `null` during deserialization
+        // TODO: we should be able to use `serde_as` or `serde_with` instead


We should open an issue to keep track of the work for this improvement, and paste it here.

adpaco-aws · 2025-02-05T16:38:55Z

cedar-policy-validator/src/json_schema.rs

+        // I tried to apply the same idea to `EntityTypeKind` but serde allows
+        // unknown fields


And you can't use deny_unknown_fields there? Or what's the issue?

I can't recall what the issue is. It seems deny_unknown_fields didn't work.

deny_unknown_fields is generally problematic

adpaco-aws · 2025-02-05T16:40:17Z

cedar-policy-validator/src/json_schema.rs

-                assert_matches!(&**element, Type::Type { ty: TypeVariant::String, loc: None }); // TODO: why is this `TypeVariant::String` in this case but `EntityOrCommon { "String" }` in all the other cases in this test? Do we accept common types as the element type for sets?
+            assert_matches!(frag.0.get(&None).unwrap().entity_types.get(&"User".parse().unwrap()).unwrap(), EntityType { kind: EntityTypeKind::Standard(user), ..} => {
+            assert_matches!(&user.tags, Some(Type::Type { ty: TypeVariant::Set { element }, ..}) => {
+                assert_matches!(&**element, Type::Type{ ty: TypeVariant::String, ..}); // TODO: why is this `TypeVariant::String` in this case but `EntityOrCommon { "String" }` in all the other cases in this test? Do we accept common types as the element type for sets?


Shouldn't we figure this out or at least have an issue for it?

I think it's beyond the scope of this PR. We can create an issue for it.

cedar-policy/src/api/err.rs

adpaco-aws · 2025-02-05T16:45:51Z

Forgot to say that I'm also a little concerned about the amount of cloned() in this code. It'd be great to avoid them wherever possible, but I acknowledge this is the first version so maybe we want to leave that for stabilization.

Signed-off-by: Shaobo He <shaobohe@amazon.com>

cdisselkoen

Should these changes be hidden behind an experimental flag?

Signed-off-by: Shaobo He <shaobohe@amazon.com>

shaobo-he-aws · 2025-02-05T18:32:02Z

Should these changes be hidden behind an experimental flag?

I don't think so. A Lean model is under development and we will use it for DRT.

Signed-off-by: Shaobo He <shaobohe@amazon.com>

adpaco-aws

Thanks! Please don't forget to open/reference issues for pending work in parts of this PR.

Signed-off-by: Shaobo He <shaobohe@amazon.com> Signed-off-by: Charlie Murphy <mutmoth@amazon.com>

prototype

a396894

Signed-off-by: Shaobo He <shaobohe@amazon.com>

shaobo-he-aws marked this pull request as draft December 16, 2024 21:39

cdisselkoen reviewed Jan 2, 2025

View reviewed changes

shaobo-he-aws added 12 commits January 3, 2025 11:26

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

7420d60

Signed-off-by: Shaobo He <shaobohe@amazon.com>

non-breaking version

4a083f8

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

39d209d

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

eb9036e

updates

a7d98ba

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

437b2d4

Signed-off-by: Shaobo He <shaobohe@amazon.com>

add validation

6b7d9b9

Signed-off-by: Shaobo He <shaobohe@amazon.com>

cedar schema format

8af90e5

Signed-off-by: Shaobo He <shaobohe@amazon.com>

more tests

9abf890

Signed-off-by: Shaobo He <shaobohe@amazon.com>

fix

ea628de

Signed-off-by: Shaobo He <shaobohe@amazon.com>

request validation

d538eb0

Signed-off-by: Shaobo He <shaobohe@amazon.com>

an interesting idea to handle ugly error messages of untagged

2b2c64d

Signed-off-by: Shaobo He <shaobohe@amazon.com>

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

View reviewed changes

shaobo-he-aws added 11 commits January 9, 2025 10:08

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

92a80b6

Signed-off-by: Shaobo He <shaobohe@amazon.com>

updates

5073408

Signed-off-by: Shaobo He <shaobohe@amazon.com>

prototype

c056456

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

cfc9a46

nonemptize

baba3ab

Signed-off-by: Shaobo He <shaobohe@amazon.com>

clean up

b2518f0

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

0e1b719

fix wasm

10486da

Signed-off-by: Shaobo He <shaobohe@amazon.com>

fix todo

c90771e

Signed-off-by: Shaobo He <shaobohe@amazon.com>

review feedback

f73d838

Signed-off-by: Shaobo He <shaobohe@amazon.com>

unified errors in various places

8811f1a

Signed-off-by: Shaobo He <shaobohe@amazon.com>

shaobo-he-aws marked this pull request as ready for review January 13, 2025 22:41

shaobo-he-aws added 2 commits January 14, 2025 10:55

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

df04187

Signed-off-by: Shaobo He <shaobohe@amazon.com>

tests for request validation

beb2946

Signed-off-by: Shaobo He <shaobohe@amazon.com>

shaobo-he-aws added 13 commits January 15, 2025 16:33

move tests around

97684b8

Signed-off-by: Shaobo He <shaobohe@amazon.com>

fix wasm?

1666d0f

Signed-off-by: Shaobo He <shaobohe@amazon.com>

clean up

0557469

Signed-off-by: Shaobo He <shaobohe@amazon.com>

updated changelog

d81d377

Signed-off-by: Shaobo He <shaobohe@amazon.com>

clippy fixes

dc15e5c

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

f10ea9a

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Merge remote-tracking branch 'origin/main' into feature/shaobo/rfc53

89289d7

Signed-off-by: Shaobo He <shaobohe@amazon.com>

for drt

cfb6f7c

Signed-off-by: Shaobo He <shaobohe@amazon.com>

protobuf stuff

77ec089

Signed-off-by: Shaobo He <shaobohe@amazon.com>

Revert "protobuf stuff"

74e46b2

This reverts commit 77ec089.

protobuf simplified

2c0404a

Signed-off-by: Shaobo He <shaobohe@amazon.com>

for drt

435017d

Signed-off-by: Shaobo He <shaobohe@amazon.com>

minor fix

6842137

Signed-off-by: Shaobo He <shaobohe@amazon.com>

shaobo-he-aws requested review from cdisselkoen, adpaco-aws and john-h-kastner-aws February 4, 2025 20:06

john-h-kastner-aws approved these changes Feb 5, 2025

View reviewed changes

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

adpaco-aws reviewed Feb 5, 2025

View reviewed changes

fix changelog

de14f9c

Signed-off-by: Shaobo He <shaobohe@amazon.com>

cdisselkoen reviewed Feb 5, 2025

View reviewed changes

address review feedback

e71275e

Signed-off-by: Shaobo He <shaobohe@amazon.com>

review feedback

73d9f2f

Signed-off-by: Shaobo He <shaobohe@amazon.com>

adpaco-aws approved these changes Feb 5, 2025

View reviewed changes

shaobo-he-aws merged commit 33bd811 into main Feb 5, 2025
17 of 19 checks passed

shaobo-he-aws deleted the feature/shaobo/rfc53 branch February 5, 2025 21:03

shaobo-he-aws mentioned this pull request Feb 5, 2025

Refactor entity type deserialization using serde_as or serde_with #1450

Open

2 tasks

chaluli pushed a commit to chaluli/cedar that referenced this pull request Feb 7, 2025

RFC 53 implementation (cedar-policy#1377)

bbbccaf

Signed-off-by: Shaobo He <shaobohe@amazon.com> Signed-off-by: Charlie Murphy <mutmoth@amazon.com>

a0js mentioned this pull request Feb 18, 2025

adds upsert_entities #1481

Closed

4 tasks

		// I tried to apply the same idea to `EntityTypeKind` but serde allows
		// unknown fields

RFC 53 implementation #1377

RFC 53 implementation #1377

Uh oh!

Conversation

shaobo-he-aws commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

cdisselkoen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adpaco-aws commented Feb 5, 2025

Uh oh!

cdisselkoen left a comment

Choose a reason for hiding this comment

Uh oh!

shaobo-he-aws commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adpaco-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shaobo-he-aws commented Dec 16, 2024 •

edited

Loading

shaobo-he-aws commented Feb 5, 2025 •

edited

Loading