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

Fix in typechecking #462

Merged
merged 5 commits into from
Dec 1, 2023
Merged

Fix in typechecking #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
39fa93e
Fix typechecking for `in Action::"..."`
john-h-kastner-aws Nov 30, 2023
81aa44f
wip
john-h-kastner-aws Dec 1, 2023
4c6897c
wip
john-h-kastner-aws Dec 1, 2023
c9fa9b9
wip
john-h-kastner-aws Dec 1, 2023
8144642
comment
john-h-kastner-aws Dec 1, 2023
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
10 changes: 3 additions & 7 deletions cedar-policy-validator/src/rbac.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,12 +383,9 @@ impl Validator {
.into_iter(),
),
// <var> in <literal euid>
PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => Box::new(
self.schema
.get_entity_types_in(euid.as_ref())
.unwrap_or_default()
.into_iter(),
),
PrincipalOrResourceConstraint::In(EntityReference::EUID(euid)) => {
Box::new(self.schema.get_entity_types_in(euid.as_ref()).into_iter())
}
PrincipalOrResourceConstraint::Eq(EntityReference::Slot)
| PrincipalOrResourceConstraint::In(EntityReference::Slot) => {
Box::new(self.schema.known_entity_types())
Expand All @@ -406,7 +403,6 @@ impl Validator {
Box::new(
self.schema
.get_entity_types_in(in_entity.as_ref())
.unwrap_or_default()
.into_iter()
.filter(move |k| &entity_type == k),
)
Expand Down
42 changes: 26 additions & 16 deletions cedar-policy-validator/src/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,18 @@ impl ValidatorSchema {

/// Return true when the entity_type_id corresponds to a valid entity type.
pub(crate) fn is_known_entity_type(&self, entity_type: &Name) -> bool {
self.entity_types.contains_key(entity_type)
is_action_entity_type(entity_type) || self.entity_types.contains_key(entity_type)
}

/// Return true when `euid` has an entity type declared by the schema. We
/// treat an Unspecified as "known" because it is always possible to declare
/// an action using an unspecified principal/resource type without first
/// declaring unspecified as an entity type in the entity types list.
pub(crate) fn euid_has_known_entity_type(&self, euid: &EntityUID) -> bool {
match euid.entity_type() {
EntityType::Specified(ety) => self.is_known_entity_type(ety),
EntityType::Unspecified => true,
}
}

/// An iterator over the action ids in the schema.
Expand All @@ -494,18 +505,18 @@ impl ValidatorSchema {
/// includes all entity types that are descendants of the type of `entity`
/// according to the schema, and the type of `entity` itself because
/// `entity in entity` evaluates to `true`.
pub(crate) fn get_entity_types_in<'a>(&'a self, entity: &'a EntityUID) -> Option<Vec<&Name>> {
let ety = match entity.entity_type() {
EntityType::Specified(ety) => Some(ety),
EntityType::Unspecified => None,
};

ety.and_then(|ety| self.get_entity_type(ety)).map(|ety| {
ety.descendants
.iter()
.chain(std::iter::once(&ety.name))
.collect()
})
pub(crate) fn get_entity_types_in<'a>(&'a self, entity: &'a EntityUID) -> Vec<&Name> {
match entity.entity_type() {
EntityType::Specified(ety) => {
let mut descendants = self
.get_entity_type(ety)
.map(|v_ety| v_ety.descendants.iter().collect::<Vec<_>>())
.unwrap_or_default();
descendants.push(ety);
descendants
}
EntityType::Unspecified => Vec::new(),
}
}

/// Get all entity types in the schema where an `{entity0} in {euids}` can
Expand All @@ -514,12 +525,11 @@ impl ValidatorSchema {
pub(crate) fn get_entity_types_in_set<'a>(
&'a self,
euids: impl IntoIterator<Item = &'a EntityUID> + 'a,
) -> Option<Vec<&Name>> {
) -> impl Iterator<Item = &Name> {
euids
.into_iter()
.map(|e| self.get_entity_types_in(e))
.collect::<Option<Vec<_>>>()
.map(|v| v.into_iter().flatten().collect::<Vec<_>>())
.flatten()
}

/// Get all action entities in the schema where `action in euids` evaluates
Expand Down
98 changes: 58 additions & 40 deletions cedar-policy-validator/src/typecheck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1886,35 +1886,54 @@ impl<'a> Typechecker<'a> {
} else {
request_env.resource_entity_type()
};
let descendants = self.schema.get_entity_types_in_set(rhs.iter());
match (var_euid, descendants) {
// We failed to lookup the descendants because the entity type
// is not declared in the schema, or we failed to get the
// principal/resource entity type because the request is
// unknown. We don't know if the euid would be in the
// descendants or not, so give it type boolean.
(_, None) | (None, _) => {
match var_euid {
// We failed to get the principal/resource entity type because
// we are typechecking a request for some action which isn't
// declared in the schema. We don't know if the euid would be
// in the descendants or not, so give it type boolean.
None => {
let in_expr = ExprBuilder::with_data(Some(Type::primitive_boolean()))
.with_same_source_info(in_expr)
.is_in(lhs_expr, rhs_expr);
if self.mode.is_partial() {
TypecheckAnswer::success(in_expr)
} else {
// This should only happen when doing partial validation
// since we never construct the undeclared action
// request environment otherwise.
TypecheckAnswer::fail(in_expr)
}
}
(Some(EntityType::Specified(var_name)), Some(descendants)) => {
Typechecker::entity_in_descendants(
var_name,
descendants,
in_expr,
lhs_expr,
rhs_expr,
)
Some(EntityType::Specified(var_name)) => {
let all_rhs_known = rhs
.iter()
.all(|e| self.schema.euid_has_known_entity_type(e));
if self.schema.is_known_entity_type(var_name) && all_rhs_known {
let descendants = self.schema.get_entity_types_in_set(rhs.iter());
Typechecker::entity_in_descendants(
var_name,
descendants,
in_expr,
lhs_expr,
rhs_expr,
)
} else {
let annotated_expr =
ExprBuilder::with_data(Some(Type::primitive_boolean()))
.with_same_source_info(in_expr)
.is_in(lhs_expr, rhs_expr);
if self.mode.is_partial() {
// In partial schema mode, undeclared entity types are
// expected.
TypecheckAnswer::success(annotated_expr)
} else {
TypecheckAnswer::fail(annotated_expr)
}
}
}
// Unspecified entities will be detected by a different part of the validator.
// Still return `TypecheckFail` so that typechecking is not considered successful.
(Some(EntityType::Unspecified), _) => TypecheckAnswer::fail(
Some(EntityType::Unspecified) => TypecheckAnswer::fail(
ExprBuilder::with_data(Some(Type::primitive_boolean()))
.with_same_source_info(in_expr)
.is_in(lhs_expr, rhs_expr),
Expand Down Expand Up @@ -1966,7 +1985,7 @@ impl<'a> Typechecker<'a> {
} else if !lhs_is_action && !non_actions.is_empty() {
self.type_of_non_action_in_entities(
lhs_euid,
non_actions.iter(),
&non_actions,
in_expr,
lhs_expr,
rhs_expr,
Expand Down Expand Up @@ -2038,34 +2057,33 @@ impl<'a> Typechecker<'a> {
fn type_of_non_action_in_entities<'b>(
&self,
lhs: &EntityUID,
rhs: impl IntoIterator<Item = &'a EntityUID> + 'a,
rhs: &Vec<EntityUID>,
in_expr: &Expr,
lhs_expr: Expr<Option<Type>>,
rhs_expr: Expr<Option<Type>>,
) -> TypecheckAnswer<'b> {
match lhs.entity_type() {
EntityType::Specified(lhs_ety) => {
let rhs_descendants = self.schema.get_entity_types_in_set(rhs);
match rhs_descendants {
Some(rhs_descendants) if self.schema.is_known_entity_type(lhs_ety) => {
Typechecker::entity_in_descendants(
lhs_ety,
rhs_descendants,
in_expr,
lhs_expr,
rhs_expr,
)
}
_ => {
let annotated_expr =
ExprBuilder::with_data(Some(Type::primitive_boolean()))
.with_same_source_info(in_expr)
.is_in(lhs_expr, rhs_expr);
if self.mode.is_partial() {
TypecheckAnswer::success(annotated_expr)
} else {
TypecheckAnswer::fail(annotated_expr)
}
let all_rhs_known = rhs
.iter()
.all(|e| self.schema.euid_has_known_entity_type(e));
if self.schema.is_known_entity_type(lhs_ety) && all_rhs_known {
let rhs_descendants = self.schema.get_entity_types_in_set(rhs.iter());
Typechecker::entity_in_descendants(
lhs_ety,
rhs_descendants,
in_expr,
lhs_expr,
rhs_expr,
)
} else {
let annotated_expr = ExprBuilder::with_data(Some(Type::primitive_boolean()))
.with_same_source_info(in_expr)
.is_in(lhs_expr, rhs_expr);
if self.mode.is_partial() {
TypecheckAnswer::success(annotated_expr)
} else {
TypecheckAnswer::fail(annotated_expr)
}
}
}
Expand Down
60 changes: 60 additions & 0 deletions cedar-policy-validator/src/typecheck/test_policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,66 @@ fn policy_in_action_impossible() {
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { User::"alice" in [Action::"view_photo"] };"#,
)
.expect("Policy should parse.");
assert_policy_typecheck_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { principal in [action] };"#,
)
.expect("Policy should parse.");
assert_policy_typecheck_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { principal in action };"#,
)
.expect("Policy should parse.");
assert_policy_typecheck_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { principal in Action::"view_photo" };"#,
)
.expect("Policy should parse.");
assert_policy_typecheck_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Action::"delete_group"] };"#,
)
.expect("Policy should parse.");
assert_policy_typecheck_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);

let p = parse_policy(
Some("0".to_string()),
r#"permit(principal, action, resource) when { principal in [Action::"view_photo", Photo::"bar"] };"#,
khieta marked this conversation as resolved.
Show resolved Hide resolved
)
.expect("Policy should parse.");
assert_policy_typecheck_permissive_fails_simple_schema(
p.clone(),
vec![TypeError::impossible_policy(p.condition())],
);
}

#[test]
Expand Down