Bugfix/aaronjeline/358 by aaronjeline · Pull Request #365 · cedar-policy/cedar

aaronjeline · 2023-10-16T20:21:24Z

Description of changes

Moves our parsers to reject duplicate keys, instead of having last-write-wins semantics.
Adds a new dependency on serde-with

Issue #, if available

#358 and #352

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):

Does not require updates because my change does not impact the Cedar Dafny model or DRT infrastructure.

Disclaimer

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

aaronjeline · 2023-10-16T20:22:32Z

cedar-policy-cli/src/lib.rs

        })
 }

-fn load_actions_from_schema(entities: Entities, schema: &Option<Schema>) -> Result<Entities> {


Removing this function has it will be incorporated into the main cedar library, and we want a very specific error semantics for it

This overlaps with my #360

but I think it's the same change, so we should be able to accept both in either order

aaronjeline · 2023-10-16T20:23:03Z

cedar-policy/tests/example_use_cases_doc.rs

-#[should_panic(
-    expected = "error occurred while evaluating policy `policy0`: entity `Action::\\\"view\\\"` does not exist"
-)]
+#[should_panic(expected = "Action::\\\"view\\\"` does not have the attribute `readOnly`")]


Actually having the entity specifies changes the error message.

This overlaps with my #360

Not surprising. We'll want to make sure yours wins.

but I think it's the same change, so we should be able to accept both in either order

john-h-kastner-aws · 2023-10-16T20:36:07Z

cedar-policy-core/src/entities/json/entities.rs

+            EntityJsonParser::new(None, Extensions::all_available(), TCComputation::ComputeNow);
+        let e = eparser.from_json_value(json).err().unwrap();
+        let bad_euid: EntityUID = r#"User::"alice""#.parse().unwrap();
+        match e {


You can do something nice like

assert_matches(e, Err(EntitiesError::Duplicate(euid)) => { assert_eq!(bad_euid, euid, r#"Returned euid should be User::"alice""#); });

for this sort of assertion

john-h-kastner-aws · 2023-10-16T20:42:30Z

cedar-policy/src/frontend/is_authorized.rs


-        let context = Context::from_json_value(self.context, schema.as_ref().map(|s| (s, &action)))
+        let context = serde_json::to_value(self.context)
+            .map_err(|e| [format!("Error encoding the context as JSON: {e}")])?;


This change should be made to the Context constructor.

Has to happen here as well, once we deserialize to json the duplicates will already be lost.

are we serializing here or deserializing here?

De-serialized to a map so that we can use an annotation to fail on duplicates. Before we pass back to Context constructor we have to serialize back to a json-value. If we want to forbid duplicates in the context contructor itself, than this api can't just take a json value.

cedar-policy/src/frontend/is_authorized.rs

cedar-policy/src/frontend/utils.rs

cdisselkoen · 2023-10-17T13:42:14Z

cedar-policy/CHANGELOG.md

 - The `Response::new()` constructor now expects a `Vec<AuthorizationError>` as its third argument.
 - Implements RFC #19, making validation slightly more strict, but more explainable.
 - Improved formatting for error messages.
+- Standardized on duplicates being errors instead of last-write-wins for parsers.


I think we should be slightly more specific -- maybe list the affected APIs?

cdisselkoen · 2023-10-17T13:43:27Z

cedar-policy/src/frontend/is_authorized.rs


-        let context = Context::from_json_value(self.context, schema.as_ref().map(|s| (s, &action)))
+        let context = serde_json::to_value(self.context)
+            .map_err(|e| [format!("Error encoding the context as JSON: {e}")])?;


are we serializing here or deserializing here?

aaronjeline · 2023-10-17T15:40:08Z

~~Blocked on: cedar-policy/cedar-java#53~~
Fixed

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

aaronjeline commented Oct 16, 2023

View reviewed changes

john-h-kastner-aws reviewed Oct 16, 2023

View reviewed changes

cdisselkoen approved these changes Oct 17, 2023

View reviewed changes

Aaron Eline added 8 commits October 17, 2023 15:40

Move to handling duplicate keys as errors

5f6170d

Updating integration tests

124c1c7

Updated changelog

15eb15c

Remove vscode metadata

b7fd845

Quick integration test change

1df68f9

Removed heap profile metadata

baac0e6

More specific changelog

6b42adf

Better error messages

f3c3457

aaronjeline force-pushed the bugfix/aaronjeline/358 branch from 72a9843 to f3c3457 Compare October 17, 2023 15:41

Fixing a typo and updating expected error msgs

3f30438

andrewmwells-amazon approved these changes Oct 17, 2023

View reviewed changes

aaronjeline merged commit b9a9dd7 into main Oct 17, 2023

aaronjeline deleted the bugfix/aaronjeline/358 branch October 17, 2023 20:04

cdisselkoen mentioned this pull request Oct 31, 2023

[Cedar 3.0] Entities JSON format should error on duplicate UIDs #159

Closed

2 tasks

shaobo-he-aws mentioned this pull request Nov 1, 2023

Duplicate entries silently ignored in entites JSON deserialization #357

Closed

2 tasks

sarahcec added the 3.0 label Nov 27, 2023

shaobo-he-aws pushed a commit that referenced this pull request May 20, 2025

Add more datastructure lemmas (#365)

c3b43ba

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

benelser pushed a commit to benelser/cedar that referenced this pull request Jan 16, 2026

Bugfix/aaronjeline/358 (cedar-policy#365)

3e19b58

Conversation

aaronjeline commented Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue #, if available

Checklist for requesting a review

Disclaimer

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronjeline Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronjeline commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aaronjeline commented Oct 16, 2023 •

edited

Loading

aaronjeline Oct 17, 2023 •

edited

Loading

aaronjeline commented Oct 17, 2023 •

edited

Loading