allow entity attributes and tags to contain Action-typed entities #1652
Conversation
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 92.75% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.95% Status: PASSED ✅ Details
|
| .iter() | ||
| .map(|action_def| action_def.entity_type().as_ref().as_ref().clone()) | ||
| .collect(); | ||
| for action_type in action_types { |
There was a problem hiding this comment.
Could you use for_each instead of the collect + loop?
There was a problem hiding this comment.
The original reason for the .collect() is to avoid mutating-while-iterating. Stated in terms of the borrow checker, the first pass over action_types (lines 613-617) needs an immutable borrow of all_defs, while the second pass (lines 618-620) needs a mutable borrow of all_defs. So, we have to completely finish iterating all_defs in the first pass before the second pass can start; we can't make the mark_as_defined_entity_type() calls in the same iterator chain.
I'm certainly open to a better solution if there's a better pattern to use here.
There was a problem hiding this comment.
We could probably obtain the set of defined action types without iterating over all_defs.action_defs since there's only ever one action type defined per name space. Maybe that'd be a little more brittle than basing it off the actual actions, but it could save some time if there are a lot of actions in one namespace
There was a problem hiding this comment.
That makes sense. Thank you!
There was a problem hiding this comment.
there's only ever one action type defined per name space
How I've implemented it in this PR, there are either 0 or 1 action types defined per namespace. I have a test which demonstrates that if there are no actions defined in a namespace, the corresponding action type in that namespace is not defined either. This is important for name resolution to work if you want to refer to unqualified Action from within a namespace block
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 92.96% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.91% Status: PASSED ✅ Details
|
| .iter() | ||
| .map(|action_def| action_def.entity_type().as_ref().as_ref().clone()) | ||
| .collect(); | ||
| for action_type in action_types { |
There was a problem hiding this comment.
We could probably obtain the set of defined action types without iterating over all_defs.action_defs since there's only ever one action type defined per name space. Maybe that'd be a little more brittle than basing it off the actual actions, but it could save some time if there are a lot of actions in one namespace
| @@ -844,6 +854,13 @@ impl ValidatorSchema { | |||
| undeclared_parent_actions: impl IntoIterator<Item = EntityUID>, | |||
| common_types: impl IntoIterator<Item = ValidatorType>, | |||
| ) -> Result<()> { | |||
| // For the purposes of this function, the set of declared entity types | |||
| // includes the types of all declared actions. | |||
| let declared_entity_types: HashSet<&EntityType> = entity_types | |||
There was a problem hiding this comment.
Could this possibly be pulled from all_defs since you updated that to include that action entity types already?
There was a problem hiding this comment.
Yep; done this in the latest rev of this PR
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Coverage ReportHead Commit: Base Commit: Download the full coverage report. Coverage of Added or Modified Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 90.74% Status: PASSED ✅ Details
Coverage of All Lines of Rust CodeRequired coverage: 80.00% Actual coverage: 81.90% Status: PASSED ✅ Details
|
…dar-policy#1652) Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Description of changes
Allow entity attributes and tags to contain Action-typed entities. This was always supported in the evaluator, but is now also supported in policy validation, entity validation, and request validation. Any action UID literals that appear in attribute values or tag values must refer to actions that actually exist in the schema.
Issue #, if available
Fixes #304
Checklist for requesting a review
The change in this PR is (choose one, and delete the other options):
cedar-policy.I confirm that this PR (choose one, and delete the other options):
I confirm that
cedar-spec(choose one, and delete the other options):I confirm that
docs.cedarpolicy.com(choose one, and delete the other options):