Skip to content
Merged
Changes from all commits
Commits
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
65 changes: 58 additions & 7 deletions cedar-policy-formatter/src/pprint/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@
* limitations under the License.
*/

use std::collections::BTreeMap;

use miette::{miette, Result, WrapErr};

use cedar_policy_core::ast::{PolicySet, Template};
use cedar_policy_core::ast::PolicySet;
use cedar_policy_core::parser::parse_policyset;
use cedar_policy_core::parser::{err::ParseErrors, text_to_cst::parse_policies};
use smol_str::ToSmolStr;

use crate::token::get_comment;

Expand All @@ -42,15 +45,19 @@ fn tree_to_pretty<T: Doc>(t: &T, context: &mut config::Context<'_>) -> Result<St
fn soundness_check(ps: &str, ast: &PolicySet) -> Result<()> {
let formatted_ast = parse_policyset(ps).wrap_err("formatter produces invalid policies")?;
let (formatted_policies, policies) = (
formatted_ast.templates().collect::<Vec<&Template>>(),
ast.templates().collect::<Vec<&Template>>(),
formatted_ast
.policies()
.map(|p| (p.id().to_smolstr(), p))
.collect::<BTreeMap<_, _>>(),
ast.policies()
.map(|p| (p.id().to_smolstr(), p))
.collect::<BTreeMap<_, _>>(),
);

if formatted_policies.len() != policies.len() {
return Err(miette!("missing formatted policies"));
}

for (f_p, p) in formatted_policies.into_iter().zip(policies.into_iter()) {
for ((f_p_id, f_p), (p_id, p)) in formatted_policies.into_iter().zip(policies.into_iter()) {
let (f_anno, anno) = (
f_p.annotations()
.map(|(k, v)| (k, &v.val))
Expand All @@ -59,7 +66,8 @@ fn soundness_check(ps: &str, ast: &PolicySet) -> Result<()> {
.map(|(k, v)| (k, &v.val))
.collect::<std::collections::BTreeMap<_, _>>(),
);
if !(f_anno == anno
if !(f_p_id == p_id
&& f_anno == anno
&& f_p.effect() == p.effect()
&& f_p.principal_constraint() == p.principal_constraint()
&& f_p.action_constraint() == p.action_constraint()
Expand All @@ -69,7 +77,7 @@ fn soundness_check(ps: &str, ast: &PolicySet) -> Result<()> {
.eq_shape(p.non_scope_constraints()))
{
return Err(miette!(
"policies differ in meaning or annotations:\noriginal: {p}\nformatted: {f_p}"
"policies differ in policy ids or meaning or annotations:\noriginal: {p}\nformatted: {f_p}"
Comment on lines -72 to +80
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: probably easier for debugging if you separate these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll leave it to future work

));
}
}
Expand Down Expand Up @@ -135,6 +143,49 @@ mod tests {

use super::*;

#[test]
fn test_soundness_check() {
let p1 = r#"permit (principal, action, resource)
when { "

a
" };"#;
let p2 = r#"permit (principal, action, resource)
when { "
a
" };"#;
assert!(soundness_check(p2, &parse_policyset(p1).unwrap()).is_err());
Comment on lines +148 to +157
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a dumb question, but why does the change in this PR help identify the issue in #862? Looking at the diff, it seems like the only change is to add a comparison of policy ids, which doesn't seem related.

Or is this test case unrelated to the changed code?

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

templates() to policies() I think (line 45)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, thanks. @shaobo-he-aws can you add a description to the PR so it's easy not to miss this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


let p1 = r#"
permit (principal, action, resource)
when { "a"};
permit (principal, action, resource)
when { "

a
" };"#;
let p2 = r#"
permit (principal, action, resource)
when { "
a
" };
permit (principal, action, resource)
when { "a"};"#;
assert!(soundness_check(p2, &parse_policyset(p1).unwrap()).is_err());

let p1 = r#"
permit (principal, action, resource)
when { "a" };
permit (principal, action, resource)
when { "b" };"#;
let p2 = r#"
permit (principal, action, resource)
when { "a" };
permit (principal, action, resource)
when { "b"};"#;
assert!(soundness_check(p2, &parse_policyset(p1).unwrap()).is_ok());
}

#[test]
fn test_format_files() {
let config = Config {
Expand Down