fix(jans-cedarling): non-required record attr getting required

- fix non required record attributes getting required if the record is also required. Signed-off-by: rmarinn <[email protected]>
JanssenProject · Jan 7, 2025 · f678da7 · f678da7
1 parent bbe8b32
commit f678da7
Show file tree

Hide file tree

Showing 3 changed files with 150 additions and 6 deletions.
diff --git a/jans-cedarling/cedarling/src/authz/entity_builder/build_attrs.rs b/jans-cedarling/cedarling/src/authz/entity_builder/build_attrs.rs
@@ -83,8 +83,10 @@ impl EntityBuilder {
 
             let expression = match attr.build_expr(src, attr_name, &self.schema) {
                 Ok(expr) => expr,
-                Err(err) if attr.is_required() => Err(err)?,
-                // silently fail when attribute isn't required
+                Err(err) if attr.is_required() => {
+                    return Err(err)?;
+                },
+                // move on to the next attribute if this isn't required
                 Err(_) => continue,
             };
 

diff --git a/jans-cedarling/cedarling/src/authz/entity_builder/build_expr.rs b/jans-cedarling/cedarling/src/authz/entity_builder/build_expr.rs
@@ -85,11 +85,14 @@ impl Attribute {
                 for (name, kind) in attrs.iter() {
                     if let Some(expr) = kind.build_expr(attr_src, name, schema)? {
                         fields.insert(name.to_string(), expr);
-                    } else if *required {
-                        return Err(BuildExprError::MissingSource(name.clone()));
                     }
                 }
-                Ok(Some(RestrictedExpression::new_record(fields)?))
+
+                if fields.is_empty() && !required {
+                    Ok(None)
+                } else {
+                    Ok(Some(RestrictedExpression::new_record(fields)?))
+                }
             },
 
             // Handle Set attributes

diff --git a/jans-cedarling/cedarling/src/authz/entity_builder/build_resource_entity.rs b/jans-cedarling/cedarling/src/authz/entity_builder/build_resource_entity.rs
@@ -87,7 +87,7 @@ mod test {
     use serde_json::json;
 
     #[test]
-    fn can_build_resource_entity() {
+    fn can_build_entity() {
         let schema = serde_json::from_value::<CedarSchemaJson>(json!({
             "Jans": {
                 "commonTypes": {
@@ -186,4 +186,143 @@ mod test {
             );
         }
     }
+
+    #[test]
+    fn can_build_entity_with_optional_attr() {
+        let schema = serde_json::from_value::<CedarSchemaJson>(json!({
+            "Jans": {
+                "commonTypes": {
+                    "Url": {
+                        "type": "Record",
+                        "attributes": {
+                            "host": { "type": "String" },
+                            "path": { "type": "String" },
+                            "protocol": { "type": "String" },
+                        },
+                    },
+                },
+                "entityTypes": {
+                    "Role": {},
+                    "HttpRequest": {
+                        "shape": {
+                            "type": "Record",
+                            "attributes":  {
+                                "url": { "type": "EntityOrCommon", "name": "Url", "required": false},
+                            },
+                        }
+                    }
+                }
+            }
+        }))
+        .expect("should successfully create test schema");
+        let builder = EntityBuilder::new(schema, EntityNames::default(), false, false);
+        let resource_data = ResourceData {
+            resource_type: "HttpRequest".to_string(),
+            id: "request-123".to_string(),
+            payload: HashMap::new(),
+        };
+        let entity = builder
+            .build_resource_entity(&resource_data)
+            .expect("expected to build resource entity");
+
+        assert!(
+            entity.attr("url").is_none(),
+            "entity should not have a `url` attribute"
+        );
+    }
+
+    #[test]
+    fn can_build_entity_with_optional_record_attr() {
+        let schema = serde_json::from_value::<CedarSchemaJson>(json!({
+            "Jans": {
+                "commonTypes": {
+                    "Url": {
+                        "type": "Record",
+                        "attributes": {
+                            "host": { "type": "String" },
+                            "path": { "type": "String" },
+                            "protocol": { "type": "String" },
+                        },
+                    },
+                },
+                "entityTypes": {
+                    "Role": {},
+                    "HttpRequest": {
+                        "shape": {
+                            "type": "Record",
+                            "attributes":  {
+                                "header": {
+                                    "type": "Record",
+                                    "attributes": {
+                                        "Accept": { "type": "EntityOrCommon", "name": "String", "required": false },
+                                    },
+                                },
+                                "url": { "type": "EntityOrCommon", "name": "Url" },
+                            },
+                        }
+                    }
+                }
+            }
+        }))
+        .expect("should successfully create test schema");
+        let builder = EntityBuilder::new(schema, EntityNames::default(), false, false);
+        let resource_data = ResourceData {
+            resource_type: "HttpRequest".to_string(),
+            id: "request-123".to_string(),
+            payload: HashMap::from([
+                (
+                    "url".to_string(),
+                    json!({"host": "protected.host", "protocol": "http", "path": "/protected"}),
+                ),
+                ("header".to_string(), json!({})),
+            ]),
+        };
+        let entity = builder
+            .build_resource_entity(&resource_data)
+            .expect("expected to build resource entity");
+
+        let url = entity
+            .attr("url")
+            .expect("entity must have an `url` attribute")
+            .unwrap();
+        if let EvalResult::Record(ref record) = url {
+            assert_eq!(record.len(), 3);
+            assert_eq!(
+                record
+                    .get("host")
+                    .expect("expected `url` to have a `host` attribute"),
+                &EvalResult::String("protected.host".to_string())
+            );
+            assert_eq!(
+                record
+                    .get("protocol")
+                    .expect("expected `url` to have a `domain` attribute"),
+                &EvalResult::String("http".to_string())
+            );
+            assert_eq!(
+                record
+                    .get("path")
+                    .expect("expected `url` to have a `path` attribute"),
+                &EvalResult::String("/protected".to_string())
+            );
+        } else {
+            panic!(
+                "expected the attribute `url` to be a record, got: {:?}",
+                url
+            );
+        }
+
+        let header = entity
+            .attr("header")
+            .expect("entity must have an `header` attribute")
+            .unwrap();
+        if let EvalResult::Record(ref record) = header {
+            assert_eq!(record.len(), 0, "the header attribute must be empty");
+        } else {
+            panic!(
+                "expected the attribute `header` to be a record, got: {:?}",
+                header
+            );
+        }
+    }
 }