feat: policy, template, and schema parsing #72

Swolebrain · 2023-12-15T20:59:17Z

Issue #, if available:

n/a

Description of changes:

Align behavior of JEntityTypeName with JEntityId. Previously, the FFI code had a difference in behavior between JEntityTypeName and JEntityId. The old code did not allow construction of an invalid id, but it did allow construction of an invalid JEntityTypeName. I changed it so it fails at construction time if it's invalid.
Also implemented the same pattern for JEntityUID, and gave it a method get_rust_repr.
New schema parse functionality
New policy parse functionality
New functionality to validate a template link - takes in a template and two EntityUIDs.

CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java

lucajone · 2023-12-20T15:49:06Z

CedarJava/src/main/java/com/cedarpolicy/model/slice/Policy.java

@@ -41,7 +54,8 @@ public Policy(
            throw new NullPointerException("Failed to construct policy from null string");
        }
        if (policyID == null) {
-            throw new NullPointerException("Failed to construct policy with null ID");
+            policyID = "policy" + idCounter;
+            idCounter += 1;


I think this could cause duplicate IDs if we construct Policy objects from multiple threads. I guess we could:

document this and leave it as-is

use an AtomicInteger for the counter (presumably the overhead here isn't massive)

allow null policy IDs in this class – I think the Rust library's constructors now take Options for policy IDs

AtomicInteger seems like a good choice, will do that

lucajone · 2023-12-20T15:49:11Z

CedarJava/src/main/java/com/cedarpolicy/model/slice/Policy.java

+    public static boolean validateTemplateLinkedPolicy(Policy p, EntityUID principal, EntityUID resource) {
+        try {
+            return validateTemplateLinkedPolicyJni(p.policySrc, principal, resource);
+        } catch (Exception e) {


Do we want to swallow every exception here? Internal exceptions might not mean an invalid policy.

What else could they mean? Bear in mind that the principal, resource, and policy have already been parsed by the library so they're valid

I guess they could manually build a Policy object from something invalid. I could consider re-parsing the policy this function takes, or making the rust-side change i mentioned below

I'm more thinking of exceptions that don't arise from the policy parsing calls specifically. For example, if we have a bug in our JNI code, perhaps that would surface as an exception from the JVM that we would want to bubble up. Possibly worrying too much here, but if we have a base class for our exceptions sitting around (InternalException?) then it might make sense to use that.

I think im convinced to make it throw

CedarJavaFFI/src/interface.rs

CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java

andrewmwells-amazon · 2023-12-22T00:39:02Z

CedarJava/src/main/java/com/cedarpolicy/model/slice/Policy.java

+     * and EntityIdentifier.parse), then this should only fail because the slots in the template don't match the instantiations
+     * (barring JNI failures).
+     * @param p Policy object constructed from a valid template. Best if built from Policy.parsePolicyTemplate
+     * @param principal EntityUid to put into the principal slot. Leave null if there's no principal slot


It's probably not as idiomatic for Java, but I strongly prefer Optional

Optional is not really a fit though, we need to surface why it failed if it fails. We have two choices that offer library ergonomics that are reasonable to java programmers:

Just throw exceptions

Use a library that implements an either monad, or implement it ourselves

Sorry, I should have been more clear. I'd prefer that the principal and resource be Optional<EntityUID> instead of using null when there's no slot. I agree we should throw an exception on failure, not return an Optional<boolean>.

andrewmwells-amazon · 2023-12-22T04:48:33Z

CedarJavaFFI/src/objects.rs

+        let basename_str = String::from(jstr_basename);
+        let mut full_type_name: Vec<String> = jstr_list_to_rust_vec(env, &namespace)?;
+        full_type_name.push(basename_str);
+        let fulll_ns_str: String = full_type_name.join("::");


Suggested change

let fulll_ns_str: String = full_type_name.join("::");

let full_ns_str: String = full_type_name.join("::");

We may also want to validate that none of the strings in namespace contain ::.

andrewmwells-amazon · 2023-12-22T04:50:29Z

CedarJavaFFI/src/objects.rs

+        let basename_str = String::from(jstr_basename);
+        let mut full_type_name: Vec<String> = jstr_list_to_rust_vec(env, &namespace)?;
+        full_type_name.push(basename_str);
+        let fulll_ns_str: String = full_type_name.join("::");


Suggested change

let fulll_ns_str: String = full_type_name.join("::");

let full_ns_str: String = full_type_name.join("::");

Same comment about checking for ::'s

Swolebrain · 2023-12-27T17:01:07Z

CedarJava/src/test/java/com/cedarpolicy/EntityTypeNameTests.java

@@ -36,7 +36,8 @@ public class EntityTypeNameTests {
        "else",
        "in",
        "like",
-        "has"
+        "has",
+        "is"


luckily enough i ran into a test failure from this lol

While it is nice that it will "eventually" catch the issue, but I really don't like non-determinism in CI. Created issue here: #73

andrewmwells-amazon · 2024-01-09T19:01:54Z

CedarJava/src/main/java/com/cedarpolicy/model/slice/Policy.java

+     * and EntityIdentifier.parse), then this should only fail because the slots in the template don't match the instantiations
+     * (barring JNI failures).
+     * @param p Policy object constructed from a valid template. Best if built from Policy.parsePolicyTemplate
+     * @param principal EntityUid to put into the principal slot. Leave null if there's no principal slot


Sorry, I should have been more clear. I'd prefer that the principal and resource be Optional<EntityUID> instead of using null when there's no slot. I agree we should throw an exception on failure, not return an Optional<boolean>.

andrewmwells-amazon · 2024-01-09T19:08:19Z

CedarJava/src/test/java/com/cedarpolicy/EntityTypeNameTests.java

@@ -36,7 +36,8 @@ public class EntityTypeNameTests {
        "else",
        "in",
        "like",
-        "has"
+        "has",
+        "is"


While it is nice that it will "eventually" catch the issue, but I really don't like non-determinism in CI. Created issue here: #73

khieta

Not a Java expert, but the new functionality looks good to me.

khieta · 2024-01-09T19:24:12Z

CedarJavaFFI/Cargo.toml

-cedar-policy = { version = "3.0", path = "../cedar/cedar-policy" } # Need latest version from github
+cedar-policy = { version = "3.0", path = "../../cedar/cedar-policy" } # Need latest version from github


The README in CedarJavaFFI says that it should be ../cedar, so you should either update this toml file or the README.

khieta · 2024-01-09T20:19:18Z

CedarJavaFFI/src/objects.rs

+        let jstr_basename = env.get_string(&basename)?;
+        let basename_str = String::from(jstr_basename);
+        let mut full_type_name: Vec<String> = jstr_list_to_rust_vec(env, &namespace_components)?;
+        full_type_name.push(basename_str);
+        let has_namespace_component_with_colon = full_type_name.iter().any(|s| s.contains("::"));
+        if has_namespace_component_with_colon {
+            return Err("components of the type name cannot contain colons".into());
+        }
+        let full_ns_str: String = full_type_name.join("::");
+        let type_name: EntityTypeName = full_ns_str.parse()?;


This looks like the same code as L42-51. Might be nice to centralize this logic in case we end up changing handling in the future.

Co-authored-by: Victor Moreno <[email protected]>

Swolebrain force-pushed the main branch 3 times, most recently from d230441 to c6f2eec Compare December 15, 2023 21:05

aaronjeline self-requested a review December 15, 2023 22:42

lucajone reviewed Dec 20, 2023

View reviewed changes

CedarJava/src/main/java/com/cedarpolicy/model/schema/Schema.java Show resolved Hide resolved

lucajone reviewed Dec 20, 2023

View reviewed changes

CedarJavaFFI/src/interface.rs Show resolved Hide resolved

Swolebrain force-pushed the main branch from c6f2eec to feb6974 Compare December 20, 2023 20:19

andrewmwells-amazon reviewed Dec 22, 2023

View reviewed changes

Swolebrain force-pushed the main branch from feb6974 to 684b8f3 Compare December 27, 2023 17:00

Swolebrain commented Dec 27, 2023

View reviewed changes

feat: policy, template, and schema parsing

d18a33b

Swolebrain force-pushed the main branch from 684b8f3 to d18a33b Compare January 9, 2024 15:53

andrewmwells-amazon mentioned this pull request Jan 9, 2024

clippy fixes cedar-policy/cedar#576

Merged

3 tasks

andrewmwells-amazon approved these changes Jan 9, 2024

View reviewed changes

khieta approved these changes Jan 9, 2024

View reviewed changes

andrewmwells-amazon merged commit 5e2cb3e into cedar-policy:main Jan 9, 2024
1 check passed

ygrignon-sfdc pushed a commit to ygrignon-sfdc/cedar-java that referenced this pull request Jan 16, 2024

feat: policy, template, and schema parsing (cedar-policy#72)

3dd97b2

Co-authored-by: Victor Moreno <[email protected]>

ygrignon-sfdc pushed a commit to ygrignon-sfdc/cedar-java that referenced this pull request Jan 16, 2024

feat: policy, template, and schema parsing (cedar-policy#72)

3369589

Co-authored-by: Victor Moreno <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: policy, template, and schema parsing #72

feat: policy, template, and schema parsing #72

Swolebrain commented Dec 15, 2023 •

edited

Loading

lucajone Dec 20, 2023

Swolebrain Dec 20, 2023

lucajone Dec 20, 2023

Swolebrain Dec 20, 2023 •

edited

Loading

lucajone Dec 20, 2023

Swolebrain Dec 20, 2023

andrewmwells-amazon Dec 22, 2023

Swolebrain Dec 27, 2023

andrewmwells-amazon Jan 9, 2024

andrewmwells-amazon Dec 22, 2023

andrewmwells-amazon Dec 22, 2023

andrewmwells-amazon Dec 22, 2023

andrewmwells-amazon Dec 22, 2023

Swolebrain Dec 27, 2023

andrewmwells-amazon Jan 9, 2024

andrewmwells-amazon Jan 9, 2024

andrewmwells-amazon Jan 9, 2024

khieta left a comment

khieta Jan 9, 2024

khieta Jan 9, 2024

	let fulll_ns_str: String = full_type_name.join("::");
	let full_ns_str: String = full_type_name.join("::");

@@ @@ -36,7 +36,8 @@ public class EntityTypeNameTests { @@
                       "else",
                       "in",
                       "like",
-                      "has"
+                      "has",
+                      "is"

		cedar-policy = { version = "3.0", path = "../cedar/cedar-policy" } # Need latest version from github
		cedar-policy = { version = "3.0", path = "../../cedar/cedar-policy" } # Need latest version from github

feat: policy, template, and schema parsing #72

feat: policy, template, and schema parsing #72

Conversation

Swolebrain commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swolebrain Dec 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swolebrain commented Dec 15, 2023 •

edited

Loading

Swolebrain Dec 20, 2023 •

edited

Loading