Entity validation behaves incorrectly on record-typed attributes

[cdisselkoen]

Before opening, please confirm:

• [X] I have searched for duplicate or closed issues.
• [X] I have read the guide for submitting bug reports.
• [X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.

Bug Category

Schemas and Validation

Describe the bug

The documentation on Entities::from_entities() says

If a schema is present, this function will also ensure that the produced entities fully conform to the schema – for instance, it will error if attributes have the wrong types (e.g., string instead of integer), or if required attributes are missing or superfluous attributes are provided.

In actuality, this function will not correctly error if inside a record-typed attribute, an (inner) superfluous attribute is provided.

Confirmed this behavior on main and 3.3.0 as of this writing. Doesn't seem to affect Entities::from_json_*() because the JSON deserialization code does catch the superfluous inner attribute. For the same reason, this bug is not reproducible in the CLI (as far as I know). It only affects users of Entities::from_entities() who constructed entities programmatically.

Expected behavior

Expected behavior to match the docs. Expected Entities::from_entities() to reject an entity with a superfluous inner attribute.

Reproduction steps

The following works in an otherwise-blank Cargo project with dependency on cedar_policy 3.3.0 (and miette 7.2.0).

use cedar_policy::{Entities, Entity, EntityUid, RestrictedExpression, Schema};
use std::collections::{HashMap, HashSet};
use std::str::FromStr;

fn main() {
    let (schema, _) = Schema::from_cedarschema_str(
        "
        entity E {
          rec: {
            foo: Long
          }
        };
        action Act appliesTo {
          principal: [E],
          resource: [E],
        };
    ",
    )
    .unwrap();
    let entity = Entity::new(
        EntityUid::from_str(r#"E::"abc""#).unwrap(),
        HashMap::from_iter([(
            "rec".into(),
            RestrictedExpression::new_record([
                ("foo".into(), RestrictedExpression::new_long(4567)),
                (
                    "extra".into(),
                    RestrictedExpression::new_string("bad".into()),
                ),
            ])
            .unwrap(),
        )]),
        HashSet::new(),
    )
    .unwrap();
    match Entities::from_entities([entity], Some(&schema)) {
        Ok(_) => panic!("expected an error due to extra inner attribute `extra`"),
        Err(e) => println!("got an error: {:?}", miette::Report::new(e)),
    }
}

Code Snippet

above

Log output

No response

Additional configuration

No response

Operating System

No response

Additional information and screenshots

No response

[andrewmwells-amazon]

The entity record becomes the restricted expr: Record { attrs: {"extra": AttributeType { attr_type: String, required: false }, "foo": AttributeType { attr_type: Long, required: false }}, open_attrs: false }

We then check if actual_ty.is_consistent_with(expected_ty). This checks for required attrs in one but not the other (assuming openis false). Because the attrs of actual_ty are all optional, this actually only checks if actual_ty has all required attrs of expected_ty.

I think we can fix this by just changing schematype_of_restricted_expr to mark existing attrs as required. Contrary to the comments on that function, I think this makes sense. Consider the possibilities:

We check if SchemaType of two concrete entities are consistent. This doesn't make sense and will always be true because their attributes are optional. We check if SchemaTypes from the schema are consistent. Here we know exactly which attributes are required/optional. We check a concrete entity vs a SchemaType from the schema. Here we get our desired behavior by making the attrs required.

[john-h-kastner-aws]

Because the attrs of actual_ty are all optional, this actually only checks if actual_ty has all required attrs of expected_ty.

See #382

[cdisselkoen]

Marking existing attrs as required seems plausible to me as long as they still .is_consistent_with() a SchemaType in which one or more of them are optional.

[andrewmwells-amazon]

Marking attributes as required affects which sets are considered heterogeneous, which would be a breaking change. I made a new method for this instead.