diff --git a/README.md b/README.md index fafe5e1..dc449e1 100644 --- a/README.md +++ b/README.md @@ -724,6 +724,13 @@ Other operators are handled accordingly. The evaluator should eventually pass all Cedar evaluator test using the DRT framework in the `cedar-spec` repo, as well as the `cedar-integration-tests` bundle. +Status: the [`cedar-sql-spec`] fork of `cedar-spec` differentially tests this evaluator against `cedar-policy`'s +authorizer with the targets `sql-is-authorized-drt` (concrete requests) and `sql-query-drt` (an unknown +`principal` and/or `resource`, every candidate checked), and runs the `cedar-integration-tests` corpus through +it. The envelope is: policies that validate strictly, schemas without extension types, and strings without NUL +characters (which Postgres `text` cannot hold); within it every handwritten integration test and every checked +corpus test agrees with `cedar-policy`. + ## Status and phases The implementation is built in phases, one branch and plan document each (`docs/plans/N-slug.md` here, with the diff --git a/docs/followups/6-corpus.md b/docs/followups/6-corpus.md new file mode 100644 index 0000000..c9a703a --- /dev/null +++ b/docs/followups/6-corpus.md @@ -0,0 +1,21 @@ +# Follow-ups for branch 6 — corpus + +## PR description + +The integration corpus runs through the crate from `cedar-sql-spec` (`docs/plans/cedar-sql-6-corpus.md`): +of 7497 corpus tests, 4304 (34413 requests) are inside the envelope and all agree with the expected +results; the 24 handwritten tests all pass. This branch records that in the README, compiles `hasTag` on +an action type as `false` (found by the corpus), and applies the review of branches 4 and 5. + +## Review findings of branches 4 and 5 (fixed here) + +- Generated join aliases joined attribute path segments with `.`, which an attribute name may contain. +- The differential tests matched loader errors on the substring `NUL`; the crate now has `Error::Nul`. +- Missing tests: a same-type principal and resource both unknown, a candidate type without rows, an enum + type, a dotted attribute name. + +## Suggested follow-ups + +- Extension types (Plan 7): 580 corpus tests and 864 requests wait on them. +- The enumerated candidates of an enum entity type are its table rows, not its declared values (the same + choice `PolicySet::query_resource` makes); worth stating in the README when Plan 7 touches enums. diff --git a/docs/plans/6-corpus.md b/docs/plans/6-corpus.md new file mode 100644 index 0000000..b16be4c --- /dev/null +++ b/docs/plans/6-corpus.md @@ -0,0 +1,23 @@ +# Plan 6 — corpus: results of the integration tests + +## Goal + +No code: this branch records that the `cedar-integration-tests` corpus runs through the crate +(`cedar-sql-spec` branch `cedar-sql-6-corpus`) and what its envelope is, in the README. + +## Design + +- The README's "Testing" section states the envelope (strictly validating policies, no extension types, no + NUL strings) and the results, and points to the differential tests. + +## Files + +`README.md`, `docs/plans/6-corpus.md`. + +## Verification + +See `cedar-sql-spec`'s `docs/plans/cedar-sql-6-corpus.md`. + +## History + +New; the sixth `cedar-sql` branch. diff --git a/src/compile.rs b/src/compile.rs index d959da7..9b16678 100644 --- a/src/compile.rs +++ b/src/compile.rs @@ -654,10 +654,9 @@ impl<'a> Compiler<'a> { if b.repr != Repr::Text { return Err(Error::Unsupported("a non-string tag")); } - let Some((_, table)) = self.config.table_for(ety) else { - return Err(Error::Unsupported("tags of an entity type without a table")); - }; - let Some(tags) = table.tags.clone() else { + // An entity type without a table (an action type) has no tags either. + let tags = self.config.table_for(ety).and_then(|(_, t)| t.tags.clone()); + let Some(tags) = tags else { if op == BinaryOp::HasTag { // The validator types `hasTag` on a tagless type as `false`. return Ok(plain( @@ -1037,25 +1036,35 @@ fn root_raw_name(root: &RootKey) -> String { } } +/// The separator of attribute path segments in generated names: attribute +/// names may contain `.`, so `principal["b.c"]` and `principal.b.c` must +/// not collide. +const SEGMENT: &str = "\x1f"; + /// The quoted alias of the join for `path` under `root` (the root itself /// for the empty path). fn alias_name(root: &RootKey, path: &[SmolStr]) -> String { if path.is_empty() { return root_name(root); } - shortened(&format!("{}.{}", root_raw_name(root), path.join("."))).to_string() + shortened(&format!( + "{}{SEGMENT}{}", + root_raw_name(root), + path.join(SEGMENT) + )) + .to_string() } /// The quoted CTE column holding the value of an attribute path. fn value_column(path: &[SmolStr]) -> String { - shortened(&format!("$v:{}", path.join("."))).to_string() + shortened(&format!("$v:{}", path.join(SEGMENT))).to_string() } fn ancestors_name(anchor: &Anchor) -> SQLIdentifier { let path = if anchor.path.is_empty() { String::new() } else { - format!(".{}", anchor.path.join(".")) + format!("{SEGMENT}{}", anchor.path.join(SEGMENT)) }; shortened(&format!("{}{path}$ancestors", root_raw_name(&anchor.root))) } diff --git a/src/error.rs b/src/error.rs index 809fae6..50c4b22 100644 --- a/src/error.rs +++ b/src/error.rs @@ -40,6 +40,10 @@ pub enum Error { /// The query returned rows this crate cannot interpret. #[error("unexpected query result: {0}")] Query(String), + /// A string holds a NUL character, which Postgres `text` cannot store: a + /// documented limitation, distinguishable from a loader bug. + #[error("the string {0:?} contains a NUL character, which Postgres cannot store")] + Nul(String), /// The entities cannot be turned into rows of the configured tables. #[error("cannot load entities: {0}")] Load(String), diff --git a/src/ident.rs b/src/ident.rs index 49bb244..6c82ac3 100644 --- a/src/ident.rs +++ b/src/ident.rs @@ -104,9 +104,7 @@ fn fnv1a(s: &str) -> u64 { /// When `s` contains a NUL character, which Postgres `text` cannot store. pub fn quoted_literal(s: &str) -> Result { if s.contains('\0') { - return Err(Error::Load(format!( - "the string {s:?} contains a NUL character, which Postgres cannot store" - ))); + return Err(Error::Nul(s.to_owned())); } Ok(format!("'{}'", s.replace('\'', "''"))) } diff --git a/src/load.rs b/src/load.rs index 5c5eaee..81d8f11 100644 --- a/src/load.rs +++ b/src/load.rs @@ -257,9 +257,7 @@ pub fn canonical_json(value: &Value) -> Result { ValueKind::Lit(Literal::Long(n)) => json!(n), ValueKind::Lit(Literal::String(s)) => { if s.contains('\0') { - return Err(Error::Load(format!( - "the string {s:?} contains a NUL character, which Postgres cannot store" - ))); + return Err(Error::Nul(s.to_string())); } json!(s.as_str()) } @@ -272,9 +270,7 @@ pub fn canonical_json(value: &Value) -> Result { .iter() .map(|(k, v)| { if k.contains('\0') { - return Err(Error::Load(format!( - "the record key {k:?} contains a NUL character, which Postgres cannot store" - ))); + return Err(Error::Nul(k.to_string())); } Ok((k.to_string(), canonical_json(v)?)) }) diff --git a/tests/authorize_pg.rs b/tests/authorize_pg.rs index cae560b..bd04e06 100644 --- a/tests/authorize_pg.rs +++ b/tests/authorize_pg.rs @@ -110,7 +110,7 @@ const D1: &str = "Doc::\"d1\""; const GHOST_NAME: &str = "User::\"ghost\".name == \"x\""; fn permit(condition: &str) -> String { - format!("permit(principal, action, resource) when {{ {condition} }};") + format!("permit(principal, action == Action::\"view\", resource) when {{ {condition} }};") } fn ids(response: &Response) -> (Vec, Vec) { @@ -543,7 +543,7 @@ fn hierarchy_and_tags() { fn decisions() { let mut fx = Fixture::new(true); let both = format!( - "{}\nforbid(principal, action, resource) when {{ principal.name == \"Alice\" }};", + "{}\nforbid(principal, action == Action::\"view\", resource) when {{ principal.name == \"Alice\" }};", permit("true") ); let r = fx.check(&both, ALICE, D1); @@ -569,7 +569,7 @@ fn decisions() { assert_eq!(ids(&r), (vec![], vec!["policy0".to_owned()])); // A forbid that errors does not deny. let forbid_errs = format!( - "{}\nforbid(principal, action, resource) when {{ {GHOST_NAME} }};", + "{}\nforbid(principal, action == Action::\"view\", resource) when {{ {GHOST_NAME} }};", permit("true") ); let r = fx.check(&forbid_errs, ALICE, D1); @@ -787,6 +787,22 @@ fn review_probes() { ALICE, ); allow(&mut fx, "principal in [principal]", ALICE); + // An attribute name containing a dot next to the path it spells. + allow( + &mut fx, + "principal has friend && principal.friend has name && principal has \"friend.name\" && principal[\"friend.name\"] == principal.friend.name", + ALICE, + ); + deny( + &mut fx, + "principal has \"friend.name\" && principal[\"friend.name\"] == \"Bob\" && principal has friend && principal.friend has name", + CAROL, + ); + allow( + &mut fx, + "principal has \"friend.name\" && principal[\"friend.name\"] == \"Bob\"", + CAROL, + ); } } @@ -804,4 +820,14 @@ fn has_tag_on_a_tagless_type() { ALICE, D1, ); + // An action type has no table and no tags. + assert_eq!( + fx.clean( + &permit("!(Action::\"view\".hasTag(principal.name))"), + ALICE, + D1 + ) + .decision, + Decision::Allow + ); } diff --git a/tests/entities/kitchen_sink.json b/tests/entities/kitchen_sink.json index 68d452c..841b19f 100644 --- a/tests/entities/kitchen_sink.json +++ b/tests/entities/kitchen_sink.json @@ -6,7 +6,8 @@ "groups": ["b", "a", "a"], "profile": {"city": "Zürich", "pets": [3, 1, 2], "boss": {"__entity": {"type": "User", "id": "bob"}}}, "friend": {"__entity": {"type": "User", "id": "bob"}}, - "friends": [{"__entity": {"type": "User", "id": "bob"}}] + "friends": [{"__entity": {"type": "User", "id": "bob"}}], + "friend.name": "Bob" }, "parents": [{"type": "Group", "id": "admins"}], "tags": {"k": "v", "it's": "q'"}}, @@ -15,7 +16,7 @@ "parents": []}, {"uid": {"type": "User", "id": "carol"}, "attrs": {"name": "Carol", "admin": false, "groups": ["x"], "profile": {"city": "Oslo", "pets": [7]}, - "friend": {"__entity": {"type": "User", "id": "ghost"}}, "friends": []}, + "friend": {"__entity": {"type": "User", "id": "ghost"}}, "friends": [], "friend.name": "Bob"}, "parents": []}, {"uid": {"type": "User", "id": "weird\"q\\s"}, "attrs": {"name": "50%_x\\y\nz", "admin": false, "groups": [], "profile": {"city": "", "pets": []}, "friends": []}, @@ -24,5 +25,9 @@ "attrs": {"owner": {"__entity": {"type": "User", "id": "alice"}}}, "parents": [{"type": "Group", "id": "admins"}], "tags": {"reviewer": {"__entity": {"type": "User", "id": "bob"}}}}, - {"uid": {"type": "Action", "id": "view"}, "attrs": {}, "parents": []} + {"uid": {"type": "Color", "id": "red"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "view"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "mate"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "empty"}, "attrs": {}, "parents": []}, + {"uid": {"type": "Action", "id": "color"}, "attrs": {}, "parents": []} ] diff --git a/tests/golden/kitchen_sink.sql b/tests/golden/kitchen_sink.sql index 9c30d92..1ccd01c 100644 --- a/tests/golden/kitchen_sink.sql +++ b/tests/golden/kitchen_sink.sql @@ -18,6 +18,11 @@ BEGIN END IF; END $cedar$; +CREATE TABLE "Color" ( + "__entity_id" TEXT NOT NULL, + "__entity_type" TEXT NOT NULL GENERATED ALWAYS AS ('Color') STORED, + PRIMARY KEY ("__entity_id") +); CREATE TABLE "Doc" ( "__entity_id" TEXT NOT NULL, "__entity_type" TEXT NOT NULL GENERATED ALWAYS AS ('Doc') STORED, @@ -30,6 +35,11 @@ CREATE TABLE "Doc_tags" ( "value" TEXT NOT NULL, PRIMARY KEY ("entity_id", "tag") ); +CREATE TABLE "Empty" ( + "__entity_id" TEXT NOT NULL, + "__entity_type" TEXT NOT NULL GENERATED ALWAYS AS ('Empty') STORED, + PRIMARY KEY ("__entity_id") +); CREATE TABLE "Group" ( "__entity_id" TEXT NOT NULL, "__entity_type" TEXT NOT NULL GENERATED ALWAYS AS ('Group') STORED, @@ -41,6 +51,7 @@ CREATE TABLE "User" ( "admin" BOOLEAN NOT NULL, "age" BIGINT, "friend" TEXT, + "friend.name" TEXT, "friends" JSONB NOT NULL, "groups" JSONB NOT NULL, "name" TEXT NOT NULL, diff --git a/tests/load_pg.rs b/tests/load_pg.rs index 981f4ae..7b9ea88 100644 --- a/tests/load_pg.rs +++ b/tests/load_pg.rs @@ -22,8 +22,12 @@ fn round_trip() { let json = std::fs::read_to_string("tests/entities/kitchen_sink.json").unwrap(); let entities = Entities::from_json_str(&json, Some(&schema)).unwrap(); let load = entities_to_sql(&entities, &schema, &config, &Postgres).unwrap(); - assert_eq!(load.actions.len(), 1); - assert_eq!(load.actions[0].uid().to_string(), "Action::\"view\""); + assert_eq!(load.actions.len(), 4); + assert!( + load.actions + .iter() + .any(|a| a.uid().to_string() == "Action::\"view\"") + ); let mut db = SharedPostgres::get().unwrap().connect().unwrap(); db.begin().unwrap(); diff --git a/tests/query_pg.rs b/tests/query_pg.rs index 634c3b1..e25ccd1 100644 --- a/tests/query_pg.rs +++ b/tests/query_pg.rs @@ -50,9 +50,15 @@ impl Fixture { } fn request(&self, principal: &str, resource: &str) -> Request { + let action = match resource.split("::").next().unwrap() { + "User" => "mate", + "Empty" => "empty", + "Color" => "color", + _ => "view", + }; Request::new( EntityUid::from_str(principal).unwrap(), - EntityUid::from_str("Action::\"view\"").unwrap(), + EntityUid::from_str(&format!("Action::\"{action}\"")).unwrap(), EntityUid::from_str(resource).unwrap(), Context::from_json_str(r#"{"ip": "1.2.3.4"}"#, None).unwrap(), Some(&self.schema), @@ -147,10 +153,15 @@ impl Fixture { } const ALICE: &str = "User::\"alice\""; +const BOB: &str = "User::\"bob\""; const D1: &str = "Doc::\"d1\""; fn permit(condition: &str) -> String { - format!("permit(principal, action, resource) when {{ {condition} }};") + permit_for("view", condition) +} + +fn permit_for(action: &str, condition: &str) -> String { + format!("permit(principal, action == Action::\"{action}\", resource) when {{ {condition} }};") } fn allowed(rows: &[QueryRow]) -> BTreeSet { @@ -291,7 +302,7 @@ fn unknown_resource_and_both() { // A forbid over both. fx.check( &format!( - "{}\nforbid(principal, action, resource) when {{ principal.name == \"Bob\" || resource.owner == principal }};", + "{}\nforbid(principal, action == Action::\"view\", resource) when {{ principal.name == \"Bob\" || resource.owner == principal }};", permit("true") ), ALICE, @@ -325,3 +336,84 @@ fn agrees_with_query_resource() { assert_eq!(allowed(&rows), expected); let _ = PolicyId::new("x"); } + +#[test] +fn same_type_zero_rows_and_enums() { + let mut fx = Fixture::new(true); + // The principal and the resource are both users. + let rows = fx.check( + &permit_for("mate", "principal == resource"), + ALICE, + ALICE, + true, + true, + ); + assert_eq!(rows.len(), 16); + assert_eq!(allowed_pairs(&rows).len(), 4); + fx.check( + &permit_for("mate", "principal in resource"), + ALICE, + ALICE, + true, + true, + ); + fx.check( + &permit("resource in principal.friends"), + ALICE, + ALICE, + true, + true, + ); + fx.check( + &permit("principal has friend && principal.friend == resource"), + ALICE, + BOB, + true, + false, + ); + fx.check( + &permit("principal has friend && principal.friend == resource"), + ALICE, + BOB, + false, + true, + ); + // A candidate type with no rows yields no rows. + let rows = fx.check( + "permit(principal, action, resource);", + ALICE, + "Empty::\"x\"", + false, + true, + ); + assert!(rows.is_empty()); + // An enum type: the candidates are its rows, not its declared values. + let rows = fx.check( + "permit(principal, action, resource);", + ALICE, + "Color::\"red\"", + false, + true, + ); + assert_eq!(rows.len(), 1); + let rows = fx.check( + &permit("resource == Color::\"blue\""), + ALICE, + "Color::\"red\"", + false, + true, + ); + assert!(allowed(&rows).is_empty()); +} + +fn allowed_pairs(rows: &[QueryRow]) -> BTreeSet<(String, String)> { + rows.iter() + .filter(|r| r.response.decision == Decision::Allow) + .map(|r| { + ( + r.principal.clone().unwrap().to_string(), + r.resource.clone().unwrap().to_string(), + ) + }) + .collect() +} diff --git a/tests/schemas/kitchen_sink.cedarschema b/tests/schemas/kitchen_sink.cedarschema index 38aee79..b32340d 100644 --- a/tests/schemas/kitchen_sink.cedarschema +++ b/tests/schemas/kitchen_sink.cedarschema @@ -8,8 +8,12 @@ entity User in [Group] = { profile: { city: String, pets: Set, boss?: User }, friend?: User, friends: Set, + "friend.name"?: String, } tags String; +entity Empty; +entity Color enum ["red", "blue"]; + entity Doc in [Group] = { owner: User, } tags User; @@ -19,3 +23,7 @@ action "view" appliesTo { resource: [Doc], context: { ip: String } }; + +action "mate" appliesTo { principal: [User], resource: [User], context: { ip: String } }; +action "empty" appliesTo { principal: [User], resource: [Empty], context: { ip: String } }; +action "color" appliesTo { principal: [User], resource: [Color], context: { ip: String } };