Skip to content

Commit

Permalink
Deny unknown fields in FFI. (#815) (#1041)
Browse files Browse the repository at this point in the history
Signed-off-by: Andrew Wells <anmwells@amazon.com>
Co-authored-by: Kesha Hietala <khieta@amazon.com>
  • Loading branch information
andrewmwells-amazon and khieta authored Jul 9, 2024
1 parent 8d0605a commit 30eb00f
Show file tree
Hide file tree
Showing 6 changed files with 169 additions and 14 deletions.
5 changes: 3 additions & 2 deletions cedar-policy/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ Cedar Language Version: 4.0

### Changed

- The API around `Request::new` has changed to remove the `Option`s
around the entity type arguments.
- The API around `Request::new` has changed to remove the `Option`s
around the entity type arguments. See [RFC 55](https://github.com/cedar-policy/rfcs/blob/main/text/0055-remove-unspecified.md).
- Significantly reworked all public-facing error types to address some issues
and improve consistency. See issue #745.
- Finalized the `ffi` module which was preview-released in 3.2.0.
Expand All @@ -35,6 +35,7 @@ Cedar Language Version: 4.0
- Changed JSON schema parser so that `Set`, `Entity`, `Record`, and `Extension`
can be common type names; updated the error message when common type names
conflict with built-in primitive type names (#974, partially resolving #973)
- Changed the FFI to error on typos or unexpected fields in the input JSON.

### Removed

Expand Down
25 changes: 13 additions & 12 deletions cedar-policy/src/ffi/is_authorized.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ pub fn is_authorized_partial_json_str(json: &str) -> Result<String, serde_json::
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct Response {
/// Authorization decision
decision: Decision,
Expand All @@ -169,6 +170,7 @@ pub struct Response {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct Diagnostics {
/// Ids of the policies that contributed to the decision.
/// If no policies applied to the request, this set will be empty.
Expand Down Expand Up @@ -239,6 +241,7 @@ impl Diagnostics {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct AuthorizationError {
/// Id of the policy where the error (or warning) occurred
#[cfg_attr(feature = "wasm", tsify(type = "string"))]
Expand Down Expand Up @@ -291,6 +294,7 @@ impl From<cedar_policy_core::authorizer::AuthorizationError> for AuthorizationEr
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct ResidualResponse {
decision: Option<Decision>,
satisfied: HashSet<PolicyId>,
Expand Down Expand Up @@ -458,6 +462,7 @@ pub enum PartialAuthorizationAnswer {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct AuthorizationCall {
/// The principal taking action
principal: EntityUid,
Expand Down Expand Up @@ -492,6 +497,7 @@ pub struct AuthorizationCall {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct PartialAuthorizationCall {
/// The principal taking action. If this field is empty, then the principal is unknown.
principal: Option<EntityUid>,
Expand Down Expand Up @@ -594,7 +600,7 @@ impl AuthorizationCall {
},
_ => {
// At least one of the `errs.push(e)` statements above must have been reached
return build_error(errs, warnings);
build_error(errs, warnings)
}
}
}
Expand Down Expand Up @@ -680,7 +686,7 @@ impl PartialAuthorizationCall {
},
_ => {
// At least one of the `errs.push(e)` statements above must have been reached
return build_error(errs, warnings);
build_error(errs, warnings)
}
}
}
Expand Down Expand Up @@ -1642,8 +1648,7 @@ mod partial_test {
"ID1": "permit(principal == User::\"alice\", action, resource);"
}
},
"entities": [],
"partial_evaluation": true
"entities": []
});

assert_is_authorized_json_partial(call);
Expand All @@ -1666,8 +1671,7 @@ mod partial_test {
"ID1": "permit(principal == User::\"alice\", action, resource);"
}
},
"entities": [],
"partial_evaluation": true
"entities": []
});

assert_is_not_authorized_json_partial(call);
Expand All @@ -1690,8 +1694,7 @@ mod partial_test {
"ID1": "permit(principal == User::\"alice\", action, resource);"
}
},
"entities": [],
"partial_evaluation": true
"entities": []
});

assert_is_residual(call, &HashSet::from(["ID1"]));
Expand All @@ -1714,8 +1717,7 @@ mod partial_test {
"ID1": "permit(principal, action, resource) when { principal == User::\"alice\" };"
}
},
"entities": [],
"partial_evaluation": true
"entities": []
});

assert_is_residual(call, &HashSet::from(["ID1"]));
Expand All @@ -1739,8 +1741,7 @@ mod partial_test {
"ID2": "forbid(principal, action, resource) unless { resource == Photo::\"door\" };"
}
},
"entities": [],
"partial_evaluation": true
"entities": []
});

assert_is_residual(call, &HashSet::from(["ID1"]));
Expand Down
1 change: 1 addition & 0 deletions cedar-policy/src/ffi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ mod utils;
pub use utils::*;
mod validate;
pub use validate::*;
mod tests;
144 changes: 144 additions & 0 deletions cedar-policy/src/ffi/tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
#[cfg(test)]
mod ffi_tests {
#[cfg(feature = "partial-eval")]
use crate::ffi::is_authorized_partial_json;
use crate::ffi::{is_authorized_json, validate_json};
use cool_asserts::assert_matches;

#[test]
fn test_fail_unknown_field_policy_slice() {
let json = serde_json::json!({
"principal": {
"type": "User",
"id": "alice"
},
"action": {
"type": "Photo",
"id": "view"
},
"resource": {
"type": "Photo",
"id": "door"
},
"context": {},
"slice": {
"policies": {},
"templatePolicies": {},
"entities": []
}
});

assert_matches!(is_authorized_json(json), Err(e) => {
assert_eq!(e.to_string(), "unknown field `slice`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
});
}

#[test]
fn test_fail_unknown_field_enable_request_validation() {
let json = serde_json::json!({
"principal": {
"type": "User",
"id": "alice"
},
"action": {
"type": "Photo",
"id": "view"
},
"resource": {
"type": "Photo",
"id": "door"
},
"context": {},
"policies": {},
"entities": [],
"enableRequestValidation": true,
});

assert_matches!(is_authorized_json(json), Err(e) => {
assert_eq!(e.to_string(), "unknown field `enableRequestValidation`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
});
}

#[test]
fn test_fail_unknown_field_policies() {
let json = serde_json::json!({
"principal": {
"type": "User",
"id": "alice"
},
"action": {
"type": "Photo",
"id": "view"
},
"resource": {
"type": "Photo",
"id": "door"
},
"context": {},
"policies": {
"policies": {}
},
"entities": []
});

assert_matches!(is_authorized_json(json), Err(e) => {
assert_eq!(e.to_string(), "unknown field `policies`, expected one of `staticPolicies`, `templates`, `templateLinks`");
});
}

#[cfg(feature = "partial-eval")]
#[test]
fn test_fail_unknown_field_partial_evaluation() {
let json = serde_json::json!({
"principal": {
"type": "User",
"id": "alice"
},
"action": {
"type": "Photo",
"id": "view"
},
"context": {},
"policies": {
"staticPolicies": {
"ID1": "permit(principal == User::\"alice\", action, resource);"
}
},
"entities": [],
"partial_evaluation": true
});

assert_matches!(is_authorized_partial_json(json), Err(e) => {
assert_eq!(e.to_string(), "unknown field `partial_evaluation`, expected one of `principal`, `action`, `resource`, `context`, `schema`, `validateRequest`, `policies`, `entities`");
});
}

#[test]
fn test_fail_unknown_field_validation() {
let json = serde_json::json!({
"schema": { "json": { "": {
"entityTypes": {
"User": {
"memberOfTypes": [ ]
},
"Photo": {
"memberOfTypes": [ ]
}
},
"actions": {
"viewPhoto": {
"appliesTo": {
"resourceTypes": [ "Photo" ],
"principalTypes": [ "User" ]
}
}
}
}}},
"Policies": "forbid(principal, action, resource);permit(principal == Photo::\"photo.jpg\", action == Action::\"viewPhoto\", resource == User::\"alice\");"
});

assert_matches!(validate_json(json), Err(e) => {
assert_eq!(e.to_string(), "unknown field `Policies`, expected one of `validationSettings`, `schema`, `policies`");
});
}
}
5 changes: 5 additions & 0 deletions cedar-policy/src/ffi/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ extern crate tsify;
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct DetailedError {
/// Main error message, including both the `miette` "message" and the
/// `miette` "causes" (uses `miette`'s default `Display` output)
Expand Down Expand Up @@ -84,6 +85,7 @@ impl From<miette::Severity> for Severity {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct SourceLabel {
/// Text of the label (if any)
pub label: Option<String>,
Expand All @@ -97,6 +99,7 @@ pub struct SourceLabel {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct SourceLocation {
/// Start of the source location (in bytes)
pub start: usize,
Expand Down Expand Up @@ -408,6 +411,7 @@ impl Default for StaticPolicySet {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct TemplateLink {
/// Id of the template to link against
template_id: PolicyId,
Expand Down Expand Up @@ -441,6 +445,7 @@ impl TemplateLink {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct PolicySet {
/// static policies
#[serde(default)]
Expand Down
3 changes: 3 additions & 0 deletions cedar-policy/src/ffi/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ pub fn validate_json_str(json: &str) -> Result<String, serde_json::Error> {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct ValidationCall {
/// Validation settings
#[serde(default)]
Expand Down Expand Up @@ -154,6 +155,7 @@ impl ValidationCall {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct ValidationSettings {
/// Whether validation is enabled. If this flag is set to `false`, then
/// only parsing is performed. The default value is `true`.
Expand All @@ -176,6 +178,7 @@ impl Default for ValidationSettings {
#[cfg_attr(feature = "wasm", derive(tsify::Tsify))]
#[cfg_attr(feature = "wasm", tsify(into_wasm_abi, from_wasm_abi))]
#[serde(rename_all = "camelCase")]
#[serde(deny_unknown_fields)]
pub struct ValidationError {
/// Id of the policy where the error (or warning) occurred
#[cfg_attr(feature = "wasm", tsify(type = "string"))]
Expand Down

0 comments on commit 30eb00f

Please sign in to comment.