Fix for #285 (#581)

cedar-policy-core/src/entities.rs

@@ -160,21 +160,16 @@ impl Entities {
     ) -> Result<Self> {
         let mut entity_map = create_entity_map(entities.into_iter())?;
         if let Some(schema) = schema {
-            // validate entities against schema.
-            // we do this before adding the actions, because we trust the
+            // Validate non-action entities against schema.
+            // We do this before adding the actions, because we trust the
             // actions were already validated as part of constructing the
             // `Schema`
             let checker = EntitySchemaConformanceChecker::new(schema, extensions);
             for entity in entity_map.values() {
-                checker.validate_entity(entity)?;
+                if !entity.uid().entity_type().is_action() {
+                    checker.validate_entity(entity)?;
+                }
             }
-            // now add the action entities from the schema
-            entity_map.extend(
-                schema
-                    .action_entities()
-                    .into_iter()
-                    .map(|e| (e.uid(), unwrap_or_clone(e))),
-            );
         }
         match tc_computation {
             TCComputation::AssumeAlreadyComputed => {}
@@ -185,6 +180,26 @@ impl Entities {
                 compute_tc(&mut entity_map, true).map_err(Box::new)?;
             }
         }
+        // Now that TC has been enforced, we can check action entities for
+        // conformance with the schema and add action entities to the store.
+        // This is fine to do after TC because the action hierarchy in the
+        // schema already satisfies TC, and action and non-action entities
+        // can never be in the same hierarchy when using schema-based parsing.
+        if let Some(schema) = schema {
+            let checker = EntitySchemaConformanceChecker::new(schema, extensions);
+            for entity in entity_map.values() {
+                if entity.uid().entity_type().is_action() {
+                    checker.validate_entity(entity)?;
+                }
+            }
+            // Add the action entities from the schema
+            entity_map.extend(
+                schema
+                    .action_entities()
+                    .into_iter()
+                    .map(|e| (e.uid(), unwrap_or_clone(e))),
+            );
+        }
         Ok(Self {
             entities: entity_map,
             mode: Mode::default(),

cedar-policy/CHANGELOG.md

@@ -41,6 +41,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Action entities in the store will pass schema-based validation without requiring
+  the transitive closure to be pre-computed. (#581, resolving #285)
+
 ## [3.0.1] - 2023-12-21
 Cedar Language Version: 3.0.0
 

cedar-policy/src/tests.rs

@@ -2894,6 +2894,136 @@ mod schema_based_parsing_tests {
             Some(Err(e)) => assert_contains_unknown(&e.to_string(), "ttt")
         );
     }
+
+    /// If a user passes actions through both the schema and the entities, then
+    /// those actions should exactly match _unless_ the `TCComputation::ComputeNow`
+    /// option is used, in which case only the TC has to match.
+    #[test]
+    fn issue_285() {
+        let schema = Schema::from_json_value(json!(
+        {"": {
+            "entityTypes": {},
+            "actions": {
+                "A": {},
+                "B": {
+                    "memberOf": [{"id": "A"}]
+                },
+                "C": {
+                    "memberOf": [{"id": "B"}]
+                }
+            }
+        }}
+        ))
+        .expect("should be a valid schema");
+
+        let entitiesjson_tc = json!(
+            [
+                {
+                    "uid": { "type": "Action", "id": "A" },
+                    "attrs": {},
+                    "parents": []
+                },
+                {
+                    "uid": { "type": "Action", "id": "B" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "A" }
+                    ]
+                },
+                {
+                    "uid": { "type": "Action", "id": "C" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "A" },
+                        { "type": "Action", "id": "B" }
+                    ]
+                }
+            ]
+        );
+
+        let entitiesjson_no_tc = json!(
+            [
+                {
+                    "uid": { "type": "Action", "id": "A" },
+                    "attrs": {},
+                    "parents": []
+                },
+                {
+                    "uid": { "type": "Action", "id": "B" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "A" }
+                    ]
+                },
+                {
+                    "uid": { "type": "Action", "id": "C" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "B" }
+                    ]
+                }
+            ]
+        );
+
+        // Both entity jsons are ok (the default TC setting is `ComputeNow`)
+        assert!(Entities::from_json_value(entitiesjson_tc.clone(), Some(&schema)).is_ok());
+        assert!(Entities::from_json_value(entitiesjson_no_tc.clone(), Some(&schema)).is_ok());
+
+        // Parsing will fail if the TC doesn't match
+        let entitiesjson_bad = json!(
+            [
+                {
+                    "uid": { "type": "Action", "id": "A" },
+                    "attrs": {},
+                    "parents": []
+                },
+                {
+                    "uid": { "type": "Action", "id": "B" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "A" }
+                    ]
+                },
+                {
+                    "uid": { "type": "Action", "id": "C" },
+                    "attrs": {},
+                    "parents": [
+                        { "type": "Action", "id": "A" }
+                    ]
+                }
+            ]
+        );
+        assert!(matches!(
+            Entities::from_json_value(entitiesjson_bad, Some(&schema)),
+            Err(EntitiesError::InvalidEntity(
+                entities::EntitySchemaConformanceError::ActionDeclarationMismatch { uid: _ }
+            ))
+        ));
+
+        // Parsing will fail if we change the TC setting
+        let schema = cedar_policy_validator::CoreSchema::new(&schema.0);
+        let parser_assume_computed = entities::EntityJsonParser::new(
+            Some(&schema),
+            extensions::Extensions::all_available(),
+            entities::TCComputation::AssumeAlreadyComputed,
+        );
+        assert!(matches!(
+            parser_assume_computed.from_json_value(entitiesjson_no_tc.clone()),
+            Err(EntitiesError::InvalidEntity(
+                entities::EntitySchemaConformanceError::ActionDeclarationMismatch { uid: _ }
+            ))
+        ));
+
+        let parser_enforce_computed = entities::EntityJsonParser::new(
+            Some(&schema),
+            extensions::Extensions::all_available(),
+            entities::TCComputation::EnforceAlreadyComputed,
+        );
+        assert!(matches!(
+            parser_enforce_computed.from_json_value(entitiesjson_no_tc),
+            Err(EntitiesError::TransitiveClosureError(_))
+        ));
+    }
 }
 
 #[cfg(not(feature = "partial-validate"))]