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

Make non-json isAuthorized and validate public, rework wasm interfaces #737

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

ncsze
Copy link
Contributor

@ncsze ncsze commented Mar 19, 2024

Description of changes

  • Made isAuthorized and validate functions in the frontend public, as well as their related structs: AuthorizationAnswer, AuthorizationCall, ValidateCall, ValidationSettings, ValidationMode, ValidateAnswer, ValidationNote
    • Added documentation to the structs and functions made public
  • Changed wasm_is_authorized and wasm_validate to use non-json versions of isAuthorized and validate, and to return the data in AuthorizationAnswer and ValidateAnswer directly (instead of in a JSON string) in a wasm-specific result struct.
  • Changed all wasm result interfaces to use untagged enums and to always contain errors: Vec<String> in their error case. In TS, users can now check for success with duck typing instead of the misleading success string.

Issue #, if available

N/A

Checklist for requesting a review

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

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new 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 formal model or DRT infrastructure.

@ncsze ncsze marked this pull request as ready for review March 19, 2024 13:18
@ncsze
Copy link
Contributor Author

ncsze commented Mar 19, 2024

Result types in TS now look like this:

export type JsonToPolicyResult = { policyText: string } | { errors: string[] };

export type PolicyToJsonResult = { policy: Policy } | { errors: string[] };

export type CheckParsePolicySetResult = { policies: number; templates: number } | { errors: string[] };

export type CheckParseTemplateResult = { slots: string[] } | { errors: string[] };

export type AuthorizationResult = { response: InterfaceResponse } | { errors: string[] };

export type CheckParseResult = null | { errors: string[] };

export type FormattingResult = { formattedPolicy: string } | { errors: string[] };

export type ValidateResult = { notes: ValidationNote[] } | { errors: string[] };

@andrewmwells-amazon
Copy link
Contributor

High-level comment: I think AuthorizationAnswer, AuthorizationCall, ValidateCall, ValidationMode, ValidateAnswer, ValidationNote are probably ok.

ValidationSettings, feels likely to change in the future.

@khieta khieta mentioned this pull request Mar 20, 2024
3 tasks
Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

While reading through this PR, I noticed a few inconsistencies that I'd like to fix before we make these data structures public. My first pass at the fixes is in #740. Can you wait to merge this PR until that one is reviewed & merged?

But, overall, I think it's fine to expose these types (once they're cleaned up a little). It would also be good to provide public constructors for types whose internal fields are not exposed (e.g., AuthorizationCall and ValidateCall). But we should make sure that the public constructors only use public types (e.g., serde_json::Value rather than JsonValueWithNoDuplicateKeys).

@khieta
Copy link
Contributor

khieta commented Mar 29, 2024

We spent some time as a group thinking about how we want to improve the JSON FFI. The result of our discussion is in #757. @cdisselkoen has already started working on this in #760 (although there's no specific timeline on the rest of the changes).

So: you may want to merge this PR sooner rather than later to avoid even more merge conflicts 🙂

cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Nick Szegheo <nicholas.szegheo@gmail.com>
@khieta khieta merged commit ea00969 into cedar-policy:main Apr 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants