From b0b22a484a2dd218cf5f93d8650aa1a283ed91b1 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 7 Mar 2024 11:38:17 -0500 Subject: [PATCH 1/4] Do not error on `in` between actions in different namespaces Fix #642 Signed-off-by: John Kastner --- cedar-policy-validator/src/typecheck.rs | 9 +- .../src/typecheck/test_namespace.rs | 119 +++++++++++++++++- 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/cedar-policy-validator/src/typecheck.rs b/cedar-policy-validator/src/typecheck.rs index cb2bb984da..203813c678 100644 --- a/cedar-policy-validator/src/typecheck.rs +++ b/cedar-policy-validator/src/typecheck.rs @@ -1865,7 +1865,14 @@ impl<'a> Typechecker<'a> { .get_entity_type(&rhs_name) .map(|ety| ety.descendants.contains(&lhs_name)) .unwrap_or(false); - if lhs_name == rhs_name || lhs_ty_in_rhs_ty { + // A schema may always declare that an action entity is a member of another action entity, + // regardless of their exact types (i.e., their namespaces), so we shouldn't treat it as an error. + let action_in_action = is_action_entity_type(&lhs_name) + && is_action_entity_type(&rhs_name); + if lhs_name == rhs_name + || action_in_action + || lhs_ty_in_rhs_ty + { TypecheckAnswer::success(type_of_in) } else { // We could actually just return `Type::False`, but this is incurs a larger Dafny proof update. diff --git a/cedar-policy-validator/src/typecheck/test_namespace.rs b/cedar-policy-validator/src/typecheck/test_namespace.rs index 0a5431c9f4..207a3389eb 100644 --- a/cedar-policy-validator/src/typecheck/test_namespace.rs +++ b/cedar-policy-validator/src/typecheck/test_namespace.rs @@ -19,6 +19,7 @@ #![cfg(test)] // GRCOV_STOP_COVERAGE +use cool_asserts::assert_matches; use serde_json::json; use std::str::FromStr; use std::vec; @@ -35,7 +36,7 @@ use super::test_utils::{ use crate::{ type_error::TypeError, types::{EntityLUB, Type}, - AttributeAccess, SchemaFragment, ValidatorSchema, + AttributeAccess, SchemaError, SchemaFragment, ValidatorSchema, }; fn namespaced_entity_type_schema() -> SchemaFragment { @@ -526,3 +527,119 @@ fn namespaced_entity_is_wrong_type_when() { )], ); } + +#[test] +fn multi_namespace_action_eq() { + let (schema, _) = SchemaFragment::from_str_natural( + r#" + action "Action" appliesTo { context: {} }; + namespace NS1 { action "Action" appliesTo { context: {} }; } + namespace NS2 { action "Action" appliesTo { context: {} }; } + "#, + ) + .unwrap(); + + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action == Action::"Action", resource);"#, + ) + .unwrap(), + ); + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action == NS1::Action::"Action", resource);"#, + ) + .unwrap(), + ); + + let policy = parse_policy( + None, + r#"permit(principal, action, resource) when { NS1::Action::"Action" == NS2::Action::"Action" };"#, + ) + .unwrap(); + assert_policy_typecheck_fails( + schema.clone(), + policy.clone(), + vec![TypeError::impossible_policy(policy.condition())], + ); +} + +#[test] +fn multi_namespace_action_in() { + let (schema, _) = SchemaFragment::from_str_natural( + r#" + namespace NS1 { action "Group"; } + namespace NS2 { action "Group" in [NS1::Action::"Group"]; } + namespace NS3 { + action "Group" in [NS2::Action::"Group"]; + action "Action" in [Action::"Group"] appliesTo { context: {} }; + } + namespace NS4 { action "Group"; } + "#, + ) + .unwrap(); + + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action in NS1::Action::"Group", resource);"#, + ) + .unwrap(), + ); + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action in NS2::Action::"Group", resource);"#, + ) + .unwrap(), + ); + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action in NS3::Action::"Group", resource);"#, + ) + .unwrap(), + ); + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#"permit(principal, action in NS3::Action::"Action", resource);"#, + ) + .unwrap(), + ); + + let policy = parse_policy( + None, + r#"permit(principal, action in NS4::Action::"Group", resource);"#, + ) + .unwrap(); + assert_policy_typecheck_fails( + schema.clone(), + policy.clone(), + vec![TypeError::impossible_policy(policy.condition())], + ); +} + +#[test] +fn multi_namespace_action_group_cycle() { + let (schema, _) = SchemaFragment::from_str_natural( + r#" + namespace A { action "Act" in C::Action::"Act"; } + namespace B { action "Act" in A::Action::"Act"; } + namespace C { action "Act" in B::Action::"Act"; } + "#, + ) + .unwrap(); + assert_matches!( + ValidatorSchema::try_from(schema), + Err(SchemaError::CycleInActionHierarchy(_)) + ) +} From f1764afd1bc9d85c3051a50f6a6884a5fc1a8fe4 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 7 Mar 2024 16:36:58 -0500 Subject: [PATCH 2/4] another test caes Signed-off-by: John Kastner --- .../src/typecheck/test_namespace.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cedar-policy-validator/src/typecheck/test_namespace.rs b/cedar-policy-validator/src/typecheck/test_namespace.rs index 207a3389eb..dca033899a 100644 --- a/cedar-policy-validator/src/typecheck/test_namespace.rs +++ b/cedar-policy-validator/src/typecheck/test_namespace.rs @@ -628,6 +628,42 @@ fn multi_namespace_action_in() { ); } +#[test] +fn test_cedar_policy_642() { + let (schema, _) = SchemaFragment::from_str_natural( + r#" + namespace NS1 { + entity SystemEntity2 in SystemEntity1; + entity SystemEntity1, PrincipalEntity; + action Group1; + } + namespace NS2 { + entity SystemEntity1 in NS1::SystemEntity2; + action "Group1" in NS1::Action::"Group1"; + action "Action1" in Action::"Group1" appliesTo { + principal: [NS1::PrincipalEntity], + resource: [NS2::SystemEntity1], + }; + } + "#, + ) + .unwrap(); + + assert_policy_typechecks( + schema.clone(), + parse_policy( + None, + r#" + permit( + principal in NS1::PrincipalEntity::"user1", + action in NS1::Action::"Group1", + resource in NS1::SystemEntity1::"entity1" + );"# + ) + .unwrap(), + ); +} + #[test] fn multi_namespace_action_group_cycle() { let (schema, _) = SchemaFragment::from_str_natural( From d978f7860ad1fc84588b4b9cf82367fa0cd60fd5 Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 7 Mar 2024 16:40:32 -0500 Subject: [PATCH 3/4] changelog Signed-off-by: John Kastner --- cedar-policy/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cedar-policy/CHANGELOG.md b/cedar-policy/CHANGELOG.md index 176e3c6f5d..b3e6bc8261 100644 --- a/cedar-policy/CHANGELOG.md +++ b/cedar-policy/CHANGELOG.md @@ -23,6 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Validation for the `in` operator to no longer reports an error when comparing actions + in different namespaces. (#704, resolving #642) + ## [3.1.0] - 2024-03-08 Cedar Language Version: 3.1.0 From 00e6051f203a8f7cc766db24349b60d532ef51bc Mon Sep 17 00:00:00 2001 From: John Kastner Date: Thu, 7 Mar 2024 16:55:54 -0500 Subject: [PATCH 4/4] fmt Signed-off-by: John Kastner --- cedar-policy-validator/src/typecheck/test_namespace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cedar-policy-validator/src/typecheck/test_namespace.rs b/cedar-policy-validator/src/typecheck/test_namespace.rs index dca033899a..a5b9b04b52 100644 --- a/cedar-policy-validator/src/typecheck/test_namespace.rs +++ b/cedar-policy-validator/src/typecheck/test_namespace.rs @@ -658,7 +658,7 @@ fn test_cedar_policy_642() { principal in NS1::PrincipalEntity::"user1", action in NS1::Action::"Group1", resource in NS1::SystemEntity1::"entity1" - );"# + );"#, ) .unwrap(), );