Skip to content

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

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.

Sign up for GitHub

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

Open
amzn-mdamine wants to merge 1 commit into
base: main
Choose a base branch
from
+196 −7
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 196 additions & 7 deletions cedar-policy-validator/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,9 +514,6 @@ impl Type {
/// it does typecheck for some permissible substitutions of the unknowns,
/// but does not typecheck for other permissible substitutions), this is
/// reported as an error.
///
/// TODO(#437): Handling of `Unknown`s is not yet complete and doesn't
/// properly behave according to the above description, as of this writing.
pub(crate) fn typecheck_partial_value(
&self,
value: &PartialValue,
Expand All @@ -526,7 +523,7 @@ impl Type {
PartialValue::Value(value) => self.typecheck_value(value, extensions),
PartialValue::Residual(expr) => match BorrowedRestrictedExpr::new(expr) {
Ok(rexpr) => self.typecheck_restricted_expr(rexpr, extensions),
Err(_) => Ok(false), // TODO(#437): instead of just reporting typecheck fails for all nontrivial residuals, we should do something more intelligent
Err(_) => Ok(false),
},
}
}
Expand All @@ -544,14 +541,14 @@ impl Type {
}

/// Does the given `BorrowedRestrictedExpr` have this validator type?
///
/// TODO(#437): Handling of restricted exprs containing `Unknown`s is not
/// yet complete or correct, as of this writing.
pub(crate) fn typecheck_restricted_expr(
&self,
restricted_expr: BorrowedRestrictedExpr<'_>,
extensions: &Extensions<'_>,
) -> Result<bool, ExtensionFunctionLookupError> {
if restricted_expr.as_unknown().is_some() {
return Ok(true);
}
Comment on lines +549 to +551
Copy link
Contributor

@adpaco-aws adpaco-aws Jan 31, 2025

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?

match self {
Type::Never => Ok(false), // no expr has type Never
Type::Primitive {
Expand Down Expand Up @@ -2520,6 +2517,198 @@ mod test {
.expect("Expected valid schema")
}

#[test]
#[cfg(feature = "partial-eval")]
fn test_typecheck_partial_value_with_unknown() {
use cedar_policy_core::ast::Context;
let context = Context::from_json_value(serde_json::json!({
"a": {
"__extn": {
"fn": "unknown",
"arg": "test_arg"
}
},
}))
.unwrap();

let typeValidator = Type::record_with_attributes(
[
(
"a".into(),
AttributeType::optional_attribute(Type::primitive_boolean()),
),
(
"b".into(),
AttributeType::optional_attribute(Type::primitive_boolean()),
),
(
"c".into(),
AttributeType::optional_attribute(Type::primitive_boolean()),
),
],
OpenTag::ClosedAttributes,
);

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

@adpaco-aws adpaco-aws Jan 31, 2025

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?

assert_eq!(result.unwrap(), true);
}

#[test]
#[cfg(feature = "partial-eval")]
fn test_typecheck_partial_value_with_embedded_unknown() {
use cedar_policy_core::ast::Context;
let context = Context::from_json_value(serde_json::json!({
"a": {
"b":{
"__extn": {
"fn": "unknown",
"arg": "test_arg"
}
}
},
}))
.unwrap();

let embedded_attributes = Type::record_with_attributes(
[
(
"b".into(),
AttributeType::optional_attribute(Type::primitive_boolean()),
),
(
"c".into(),
AttributeType::optional_attribute(Type::primitive_boolean()),
),
],
OpenTag::ClosedAttributes,
);

let typeValidator = Type::record_with_attributes(
[(
"a".into(),
AttributeType::optional_attribute(embedded_attributes),
)],
OpenTag::ClosedAttributes,
);

let result = typeValidator
.typecheck_partial_value(&context.clone().into(), Extensions::all_available());

// c is optional attribute it is not required to provide it.
assert_eq!(result.unwrap(), true);
}

#[test]
#[cfg(feature = "partial-eval")]
fn test_typecheck_partial_value_with_unknown1() {
use cedar_policy_core::ast::Context;
let context = Context::from_json_value(serde_json::json!({
"foo": 1,
"bar": {
"__extn": {
"fn": "unknown",
"arg": "test_arg"
}
},
}))
.unwrap();

let typeValidator = Type::record_with_attributes(
[
(
"foo".into(),
AttributeType::required_attribute(Type::primitive_long()),
),
(
"bar".into(),
AttributeType::required_attribute(Type::primitive_string()),
),
],
OpenTag::ClosedAttributes,
);

let result = typeValidator
.typecheck_partial_value(&context.clone().into(), Extensions::all_available());
//success both foo and bar are provided
assert_eq!(result.unwrap(), true);
}

#[test]
#[cfg(feature = "partial-eval")]
fn test_typecheck_partial_value_with_unknown2() {
use cedar_policy_core::ast::Context;
let context = Context::from_json_value(serde_json::json!({
"bar": {
"__extn": {
"fn": "unknown",
"arg": "test_arg"
}
},
"foo": 1,
}))
.unwrap();

let typeValidator = Type::record_with_attributes(
[
(
"foo".into(),
AttributeType::required_attribute(Type::primitive_string()),
),
(
"bar".into(),
AttributeType::required_attribute(Type::primitive_string()),
),
],
OpenTag::ClosedAttributes,
);

let result = typeValidator
.typecheck_partial_value(&context.clone().into(), Extensions::all_available());
//fail foo is string in schema not long
assert_eq!(result.unwrap(), false);
}

#[test]
#[cfg(feature = "partial-eval")]
fn test_typecheck_partial_value_with_unknown3() {
use cedar_policy_core::ast::Context;
let context = Context::from_json_value(serde_json::json!({
"bar": {
"__extn": {
"fn": "unknown",
"arg": "test_arg"
}
},
"foo": 1,
}))
.unwrap();

let typeValidator = Type::record_with_attributes(
[
(
"foo".into(),
AttributeType::required_attribute(Type::primitive_long()),
),
(
"bar".into(),
AttributeType::required_attribute(Type::primitive_string()),
),
(
"baz".into(),
AttributeType::required_attribute(Type::primitive_string()),
),
],
OpenTag::ClosedAttributes,
);

let result = typeValidator
.typecheck_partial_value(&context.clone().into(), Extensions::all_available());

//fail baz is required in the schema
assert_eq!(result.unwrap(), false);
}

/// Test cases with entity type Action are interesting because Action
/// does not need to be declared in the entity type list.
#[test]
Expand Down