Optionally validate requests (#393)

Co-authored-by: Andrew Wells <130512013+andrewmwells-amazon@users.noreply.github.com>

cedar-integration-tests/sample-data/sandbox_a/schema.cedarschema.json

@@ -76,10 +76,10 @@
       "view": {
         "appliesTo": {
           "resourceTypes": [
-            "Photo"
+            "Photo", "Video"
           ],
           "principalTypes": [
-            "User"
+            "User", "Administrator"
           ],
           "context": {
             "type": "Record",

cedar-integration-tests/tests/multi/5.json

@@ -41,6 +41,7 @@
       "context": {
         "authenticated": true
       },
+      "enable_request_validation": false,
       "decision": "Deny",
       "reasons": [],
       "errors": [
@@ -83,6 +84,7 @@
       "context": {
         "authenticated": false
       },
+      "enable_request_validation": false,
       "decision": "Deny",
       "reasons": [
         "policy1"
@@ -128,11 +130,12 @@
       "context": {
         "authenticated": true
       },
+      "enable_request_validation": false,
       "decision": "Allow",
       "reasons": [
         "policy2"
       ],
       "errors": []
     }
   ]
-}
+}

cedar-policy-cli/CHANGELOG.md

@@ -4,6 +4,8 @@
 
 ### Added
 
+- Requests are now validated by default if a schema is provided. This can be
+  disabled with `--request-validation=false`.
 - The `-s` short form can now be used for `--schema` across all subcommands.
 
 ### Changed

cedar-policy-cli/src/lib.rs

@@ -21,7 +21,7 @@
 
 mod err;
 
-use clap::{Args, Parser, Subcommand, ValueEnum};
+use clap::{ArgAction, Args, Parser, Subcommand, ValueEnum};
 use miette::{miette, IntoDiagnostic, NamedSource, Report, Result, WrapErr};
 use serde::{Deserialize, Serialize};
 use std::{
@@ -139,10 +139,19 @@ pub struct RequestArgs {
     /// --principal, --action, etc.
     #[arg(long = "request-json", value_name = "FILE", conflicts_with_all = &["principal", "action", "resource", "context_json_file"])]
     pub request_json_file: Option<String>,
+    /// Whether to enable request validation. This has no effect if a schema is
+    /// not provided.
+    #[arg(long = "request-validation", action = ArgAction::Set, default_value_t = true)]
+    pub request_validation: bool,
 }
 
 impl RequestArgs {
     /// Turn this `RequestArgs` into the appropriate `Request` object
+    ///
+    /// `schema` will be used for schema-based parsing of the context, and also
+    /// (if `self.request_validation` is `true`) for request validation.
+    ///
+    /// `self.request_validation` has no effect if `schema` is `None`.
     fn get_request(&self, schema: Option<&Schema>) -> Result<Request> {
         match &self.request_json_file {
             Some(jsonfile) => {
@@ -182,7 +191,18 @@ impl RequestArgs {
                 )
                 .into_diagnostic()
                 .wrap_err_with(|| format!("failed to create a context from {jsonfile}"))?;
-                Ok(Request::new(principal, action, resource, context))
+                Request::new(
+                    principal,
+                    action,
+                    resource,
+                    context,
+                    if self.request_validation {
+                        schema
+                    } else {
+                        None
+                    },
+                )
+                .map_err(|e| miette!("{e}"))
             }
             None => {
                 let principal = self
@@ -224,7 +244,18 @@ impl RequestArgs {
                         })?,
                     },
                 };
-                Ok(Request::new(principal, action, resource, context))
+                Request::new(
+                    principal,
+                    action,
+                    resource,
+                    context,
+                    if self.request_validation {
+                        schema
+                    } else {
+                        None
+                    },
+                )
+                .map_err(|e| miette!("{e}"))
             }
         }
     }

cedar-policy-cli/tests/integration_tests/main.rs

@@ -71,6 +71,9 @@ struct JsonRequest {
     resource: Option<serde_json::Value>,
     /// Context for the request
     context: serde_json::Value,
+    /// Whether to enable request validation for this request
+    #[serde(default = "constant_true")]
+    enable_request_validation: bool,
     /// Expected decision for the request
     decision: Decision,
     /// Expected "reasons" for the request
@@ -79,6 +82,10 @@ struct JsonRequest {
     errors: Vec<String>,
 }
 
+fn constant_true() -> bool {
+    true
+}
+
 fn value_to_euid_string(v: serde_json::Value) -> Result<String, impl std::error::Error> {
     EntityUid::from_json(v).map(|euid| euid.to_string())
 }
@@ -186,6 +193,9 @@ fn perform_integration_test_from_json(jsonfile: impl AsRef<Path>) {
             entity_args.push("--action".to_string());
             entity_args.push(value_to_euid_string(s).unwrap());
         }
+        if !json_request.enable_request_validation {
+            entity_args.push("--request-validation=false".to_string());
+        }
 
         let authorize_cmd = assert_cmd::Command::cargo_bin("cedar")
             .expect("bin exists")
@@ -212,15 +222,33 @@ fn perform_integration_test_from_json(jsonfile: impl AsRef<Path>) {
             .expect("output should be valid UTF-8");
 
         for error in &json_request.errors {
-            assert!(output.contains(error));
+            assert!(
+                output.contains(error),
+                "test {} failed for request \"{}\": output does not contain expected error {error:?}.\noutput was: {output}\nstderr was: {}",
+                jsonfile.display(),
+                &json_request.desc,
+                String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"),
+            );
         }
 
         if json_request.reasons.is_empty() {
-            assert!(output.contains("no policies applied to this request"));
+            assert!(
+                output.contains("no policies applied to this request"),
+                "test {} failed for request \"{}\": output does not contain the string \"no policies applied to this request\", as expected.\noutput was: {output}\nstderr was: {}",
+                jsonfile.display(),
+                &json_request.desc,
+                String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"),
+            );
         } else {
             assert!(output.contains("this decision was due to the following policies"));
             for reason in &json_request.reasons {
-                assert!(output.contains(&reason.escape_debug().to_string()));
+                assert!(
+                    output.contains(&reason.escape_debug().to_string()),
+                    "test {} failed for request \"{}\": output does not contain the reason string {reason:?}.\noutput was: {output}\nstderr was: {}",
+                    jsonfile.display(),
+                    &json_request.desc,
+                    String::from_utf8(authorize_cmd.get_output().stderr.clone()).expect("stderr should be valid UTF-8"),
+                );
             }
         };
     }

cedar-policy-cli/tests/sample.rs

@@ -71,6 +71,7 @@ fn run_authorize_test_with_linked_policies(
             resource: Some(resource.into()),
             context_json_file: None,
             request_json_file: None,
+            request_validation: true,
         },
         policies_file: policies_file.into(),
         template_linked_file: links_file.map(|x| x.to_string()),
@@ -134,6 +135,7 @@ fn run_authorize_test_context(
             resource: Some(resource.into()),
             context_json_file: Some(context_file.into()),
             request_json_file: None,
+            request_validation: true,
         },
         policies_file: policies_file.into(),
         template_linked_file: None,
@@ -159,6 +161,7 @@ fn run_authorize_test_json(
             resource: None,
             context_json_file: None,
             request_json_file: Some(request_json.into()),
+            request_validation: true,
         },
         policies_file: policies_file.into(),
         template_linked_file: None,
@@ -541,6 +544,7 @@ fn run_evaluate_test(
             resource: None,
             context_json_file: None,
             request_json_file: Some(request_json_file.into()),
+            request_validation: true,
         },
         expression: expression.to_owned(),
     };

cedar-policy-core/src/ast/request.rs

@@ -79,34 +79,53 @@ impl EntityUIDEntry {
 }
 
 impl Request {
-    /// Default constructor
-    pub fn new(
+    /// Default constructor.
+    ///
+    /// If `schema` is provided, this constructor validates that this `Request`
+    /// complies with the given `schema`.
+    pub fn new<S: RequestSchema>(
         principal: EntityUID,
         action: EntityUID,
         resource: EntityUID,
         context: Context,
-    ) -> Self {
-        Self {
+        schema: Option<&S>,
+        extensions: Extensions<'_>,
+    ) -> Result<Self, S::Error> {
+        let req = Self {
             principal: EntityUIDEntry::concrete(principal),
             action: EntityUIDEntry::concrete(action),
             resource: EntityUIDEntry::concrete(resource),
             context: Some(context),
+        };
+        if let Some(schema) = schema {
+            schema.validate_request(&req, extensions)?;
         }
+        Ok(req)
     }
 
-    /// Create a new request with potentially unknown (for partial eval) head variables
-    pub fn new_with_unknowns(
+    /// Create a new `Request` with potentially unknown (for partial eval) variables.
+    ///
+    /// If `schema` is provided, this constructor validates that this `Request`
+    /// complies with the given `schema` (at least to the extent that we can
+    /// validate with the given information)
+    pub fn new_with_unknowns<S: RequestSchema>(
         principal: EntityUIDEntry,
         action: EntityUIDEntry,
         resource: EntityUIDEntry,
         context: Option<Context>,
-    ) -> Self {
-        Self {
+        schema: Option<&S>,
+        extensions: Extensions<'_>,
+    ) -> Result<Self, S::Error> {
+        let req = Self {
             principal,
             action,
             resource,
             context,
+        };
+        if let Some(schema) = schema {
+            schema.validate_request(&req, extensions)?;
         }
+        Ok(req)
     }
 
     /// Get the principal associated with the request
@@ -152,7 +171,7 @@ impl std::fmt::Display for Request {
 }
 
 /// `Context` field of a `Request`
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub struct Context {
     /// an `Expr::Record` that qualifies as a "restricted expression"
     /// INVARIANT: This must be of the variant `Record`
@@ -262,6 +281,32 @@ impl std::fmt::Display for Context {
     }
 }
 
+/// Trait for schemas capable of validating `Request`s
+pub trait RequestSchema {
+    /// Error type returned when a request fails validation
+    type Error: std::error::Error;
+    /// Validate the given `request`, returning `Err` if it fails validation
+    fn validate_request(
+        &self,
+        request: &Request,
+        extensions: Extensions<'_>,
+    ) -> Result<(), Self::Error>;
+}
+
+/// A `RequestSchema` that does no validation and always reports a passing result
+#[derive(Debug, Clone)]
+pub struct RequestSchemaAllPass;
+impl RequestSchema for RequestSchemaAllPass {
+    type Error = std::convert::Infallible;
+    fn validate_request(
+        &self,
+        _request: &Request,
+        _extensions: Extensions<'_>,
+    ) -> Result<(), Self::Error> {
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 mod test {