support hasTag() and getTag() in the validator #1218

cdisselkoen · 2024-09-20T13:50:42Z

Description of changes

Support .hasTag() and .getTag() (RFC 82) in the validator.

Issue #, if available

RFC 82

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):

Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)

Signed-off-by: Craig Disselkoen <[email protected]>

aaronjeline

Minor comments

cedar-policy-validator/src/typecheck.rs

cedar-policy-validator/src/types/capability.rs

Signed-off-by: Craig Disselkoen <[email protected]>

john-h-kastner-aws

Looks pretty good, but I think there might be a bug in tag_types for the AnyEntity case (so permissive validation only)

cedar-policy-validator/src/typecheck/test/tags.rs

john-h-kastner-aws · 2024-09-20T15:06:40Z

cedar-policy-validator/src/diagnostics/validation_errors.rs

    ContainsAnyAll,
+    /// While computing the type of a `.getTag()` operation
+    #[error("tag types for a `.getTag()` operation")]
+    GetTag,


I don't see a test covering this variant. Probably would need to use the permissive validation mode.

Thanks for this -- caught a case where we were actually throwing the new internal invariant violation error

Well, added a test permissive_tags that caught one bug in permissive validation of tags, but doesn't actually exercise this case. I'm not as familiar with the validation algorithm -- any help on how I can construct a legal expression e of arbitrary LUB type? (You can see in the new test that if-then-else is not legal for constructing arbitrary LUBs, even in permissive mode)

Doesn't it? I see an error message I see tag types for a `.getTag()` operation must have compatible types as the expected error message for a test case

I guess if you want a test case where this error might be raised but isn't you could do something with record type tags. So, entity A tags { foo : Long }; and entity B tags { foo : Long, bar: Long}

john-h-kastner-aws · 2024-09-20T15:10:16Z

cedar-policy-validator/src/typecheck.rs

+                                ) {
+                                    Ok(ty) => ty,
+                                    Err(e) => {
+                                        type_errors.push(ValidationError::incompatible_types(


This is a bit different from how the same situation is handled for an attribute access. IIRC, in that case we would call the access unsafe instead of complaining about the LUB directly. Not sure which ultimately gives the better user experience, but it'd be nice if these similar cases were handled in the same way.

john-h-kastner-aws · 2024-09-20T15:11:48Z

cedar-policy-validator/src/typecheck/test/tags.rs

+    expect_err(
+        src,
+        &miette::Report::new(error),
+        &ExpectedErrorMessageBuilder::error(r#"for policy `0`, unable to guarantee safety of access to tag `"foo"` on entity type `Blank`"#)


I expected this to be a NoTagsAllowed error, but the message looks like a UnsafeTagAccess. Is that correct? Should we have another test for NoTagsAllowed?

I think it's correct. Compare that the same policy with a has check does validate (see test immediately below), so suggesting the has check is a valid suggestion. I think that NoTagsAllowed is actually unreachable, because it's only thrown when you do have a capability but the entity type doesn't support tags, and I don't think there's a way to have a capability when the entity type doesn't support tags, because the .hasTag operation would be typed false and prevent typechecking of the branch where you would have had the capability.

Makes sense.

Suggesting a has check when that has would always be false isn't ideal and we could probably add another flag to the error to catch it (left a similar common on the action error message).

cedar-policy-validator/src/typecheck.rs

john-h-kastner-aws · 2024-09-20T16:15:35Z

cedar-policy-validator/src/typecheck.rs

+            EntityRecordKind::AnyEntity => Ok(self
+                .schema
+                .entity_types()
+                .filter_map(|(_, vety)| vety.tag_type())


This might not work. IIRC, AnyEntity could include actions

Can you elaborate what's wrong here? I assume that AnyEntity means a LUB of all entities in the schema, and regardless of whether that includes actions or not, this function is documented to ignore any entity types that do not have tags (which will include all actions)

Ok. I think the code is correct overall. I guess I'm still confused about what exactly this function is supposed to compute. Does it just need to be a super set of the possible tags types for all types in kind?

It's very possible the spec (in the comments) for this function is wrong. It's used to determine the type of a .getTag() expression. Hard to think through the correct behavior of e.getTag(expr) when e is some LUB that contains some entities that do and some entities that do not have tags, or contains entities with different tag types. Maybe the former case should just be an error?

We could soundly type e.getTag(expr) for an e without tags as ⊥ (as long as we have the appropriate capability), so from that perspective it doesn't impact the tag LUB so we can just ignore it which looks like what you're doing here.

That's not quite how entity attribute are handled atm. IIRC (haven't tested) e.attr when an entity type in e doesn't not have the attribute is an error even when there's an appropriate capability.

Signed-off-by: Craig Disselkoen <[email protected]> Co-authored-by: John Kastner <[email protected]>

…sed (#1213) Signed-off-by: Craig Disselkoen <[email protected]>

Signed-off-by: Craig Disselkoen <[email protected]>

john-h-kastner-aws · 2024-09-20T19:49:06Z

cedar-policy-validator/src/typecheck/test/tags.rs

+        src,
+        &miette::Report::new(error),
+        &ExpectedErrorMessageBuilder::error(r#"for policy `0`, unable to guarantee safety of access to tag `"foo"`"#)
+            .help(r#"try testing for the tag's presence with `.hasTag("foo") && ..`"#)


Room for specializing this error message. Technically it'll fix the error, but leave you with a warning.

Signed-off-by: Craig Disselkoen <[email protected]>

john-h-kastner-aws

This all looks good IMO. Maybe there's more discussion to be had about the exact behavior in permissive validation, but that's experimental (and so is this PR, so doubly experimental) and the current behavior should be sound, so I'm happy with it

cdisselkoen · 2024-09-25T14:57:01Z

Merging per the above comment.

Signed-off-by: Craig Disselkoen <[email protected]> Co-authored-by: John Kastner <[email protected]> Co-authored-by: Kesha Hietala <[email protected]>

support hasTag() and getTag() in the validator

f7d2a30

Signed-off-by: Craig Disselkoen <[email protected]>

cdisselkoen requested review from aaronjeline and john-h-kastner-aws September 20, 2024 13:50

changelog

bdedc66

Signed-off-by: Craig Disselkoen <[email protected]>

aaronjeline approved these changes Sep 20, 2024

View reviewed changes

cedar-policy-validator/src/typecheck.rs Outdated Show resolved Hide resolved

cedar-policy-validator/src/types/capability.rs Outdated Show resolved Hide resolved

cdisselkoen added 3 commits September 20, 2024 14:07

cargo fmt

4ca507b

Signed-off-by: Craig Disselkoen <[email protected]>

review comment

460d77e

Signed-off-by: Craig Disselkoen <[email protected]>

tweak comment

921b7e4

Signed-off-by: Craig Disselkoen <[email protected]>

shaobo-he-aws approved these changes Sep 20, 2024

View reviewed changes

john-h-kastner-aws reviewed Sep 20, 2024

View reviewed changes

cdisselkoen and others added 5 commits September 20, 2024 13:48

Update cedar-policy-validator/src/typecheck.rs

8d0585f

Signed-off-by: Craig Disselkoen <[email protected]> Co-authored-by: John Kastner <[email protected]>

update PR template with reminder about docs (#1216)

40f365a

handle RFC 82 entity tags in entity JSON parsing, including schema-ba…

17b84b2

…sed (#1213) Signed-off-by: Craig Disselkoen <[email protected]>

tests for tags on actions

6851350

Signed-off-by: Craig Disselkoen <[email protected]>

internal invariant violation error

a6ffab8

Signed-off-by: Craig Disselkoen <[email protected]>

john-h-kastner-aws reviewed Sep 20, 2024

View reviewed changes

john-h-kastner-aws approved these changes Sep 20, 2024

View reviewed changes

cdisselkoen added 3 commits September 23, 2024 17:19

fixed a bug; add another test

ab4b4bf

Signed-off-by: Craig Disselkoen <[email protected]>

add a cfg(feature)

0363c89

Signed-off-by: Craig Disselkoen <[email protected]>

fix no-features build

c0853ad

Signed-off-by: Craig Disselkoen <[email protected]>

john-h-kastner-aws approved these changes Sep 24, 2024

View reviewed changes

cdisselkoen merged commit 5936f5f into main Sep 25, 2024
19 checks passed

cdisselkoen deleted the cdisselkoen/rfc-82-validator branch September 25, 2024 14:57

shaobo-he-aws pushed a commit that referenced this pull request Sep 25, 2024

support hasTag() and getTag() in the validator (#1218)

9f4d17f

Signed-off-by: Craig Disselkoen <[email protected]> Co-authored-by: John Kastner <[email protected]> Co-authored-by: Kesha Hietala <[email protected]>

shaobo-he-aws mentioned this pull request Sep 25, 2024

Cherry-pick PRs to the 4.1.x release #1232

Closed

support hasTag() and getTag() in the validator #1218

support hasTag() and getTag() in the validator #1218

Uh oh!

Conversation

cdisselkoen commented Sep 20, 2024

Description of changes

Issue #, if available

Checklist for requesting a review

Uh oh!

aaronjeline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

john-h-kastner-aws left a comment

Choose a reason for hiding this comment

Uh oh!

cdisselkoen commented Sep 25, 2024

Uh oh!

Uh oh!

Uh oh!

john-h-kastner-aws Sep 23, 2024 •

edited

Loading

john-h-kastner-aws Sep 23, 2024 •

edited

Loading