Skip to content
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.

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

Add basic EST round-trip testing #215

Merged
merged 6 commits into from
Feb 16, 2024
Merged

Add basic EST round-trip testing #215

merged 6 commits into from
Feb 16, 2024

Conversation

khieta
Copy link
Contributor

@khieta khieta commented Feb 11, 2024

Issue #, if available:

#204

Description of changes:

This PR renames the pp target to roundtrip and adds two additional checks per policy:

  1. Check that parsing a displayed policy using parser::parse_policy_or_template_to_est and converting to JSON produces no errors.
  2. Check that converting a policy to JSON and back preserves the policy structure.

To test, I reverted cedar#601 and confirmed that the underlying bug was found quickly by check 1. Check 2 didn't turn up any bugs during an overnight run, but it seems useful(?)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@cdisselkoen cdisselkoen left a comment

Choose a reason for hiding this comment

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

See also the tests and test utilities in est.rs, which define three different kinds of roundtrips involving EST, one of which is always lossless.

Comment on lines 87 to 89
// Panics if any step fails. The resulting policy may be different from the input
// (since the roundtrip is lossy), so this function only checks that the round-trip
// succeeds without error.
Copy link
Contributor

Choose a reason for hiding this comment

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

The lossy step is EST->AST or CST->AST, neither of which is included in this roundtrip I think? (I believe this takes AST input and goes AST->text->EST->JSON?) So I think we actually could check that the resulting policy matches the input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EST->AST would be required here because check_policy_equivalence expects two ASTs.

But it would be possible to define this function for ESTs too. I'll update this PR later to include more variants, inspired by the helper functions in est.rs.

Copy link
Contributor Author

@khieta khieta Feb 12, 2024

Choose a reason for hiding this comment

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

Update: just looked at this again, and I think this step actually should be lossless. I assumed the translation was supposed to be lossy since DRT failed when I compare the outputs. But looking at the failure again, it looks like DRT was pointing out an actual error: cedar#618, which should be resolved by cedar#622. I'll add in the code to do the AST comparison, and test again once cedar#622 is finished.

@khieta khieta linked an issue Feb 12, 2024 that may be closed by this pull request
2 tasks
@khieta khieta merged commit 67744b2 into main Feb 16, 2024
3 checks passed
@khieta khieta deleted the khieta/add-est-to-pp branch February 16, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Round trip testing for JSON policy format
3 participants