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

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

john-h-kastner-aws

Looks pretty good

john-h-kastner-aws · 2025-01-08T14:39:48Z

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

+
+    /// Return valid EID choices if the entity type is enumerated otherwise
+    /// return `None`
+    fn choices(&self) -> Option<Vec<SmolStr>>;


Might be better off returning slice or an iterator?

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

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

cedar-policy-validator/src/coreschema.rs

john-h-kastner-aws · 2025-01-15T14:10:01Z

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


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

cedar-policy-validator/src/types.rs

Signed-off-by: Shaobo He <[email protected]>

prototype

a396894

Signed-off-by: Shaobo He <[email protected]>

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

non-breaking version

4a083f8

Signed-off-by: Shaobo He <[email protected]>

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

39d209d

Signed-off-by: Shaobo He <[email protected]>

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

eb9036e

updates

a7d98ba

Signed-off-by: Shaobo He <[email protected]>

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

437b2d4

Signed-off-by: Shaobo He <[email protected]>

add validation

6b7d9b9

Signed-off-by: Shaobo He <[email protected]>

cedar schema format

8af90e5

Signed-off-by: Shaobo He <[email protected]>

more tests

9abf890

Signed-off-by: Shaobo He <[email protected]>

fix

ea628de

Signed-off-by: Shaobo He <[email protected]>

request validation

d538eb0

Signed-off-by: Shaobo He <[email protected]>

an interesting idea to handle ugly error messages of untagged

2b2c64d

Signed-off-by: Shaobo He <[email protected]>

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

updates

5073408

Signed-off-by: Shaobo He <[email protected]>

prototype

c056456

Signed-off-by: Shaobo He <[email protected]>

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

cfc9a46

nonemptize

baba3ab

Signed-off-by: Shaobo He <[email protected]>

clean up

b2518f0

Signed-off-by: Shaobo He <[email protected]>

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

0e1b719

fix wasm

10486da

Signed-off-by: Shaobo He <[email protected]>

fix todo

c90771e

Signed-off-by: Shaobo He <[email protected]>

review feedback

f73d838

Signed-off-by: Shaobo He <[email protected]>

unified errors in various places

8811f1a

Signed-off-by: Shaobo He <[email protected]>

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

tests for request validation

beb2946

Signed-off-by: Shaobo He <[email protected]>

added more tests

698b4b6

Signed-off-by: Shaobo He <[email protected]>

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

View reviewed changes

cedar-policy-validator/src/types.rs Outdated Show resolved Hide resolved

shaobo-he-aws added 10 commits January 15, 2025 13:38

better entity validation

bdef431

Signed-off-by: Shaobo He <[email protected]>

updated tests

f8b592d

Signed-off-by: Shaobo He <[email protected]>

fix

4c01686

Signed-off-by: Shaobo He <[email protected]>

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

6717a07

refactor

0c65e1e

Signed-off-by: Shaobo He <[email protected]>

move tests around

97684b8

Signed-off-by: Shaobo He <[email protected]>

fix wasm?

1666d0f

Signed-off-by: Shaobo He <[email protected]>

clean up

0557469

Signed-off-by: Shaobo He <[email protected]>

updated changelog

d81d377

Signed-off-by: Shaobo He <[email protected]>

clippy fixes

dc15e5c

Signed-off-by: Shaobo He <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC 53 implementation #1377

RFC 53 implementation #1377

shaobo-he-aws commented Dec 16, 2024 •

edited

Loading

cdisselkoen left a comment

cdisselkoen Jan 2, 2025

john-h-kastner-aws left a comment

john-h-kastner-aws Jan 8, 2025

john-h-kastner-aws Jan 8, 2025

shaobo-he-aws Jan 15, 2025

john-h-kastner-aws Jan 15, 2025

john-h-kastner-aws Jan 15, 2025

RFC 53 implementation #1377

Are you sure you want to change the base?

RFC 53 implementation #1377

Conversation

shaobo-he-aws commented Dec 16, 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

cdisselkoen Jan 2, 2025

Choose a reason for hiding this comment

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

john-h-kastner-aws Jan 8, 2025

Choose a reason for hiding this comment

john-h-kastner-aws Jan 8, 2025

Choose a reason for hiding this comment

shaobo-he-aws Jan 15, 2025

Choose a reason for hiding this comment

john-h-kastner-aws Jan 15, 2025

Choose a reason for hiding this comment

john-h-kastner-aws Jan 15, 2025

Choose a reason for hiding this comment

shaobo-he-aws commented Dec 16, 2024 •

edited

Loading