Report source location for unknown entity type and action errors (#802)

Signed-off-by: John Kastner <[email protected]>

Author: john-h-kastner-aws

cedar-policy-core/src/ast/entity.rs

@@ -18,6 +18,7 @@ use crate::ast::*;
 use crate::evaluator::{EvaluationError, RestrictedEvaluator};
 use crate::extensions::Extensions;
 use crate::parser::err::ParseErrors;
+use crate::parser::Loc;
 use crate::transitive_closure::TCNode;
 use crate::FromNormalizedStr;
 use itertools::Itertools;
@@ -62,13 +63,43 @@ impl std::fmt::Display for EntityType {
 }
 
 /// Unique ID for an entity. These represent entities in the AST.
-#[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
+#[derive(Serialize, Deserialize, Debug, Clone)]
 pub struct EntityUID {
     /// Typename of the entity
     ty: EntityType,
     /// EID of the entity
     eid: Eid,
+    /// Location of the entity in policy source
+    #[serde(skip)]
+    loc: Option<Loc>,
+}
+
+/// `PartialEq` implementation ignores the `loc`.
+impl PartialEq for EntityUID {
+    fn eq(&self, other: &Self) -> bool {
+        self.ty == other.ty && self.eid == other.eid
+    }
+}
+impl Eq for EntityUID {}
+
+impl std::hash::Hash for EntityUID {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        // hash the ty and eid, in line with the `PartialEq` impl which compares
+        // the ty and eid.
+        self.ty.hash(state);
+        self.eid.hash(state);
+    }
+}
+
+impl PartialOrd for EntityUID {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+impl Ord for EntityUID {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.ty.cmp(&other.ty).then(self.eid.cmp(&other.eid))
+    }
 }
 
 impl StaticallyTyped for EntityUID {
@@ -87,6 +118,7 @@ impl EntityUID {
         Self {
             ty: Self::test_entity_type(),
             eid: Eid(eid.into()),
+            loc: None,
         }
     }
     // by default, Coverlay does not track coverage for lines after a line
@@ -113,6 +145,7 @@ impl EntityUID {
         Ok(Self {
             ty: EntityType::Specified(Name::parse_unqualified_name(typename)?),
             eid: Eid(eid.into()),
+            loc: None,
         })
     }
 
@@ -122,11 +155,17 @@ impl EntityUID {
         (self.ty, self.eid)
     }
 
+    /// Get the source location for this `EntityUID`.
+    pub fn loc(&self) -> Option<&Loc> {
+        self.loc.as_ref()
+    }
+
     /// Create a nominally-typed `EntityUID` with the given typename and EID
-    pub fn from_components(name: Name, eid: Eid) -> Self {
+    pub fn from_components(name: Name, eid: Eid, loc: Option<Loc>) -> Self {
         Self {
             ty: EntityType::Specified(name),
             eid,
+            loc,
         }
     }
 
@@ -135,6 +174,7 @@ impl EntityUID {
         Self {
             ty: EntityType::Unspecified,
             eid,
+            loc: None,
         }
     }
 
@@ -175,6 +215,17 @@ impl FromNormalizedStr for EntityUID {
     }
 }
 
+#[cfg(feature = "arbitrary")]
+impl<'a> arbitrary::Arbitrary<'a> for EntityUID {
+    fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
+        Ok(Self {
+            ty: u.arbitrary()?,
+            eid: u.arbitrary()?,
+            loc: None,
+        })
+    }
+}
+
 /// EID type is just a SmolStr for now
 #[derive(Serialize, Deserialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)]
 pub struct Eid(SmolStr);
@@ -497,12 +548,14 @@ mod test {
         let e2 = EntityUID::from_components(
             Name::parse_unqualified_name("test_entity_type").expect("should be a valid identifier"),
             Eid("foo".into()),
+            None,
         );
         let e3 = EntityUID::unspecified_from_eid(Eid("foo".into()));
         let e4 = EntityUID::unspecified_from_eid(Eid("bar".into()));
         let e5 = EntityUID::from_components(
             Name::parse_unqualified_name("Unspecified").expect("should be a valid identifier"),
             Eid("foo".into()),
+            None,
         );
 
         // an EUID is equal to itself

cedar-policy-core/src/ast/name.rs

@@ -29,13 +29,42 @@ use super::PrincipalOrResource;
 /// This is the `Name` type used to name types, functions, etc.
 /// The name can include namespaces.
 /// Clone is O(1).
-#[derive(Debug, PartialEq, Eq, Clone, Hash, PartialOrd, Ord)]
-#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
+#[derive(Debug, Clone)]
 pub struct Name {
     /// Basename
     pub(crate) id: Id,
     /// Namespaces
     pub(crate) path: Arc<Vec<Id>>,
+    /// Location of the name in source
+    pub(crate) loc: Option<Loc>,
+}
+
+/// `PartialEq` implementation ignores the `loc`.
+impl PartialEq for Name {
+    fn eq(&self, other: &Self) -> bool {
+        self.id == other.id && self.path == other.path
+    }
+}
+impl Eq for Name {}
+
+impl std::hash::Hash for Name {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        // hash the ty and eid, in line with the `PartialEq` impl which compares
+        // the ty and eid.
+        self.id.hash(state);
+        self.path.hash(state);
+    }
+}
+
+impl PartialOrd for Name {
+    fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
+        Some(self.cmp(other))
+    }
+}
+impl Ord for Name {
+    fn cmp(&self, other: &Self) -> std::cmp::Ordering {
+        self.id.cmp(&other.id).then(self.path.cmp(&other.path))
+    }
 }
 
 /// A shortcut for `Name::unqualified_name`
@@ -61,10 +90,11 @@ impl TryFrom<Name> for Id {
 
 impl Name {
     /// A full constructor for `Name`
-    pub fn new(basename: Id, path: impl IntoIterator<Item = Id>) -> Self {
+    pub fn new(basename: Id, path: impl IntoIterator<Item = Id>, loc: Option<Loc>) -> Self {
         Self {
             id: basename,
             path: Arc::new(path.into_iter().collect()),
+            loc,
         }
     }
 
@@ -73,6 +103,7 @@ impl Name {
         Self {
             id,
             path: Arc::new(vec![]),
+            loc: None,
         }
     }
 
@@ -82,15 +113,21 @@ impl Name {
         Ok(Self {
             id: s.parse()?,
             path: Arc::new(vec![]),
+            loc: None,
         })
     }
 
     /// Given a type basename and a namespace (as a `Name` itself),
     /// return a `Name` representing the type's fully qualified name
-    pub fn type_in_namespace(basename: Id, namespace: Name) -> Name {
+    pub fn type_in_namespace(basename: Id, namespace: Name, loc: Option<Loc>) -> Name {
         let mut path = Arc::unwrap_or_clone(namespace.path);
         path.push(namespace.id);
-        Name::new(basename, path)
+        Name::new(basename, path, loc)
+    }
+
+    /// Get the source location
+    pub fn loc(&self) -> Option<&Loc> {
+        self.loc.as_ref()
     }
 
     /// Get the basename of the `Name` (ie, with namespaces stripped).
@@ -129,6 +166,7 @@ impl Name {
                         .namespace_components()
                         .chain(std::iter::once(namespace.basename()))
                         .cloned(),
+                    self.loc().cloned(),
                 ),
                 None => self.clone(),
             }
@@ -208,6 +246,17 @@ impl<'de> Deserialize<'de> for Name {
     }
 }
 
+#[cfg(feature = "arbitrary")]
+impl<'a> arbitrary::Arbitrary<'a> for Name {
+    fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
+        Ok(Self {
+            id: u.arbitrary()?,
+            path: u.arbitrary()?,
+            loc: None,
+        })
+    }
+}
+
 /// Identifier for a slot
 /// Clone is O(1).
 // This simply wraps a separate enum -- currently `ValidSlotId` -- in case we

cedar-policy-core/src/ast/policy.rs

@@ -2137,6 +2137,7 @@ mod test {
         let id = EntityUID::from_components(
             name::Name::unqualified_name(id::Id::new_unchecked("s")),
             entity::Eid::new("eid"),
+            None,
         );
         let mut i = EntityIterator::One(&id);
         assert_eq!(i.next(), Some(&id));
@@ -2148,10 +2149,12 @@ mod test {
         let id1 = EntityUID::from_components(
             name::Name::unqualified_name(id::Id::new_unchecked("s")),
             entity::Eid::new("eid1"),
+            None,
         );
         let id2 = EntityUID::from_components(
             name::Name::unqualified_name(id::Id::new_unchecked("s")),
             entity::Eid::new("eid2"),
+            None,
         );
         let v = vec![&id1, &id2];
         let mut i = EntityIterator::Bunch(v);

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

@@ -185,6 +185,7 @@ impl TryFrom<TypeAndId> for EntityUID {
         Ok(EntityUID::from_components(
             Name::from_normalized_str(&e.entity_type)?,
             Eid::new(e.id),
+            None,
         ))
     }
 }

cedar-policy-core/src/est/expr.rs

@@ -1038,6 +1038,7 @@ fn interpret_primary(
                             __entity: TypeAndId::from(ast::EntityUID::from_components(
                                 name,
                                 ast::Eid::new(eid.clone()),
+                                None,
                             )),
                         })))
                     }
@@ -1065,6 +1066,7 @@ fn interpret_primary(
                                 .and_then(|id| id.to_string().parse().map_err(Into::into))
                         })
                         .collect::<Result<Vec<ast::Id>, ParseErrors>>()?,
+                    Some(node.loc.clone()),
                 ))),
                 (path, id) => {
                     let (l, r, src) = match (path.first(), path.last()) {

cedar-policy-core/src/parser/cst_to_ast.rs

@@ -2118,7 +2118,9 @@ impl Node<Option<cst::Name>> {
 
         // computation and error generation is complete, so fail or construct
         match (maybe_name, path.len()) {
-            (Some(r), len) if len == name.path.len() => Some(construct_name(path, r)),
+            (Some(r), len) if len == name.path.len() => {
+                Some(construct_name(path, r, self.loc.clone()))
+            }
             _ => None,
         }
     }
@@ -2227,7 +2229,7 @@ impl Node<Option<cst::Ref>> {
                 };
 
                 match (maybe_path, maybe_eid) {
-                    (Some(p), Some(e)) => Some(construct_refr(p, e)),
+                    (Some(p), Some(e)) => Some(construct_refr(p, e, self.loc.clone())),
                     _ => None,
                 }
             }
@@ -2343,15 +2345,16 @@ fn construct_string_from_var(v: ast::Var) -> SmolStr {
         ast::Var::Context => "context".into(),
     }
 }
-fn construct_name(path: Vec<ast::Id>, id: ast::Id) -> ast::Name {
+fn construct_name(path: Vec<ast::Id>, id: ast::Id, loc: Loc) -> ast::Name {
     ast::Name {
         id,
         path: Arc::new(path),
+        loc: Some(loc),
     }
 }
-fn construct_refr(p: ast::Name, n: SmolStr) -> ast::EntityUID {
+fn construct_refr(p: ast::Name, n: SmolStr, loc: Loc) -> ast::EntityUID {
     let eid = ast::Eid::new(n);
-    ast::EntityUID::from_components(p, eid)
+    ast::EntityUID::from_components(p, eid, Some(loc))
 }
 fn construct_expr_ref(r: ast::EntityUID, loc: Loc) -> ast::Expr {
     ast::ExprBuilder::new().with_source_loc(loc).val(r)

cedar-policy-validator/src/human_schema/ast.rs

@@ -125,7 +125,7 @@ struct PathInternal {
 
 impl From<PathInternal> for Name {
     fn from(value: PathInternal) -> Self {
-        Self::new(value.basename, value.namespace)
+        Self::new(value.basename, value.namespace, None)
     }
 }
 

cedar-policy-validator/src/human_schema/to_json_schema.rs

@@ -434,6 +434,7 @@ impl<'a> ConversionContext<'a> {
                 &Some(Name::new(
                     prefix_base.clone(),
                     prefix_prefix.into_iter().cloned(),
+                    None,
                 )),
             ),
             None =>

cedar-policy-validator/src/lib.rs

@@ -135,7 +135,6 @@ impl Validator {
             Some(
                 self.validate_entity_types(p)
                     .chain(self.validate_action_ids(p))
-                    .map(move |note| ValidationError::with_policy_id(p.id(), None, note))
                     // We could usefully update this pass to apply to partial
                     // schema if it only failed when there is a known action
                     // applied to known principal/resource entity types that are
@@ -295,6 +294,8 @@ mod test {
                 Some("Action::\"action\"".to_string()),
             ),
         );
+
+        assert!(!result.validation_passed());
         assert!(result
             .validation_errors()
             .any(|x| x.error_kind() == principal_err.error_kind()));
@@ -379,6 +380,7 @@ mod test {
             ast::EntityUID::from_components(
                 "some_namespace::Photo".parse().unwrap(),
                 ast::Eid::new("foo"),
+                None,
             ),
         );
         set.link(
@@ -397,6 +399,7 @@ mod test {
             ast::EntityUID::from_components(
                 "some_namespace::Undefined".parse().unwrap(),
                 ast::Eid::new("foo"),
+                None,
             ),
         );
         set.link(
@@ -432,6 +435,7 @@ mod test {
             ast::EntityUID::from_components(
                 "some_namespace::User".parse().unwrap(),
                 ast::Eid::new("foo"),
+                None,
             ),
         );
         set.link(
@@ -450,7 +454,9 @@ mod test {
             loc.clone(),
             ValidationErrorKind::invalid_action_application(false, false),
         );
-        assert!(result.validation_errors().any(|x| x == &invalid_action_err));
+        assert!(result
+            .validation_errors()
+            .any(|x| x.error_kind() == invalid_action_err.error_kind()));
 
         Ok(())
     }