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.

Sign up for GitHub

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

Improve error messages for request validation errors #1357

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Improve error messages for request validation errors #1357

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fa40504
improve some request-validation error messages
cdisselkoen Dec 5, 2024
2e4b897
improve help text
cdisselkoen Dec 5, 2024
5294a33
don't print large contexts in error message
cdisselkoen Dec 10, 2024
519e207
bounded display for Value
cdisselkoen Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 39 additions & 27 deletions cedar-policy-core/src/ast/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use crate::ast::*;
use crate::parser::Loc;
use std::collections::{BTreeMap, BTreeSet, HashSet};
use std::str::FromStr;
use std::sync::Arc;

use itertools::Itertools;
Expand Down Expand Up @@ -484,38 +485,49 @@ impl std::fmt::Display for ValueKind {
fast,
authoritative,
}) => {
match authoritative.len() {
0 => write!(f, "[]"),
n @ 1..=5 => {
write!(f, "[")?;
if let Some(rc) = fast {
// sort the elements, because we want the Display output to be
// deterministic, particularly for tests which check equality
// of error messages
for (i, item) in rc.as_ref().iter().sorted_unstable().enumerate() {
write!(f, "{item}")?;
if i < n - 1 {
write!(f, ", ")?;
}
}
} else {
// don't need to sort the elements in this case because BTreeSet iterates
// in a deterministic order already
for (i, item) in authoritative.as_ref().iter().enumerate() {
write!(f, "{item}")?;
if i < n - 1 {
write!(f, ", ")?;
}
}
write!(f, "[")?;
if let Some(rc) = fast {
// sort the elements, because we want the Display output to be
// deterministic, particularly for tests which check equality
// of error messages
for (i, item) in rc.as_ref().iter().sorted_unstable().enumerate() {
write!(f, "{item}")?;
if i < authoritative.len() - 1 {
write!(f, ", ")?;
}
}
} else {
// don't need to sort the elements in this case because BTreeSet iterates
// in a deterministic order already
for (i, item) in authoritative.as_ref().iter().enumerate() {
write!(f, "{item}")?;
if i < authoritative.len() - 1 {
write!(f, ", ")?;
}
write!(f, "]")?;
Ok(())
}
n => write!(f, "<set with {} elements>", n),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we keep this case for the same reason we applied #887? Similarly should consider adding a case like this for the record branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A goal for the policy pretty-printer is to produce valid, parseable syntax. Is a similar goal reasonable here, that the value pretty-printer should produce syntax that is parseable as a Cedar expression?

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fair point.

But, for request validation errors in particular, I don't think it's a reasonable goal. It isn't necessary to have InvalidContextError send back the entire context when the context is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the request-validation error message to truncate after 5 key-value pairs in the Context record. Left the Display impl for Value as printing valid Cedar syntax. Is that a good compromise between the competing goals here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it doesn't guard against large sets/records located recursively inside the Context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to new bounded_display() method on Value and ValueKind, which shares code with their Display impls. Addresses bounding the display of large sets and records, including recursively; doesn't address large literals or extension values (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems good enough for now. To me it feels more likely that the context record will be very large in the normal operation of an application than very large individual strings. If this turns about to be wrong we can truncate these as well.

}
write!(f, "]")?;
Ok(())
}
Self::Record(record) => {
write!(f, "<first-class record with {} fields>", record.len())
write!(f, "{{")?;
for (i, (k, v)) in record.as_ref().iter().enumerate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This looks nicer in the general case, but it's shown as `{}` in the case with 0 fields. In that case, I'm not sure the help message is so useful (might take a while to realize it's an empty context).

match UnreservedId::from_str(k) {
Ok(k) => {
// we can omit the quotes around the key, it's a valid identifier and not a reserved keyword
write!(f, "{k}: {v}")?;
}
Err(_) => {
// put quotes around the key
write!(f, "\"{k}\": {v}")?;
}
}
if i < record.len() - 1 {
write!(f, ", ")?;
}
}
write!(f, "}}")?;
Ok(())
}
Self::ExtensionValue(ev) => write!(f, "{}", RestrictedExpr::from(ev.as_ref().clone())),
}
Expand Down
85 changes: 79 additions & 6 deletions cedar-policy-validator/src/coreschema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ impl ast::RequestSchema for ValidatorSchema {
return Err(request_validation_errors::InvalidPrincipalTypeError {
principal_ty: principal.entity_type().clone(),
action: Arc::clone(action),
valid_principal_tys: validator_action_id
.applies_to_principals()
.cloned()
.collect(),
}
.into());
}
Expand All @@ -213,6 +217,10 @@ impl ast::RequestSchema for ValidatorSchema {
return Err(request_validation_errors::InvalidResourceTypeError {
resource_ty: resource.entity_type().clone(),
action: Arc::clone(action),
valid_resource_tys: validator_action_id
.applies_to_resources()
.cloned()
.collect(),
}
.into());
}
Expand Down Expand Up @@ -296,6 +304,7 @@ pub enum RequestValidationError {
/// Errors related to validation
pub mod request_validation_errors {
use cedar_policy_core::ast;
use itertools::Itertools;
use miette::Diagnostic;
use std::sync::Arc;
use thiserror::Error;
Expand Down Expand Up @@ -349,11 +358,32 @@ pub mod request_validation_errors {
/// not valid for the request action
#[derive(Debug, Error, Diagnostic)]
#[error("principal type `{principal_ty}` is not valid for `{action}`")]
#[diagnostic(help("{}", invalid_principal_type_help(&.valid_principal_tys, .action.as_ref())))]
pub struct InvalidPrincipalTypeError {
/// Principal type which is not valid
pub(super) principal_ty: ast::EntityType,
/// Action which it is not valid for
pub(super) action: Arc<ast::EntityUID>,
/// Principal types which actually are valid for that `action`
pub(super) valid_principal_tys: Vec<ast::EntityType>,
}

fn invalid_principal_type_help(
valid_principal_tys: &[ast::EntityType],
action: &ast::EntityUID,
) -> String {
if valid_principal_tys.is_empty() {
format!("no principal types are valid for `{action}`")
} else {
format!(
"valid principal types for `{action}`: {}",
valid_principal_tys
.iter()
.sorted_unstable()
.map(|et| format!("`{et}`"))
.join(", ")
)
}
}

impl InvalidPrincipalTypeError {
Expand All @@ -366,17 +396,43 @@ pub mod request_validation_errors {
pub fn action(&self) -> &ast::EntityUID {
&self.action
}

/// Principal types which actually are valid for that action
pub fn valid_principal_tys(&self) -> impl Iterator<Item = &ast::EntityType> {
self.valid_principal_tys.iter()
}
}

/// Request resource is of a type that is declared in the schema, but is
/// not valid for the request action
#[derive(Debug, Error, Diagnostic)]
#[error("resource type `{resource_ty}` is not valid for `{action}`")]
#[diagnostic(help("{}", invalid_resource_type_help(&.valid_resource_tys, .action.as_ref())))]
pub struct InvalidResourceTypeError {
/// Resource type which is not valid
pub(super) resource_ty: ast::EntityType,
/// Action which it is not valid for
pub(super) action: Arc<ast::EntityUID>,
/// Resource types which actually are valid for that `action`
pub(super) valid_resource_tys: Vec<ast::EntityType>,
}

fn invalid_resource_type_help(
valid_resource_tys: &[ast::EntityType],
action: &ast::EntityUID,
) -> String {
if valid_resource_tys.is_empty() {
format!("no resource types are valid for `{action}`")
} else {
format!(
"valid resource types for `{action}`: {}",
valid_resource_tys
.iter()
.sorted_unstable()
.map(|et| format!("`{et}`"))
.join(", ")
)
}
}

impl InvalidResourceTypeError {
Expand All @@ -389,6 +445,11 @@ pub mod request_validation_errors {
pub fn action(&self) -> &ast::EntityUID {
&self.action
}

/// Resource types which actually are valid for that action
pub fn valid_resource_tys(&self) -> impl Iterator<Item = &ast::EntityType> {
self.valid_resource_tys.iter()
}
}

/// Context does not comply with the shape specified for the request action
Expand Down Expand Up @@ -756,7 +817,13 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"principal type `Album` is not valid for `Action::"view_photo"`"#).build());
expect_err(
"",
&miette::Report::new(e),
&ExpectedErrorMessageBuilder::error(r#"principal type `Album` is not valid for `Action::"view_photo"`"#)
.help(r#"valid principal types for `Action::"view_photo"`: `Group`, `User`"#)
.build(),
);
}
);
}
Expand All @@ -774,7 +841,13 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"resource type `Group` is not valid for `Action::"view_photo"`"#).build());
expect_err(
"",
&miette::Report::new(e),
&ExpectedErrorMessageBuilder::error(r#"resource type `Group` is not valid for `Action::"view_photo"`"#)
.help(r#"valid resource types for `Action::"view_photo"`: `Photo`"#)
.build(),
);
}
);
}
Expand All @@ -792,7 +865,7 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `<first-class record with 0 fields>` is not valid for `Action::"edit_photo"`"#).build());
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `{}` is not valid for `Action::"edit_photo"`"#).build());
}
);
}
Expand All @@ -818,7 +891,7 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `<first-class record with 2 fields>` is not valid for `Action::"edit_photo"`"#).build());
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `{admin_approval: true, extra: 42}` is not valid for `Action::"edit_photo"`"#).build());
}
);
}
Expand All @@ -844,7 +917,7 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `<first-class record with 1 fields>` is not valid for `Action::"edit_photo"`"#).build());
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `{admin_approval: [true]}` is not valid for `Action::"edit_photo"`"#).build());
}
);
}
Expand Down Expand Up @@ -873,7 +946,7 @@ mod test {
Extensions::all_available(),
),
Err(e) => {
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `<first-class record with 1 fields>` is not valid for `Action::"edit_photo"`"#).build());
expect_err("", &miette::Report::new(e), &ExpectedErrorMessageBuilder::error(r#"context `{admin_approval: [true, -1001]}` is not valid for `Action::"edit_photo"`"#).build());
}
);
}
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy/src/ffi/is_authorized.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1582,7 +1582,7 @@ mod test {
assert_exactly_one_error(
&errs,
"resource type `User` is not valid for `Action::\"view\"`",
None,
Some("valid resource types for `Action::\"view\"`: `Photo`"),
);
assert_is_authorized_json(bad_call_req_validation_disabled);
}
Expand Down
Loading
Loading