Skip to content

Commit

Permalink
Clippy fixes (#1269)
Browse files Browse the repository at this point in the history 
Signed-off-by: John Kastner <[email protected]>
  • Loading branch information
@john-h-kastner-aws
john-h-kastner-aws authored Oct 11, 2024
1 parent b564880 commit 1729f98
Show file tree
Hide file tree
Showing 10 changed files with 109 additions and 127 deletions.
39 changes: 18 additions & 21 deletions cedar-policy-cli/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ struct RequestJSON {
#[cfg(feature = "partial-eval")]
/// This struct is the serde structure expected for --request-json
#[derive(Deserialize)]
pub(self) struct PartialRequestJSON {
struct PartialRequestJSON {
/// Principal for the request
pub(self) principal: Option<String>,
/// Action for the request
Expand Down Expand Up @@ -1262,27 +1262,24 @@ pub fn partial_authorize(args: &PartiallyAuthorizeArgs) -> CedarExitCode {
args.timing,
);
match ans {
Ok(ans) => {
let status = match ans.decision() {
Some(Decision::Allow) => {
println!("ALLOW");
CedarExitCode::Success
}
Some(Decision::Deny) => {
println!("DENY");
CedarExitCode::AuthorizeDeny
}
None => {
println!("UNKNOWN");
println!("All policy residuals:");
for p in ans.nontrivial_residuals() {
println!("{p}");
}
CedarExitCode::Unknown
Ok(ans) => match ans.decision() {
Some(Decision::Allow) => {
println!("ALLOW");
CedarExitCode::Success
}
Some(Decision::Deny) => {
println!("DENY");
CedarExitCode::AuthorizeDeny
}
None => {
println!("UNKNOWN");
println!("All policy residuals:");
for p in ans.nontrivial_residuals() {
println!("{p}");
}
};
status
}
CedarExitCode::Unknown
}
},
Err(errs) => {
for err in errs {
println!("{err:?}");
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-validator/src/cedar_schema/test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2344,7 +2344,7 @@ mod entity_tags {
assert_matches!(ty, json_schema::Type::Type(json_schema::TypeVariant::EntityOrCommon { type_name }) => {
assert_eq!(&format!("{type_name}"), "String");
});
assert_eq!(*required, true);
assert!(*required);
});
});
});
Expand Down
9 changes: 3 additions & 6 deletions cedar-policy-validator/src/cedar_schema/to_json_schema.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,8 @@ impl TryFrom<Namespace> for json_schema::NamespaceDefinition<RawName> {
let name_loc = decl.name.loc.clone();
let id = UnreservedId::try_from(decl.name.node)
.map_err(|e| ToJsonSchemaError::reserved_name(e.name(), name_loc.clone()))?;
let ctid = json_schema::CommonTypeId::new(id).map_err(|e| match e {
json_schema::ReservedCommonTypeBasenameError { id } => {
ToJsonSchemaError::reserved_keyword(id, name_loc)
}
})?;
let ctid = json_schema::CommonTypeId::new(id)
.map_err(|e| ToJsonSchemaError::reserved_keyword(e.id, name_loc))?;
Ok((ctid, cedar_type_to_json_type(decl.def)))
})
.collect::<Result<_, ToJsonSchemaError>>()?;
Expand Down Expand Up @@ -348,7 +345,7 @@ fn convert_entity_decl(
let etype = json_schema::EntityType {
member_of_types: e.member_of_types.into_iter().map(RawName::from).collect(),
shape: convert_attr_decls(e.attrs),
tags: e.tags.map(|tag_ty| cedar_type_to_json_type(tag_ty)),
tags: e.tags.map(cedar_type_to_json_type),
};

// Then map over all of the bound names
Expand Down
10 changes: 2 additions & 8 deletions cedar-policy-validator/src/diagnostics/validation_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,8 @@ impl Diagnostic for HierarchyNotRespected {
}
}

#[derive(Debug, Clone, Hash, Eq, PartialEq, Error, Copy, Ord, PartialOrd)]
/// Represents how many entity dereferences can be applied to a node.
#[derive(Default, Debug, Clone, Hash, Eq, PartialEq, Error, Copy, Ord, PartialOrd)]
pub struct EntityDerefLevel {
/// A negative value `-n` represents `n` too many dereferences
pub level: i64,
Expand All @@ -506,12 +506,6 @@ impl From<u32> for EntityDerefLevel {
}
}

impl Default for EntityDerefLevel {
fn default() -> Self {
Self { level: 0 }
}
}

impl Add for EntityDerefLevel {
type Output = Self;

Expand Down Expand Up @@ -557,7 +551,7 @@ impl Diagnostic for EntityDerefLevelViolation {
impl_diagnostic_from_source_loc_opt_field!(source_loc);

fn help<'a>(&'a self) -> Option<Box<dyn Display + 'a>> {
Some(Box::new(format!("Consider increasing the level")))
Some(Box::new("Consider increasing the level"))
}
}

Expand Down
42 changes: 20 additions & 22 deletions cedar-policy-validator/src/level_validate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl Validator {
);
(peekable_errors.chain(levels_errors), warnings)
} else {
(peekable_errors.into_iter().chain(vec![]), warnings)
(peekable_errors.chain(vec![]), warnings)
}
}

Expand All @@ -69,10 +69,9 @@ impl Validator {
match policy_check {
PolicyCheck::Success(e) | PolicyCheck::Irrelevant(_, e) => {
let res =
self.check_entity_deref_level_helper(&e, max_allowed_level, policy_id);
match res.1 {
Some(e) => errs.push(ValidationError::EntityDerefLevelViolation(e)),
None => (),
Self::check_entity_deref_level_helper(&e, max_allowed_level, policy_id);
if let Some(e) = res.1 {
errs.push(ValidationError::EntityDerefLevelViolation(e))
}
}
// PANIC SAFETY: We only validate the level after validation passed
Expand All @@ -95,8 +94,7 @@ impl Validator {

/// Walk the type-annotated AST and compute the used level and possible violation
/// Returns a tuple of `(actual level used, optional violation information)`
fn check_entity_deref_level_helper<'a>(
&'a self,
fn check_entity_deref_level_helper(
e: &cedar_policy_core::ast::Expr<Option<crate::types::Type>>,
max_allowed_level: &EntityDerefLevel,
policy_id: &PolicyID,
Expand All @@ -108,7 +106,7 @@ impl Validator {
EntityDerefLevel { level: 0 }, //Literals can't be dereferenced
None,
),
ExprKind::Var(_) => (max_allowed_level.clone(), None), //Roots start at `max_allowed_level`
ExprKind::Var(_) => (*max_allowed_level, None), //Roots start at `max_allowed_level`
ExprKind::Slot(_) => (EntityDerefLevel { level: 0 }, None), //Slot will be replaced by Entity literal so treat the same
ExprKind::Unknown(_) => (
EntityDerefLevel { level: 0 }, //Can't dereference an unknown
Expand All @@ -122,25 +120,25 @@ impl Validator {
let es = [test_expr, then_expr, else_expr];
let v: Vec<(EntityDerefLevel, Option<_>)> = es
.iter()
.map(|l| self.check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.map(|l| Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.collect();
Self::min(v)
}
ExprKind::And { left, right } | ExprKind::Or { left, right } => {
let es = [left, right];
let v: Vec<(EntityDerefLevel, Option<_>)> = es
.iter()
.map(|l| self.check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.map(|l| Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.collect();
Self::min(v)
}
ExprKind::UnaryApp { arg, .. } => {
self.check_entity_deref_level_helper(arg, max_allowed_level, policy_id)
Self::check_entity_deref_level_helper(arg, max_allowed_level, policy_id)
}
// `In` operator decrements the LHS only
ExprKind::BinaryApp { op, arg1, arg2 } if op == &BinaryOp::In => {
let lhs = self.check_entity_deref_level_helper(arg1, max_allowed_level, policy_id);
let rhs = self.check_entity_deref_level_helper(arg2, max_allowed_level, policy_id);
let lhs = Self::check_entity_deref_level_helper(arg1, max_allowed_level, policy_id);
let rhs = Self::check_entity_deref_level_helper(arg2, max_allowed_level, policy_id);
let lhs = (lhs.0.decrement(), lhs.1);
let new_level = Self::min(vec![lhs, rhs]).0;
if new_level.level < 0 {
Expand All @@ -150,7 +148,7 @@ impl Validator {
source_loc: e.source_loc().cloned(),
policy_id: policy_id.clone(),
actual_level: new_level,
allowed_level: max_allowed_level.clone(),
allowed_level: *max_allowed_level,
}),
)
} else {
Expand All @@ -161,14 +159,14 @@ impl Validator {
let es = [arg1, arg2];
let v: Vec<(EntityDerefLevel, Option<_>)> = es
.iter()
.map(|l| self.check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.map(|l| Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.collect();
Self::min(v)
}
ExprKind::ExtensionFunctionApp { args, .. } => {
let v: Vec<(EntityDerefLevel, Option<_>)> = args
.iter()
.map(|l| self.check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.map(|l| Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.collect();
Self::min(v)
}
Expand All @@ -179,7 +177,7 @@ impl Validator {
ExprKind::Record(m) => {
// PANIC SAFETY: Validation checked that this access is safe
#[allow(clippy::unwrap_used)]
self.check_entity_deref_level_helper(
Self::check_entity_deref_level_helper(
m.get(attr).unwrap(),
max_allowed_level,
policy_id,
Expand All @@ -196,7 +194,7 @@ impl Validator {
{
Some(ty) => {
let child_level_info =
self.check_entity_deref_level_helper(expr, max_allowed_level, policy_id);
Self::check_entity_deref_level_helper(expr, max_allowed_level, policy_id);
match ty {
Type::EntityOrRecord(EntityRecordKind::Entity { .. })
| Type::EntityOrRecord(EntityRecordKind::ActionEntity { .. }) => {
Expand All @@ -209,7 +207,7 @@ impl Validator {
source_loc: e.source_loc().cloned(),
policy_id: policy_id.clone(),
actual_level: new_level,
allowed_level: max_allowed_level.clone(),
allowed_level: *max_allowed_level,
}),
)
} else {
Expand All @@ -228,20 +226,20 @@ impl Validator {
None => unreachable!("Expected type-annotated AST"),
},
ExprKind::Like { expr, .. } | ExprKind::Is { expr, .. } => {
self.check_entity_deref_level_helper(expr, max_allowed_level, policy_id)
Self::check_entity_deref_level_helper(expr, max_allowed_level, policy_id)
}
ExprKind::Set(elems) => {
let v: Vec<(EntityDerefLevel, Option<_>)> = elems
.iter()
.map(|l| self.check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.map(|l| Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id))
.collect();
Self::min(v)
}
ExprKind::Record(fields) => {
let v: Vec<(EntityDerefLevel, Option<_>)> = fields
.iter()
.map(|(_, l)| {
self.check_entity_deref_level_helper(l, max_allowed_level, policy_id)
Self::check_entity_deref_level_helper(l, max_allowed_level, policy_id)
})
.collect();
Self::min(v)
Expand Down
8 changes: 4 additions & 4 deletions cedar-policy-validator/src/schema.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4507,7 +4507,7 @@ mod entity_tags {
owner: User,
} tags Set<String>;
";
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, &Extensions::all_available())), Ok((schema, warnings)) => {
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, Extensions::all_available())), Ok((schema, warnings)) => {
assert!(warnings.is_empty());
let user = assert_entity_type_exists(&schema, "User");
assert_matches!(user.tag_type(), Some(Type::Set { element_type: Some(el_ty) }) => {
Expand Down Expand Up @@ -4557,7 +4557,7 @@ mod entity_tags {
},
"actions": {}
}});
assert_matches!(ValidatorSchema::from_json_value(json.clone(), &Extensions::all_available()), Ok(schema) => {
assert_matches!(ValidatorSchema::from_json_value(json.clone(), Extensions::all_available()), Ok(schema) => {
let user = assert_entity_type_exists(&schema, "User");
assert_matches!(user.tag_type(), Some(Type::Set { element_type: Some(el_ty) }) => {
assert_matches!(&**el_ty, Type::Primitive { primitive_type: Primitive::String });
Expand Down Expand Up @@ -4590,7 +4590,7 @@ mod entity_tags {
entity Foo7 in E tags Set<Set<{a: Blah}>>;
entity Foo8 in E tags Foo7;
";
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, &Extensions::all_available())), Ok((schema, warnings)) => {
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, Extensions::all_available())), Ok((schema, warnings)) => {
assert!(warnings.is_empty());
let e = assert_entity_type_exists(&schema, "E");
assert_matches!(e.tag_type(), None);
Expand All @@ -4616,7 +4616,7 @@ mod entity_tags {
#[test]
fn invalid_tags() {
let src = "entity E tags Undef;";
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, &Extensions::all_available())), Err(e) => {
assert_matches!(collect_warnings(ValidatorSchema::from_cedarschema_str(src, Extensions::all_available())), Err(e) => {
expect_err(
src,
&miette::Report::new(e),
Expand Down
2 changes: 1 addition & 1 deletion cedar-policy-validator/src/schema/raw_name.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ impl ConditionalName {
/// `all_defs` also internally includes [`InternalName`]s, because some
/// names containing `__cedar` might be internally defined/valid, even
/// though it is not valid for _end-users_ to define those names.
pub fn resolve<'a>(self, all_defs: &AllDefs) -> Result<InternalName, TypeNotDefinedError> {
pub fn resolve(self, all_defs: &AllDefs) -> Result<InternalName, TypeNotDefinedError> {
for possibility in &self.possibilities {
// Per RFC 24, we give priority to trying to resolve to a common
// type, before trying to resolve to an entity type.
Expand Down
10 changes: 4 additions & 6 deletions cedar-policy-validator/src/typecheck.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1503,7 +1503,7 @@ impl<'a> Typechecker<'a> {
ExprBuilder::with_data(Some(type_of_has))
.with_same_source_loc(bin_expr)
.binary_app(BinaryOp::HasTag, expr_ty_arg1, expr_ty_arg2),
CapabilitySet::singleton(Capability::new_borrowed_tag(arg1, &arg2)),
CapabilitySet::singleton(Capability::new_borrowed_tag(arg1, arg2)),
)
})
}),
Expand All @@ -1515,9 +1515,7 @@ impl<'a> Typechecker<'a> {
arg1,
Type::any_entity_reference(),
type_errors,
|actual| match actual {
_ => None,
},
|_actual| None,
)
.then_typecheck(|expr_ty_arg1, _| {
self.expect_type(
Expand Down Expand Up @@ -1554,7 +1552,7 @@ impl<'a> Typechecker<'a> {
);
}
};
if prior_capability.contains(&Capability::new_borrowed_tag(arg1, &arg2)) {
if prior_capability.contains(&Capability::new_borrowed_tag(arg1, arg2)) {
// Determine the set of possible tag types for this access.
let tag_types = match self.tag_types(kind) {
Ok(tag_types) => tag_types,
Expand Down Expand Up @@ -1602,7 +1600,7 @@ impl<'a> Typechecker<'a> {
// compute the LUB of all the relevant tag types, and assign that
// as the type.
let tag_type = match Type::reduce_to_least_upper_bound(
&self.schema,
self.schema,
tag_types.clone(),
self.mode,
) {
Expand Down
Loading

0 comments on commit 1729f98

Please sign in to comment.