Skip to content

Fix for core change making non scope constraints an Option #749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
john-h-kastner-aws merged 1 commit into from
Sep 29, 2025
Merged

Fix for core change making non scope constraints an Option #749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 6 additions & 50 deletions cedar-drt/fuzz/fuzz_targets/formatter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@

#![no_main]

use cedar_drt::check_policy_equivalence;
use cedar_drt::logger::initialize_log;
use cedar_drt_inner::fuzz_target;

use cedar_policy_core::ast::{AnyId, StaticPolicy, Template};
use cedar_policy_core::ast::{StaticPolicy, Template};
use cedar_policy_core::parser::{self, parse_policy};
use cedar_policy_formatter::token::{Comment, Token, WrappedToken};
use cedar_policy_formatter::{policies_str_to_pretty, Config};
Expand All @@ -32,8 +33,7 @@ use logos::Logos;
use rand::rngs::SmallRng;
use rand::{Rng, SeedableRng};
use similar_asserts::SimpleDiff;
use smol_str::SmolStr;
use std::collections::HashMap;
use std::sync::Arc;
use uuid::Builder;

// A thin wrapper for policy
Expand Down Expand Up @@ -162,53 +162,9 @@ fuzz_target!(|input: FuzzTargetInput| {
t.slots().collect::<Vec<_>>().is_empty(),
"\nold template slots should be empty\n"
);
// just dump to standard hashmaps to check equality without order.
// also ignore source locations, which are not preserved in this roundtrip
let roundtripped_anno: HashMap<&AnyId, &SmolStr> = roundtripped
.annotations()
.map(|(k, v)| (k, &v.val))
.collect();
let original_anno: HashMap<&AnyId, &SmolStr> =
t.annotations().map(|(k, v)| (k, &v.val)).collect();
assert_eq!(
original_anno, roundtripped_anno,
"\nannotations should be the same, found:\noriginal: {original_anno:?}\nroundtripped: {roundtripped_anno:?}\n",
);
assert_eq!(
roundtripped.effect(),
t.effect(),
"\nnew effect: {:?}\nold effect: {:?}\n",
roundtripped.effect(),
t.effect()
);
assert_eq!(
roundtripped.principal_constraint(),
t.principal_constraint(),
"\nnew principal constraint: {:?}\nold principal constraint: {:?}\n",
roundtripped.principal_constraint(),
t.principal_constraint()
);
assert_eq!(
roundtripped.action_constraint(),
t.action_constraint(),
"\nnew action constraint: {:?}\nold action constraint: {:?}\n",
roundtripped.action_constraint(),
t.action_constraint()
);
assert_eq!(
roundtripped.resource_constraint(),
t.resource_constraint(),
"\nnew resource constraint: {:?}\nold resource constraint: {:?}\n",
roundtripped.resource_constraint(),
t.resource_constraint()
);
assert!(
roundtripped
.non_scope_constraints()
.eq_shape(t.non_scope_constraints()),
"\nnew policy condition: {}\nold policy condition: {}\n",
roundtripped.non_scope_constraints(),
t.non_scope_constraints(),
check_policy_equivalence(
&Into::<Arc<Template>>::into(p),
&Into::<Arc<Template>>::into(roundtripped),
);
}
Err(err) => panic!(
Expand Down
69 changes: 35 additions & 34 deletions cedar-drt/src/parsing_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use smol_str::SmolStr;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::fmt::Write;
use std::hash::{Hash, Hasher};
use std::mem;

// Check that two policies are equivalent, ignoring policy ids and source
// locations. Panic if the two policies are not the same.
Expand All @@ -34,18 +35,26 @@ pub fn check_policy_equivalence(p_old: &Template, p_new: &Template) {
similar_asserts::assert_eq!(p_new.principal_constraint(), p_old.principal_constraint(),);
similar_asserts::assert_eq!(p_new.action_constraint(), p_old.action_constraint(),);
similar_asserts::assert_eq!(p_new.resource_constraint(), p_old.resource_constraint(),);
assert!(
p_new
.non_scope_constraints()
.eq_shape(p_old.non_scope_constraints()),
"{}",
similar_asserts::SimpleDiff::from_str(
&p_new.non_scope_constraints().to_string(),
&p_old.non_scope_constraints().to_string(),
"new",
"old"
)
);
match (p_new.non_scope_constraints(), p_old.non_scope_constraints()) {
(Some(p_new), Some(p_old)) if !p_new.eq_shape(p_old) => {
panic!(
"{}",
similar_asserts::SimpleDiff::from_str(
&p_new.to_string(),
&p_old.to_string(),
"new",
"old"
)
)
}
(Some(p_new), None) => {
panic!("New policy added non scope constraint {p_new}")
}
(None, Some(p_old)) => {
panic!("New policy dropped non scope constraint {p_old}")
}
_ => (),
}
}

// Check that we don't see a few specific errors, which correspond to violations
Expand Down Expand Up @@ -79,10 +88,14 @@ impl PartialEq for PreservedTemplate<'_> {
&& self.template.action_constraint() == other.template.action_constraint()
&& self.template.resource_constraint() == other.template.resource_constraint()
&& self.annotations == other.annotations
&& self
.template
.non_scope_constraints()
.eq_shape(other.template.non_scope_constraints())
&& match (
self.template.non_scope_constraints(),
other.template.non_scope_constraints(),
) {
(Some(s), Some(o)) => s.eq_shape(o),
(Some(_), None) | (None, Some(_)) => false,
(None, None) => true,
}
}
}

Expand All @@ -92,7 +105,10 @@ impl Hash for PreservedTemplate<'_> {
self.template.principal_constraint().hash(hasher);
self.template.action_constraint().hash(hasher);
self.template.resource_constraint().hash(hasher);
self.template.non_scope_constraints().hash_shape(hasher);
mem::discriminant(&self.template.non_scope_constraints()).hash(hasher);
if let Some(expr) = self.template.non_scope_constraints() {
expr.hash_shape(hasher);
}
self.annotations.hash(hasher);
}
}
Expand Down Expand Up @@ -129,26 +145,11 @@ pub fn policy_set_to_text(policy_set: &cedar_policy_core::ast::PolicySet) -> Str
let mut res = String::new();
let mut iter = policy_set.all_templates();
if let Some(template) = iter.next() {
to_cedar_text(template, &mut res).unwrap();
write!(res, "{template}").unwrap();
for template in iter {
writeln!(&mut res).unwrap();
to_cedar_text(template, &mut res).unwrap();
write!(res, "{template}").unwrap();
}
}
res
}

fn to_cedar_text(template: &Template, f: &mut impl std::fmt::Write) -> std::fmt::Result {
for (k, v) in template.annotations() {
writeln!(f, "@{}(\"{}\")", k, v.val.escape_debug())?
}
write!(
f,
"{}(\n {},\n {},\n {}\n) when {{\n {}\n}};",
template.effect(),
template.principal_constraint(),
template.action_constraint(),
template.resource_constraint(),
template.non_scope_constraints()
)
}
4 changes: 2 additions & 2 deletions cedar-policy-generators/src/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl From<GeneratedPolicy> for StaticPolicy {
gen.principal_constraint.into(),
gen.action_constraint.into(),
gen.resource_constraint.into(),
gen.abac_constraints,
Some(gen.abac_constraints),
)
.unwrap() // Will panic if the GeneratedPolicy contains a slot.
}
Expand All @@ -168,7 +168,7 @@ impl From<GeneratedPolicy> for Template {
gen.principal_constraint.into(),
gen.action_constraint.into(),
gen.resource_constraint.into(),
gen.abac_constraints,
Some(gen.abac_constraints),
)
}
}
Expand Down