Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FFI nits #837

Merged
merged 14 commits into from
Jun 4, 2024
Merged

FFI nits #837

merged 14 commits into from
Jun 4, 2024

Conversation

khieta
Copy link
Contributor

@khieta khieta commented May 3, 2024

Description of changes

A variety of small updates to the FFI to move us closer to completing #757. Downstream breaks expected.

  • In the validation FFI, changed policy_set to policies for consistency with the authorization FFI.
  • Standardized on using PolicyId for policy ids instead of SmolStr.
  • Re-exported JsonValueWithNoDuplicateKeys in the public FFI API.
  • Switched to using camelCase for ValidationMode and Decision. This will impact the current serialization of decisions & will likely lead to downstream breaks.
  • Updated ValidationSettings to match the proposal in Cedar FFI Overhaul #757. Also updated the logic so that the "enabled" flag is actually used for something.
  • Since this PR already makes a change that requires updating the integration tests (namely "Allow" → "allow"), I snuck in another breaking change to switch the integration tests to use camelCase.

Issue #, if available

#757

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

  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)

Note: Also require updates to cedar-java and cedar-integration-tests.

@khieta khieta added the breaking-change This is (likely) a breaking change label May 3, 2024
khieta added 2 commits May 3, 2024 13:17
Signed-off-by: Kesha Hietala <khieta@amazon.com>
Signed-off-by: Kesha Hietala <khieta@amazon.com>
Signed-off-by: Kesha Hietala <khieta@amazon.com>
cedar-policy/src/ffi/is_authorized.rs Outdated Show resolved Hide resolved
cedar-policy/src/ffi/is_authorized.rs Outdated Show resolved Hide resolved
#[derive(Default, Eq, PartialEq, Copy, Clone, Debug, Serialize, Deserialize)]
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[non_exhaustive]
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the experimental modes visible in the ffi api where they weren't before. That probably a good thing, but worth thinking about whether that could go wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't break anything since the WASM code directly uses the Rust FFI definitions. It should just silently add support for the new validation modes if the cedar_policy crate is compiled with the appropriate flags. What other complications were you thinking of?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they should be visible in the FFI.

@@ -444,18 +446,18 @@ pub enum PartialAuthorizationAnswer {
pub struct AuthorizationCall {
/// The principal taking action
#[cfg_attr(feature = "wasm", tsify(type = "string|{type: string, id: string}"))]
principal: Option<JsonValueWithNoDuplicateKeys>,
principal: Option<serde_json::Value>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I like this. We went through some effort previously to catch this duplicate key case, and I think that was the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that we want the FFI to use simple types where possible, and users can decide how to handle duplicates when they construct the serde_json::Value.

The other option is to move JsonValueWithNoDuplicateKeys from core to the public API -- would you prefer that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, we decided downstream users should handle duplicates as they see fit. So in principle I'm fine with moving JsonValueWithNoDuplicateKeys to the public API. I'm not sure this will help anyone using the FFI though because they'll still likely use an equivalent of serde_json::Valueon the other side of the interface.

@cdisselkoen cdisselkoen mentioned this pull request May 9, 2024
3 tasks
khieta and others added 2 commits May 9, 2024 09:42
Co-authored-by: Craig Disselkoen <cdiss@amazon.com>
@khieta
Copy link
Contributor Author

khieta commented Jun 3, 2024

Returning to working on this PR! 😄

The latest commit re-exports the JsonValueWithNoDuplicateKeys type, as discussed in comment threads and in-person. Remaining outstanding question on my end is: Are we ok with the switch from PolicyIdSmolStr, or would we prefer to standardize on PolicyId?

@cdisselkoen
Copy link
Contributor

Slightly prefer PolicyId, but happy with either one. Either way, I expect we'll keep tsify(type = "string"), so we're not really getting additional semver flexibility (the PolicyId type isn't really opaque at the FFI layer). So using the PolicyId name would just be for documentation purposes, as I understand it, and because serde would do the right thing automatically, making it slightly more convenient to implement the FFI layer.

@khieta
Copy link
Contributor Author

khieta commented Jun 3, 2024

Latest version standardizes on using PolicyId instead of SmolStr. The downstream fix for cedar-spec is in cedar-spec#346 -- I'll work on the updates needed for cedar-integration-tests and cedar-java tomorrow.

@khieta khieta merged commit ed6f000 into main Jun 4, 2024
10 of 16 checks passed
@khieta khieta deleted the khieta/ffi-nits branch June 4, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This is (likely) a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants