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

fix 437, properly handle unknowns in the context. #1447

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amzn-mdamine
Copy link

@amzn-mdamine amzn-mdamine commented Jan 29, 2025

Description of changes

Fixes the issues identified in #437

This change fix the validation of unknowns when validating the context.

Unit test have been added to meet the requirements specified in #437:

Some contexts containing unknowns should pass request validation, and others fail. For instance:

  • if the context contains attributes foo: 1 and bar: unknown, and the schema requires foo: long and bar: string, this should pass.
  • if the context contains attributes foo: 1 and bar: unknown, and the schema requires foo: string and bar:string, this should fail.
  • if the context contains attributes foo: 1 and bar: unknown, and the schema requires foo: long, bar: string, and baz: string, this should fail.

Issue #, if available

Resolves : #437

Checklist for requesting a review

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

  • A bug fix or other functionality change requiring a patch to cedar-policy.

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

  • Does not update the CHANGELOG because my change does not significantly impact released code.

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.

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.

Thanks for this contribution!

We can also remove the TODO in the comments on typecheck_restricted_expr().

@amzn-mdamine
Copy link
Author

Thanks for this contribution!

We can also remove the TODO in the comments on typecheck_restricted_expr().

Thank you for reviewing the PR.
I have removed the TODO comments.

Copy link
Contributor

@adpaco-aws adpaco-aws left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I'm not familiar with this code so I'm wondering how typechecking interacts in this case with the extensions. Is the use of unknowns already checked against the extensions before we get to this code?

Comment on lines +549 to +551
if restricted_expr.as_unknown().is_some() {
return Ok(true);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check if the partial-eval extension is enabled?

Comment on lines +2552 to +2553
let result = typeValidator
.typecheck_partial_value(&context.clone().into(), Extensions::all_available());
Copy link
Contributor

Choose a reason for hiding this comment

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

All the tests use Extensions::all_available(), but what happens if we don't use any?

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.

Request validation doesn't properly handle Contexts containing unknowns
3 participants