Skip to content

use PolicyId when constructing policies #1055

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
merged 2 commits into from
Jul 10, 2024
Merged

use PolicyId when constructing policies #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee4c172
first pass
khieta Jul 10, 2024
876d73e
merge w/ main
khieta Jul 10, 2024
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
4 changes: 2 additions & 2 deletions cedar-policy-core/src/ast/policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2203,7 +2203,7 @@ mod test {
#[test]
fn unexpected_templates() {
let policy_str = r#"permit(principal == ?principal, action, resource);"#;
assert_matches!(parse_policy(Some("id".into()), policy_str), Err(e) => {
assert_matches!(parse_policy(Some(PolicyID::from_string("id")), policy_str), Err(e) => {
expect_exactly_one_error(policy_str, &e, &ExpectedErrorMessageBuilder::error(
"expected a static policy, got a template containing the slot ?principal"
)
Expand All @@ -2215,7 +2215,7 @@ mod test {

let policy_str =
r#"permit(principal == ?principal, action, resource) when { ?principal == 3 } ;"#;
assert_matches!(parse_policy(Some("id".into()), policy_str), Err(e) => {
assert_matches!(parse_policy(Some(PolicyID::from_string("id")), policy_str), Err(e) => {
expect_some_error_matches(policy_str, &e, &ExpectedErrorMessageBuilder::error(
"expected a static policy, got a template containing the slot ?principal"
)
Expand Down
34 changes: 21 additions & 13 deletions cedar-policy-core/src/ast/policy_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,11 +511,14 @@ mod test {
#[test]
fn link_conflicts() {
let mut pset = PolicySet::new();
let p1 = parser::parse_policy(Some("id".into()), "permit(principal,action,resource);")
.expect("Failed to parse");
let p1 = parser::parse_policy(
Some(PolicyID::from_string("id")),
"permit(principal,action,resource);",
)
.expect("Failed to parse");
pset.add_static(p1).expect("Failed to add!");
let template = parser::parse_policy_template(
Some("t".into()),
Some(PolicyID::from_string("t")),
"permit(principal == ?principal, action, resource);",
)
.expect("Failed to parse");
Expand Down Expand Up @@ -544,16 +547,18 @@ mod test {
#[test]
fn policyset_add() {
let mut pset = PolicySet::new();
let static_policy =
parser::parse_policy(Some("id".into()), "permit(principal,action,resource);")
.expect("Failed to parse");
let static_policy = parser::parse_policy(
Some(PolicyID::from_string("id")),
"permit(principal,action,resource);",
)
.expect("Failed to parse");
let static_policy: Policy = static_policy.into();
pset.add(static_policy)
.expect("Adding static policy in Policy form should succeed");

let template = Arc::new(
parser::parse_policy_template(
Some("t".into()),
Some(PolicyID::from_string("t")),
"permit(principal == ?principal, action, resource);",
)
.expect("Failed to parse"),
Expand Down Expand Up @@ -601,7 +606,7 @@ mod test {

let template2 = Arc::new(
parser::parse_policy_template(
Some("t".into()),
Some(PolicyID::from_string("t")),
"forbid(principal, action, resource == ?resource);",
)
.expect("Failed to parse"),
Expand Down Expand Up @@ -630,10 +635,13 @@ mod test {
#[test]
fn policy_conflicts() {
let mut pset = PolicySet::new();
let p1 = parser::parse_policy(Some("id".into()), "permit(principal,action,resource);")
.expect("Failed to parse");
let p1 = parser::parse_policy(
Some(PolicyID::from_string("id")),
"permit(principal,action,resource);",
)
.expect("Failed to parse");
let p2 = parser::parse_policy(
Some("id".into()),
Some(PolicyID::from_string("id")),
"permit(principal,action,resource) when { false };",
)
.expect("Failed to parse");
Expand All @@ -647,12 +655,12 @@ mod test {
#[test]
fn template_filtering() {
let template = parser::parse_policy_template(
Some("template".into()),
Some(PolicyID::from_string("template")),
"permit(principal == ?principal, action, resource);",
)
.expect("Template Parse Failure");
let static_policy = parser::parse_policy(
Some("static".into()),
Some(PolicyID::from_string("static")),
"permit(principal, action, resource);",
)
.expect("Static parse failure");
Expand Down
32 changes: 16 additions & 16 deletions cedar-policy-core/src/authorizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,25 +233,25 @@ mod test {
forbid(principal, action, resource);
"#;

let p1 = parser::parse_policy(Some("1".into()), p1_src).unwrap();
let p1 = parser::parse_policy(Some(PolicyID::from_string("1")), p1_src).unwrap();
pset.add_static(p1).unwrap();

let ans = a.is_authorized(q.clone(), &pset, &entities);
assert_eq!(ans.decision, Decision::Allow);

pset.add_static(parser::parse_policy(Some("2".into()), p2_src).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("2")), p2_src).unwrap())
.unwrap();

let ans = a.is_authorized(q.clone(), &pset, &entities);
assert_eq!(ans.decision, Decision::Allow);

pset.add_static(parser::parse_policy(Some("3".into()), p3_src).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("3")), p3_src).unwrap())
.unwrap();

let ans = a.is_authorized(q.clone(), &pset, &entities);
assert_eq!(ans.decision, Decision::Allow);

pset.add_static(parser::parse_policy(Some("4".into()), p4_src).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("4")), p4_src).unwrap())
.unwrap();

let ans = a.is_authorized(q, &pset, &entities);
Expand Down Expand Up @@ -407,15 +407,15 @@ mod test {
};
"#;

pset.add_static(parser::parse_policy(Some("1".to_string()), src1).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("1")), src1).unwrap())
.unwrap();
pset.add_static(parser::parse_policy(Some("2".to_string()), src2).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("2")), src2).unwrap())
.unwrap();

let r = a.is_authorized_core(q.clone(), &pset, &es).decision();
assert_eq!(r, Some(Decision::Allow));

pset.add_static(parser::parse_policy(Some("3".to_string()), src3).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("3")), src3).unwrap())
.unwrap();

let r = a.is_authorized_core(q.clone(), &pset, &es).decision();
Expand Down Expand Up @@ -454,9 +454,9 @@ mod test {
unknown("test")
};
"#;
pset.add_static(parser::parse_policy(Some("1".to_string()), src1).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("1")), src1).unwrap())
.unwrap();
pset.add_static(parser::parse_policy(Some("2".to_string()), src2).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("2")), src2).unwrap())
.unwrap();

let r = a.is_authorized_core(q.clone(), &pset, &es);
Expand Down Expand Up @@ -501,7 +501,7 @@ mod test {
permit(principal, action, resource) when { false };
"#;

pset.add_static(parser::parse_policy(Some("1".into()), src1).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("1")), src1).unwrap())
.unwrap();
let r = a.is_authorized_core(q.clone(), &pset, &es);
assert_eq!(r.decision(), Some(Decision::Deny));
Expand All @@ -510,7 +510,7 @@ mod test {
forbid(principal, action, resource) when { unknown("a") };
"#;

pset.add_static(parser::parse_policy(Some("2".into()), src2).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("2")), src2).unwrap())
.unwrap();
let r = a.is_authorized_core(q.clone(), &pset, &es);
assert_eq!(r.decision(), Some(Decision::Deny));
Expand All @@ -522,9 +522,9 @@ mod test {
permit(principal, action, resource) when { true };
"#;

pset.add_static(parser::parse_policy(Some("3".into()), src3).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("3")), src3).unwrap())
.unwrap();
pset.add_static(parser::parse_policy(Some("4".into()), src4).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("4")), src4).unwrap())
.unwrap();
let r = a.is_authorized_core(q.clone(), &pset, &es);
assert_eq!(r.decision(), Some(Decision::Deny));
Expand Down Expand Up @@ -566,9 +566,9 @@ mod test {
forbid(principal, action, resource) when { true };
"#;

pset.add_static(parser::parse_policy(Some("1".into()), src1).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("1")), src1).unwrap())
.unwrap();
pset.add_static(parser::parse_policy(Some("2".into()), src2).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("2")), src2).unwrap())
.unwrap();

let r = a.is_authorized_core(q.clone(), &pset, &es);
Expand All @@ -580,7 +580,7 @@ mod test {
let r2: Response = r.reauthorize(&map, &a, &es).unwrap().into();
assert_eq!(r2.decision, Decision::Allow);

pset.add_static(parser::parse_policy(Some("3".into()), src3).unwrap())
pset.add_static(parser::parse_policy(Some(PolicyID::from_string("3")), src3).unwrap())
.unwrap();
let r = a.is_authorized_core(q.clone(), &pset, &es);
assert_eq!(r.decision(), Some(Decision::Deny));
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-core/src/evaluator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4222,7 +4222,7 @@ pub mod test {
#[test]
fn template_interp() {
let t = parse_policy_template(
Some("template".to_string()),
Some(PolicyID::from_string("template")),
r#"permit(principal == ?principal, action, resource);"#,
)
.expect("Parse Error");
Expand Down
26 changes: 10 additions & 16 deletions cedar-policy-core/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,13 +104,10 @@ pub fn parse_policyset_to_ests_and_pset(
/// If `id` is Some, then the resulting template will have that `id`.
/// If the `id` is None, the parser will use "policy0".
pub fn parse_policy_template(
id: Option<String>,
id: Option<ast::PolicyID>,
text: &str,
) -> Result<ast::Template, err::ParseErrors> {
let id = match id {
Some(id) => ast::PolicyID::from_string(id),
None => ast::PolicyID::from_string("policy0"),
};
let id = id.unwrap_or(ast::PolicyID::from_string("policy0"));
let cst = text_to_cst::parse_policy(text)?;
cst.to_policy_template(id)
}
Expand All @@ -135,11 +132,11 @@ pub fn parse_policy_template_to_est_and_ast(
/// simple main function for parsing a policy.
/// If `id` is Some, then the resulting policy will have that `id`.
/// If the `id` is None, the parser will use "policy0".
pub fn parse_policy(id: Option<String>, text: &str) -> Result<ast::StaticPolicy, err::ParseErrors> {
let id = match id {
Some(id) => ast::PolicyID::from_string(id),
None => ast::PolicyID::from_string("policy0"),
};
pub fn parse_policy(
id: Option<ast::PolicyID>,
text: &str,
) -> Result<ast::StaticPolicy, err::ParseErrors> {
let id = id.unwrap_or(ast::PolicyID::from_string("policy0"));
let cst = text_to_cst::parse_policy(text)?;
cst.to_policy(id)
}
Expand All @@ -148,13 +145,10 @@ pub fn parse_policy(id: Option<String>, text: &str) -> Result<ast::StaticPolicy,
/// EST of the original policy without any of the lossy transforms involved in
/// converting to AST.
pub fn parse_policy_to_est_and_ast(
id: Option<String>,
id: Option<ast::PolicyID>,
text: &str,
) -> Result<(est::Policy, ast::StaticPolicy), err::ParseErrors> {
let id = match id {
Some(id) => ast::PolicyID::from_string(id),
None => ast::PolicyID::from_string("policy0"),
};
let id = id.unwrap_or(ast::PolicyID::from_string("policy0"));
let cst = text_to_cst::parse_policy(text)?;
let ast = cst.to_policy(id)?;
let est = cst.try_into_inner()?.try_into()?;
Expand Down Expand Up @@ -338,7 +332,7 @@ mod tests {
for template in all_templates().map(Template::from) {
let id = template.id();
let src = format!("{template}");
let parsed = parse_policy_template(Some(id.to_string()), &src).unwrap();
let parsed = parse_policy_template(Some(ast::PolicyID::from_string(id)), &src).unwrap();
assert_eq!(
parsed.slots().collect::<HashSet<_>>(),
template.slots().collect::<HashSet<_>>()
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-core/src/parser/cst_to_ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2737,7 +2737,7 @@ mod tests {
#[test]
fn issue_wf_5046() {
let policy = parse_policy(
Some("WF-5046".into()),
Some(ast::PolicyID::from_string("WF-5046")),
r#"permit(
principal,
action in [Action::"action"],
Expand Down
6 changes: 3 additions & 3 deletions cedar-policy-validator/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,13 +261,13 @@ mod test {
let validator = Validator::new(schema);

let policy_a_src = r#"permit(principal in foo_type::"a", action == Action::"actin", resource == bar_type::"b");"#;
let policy_a = parser::parse_policy(Some("pola".to_string()), policy_a_src)
let policy_a = parser::parse_policy(Some(PolicyID::from_string("pola")), policy_a_src)
.expect("Test Policy Should Parse");
set.add_static(policy_a.clone())
.expect("Policy already present in PolicySet");

let policy_b_src = r#"permit(principal in foo_tye::"a", action == Action::"action", resource == br_type::"b");"#;
let policy_b = parser::parse_policy(Some("polb".to_string()), policy_b_src)
let policy_b = parser::parse_policy(Some(PolicyID::from_string("polb")), policy_b_src)
.expect("Test Policy Should Parse");
set.add_static(policy_b.clone())
.expect("Policy already present in PolicySet");
Expand Down Expand Up @@ -357,7 +357,7 @@ mod test {
let validator = Validator::new(schema);

let t = parser::parse_policy_template(
Some("template".to_string()),
Some(PolicyID::from_string("template")),
r#"permit(principal == some_namespace::User::"Alice", action, resource in ?resource);"#,
)
.expect("Parse Error");
Expand Down
10 changes: 5 additions & 5 deletions cedar-policy-validator/src/rbac.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1461,7 +1461,7 @@ mod test {
.try_into()
.unwrap();
let policy = parse_policy(
Some("0".to_string()),
Some(PolicyID::from_string("0")),
r#"permit(principal == a::"p", action, resource == a::"r");"#,
)
.unwrap();
Expand All @@ -1476,7 +1476,7 @@ mod test {
#[cfg(feature = "partial-validate")]
mod partial_schema {
use cedar_policy_core::{
ast::{StaticPolicy, Template},
ast::{PolicyID, StaticPolicy, Template},
parser::parse_policy,
};

Expand Down Expand Up @@ -1508,21 +1508,21 @@ mod partial_schema {
#[test]
fn undeclared_entity_type_partial_schema() {
let policy = parse_policy(
Some("0".to_string()),
Some(PolicyID::from_string("0")),
r#"permit(principal == User::"alice", action, resource);"#,
)
.unwrap();
assert_validates_with_empty_schema(policy);

let policy = parse_policy(
Some("0".to_string()),
Some(PolicyID::from_string("0")),
r#"permit(principal, action == Action::"view", resource);"#,
)
.unwrap();
assert_validates_with_empty_schema(policy);

let policy = parse_policy(
Some("0".to_string()),
Some(PolicyID::from_string("0")),
r#"permit(principal, action, resource == Photo::"party.jpg");"#,
)
.unwrap();
Expand Down
8 changes: 4 additions & 4 deletions cedar-policy-validator/src/str_checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ mod test {
"#;

let mut s = PolicySet::new();
let p = parse_policy(Some("test".to_string()), src).unwrap();
let p = parse_policy(Some(PolicyID::from_string("test")), src).unwrap();
s.add_static(p).unwrap();
let warnings =
confusable_string_checks(s.policies().map(|p| p.template())).collect::<Vec<_>>();
Expand Down Expand Up @@ -196,7 +196,7 @@ mod test {
};
"#;
let mut s = PolicySet::new();
let p = parse_policy(Some("test".to_string()), src).unwrap();
let p = parse_policy(Some(PolicyID::from_string("test")), src).unwrap();
s.add_static(p).unwrap();
let warnings = confusable_string_checks(s.policies().map(|p| p.template()));
assert_eq!(warnings.count(), 2);
Expand All @@ -210,7 +210,7 @@ mod test {
};
"#;
let mut s = PolicySet::new();
let p = parse_policy(Some("test".to_string()), src).unwrap();
let p = parse_policy(Some(PolicyID::from_string("test")), src).unwrap();
s.add_static(p).unwrap();
let warnings =
confusable_string_checks(s.policies().map(|p| p.template())).collect::<Vec<_>>();
Expand Down Expand Up @@ -239,7 +239,7 @@ mod test {
};
"#;
let mut s = PolicySet::new();
let p = parse_policy(Some("test".to_string()), src).unwrap();
let p = parse_policy(Some(PolicyID::from_string("test")), src).unwrap();
s.add_static(p).unwrap();
let warnings =
confusable_string_checks(s.policies().map(|p| p.template())).collect::<Vec<_>>();
Expand Down
Loading