Null JSON values in context and entity attribute JSON data #37
Conversation
| This is our current behavior. | ||
| As stated above, it limits what data can be used to construct a context, forcing users to remove it in their own preprocessing passes. | ||
| Cedar should be easy and ergonomic to use, so we should hesitate to offload work users without good reason, but, if the correct interpretation of `null` is not always the same, it may be the correct choice here. | ||
|
|
||
| If we take this alternative, we should keep in mind that many users are likely to find themselves writing the same filter to remove `null` from their data, so we should consider providing this filter ourselves, even if it is not an integral part of the Cedar data format. |
There was a problem hiding this comment.
This doesn't sound that hard, actually. There are lots of libraries for easy JSON parsing/transformation, no? I think that it might be confusing that Cedar drops nulls for attributes, but errors when they are provided in sets. Better to let users drop null if it's acceptable to them, wherever null might appear?
There was a problem hiding this comment.
And the drawbacks you've listed seem non-trivial ...
There was a problem hiding this comment.
Yes, based on the drawbacks I'm also no longer fully convinced that dropping null attribute only is the right solution. Dropping null everywhere actually seems better, but the example with all is very misleading.
There was a problem hiding this comment.
I don't think there's too much value in us providing the filter ourselves. Our filter would presumably just be a wrapper around some existing JSON parsing function. Also, the context/entity data may be coming from other languages (not Rust) and it may make more sense to do the filtering there.
There was a problem hiding this comment.
We have the data as JSONValue objects, so we're in a position to transform it easily and correctly. My concern is that someone might have JSON data as a string, so we're requiring them to parse and update it, or try to do some ad-hoc string operations.
There was a problem hiding this comment.
I'd like to see the scope of this RFC expanded to Cedar's JSON representations for schemas and policies. Although those are conceptually a little different (they're "code"/"metadata" rather than "data") I think it's useful to have the discussion all in one place.
| ### Unexpected behavior | ||
|
|
||
| Ignoring `null` may not be the desired behavior for all users. | ||
| It seems reasonable that a `null` attribute value can be safely ignored, but a user may expect that `context has foo` evaluates to `true` if `foo` is present in their JSON context data. | ||
| It will instead evaluate to `false` if `foo` is present but has a `null` value. | ||
|
|
||
| In support of this point, observe how JavaScript treats objects containing `null`: | ||
| ```js | ||
| Object.hasOwn({foo: null}, 'foo') // true | ||
| ``` |
There was a problem hiding this comment.
This seems to be a pretty big drawback -- it's making me lean more towards disallowing null.
There was a problem hiding this comment.
What errors does a user see when they pass in a null value somewhere? Could we address the issue in this RFC by just making clearer error messages to explain why null values aren't allowed?
There was a problem hiding this comment.
Agreed. This is a major draw back.
The current error is data did not match any variant of untagged enum CedarValueJson, so, yes, this need to be improved, but from the complaints we've heard about this, users do understand that null is the problem.
| This is our current behavior. | ||
| As stated above, it limits what data can be used to construct a context, forcing users to remove it in their own preprocessing passes. | ||
| Cedar should be easy and ergonomic to use, so we should hesitate to offload work users without good reason, but, if the correct interpretation of `null` is not always the same, it may be the correct choice here. | ||
|
|
||
| If we take this alternative, we should keep in mind that many users are likely to find themselves writing the same filter to remove `null` from their data, so we should consider providing this filter ourselves, even if it is not an integral part of the Cedar data format. |
There was a problem hiding this comment.
I don't think there's too much value in us providing the filter ourselves. Our filter would presumably just be a wrapper around some existing JSON parsing function. Also, the context/entity data may be coming from other languages (not Rust) and it may make more sense to do the filtering there.
| These are JSON objects, so this RFC as written up to this point implies ignoring `null` on attributes appearing here. | ||
| We will instead continue to treat `null` as an error in this context. |
There was a problem hiding this comment.
Curious what the reasoning is either way -- why should we accept or not accept "__entity": null and similar.
There was a problem hiding this comment.
Expanded on this bit in the text. Treating it as {} would implicitly make it a record instead of an entity. We could parse null as an entity (unspecified?), but I think that's beyond the scope of the problem this RFC wants to solve.
| This is a good alternative if unless believe that the majority of users want the same non-error parsing of `null`. | ||
| In that case we would be requiring that many users independently re-implement the same preprocessing pass. | ||
| Even in this case, supporting one non-error alternative (e.g., this rfc, or ignore `null` everywhere) would still force users wanting a different option to write their own preprocessing pass while making the behavior when a `null` is included in their data more difficult to diagnose. | ||
| Rather than see an error when parsing, an unexpected parsing of `null` values could result in unexpected authorization decision. | ||
|
|
There was a problem hiding this comment.
I find this argument compelling for staying with the status quo, so I propose to reject this RFC.
In particular: The "ignore attributes set to null" behavior is easily implemented as a pre-pass by customers themselves, but the alternative "error on occurrences of null" anywhere becomes impossible if we change the status quo. We would need a lot of customers to prefer the proposed behavior to override customers who don't want it. Moreover, they would have to be comfortable with the half-measure of only ignoring null in top-level attributes and nowhere else; otherwise they would still need to write their own code to deal with null. I have not seen evidence of a clamor from customers for the proposed behavior change.
A possible alternative would be to provide some sort of utility function that does the JSON null-scrubbing optionally, which customers could invoke prior to converting the JSON to a context or entity, if they wished. But there is no need for an RFC for that.
|
After discussion on this PR, I am convinced that the drawbacks as listed in the RFC outweigh the benefit of accepting more JSON values, so I am moving this RFC into the final comment period with intent to reject. |
john-h-kastner-aws
added
final-comment-period
|
If we do reject this RFC as proposed, do we need any documentation updates to make Cedar's behavior on |
Yes. Some concrete action items we'll open as issues on relevant repositories if this is rejected:
|
|
Closing this RFC as rejected at the end of the final comment period. |
john-h-kastner-aws
removed
the
final-comment-period
khieta
added
the
rejected
Rendered
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.