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

Cedar FFI Overhaul #757

Closed
khieta opened this issue Mar 28, 2024 · 8 comments · Fixed by #1014
Closed

Cedar FFI Overhaul #757

khieta opened this issue Mar 28, 2024 · 8 comments · Fixed by #1014
Assignees
Labels
4.0 backlog We hope to work on this in the future breaking-change This is (likely) a breaking change feature-request Request for a new feature papercut

Comments

@khieta
Copy link
Contributor

khieta commented Mar 28, 2024

Category

User level API features/changes

Describe the feature you'd like to request

The code in cedar-policy/src/ffi/ acts as our public interface for JSON-based FFI. It is currently used by the Java and WASM bindings. We plan to use it for the differential testing harness too, in the future (see #75, #76, #125). But there are several rough edges in our frontend interface as it is now. This issue proposes to overhaul our FFI interface. This will result in breaking changes that will need to be delayed to the next major release (as of writing, 4.0 or greater).

Proposed API

Entry point for is_authorized

is_authorized : AuthorizationCall -> AuthorizationAnswer
is_authorized_json : serde_json::Value -> Result<serde_json::Value, serde_json::Error>
is_authorized_json_str : &str -> Result<String, serde_json::Error>

AuthorizationCall type

struct AuthorizationCall {
    principal:EntityUid,
    action: EntityUid,
    resource:EntityUid,
    context: Context,
    schema: Option<Schema>,
    validate_request: bool,
    entities: Entities,
    policies: PolicySet
}

struct EntityUid(JsonValueWithNoDuplicateKeys); // see <https://docs.cedarpolicy.com/policies/json-format.html>

struct Context(JsonValueWithNoDuplicateKeys); // see <https://docs.cedarpolicy.com/auth/entities-syntax.html>

enum Schema {
    Human(String),
    Json(JsonValueWithNoDuplicateKeys), // see <https://docs.cedarpolicy.com/schema/json-schema.html>
}

struct Entities(JsonValueWithNoDuplicateKeys); // see <https://docs.cedarpolicy.com/auth/entities-syntax.html>

struct PolicySet{
    static_policies: StaticPolicySet,
    templates: HashMap<PolicyId, Template>,
    template_links: HashSet<TemplateLink>,
}

enum StaticPolicySet {
    Concatenated(String),
    Set(HashSet<Policy>),
    Map(HashMap<PolicyId, Policy>),
}

enum Policy {
    Human(String),
    Json(serde_json::Value), // see <https://docs.cedarpolicy.com/policies/json-format.html>
}

enum Template {
    Human(String),
    Json(serde_json::Value), // see <https://docs.cedarpolicy.com/policies/json-format.html>
}

struct TemplateLink {
    template_id: SmolStr,
    new_id: SmolStr,
    values: HashMap<SlotId, EntityUid>,
}

AuthorizationAnswer type

enum AuthorizationAnswer {
    Failure { 
        errors: Vec<DetailedError>, // `DetailedError` is a custom JSON format to describe miette errors
        warnings: Vec<DetailedError>
    },
    Success { 
        response: Response,
        warnings: Vec<DetailedError>
    },
}

pub struct Response {
    decision: Decision,
    diagnostics: Diagnostics,
}

pub struct Diagnostics {
    reason: HashSet<PolicyId>,
    errors: HashSet<AuthorizationError>,
}

struct AuthorizationError {
    policy_id: PolicyId,
    error: DetailedError,
}

Entry point for validate

validate : ValidationCall -> ValidationAnswer
validate_json : serde_json::Value -> Result<serde_json::Value, serde_json::Error>
validate_json_str : &str -> Result<String, serde_json::Error>

ValidationCall type

struct ValidationCall {
    validation_settings: ValidationSettings,
    schema: Schema,
    policies: PolicySet,
}

struct ValidationSettings {
    enabled: bool,
    mode: ValidationMode,
}

ValidationAnswer type

enum ValidationAnswer {
    Failure { 
        errors: Vec<DetailedError>,
        warnings: Vec<DetailedError>
    },
    Success {
        validation_errors: HashSet<ValidationError>,
        validation_warnings: HashSet<ValidationError>,
        warnings: Vec<DetailedError>
    },
}

struct ValidationError {
    policy_id: PolicyId,
    error: DetailedError // content of `DetailedError` distinguishes "warnings" from "errors"
}
@khieta khieta added backlog We hope to work on this in the future feature-request Request for a new feature breaking-change This is (likely) a breaking change papercut labels Mar 28, 2024
@cdisselkoen
Copy link
Contributor

This proposes is_authorized_json() returns serde_json::Value encoding an AuthorizationAnswer. This implies that we return AuthorizationAnswer::Failure in all failure cases? The current FFI returns InterfaceResult, which distinguishes between InterfaceResult::Failure (any error that occurs prior to even calling the main Rust is_authorized()) and InterfaceResult::Success(AuthorizationAnswer::Failure) (returned when the main Rust is_authorized() returns an error). Just checking that this change is intentional, that we want to collapse these two error cases?

@cdisselkoen
Copy link
Contributor

Relatedly, what should we do if there is an error converting the input serde_json::Value to AuthorizationCall, or the output AuthorizationAnswer to serde_json::Value? It may not be possible to return serde_json::Value without a Result, with at least serde_json::Error in the error type, even if all other errors are reported in AuthorizationAnswer::Failure

@cdisselkoen
Copy link
Contributor

I propose that we return Result<serde_json::Value, serde_json::Error> and Result<String, serde_json::Error> in is_authorized_json() and is_authorized_json_str() respectively, and that we return Err iff either deserialization to AuthorizationCall or serialization from AuthorizationAnswer fail.

@cdisselkoen
Copy link
Contributor

cdisselkoen commented Mar 28, 2024

Another question: Do we want to rename the frontend module? Is there a better, more descriptive name? This proposal has an enum Schema; if we take that as-is, users will see cedar_policy::Schema (the real Schema type) as well as cedar_policy::frontend::is_authorized::Schema (this new type). Maybe frontend is the best name, maybe there's some better name?

Also, do we want to rethink the current structure where users see three public submodules is_authorized, utils, and validate, with some cross-references among them? Personally, for the latter question, I would prefer everything pub used in frontend so that end-users see a flat structure, with everything just declared in frontend (or whatever module name we want).

@khieta
Copy link
Contributor Author

khieta commented Mar 29, 2024

Thanks for the suggestions @cdisselkoen!

  • I agree that it makes sense to return a Result type for the *_json and *_json_str variants. Will update.
  • What do you think about ffi instead of frontend? Or json_ffi?
  • I also prefer a flat structure. Will have to keep an eye on this when reviewing the eventual PR.

@cdisselkoen
Copy link
Contributor

Implemented several of the suggestions in this issue in #760; see notes there.

As I work on implementing more of the suggestions in this issue, found another problem: serde_json::Value is not Hash, so we can't have HashSet<serde_json::Value>. I propose Vec instead.

@cdisselkoen
Copy link
Contributor

As requested in #793, keeping this issue up to date with changes proposed/accepted in #800:

@khieta
Copy link
Contributor Author

khieta commented Jun 20, 2024

Update: the changes proposed in this issue are implemented in #1014. The issue comment has been updated to match the implementation in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 backlog We hope to work on this in the future breaking-change This is (likely) a breaking change feature-request Request for a new feature papercut
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants