Repository: cedar-policy/cedar
Pull request #1397 (2024-12-30 20:19:00)

Allow schema annotation keys to be any identifiers including reserved…
… ones

## Description of changes

## Issue #, if available

## Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

- [x] A change "invisible" to users (e.g., documentation, changes to "internal" crates like `cedar-policy-core`, `cedar-validator`, etc.)

I confirm that this PR (choose one, and delete the other options):

- [x] Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that [`cedar-spec`](https://github.com/cedar-policy/cedar-spec) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that [`docs.cedarpolicy.com`](https://docs.cedarpolicy.com/) (choose one, and delete the other options):

- [x] Does not require updates because my change does not impact the Cedar language specification.



Patch:
diff --git a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
index 3db5e48a..91052835 100644
--- a/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
+++ b/cedar-policy-validator/src/cedar_schema/grammar.lalrpop
@@ -90,14 +90,7 @@ match {
 
 }
 
-#[inline]
-AnyIdent: Node<AnyId> = {
-    <l:@L> <id:IDENTIFIER> <r:@R>
-        // `IDENTIFIER` should produce a valid `AnyId`
-        => Node::with_source_loc(AnyId::from_str(id).unwrap(), Loc::new(l..r, Arc::clone(src))),
-}
-
-// Annotations := {'@' IDENTIFIER '(' String ')'}
+// Annotations := {'@' AnyIdent '(' String ')'}
 Annotation: Node<(Node<AnyId>, Option<Node<SmolStr>>)> = {
     <l:@L> "@" <key:AnyIdent> <value: ("(" <STR> ")")?> <r:@R> => Node::with_source_loc((key, value), Loc::new(l..r, Arc::clone(src)))
 }
@@ -225,8 +218,15 @@ Comma<E>: Vec<E> = {
     },
 }
 
-// IDENT := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']*
+// IDENT := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']* - RESERVED
 Ident: Node<Id> = {
+    <id: AnyIdent> =>? Id::from_str(id.node.as_ref()).map(|i| Node::with_source_loc(i, id.loc.clone())).map_err(|err : cedar_policy_core::parser::err::ParseErrors| ParseError::User {
+        error: UserError::ReservedIdentifierUsed(Node::with_source_loc(id.node.to_smolstr(), id.loc.clone()))
+    }),
+}
+
+// AnyIdent := ['_''a'-'z''A'-'Z']['_''a'-'z''A'-'Z''0'-'9']*
+AnyIdent: Node<AnyId> = {
     <l:@L> NAMESPACE <r:@R>
         => Node::with_source_loc("namespace".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> ENTITY <r:@R>
@@ -256,17 +256,9 @@ Ident: Node<Id> = {
     <l:@L> TYPE <r:@R>
         => Node::with_source_loc("type".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> IN <r:@R>
-        =>? Err(ParseError::User {
-            error: UserError::ReservedIdentifierUsed(Node::with_source_loc("in".into(), Loc::new(l..r, Arc::clone(src))))
-        }),
+        => Node::with_source_loc("in".parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
     <l:@L> <i:IDENTIFIER> <r:@R>
-        =>? Id::from_str(i)
-        .map(|id : Id| Node::with_source_loc(id, Loc::new(l..r, Arc::clone(src))))
-        .map_err(|err : cedar_policy_core::parser::err::ParseErrors|
-            ParseError::User {
-                error: UserError::ReservedIdentifierUsed(Node::with_source_loc(i.to_smolstr(), Loc::new(l..r, Arc::clone(src))))
-            }
-        )
+        => Node::with_source_loc(i.parse().unwrap(), Loc::new(l..r, Arc::clone(src))),
 }
 
 STR: Node<SmolStr> = {


Test patch:
diff --git a/cedar-policy-validator/src/cedar_schema/test.rs b/cedar-policy-validator/src/cedar_schema/test.rs
index 4cf7628c..19a14e4c 100644
--- a/cedar-policy-validator/src/cedar_schema/test.rs
+++ b/cedar-policy-validator/src/cedar_schema/test.rs
@@ -1178,6 +1178,8 @@ mod translator_tests {
         ValidatorSchema,
     };
 
+    use super::SPECIAL_IDS;
+
     // We allow translating schemas that violate RFC 52 to `json_schema::Fragment`.
     // The violations are reported during further translation to `ValidatorSchema`
     #[test]
@@ -1999,6 +2001,28 @@ mod translator_tests {
         .expect("should translate to JSON schema");
         assert_eq!(serde_json::to_value(schema).unwrap(), json_value);
     }
+
+    #[test]
+    fn any_id() {
+        for id in SPECIAL_IDS {
+            test_translation(
+                &format!("@{id} entity User {{}};"),
+                serde_json::json!({
+                    "": {
+                        "entityTypes": {
+                            "User": {
+                                "annotations": {
+                                    id: "",
+                                }
+                            }
+                        },
+                        "actions": {},
+                    }
+                }),
+            )
+        }
+    }
+
     #[test]
     fn annotations() {
         // namespace annotations
@@ -2634,6 +2658,28 @@ mod entity_tags {
     }
 }
 
+#[cfg(test)]
+pub(crate) const SPECIAL_IDS: [&str; 18] = [
+    "principal",
+    "action",
+    "resource",
+    "context",
+    "true",
+    "false",
+    "permit",
+    "forbid",
+    "when",
+    "unless",
+    "in",
+    "has",
+    "like",
+    "is",
+    "if",
+    "then",
+    "else",
+    "__cedar",
+];
+
 // RFC 48 test cases
 #[cfg(test)]
 mod annotations {
@@ -2641,6 +2687,17 @@ mod annotations {
 
     use crate::cedar_schema::parser::parse_schema;
 
+    use super::SPECIAL_IDS;
+
+    // test if annotation keys can be any id
+    #[test]
+    fn any_id() {
+        for id in SPECIAL_IDS {
+            let schema_str = format!("@{id} entity User {{}};");
+            assert_matches!(parse_schema(&schema_str), Ok(_));
+        }
+    }
+
     #[test]
     fn no_keys() {
         assert_matches!(