fix(jans-cedarling): remove unwrap and improve error messages

Signed-off-by: rmarinn <[email protected]>

jans-cedarling/cedarling/src/authz/entity_builder.rs

@@ -12,12 +12,12 @@ mod build_user_entity;
 mod build_workload_entity;
 mod mapping;
 
-use crate::common::cedar_schema::cedar_json::CedarSchemaJson;
 use crate::common::cedar_schema::CEDAR_NAMESPACE_SEPARATOR;
+use crate::common::cedar_schema::cedar_json::CedarSchemaJson;
 use crate::common::policy_store::TokenKind;
 use crate::jwt::{Token, TokenClaimTypeError};
 use crate::{AuthorizationConfig, ResourceData};
-use build_attrs::{build_entity_attrs_from_tkn, BuildAttrError, ClaimAliasMap};
+use build_attrs::{BuildAttrError, ClaimAliasMap, build_entity_attrs_from_tkn};
 use build_expr::*;
 use build_resource_entity::{BuildResourceEntityError, JsonTypeError};
 use build_role_entity::BuildRoleEntityError;
@@ -209,7 +209,8 @@ fn build_entity(
     }
 
     // Build entity attributes
-    let entity_attrs = build_entity_attrs_from_tkn(schema, entity_type, token, claim_aliases)?;
+    let entity_attrs = build_entity_attrs_from_tkn(schema, entity_type, token, claim_aliases)
+        .map_err(BuildEntityError::BuildAttribute)?;
 
     // Build cedar entity
     let entity_type_name =
@@ -257,9 +258,7 @@ pub enum BuildEntityError {
     #[error("the entity `{0}` is not defined in the schema")]
     EntityNotInSchema(String),
     #[error(transparent)]
-    BuildExpression(#[from] BuildExprError),
-    #[error(transparent)]
-    BuildEntityAttr(#[from] BuildAttrError),
+    BuildAttribute(#[from] BuildAttrError),
     #[error("got {0} token, expected: {1}")]
     InvalidToken(TokenKind, TokenKind),
 }

jans-cedarling/cedarling/src/authz/entity_builder/build_attrs.rs

@@ -30,15 +30,19 @@ pub fn build_entity_attrs_from_tkn(
 
     for (attr_name, attr) in shape.attrs.iter() {
         let expression = if let Some(mapping) = token.claim_mapping().get(attr_name) {
-            let claim = claims
-                .get(attr_name)
-                .ok_or_else(|| BuildAttrError::MissingSource(attr_name.to_string()))?;
+            let claim = claims.get(attr_name).ok_or_else(|| {
+                BuildAttrError::new(
+                    attr_name,
+                    BuildAttrErrorKind::MissingSource(attr_name.to_string()),
+                )
+            })?;
             let mapped_claim = mapping.apply_mapping(claim);
-            attr.build_expr(&mapped_claim, attr_name, schema)?
+            attr.build_expr(&mapped_claim, attr_name, schema)
+                .map_err(|err| BuildAttrError::new(attr_name, err.into()))?
         } else {
             match attr.build_expr(&claims, attr_name, schema) {
                 Ok(expr) => expr,
-                Err(err) if attr.is_required() => Err(err)?,
+                Err(err) if attr.is_required() => Err(BuildAttrError::new(attr_name, err.into()))?,
                 // silently fail when attribute isn't required
                 Err(_) => continue,
             }
@@ -68,7 +72,10 @@ pub fn build_entity_attrs_from_values(
         let val = match src.get(attr_name) {
             Some(val) => val,
             None if attr.is_required() => {
-                return Err(BuildAttrError::MissingSource(attr_name.to_string()));
+                return Err(BuildAttrError::new(
+                    attr_name,
+                    BuildAttrErrorKind::MissingSource(attr_name.to_string()),
+                ));
             },
             _ => continue,
         };
@@ -83,7 +90,7 @@ pub fn build_entity_attrs_from_values(
         let expression = match attr.build_expr(src, attr_name, schema) {
             Ok(expr) => expr,
             Err(err) if attr.is_required() => {
-                return Err(err)?;
+                return Err(BuildAttrError::new(attr_name, err.into()))?;
             },
             // move on to the next attribute if this isn't required
             Err(_) => continue,
@@ -118,10 +125,27 @@ fn apply_claim_aliases(claims: &mut HashMap<String, Value>, aliases: Vec<ClaimAl
 }
 
 #[derive(Debug, thiserror::Error)]
-pub enum BuildAttrError {
-    #[error("failed to build entity attribute due to a missing attribute source: `{0}`")]
+#[error("failed to build `{attr_name}` attribute: `{source}`")]
+pub struct BuildAttrError {
+    attr_name: String,
+    #[source]
+    source: BuildAttrErrorKind,
+}
+
+impl BuildAttrError {
+    fn new(name: impl ToString, src: BuildAttrErrorKind) -> Self {
+        Self {
+            attr_name: name.to_string(),
+            source: src,
+        }
+    }
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum BuildAttrErrorKind {
+    #[error("missing attribute source: `{0}`")]
     MissingSource(String),
-    #[error(transparent)]
+    #[error("failed to build restricted expression: {0}")]
     BuildExpression(#[from] BuildExprError),
 }
 
@@ -208,7 +232,14 @@ mod test {
         let err = build_entity_attrs_from_tkn(&schema, &entity_type, &token, Vec::new())
             .expect_err("should error due to missing source");
         assert!(
-            matches!(err, BuildAttrError::BuildExpression(BuildExprError::MissingSource(ref src_name)) if src_name == "client_id"),
+            matches!(
+                err,
+                BuildAttrError {
+                    attr_name: ref name,
+                    source: BuildAttrErrorKind::BuildExpression(BuildExprError::MissingSource(ref src_name))}
+                        if name == "client_id" &&
+                           src_name == "client_id"
+            ),
             "expected MissingSource error but got: {:?}",
             err,
         );
@@ -273,7 +304,13 @@ mod test {
         let err = build_entity_attrs_from_values(&schema, &entity_type, &src_values)
             .expect_err("should error due to missing source");
         assert!(
-            matches!(err, BuildAttrError::MissingSource(ref src_name) if src_name == "client_id"),
+            matches!(
+                err, 
+                BuildAttrError{
+                    attr_name: ref name, 
+                    source: BuildAttrErrorKind::MissingSource(ref src_name)} 
+                        if name == "client_id" && 
+                           src_name == "client_id"),
             "expected MissingSource error but got: {:?}",
             err,
         );

jans-cedarling/cedarling/src/authz/entity_builder/build_expr.rs

@@ -3,10 +3,11 @@
 //
 // Copyright (c) 2024, Gluu, Inc.
 
-use crate::common::cedar_schema::cedar_json::attribute::Attribute;
 use crate::common::cedar_schema::cedar_json::CedarSchemaJson;
+use crate::common::cedar_schema::cedar_json::attribute::Attribute;
 use cedar_policy::{
-    EntityId, EntityTypeName, EntityUid, ExpressionConstructionError, RestrictedExpression,
+    EntityId, EntityTypeName, EntityUid, ExpressionConstructionError, ParseErrors,
+    RestrictedExpression,
 };
 use serde_json::Value;
 use std::collections::HashMap;
@@ -139,7 +140,8 @@ impl Attribute {
                         return Ok(None);
                     }
 
-                    let type_name = EntityTypeName::from_str(&name).unwrap();
+                    let type_name = EntityTypeName::from_str(&name)
+                        .map_err(|e| BuildExprError::ParseEntityTypeName(name, e))?;
                     let type_id = EntityId::new(claim);
                     let uid = EntityUid::from_type_name_and_id(type_name, type_id);
                     Ok(Some(RestrictedExpression::new_entity_uid(uid)))
@@ -210,14 +212,12 @@ pub enum BuildExprError {
     TypeMismatch(#[from] KeyedJsonTypeError),
     #[error(transparent)]
     ConstructionError(#[from] ExpressionConstructionError),
-    #[error(
-        "failed to build restricted expression for `{0}` since the type could not be determined"
-    )]
+    #[error("the type of `{0}` could not be determined")]
     UnkownType(String),
-    #[error(
-        "failed to build entity restricted expression for `{0}` since it is not in the schema"
-    )]
+    #[error("the entity type `{0}` is not in the schema")]
     EntityNotInSchema(String),
+    #[error("failed to parse entity type name \"{0}\": {1}")]
+    ParseEntityTypeName(String, ParseErrors),
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -257,7 +257,7 @@ impl KeyedJsonTypeError {
 mod test {
     use crate::{
         authz::entity_builder::BuildExprError,
-        common::cedar_schema::cedar_json::{attribute::Attribute, CedarSchemaJson},
+        common::cedar_schema::cedar_json::{CedarSchemaJson, attribute::Attribute},
     };
     use serde_json::json;
     use std::collections::HashMap;