Automatically add action entities to Entities; validate entities against schema

[cdisselkoen]

Description of changes

• Entities::from_*() now automatically adds action entities from the schema, if a schema is provided
• Entities::from_*() now validates entities against the schema, if a schema is provided
• Entities::add_*() now validates entities against the schema, if a schema is provided
• Adds Entity::validate() to the public API, which validates a single entity against a schema
• Entities::from_entities() and Entities::add_entities() now take an optional schema argument
• Significant refactoring of entity-validation code in Core to facilitate all of the above changes
• includes refactoring of some errors and tweaks to some error messages
• some small refactors are also included in this PR that could technically be independent, e.g. passing schemas by reference to EntityJsonParser. If folks feel it's important, I can split those into a separate PR to declutter this one slightly. But this one is probably going to be a big PR regardless

Issue #, if available

Fixes #286

Checklist for requesting a review

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

• A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing 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):

• I'm not sure how my change impacts cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[cdisselkoen]

Pushed some tests and I'm marking this ready for review.

[cdisselkoen]

I remembered another reason why it might make sense to do it the way this PR currently has it (optional schema argument for the Entities constructors, standalone validation function for Entity). If you do the optional schema argument in both places, and pass the schema argument both times (which we should encourage users to do, if they have a schema), then this would result in double-validation on the Entities::from_entities() path, wasting work. We should try to have validation happen only once. Either it happens when constructing Entities, as currently in this PR, or it happens when constructing Entity and then Entities::from_entities() doesn't need to revalidate. The latter might make more sense, for the reasons you bring up. It might be a little more confusing; Entities::from_entities() probably still needs the optional schema argument for the purpose of adding schema actions, but won't be using that schema to perform validation. Nonetheless, it allows us to have an arguably better interface and avoid Entity::validate()? Thoughts?

[khieta]

Thinking about it some more... adding a schema argument to Entity::new might cause more problems than it solves. Like you said, we don't want to re-validate every Entity when constructing an Entities object. But in order to know whether an entity was already validated (and what schema it was validated with), we'd have to store a pointer to a (optional) schema in every entity.

So I retract my suggestion. I think it's ok to only do validation on constructing an Entities object, because this is the object that will be passed to the top-level is_authorized function. Users can use Entity::validate as a debugging tool in case they get confused when validation of a full Entities object fails.

New suggestion: update the comment for the public-facing validate function to say that this operation will be automatically performed when constructing an Entities object, so users don't wonder if they need to do Entity::validate in addition to passing the schema to Entities::from_*.

Re #191: now I'm torn on whether we want a separate validate function or an optional schema argument. The first would be consistent with Entity::new while the second would be consistent with Entities::from_*. Request::new is similar to Entities::from_* because it constructs an input for the top-level is_authorized function. But Entity::new and Request::new have the same name 🤔

[cdisselkoen]

in order to know whether an entity was already validated (and what schema it was validated with), we'd have to store a pointer to a (optional) schema in every entity.

I don't think this is necessarily the case. Since validation is optional anyway, we can just say we have an internal invariant that if you have an Entity object, it's already been validated to the extent you want it validated (and we ensure that you have the option to validate on every path to constructing an Entity object). Then Entities::from_entities() could skip validation based on this internal invariant.

Alternately if we want to be a little more explicit, we could just have a bool on each Entity indicating whether it's been validated, without referencing the schema it was validated with. This would work for the (vast majority of?) users who only have one schema, and who won't update their schema without restarting their app (and thus rebuilding the in-memory entity objects). It's a little unclear how we would use this bool, probably Entities::from_entities() could perform validation on anything not already validated (which only helps if Entities::from_entities() was given a schema but Entity::new() wasn't), maybe we could also expose a getter so users could check if a given entity was validated (but we don't currently offer that for Entities either; but we could).

Users can use Entity::validate() as a debugging tool

Yeah, I think this is the motivating use case for Entity::validate(), in the version in which it currently exists in this PR.

update the comment for the public-facing validate function ...

Yeah, this is certainly easy and good to do, if we stay with the way it currently is in this PR.

I'm still not convinced we don't want to add an optional schema argument to Entity::new() after all, and just have the internal invariant described above. As a bonus, users might get more targeted or immediate errors -- we can fail creating the Entity long before the user attempts to add it to an Entities.

Re #191

This is another reason why adding the optional schema argument to Entity::new() might make more sense.

[cdisselkoen]

I note that in the discussion on RFC 11 we decided not to add an optional schema argument to the existing Request::new(), but we also did not decide to write a separate Request::validate(). We proposed a middle ground, a Request::new_with_validation(). In the same spirit, in this PR we could have Entity::new_with_validation() instead of Entity::validate() or adding an optional argument to Entity::new(). This would still have the double-validation problem unless we removed validation from Entities::from_entities().

[mwhicks1]

To add my 2c: I like the idea of just supporting validation on Entities, and leaving Entity::new() alone. @khieta's reasoning is persuasive to me:

I think it's ok to only do validation on constructing an Entities object, because this is the object that will be passed to the top-level is_authorized function.

On a different topic: While it might be useful to have an Entity::validate() for debugging purposes, adding it just increases the APIs that we need to support, going forward. I think there's something to be said keeping the number of APIs as small and simple as possible, unless there is a compelling need. (If you want to validate a single entity, can you not create a singleton Entities and validate that?) I'm not getting the sense there is a compelling need; if one emerges, we can add this later.

[cdisselkoen]

Makes sense to me. I have now removed Entity::validate(), and left everything else as it already was in the PR.