From f678da7997b8ca612766a590d513e714c4b75f1d Mon Sep 17 00:00:00 2001 From: rmarinn <34529290+rmarinn@users.noreply.github.com> Date: Tue, 7 Jan 2025 18:57:49 +0800 Subject: [PATCH] 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 <34529290+rmarinn@users.noreply.github.com> --- .../src/authz/entity_builder/build_attrs.rs | 6 +- .../src/authz/entity_builder/build_expr.rs | 9 +- .../entity_builder/build_resource_entity.rs | 141 +++++++++++++++++- 3 files changed, 150 insertions(+), 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 index 932a1c84cd5..f899c53b419 100644 --- 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 index aa773f15deb..618e1015a26 100644 --- 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 index 50fced82520..acfd91d4460 100644 --- 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::(json!({ "Jans": { "commonTypes": { @@ -186,4 +186,143 @@ mod test { ); } } + + #[test] + fn can_build_entity_with_optional_attr() { + let schema = serde_json::from_value::(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::(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 + ); + } + } }