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

Entity Literal Substitution #1233

Merged
merged 9 commits into from
Sep 27, 2024
Merged

Conversation

andrewmwells-amazon
Copy link
Contributor

@andrewmwells-amazon andrewmwells-amazon commented Sep 25, 2024

Description of changes

API for Entity literal substitution.

Issue #, if available

#1120

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

@shaobo-he-aws
Copy link
Contributor

There seem to be conflicts with main.

Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
@andrewmwells-amazon andrewmwells-amazon force-pushed the andrewmwells/entity_lit_sub branch 2 times, most recently from 7c39a22 to 2e8bd28 Compare September 26, 2024 21:27
Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
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.

Looks good overall; tiny nits and one question about an .expect()

cedar-policy-core/src/entities/json/value.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/entities/json/value.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/est/expr.rs Outdated Show resolved Hide resolved
Comment on lines +2958 to +2962
// PANIC SAFETY: This can't fail for a policy that was already constructed
#[allow(clippy::expect_used)]
let est = cloned_est
.sub_entity_literals(&mapping)
.expect("Internal error, failed to sub entity literals.");
Copy link
Contributor

Choose a reason for hiding this comment

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

help me understand why this expect is safe -- the panic-safety comment notes that the est must be valid; but what if the mapping isn't valid or something?

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 this particular part will be panic-free because we're substituting EntityUIDs for EntityUIDs. I'm less confident about the construction of the AST.

My original thought was that this method can't ensure a policy that previously passed validation still passes after substitution, so the caller will be responsible for checking that policies are valid. I'm not sure that's the best idea. There are some errors that we could catch e.g., mapping Action::"view" -> User::"Alice". Maybe we should try to catch as many as possible and then the caller still needs to re-validate against their schema?

Another option would be to require that the mapping not change entity types.

Copy link
Contributor Author

@andrewmwells-amazon andrewmwells-amazon Sep 27, 2024

Choose a reason for hiding this comment

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

Added test based on this that would have caused the AST check below to panic. Now returning an error type again, but I'm not sure reusing the error type is ideal because I suspect there are only a few ways this substitution could fail (the only one I've found so far is the Action -> User one).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanted the mapping to not change entity types, it could be an Eid->Eid mapping instead of EntityUID->EntityUID. If this fits the need, this might be a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that's the right call for now? It would work well for the internal use case, but I'm hesitant because I think using the same type for human-readable and UUIDs is an anti-pattern. It requires adding constraints like disallowing some prefix from the human-readable Eid and requiring it for the UUIDs.

Signed-off-by: Andrew Wells <anmwells@amazon.com>
Signed-off-by: Andrew Wells <anmwells@amazon.com>
@andrewmwells-amazon andrewmwells-amazon merged commit b7c9dc3 into main Sep 27, 2024
19 checks passed
@andrewmwells-amazon andrewmwells-amazon deleted the andrewmwells/entity_lit_sub branch September 27, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants