Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
12 changes: 6 additions & 6 deletions cedar-policy-core/src/tpe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,36 +457,36 @@ when { principal in resource.editors };
.static_policies()
.find(|p| matches!(p.annotation(&id), Some(Annotation {val, ..}) if val == "3"))
.unwrap();
let false_permits: HashSet<PolicyID> = residuals
let false_permits: HashSet<&PolicyID> = residuals
.false_permits()
.map(|p| p.get_policy_id())
.collect();
assert!(false_permits.len() == 2);
assert!(false_permits.contains(policy0.id()));
assert!(false_permits.contains(policy3.id()));
let false_forbids: HashSet<PolicyID> = residuals
let false_forbids: HashSet<&PolicyID> = residuals
.false_forbids()
.map(|p| p.get_policy_id())
.collect();
assert!(false_forbids.is_empty());
let true_permits: HashSet<PolicyID> = residuals
let true_permits: HashSet<&PolicyID> = residuals
.satisfied_permits()
.map(|p| p.get_policy_id())
.collect();
assert!(true_permits.is_empty());
let true_forbids: HashSet<PolicyID> = residuals
let true_forbids: HashSet<&PolicyID> = residuals
.satisfied_forbids()
.map(|p| p.get_policy_id())
.collect();
assert!(true_forbids.is_empty());
let non_trivial_permits: HashSet<PolicyID> = residuals
let non_trivial_permits: HashSet<&PolicyID> = residuals
.residual_permits()
.map(|p| p.get_policy_id())
.collect();
assert!(non_trivial_permits.len() == 2);
assert!(non_trivial_permits.contains(policy1.id()));
assert!(non_trivial_permits.contains(policy2.id()));
let non_trivial_forbids: HashSet<PolicyID> = residuals
let non_trivial_forbids: HashSet<&PolicyID> = residuals
.residual_forbids()
.map(|p| p.get_policy_id())
.collect();
Expand Down
44 changes: 26 additions & 18 deletions cedar-policy-core/src/tpe/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ impl ResidualPolicy {
}

/// Get the [`PolicyID`]
pub fn get_policy_id(&self) -> PolicyID {
self.policy.id().clone()
pub fn get_policy_id(&self) -> &PolicyID {
self.policy.id()
Comment thread
john-h-kastner-aws marked this conversation as resolved.
}
Comment thread
john-h-kastner-aws marked this conversation as resolved.

/// All literal uids referenced by this residual
Expand Down Expand Up @@ -88,13 +88,13 @@ pub struct Response<'a> {
decision: Option<Decision>,
residuals: HashMap<PolicyID, ResidualPolicy>,
// All of the [`Effect::Permit`] policies that were satisfied
satisfied_permits: HashSet<PolicyID>,
true_permits: HashSet<PolicyID>,
// All of the [`Effect::Permit`] policies that were not satisfied
false_permits: HashSet<PolicyID>,
// All of the [`Effect::Permit`] policies that evaluated to a residual
residual_permits: HashSet<PolicyID>,
// All of the [`Effect::Forbid`] policies that were satisfied
satisfied_forbids: HashSet<PolicyID>,
true_forbids: HashSet<PolicyID>,
// All of the [`Effect::Forbid`] policies that were not satisfied
false_forbids: HashSet<PolicyID>,
// All of the [`Effect::Forbid`] policies that evaluated to a residual
Expand All @@ -117,10 +117,10 @@ impl<'a> Response<'a> {
schema: &'a ValidatorSchema,
) -> Self {
let mut residual_map = HashMap::new();
let mut satisfied_permits = HashSet::new();
let mut true_permits = HashSet::new();
let mut false_permits = HashSet::new();
let mut residual_permits = HashSet::new();
let mut satisfied_forbids = HashSet::new();
let mut true_forbids = HashSet::new();
let mut false_forbids = HashSet::new();
let mut residual_forbids = HashSet::new();
for rp in residuals {
Expand All @@ -130,28 +130,28 @@ impl<'a> Response<'a> {
match rp.get_effect() {
Effect::Forbid => {
if r.is_true() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: feels like this could be a good candidate to have an enum / match statement for in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We could write this as a match on the residual, but I don't think I see how it would make the code any nicer

satisfied_forbids.insert(id);
true_forbids.insert(id.clone());
} else if r.is_false() || r.is_error() {
false_forbids.insert(id);
false_forbids.insert(id.clone());
} else {
residual_forbids.insert(id);
residual_forbids.insert(id.clone());
}
}
Effect::Permit => {
if r.is_true() {
satisfied_permits.insert(id);
true_permits.insert(id.clone());
} else if r.is_false() || r.is_error() {
false_permits.insert(id);
false_permits.insert(id.clone());
} else {
residual_permits.insert(id);
residual_permits.insert(id.clone());
}
}
}
}

let decision = match (
!satisfied_forbids.is_empty(),
!satisfied_permits.is_empty(),
!true_forbids.is_empty(),
!true_permits.is_empty(),
!residual_permits.is_empty(),
!residual_forbids.is_empty(),
) {
Expand All @@ -170,10 +170,10 @@ impl<'a> Response<'a> {
Self {
decision,
residuals: residual_map,
satisfied_permits,
true_permits,
false_permits,
residual_permits,
satisfied_forbids,
true_forbids,
false_forbids,
residual_forbids,
request,
Expand All @@ -188,7 +188,7 @@ impl<'a> Response<'a> {
clippy::unwrap_used,
reason = "we know that the policy ids are in the residuals map"
)]
self.satisfied_permits
self.true_permits
.iter()
.map(|id| self.residuals.get(id).unwrap())
}
Expand All @@ -199,7 +199,7 @@ impl<'a> Response<'a> {
clippy::unwrap_used,
reason = "we know that the policy ids are in the residuals map"
)]
self.satisfied_forbids
self.true_forbids
.iter()
.map(|id| self.residuals.get(id).unwrap())
}
Expand Down Expand Up @@ -258,6 +258,14 @@ impl<'a> Response<'a> {
self.decision
}

/// Get the determining policies for the authorization decision
pub fn reason(&self) -> Option<impl Iterator<Item = &PolicyID>> {
match self.decision? {
Decision::Allow => Some(self.true_permits.iter()),
Decision::Deny => Some(self.true_forbids.iter()),
}
}

/// Perform reauthorization
pub fn reauthorize(
&self,
Expand Down
1 change: 1 addition & 0 deletions cedar-policy/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Starting with version 3.2.4, changes marked with a star (*) are _language breaki

### Added
- Public syntax tree (`pst`) support for `variadic-is-in-range` feature: a variadic `isInRange` is modelled by a `pst::Expr::VariadicOp{...}` in the PST (#2380).
- Functions for inspecting the determining policies on a TPE response (`TpeResponse::reason`, `TpeResponse::satisfied_permits`, and `TpeResponse::satisfied_forbids`).
Comment thread
Copilot marked this conversation as resolved.
Outdated

### Changed

Expand Down
95 changes: 91 additions & 4 deletions cedar-policy/src/api/tpe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ use smol_str::SmolStr;

use crate::{
api, tpe_err, Authorizer, Context, Entities, Entity, EntityId, EntityTypeName, EntityUid,
PartialEntityError, PartialRequestCreationError, PermissionQueryError, Policy, PolicySet,
Request, RequestValidationError, RestrictedExpression, Schema, TpeReauthorizationError,
PartialEntityError, PartialRequestCreationError, PermissionQueryError, Policy, PolicyId,
PolicySet, Request, RequestValidationError, RestrictedExpression, Schema,
TpeReauthorizationError,
};

/// A partial [`EntityUid`].
Expand Down Expand Up @@ -384,6 +385,54 @@ impl TpeResponse<'_> {
self.0.decision()
}

/// Get the determining policies for the partial authorization decision.
/// These are a subset of the determining policies for any subsequent
/// concrete reauthorization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the for any subsequent concrete reauthorization part of the comment, the returned policies already evaluated to true, so there is no need for reauthorization?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is that if you were to do concrete authorization, there might be more determining policies that aren't represented here. If you want to take some action based on which policies apply to a request, this might be important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

expanded comments substantially. let me know if it makes sense

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe determining policies for any subsequent concrete reauthorization could be reworded as determining policies of an concrete authorization request with a complete Entity store? I guess that's what you mean?

///
/// When [`TpeResponse::decision`] returns a concrete allow or deny, the
/// determining policies returned by this function are the satisfied permits
/// or forbids respectively.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
///
/// If partial authorization does not reach a decision, then this function
/// returns `None`. It's reasonable to treat this response as "no known
/// determining policies", in which case you can call this function as
/// `response.reason().into_iter().flatten()`.
pub fn reason(&self) -> Option<impl Iterator<Item = &PolicyId>> {
Some(self.0.reason()?.map(PolicyId::ref_cast))
}

Comment thread
john-h-kastner-aws marked this conversation as resolved.
/// Get the permit policies that are concretely satisfied by the partial request.
Comment thread
luxas marked this conversation as resolved.
///
/// To properly interpret the ids returned from this function you need to
/// consider them in the context of [`TpeResponse::decision`]:
/// * For a concrete `Allow` decision, these are a subset of the concrete
/// determining policies and are exactly the policies returned by
/// [`TpeResponse::reason`].
/// * For a concrete `Deny` decision, these are not determining policies. The
/// iterator may be empty if no permits were satisfied, or it may contain
/// satisfied permits which have been overridden by at least one satisfied
/// forbid policy.
/// * For an unknown decision, these will be a subset of the determining
/// policies _if_ the eventual concrete authorization decision is `Allow`,
/// but they may still be overridden by any non-trivial residual forbid
/// policy.
pub fn true_permits(&self) -> impl Iterator<Item = &PolicyId> {
self.0
.satisfied_permits()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

one missing getter rename, or don't we want to update all of these at the same time?

.map(|rp| PolicyId::ref_cast(rp.get_policy_id()))
}
Comment thread
john-h-kastner-aws marked this conversation as resolved.

/// Get the forbid policies that are concretely satisfied by the partial request.
///
/// Presence of any satisfied forbids guarantees that they are exactly the
/// policies returned by [`TpeResponse::reason`] and that [`TpeResponse::decision`]
/// must return a concrete `Deny`.
pub fn true_forbids(&self) -> impl Iterator<Item = &PolicyId> {
self.0
.satisfied_forbids()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems like one missing getter rename

.map(|rp| PolicyId::ref_cast(rp.get_policy_id()))
}

/// Perform reauthorization
pub fn reauthorize(
&self,
Expand Down Expand Up @@ -1259,6 +1308,7 @@ unless
);

assert_eq!(response.decision(), None);
assert!(response.reason().is_none());

let request = Request::new(
EntityUid::from_type_name_and_id(
Expand Down Expand Up @@ -2115,8 +2165,8 @@ when { principal in resource.admins };
use itertools::Itertools;

use crate::{
Context, Entities, PartialEntities, PartialEntityUid, PartialRequest, PolicySet,
PrincipalQueryRequest, ResourceQueryRequest, Schema,
Context, Entities, PartialEntities, PartialEntityUid, PartialRequest, PolicyId,
PolicySet, PrincipalQueryRequest, ResourceQueryRequest, Schema,
};
use std::{i64, str::FromStr};

Expand Down Expand Up @@ -2153,6 +2203,10 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Allow));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
vec![&PolicyId::new("policy0")]
);
}

#[test]
Expand Down Expand Up @@ -2214,6 +2268,14 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
vec![&PolicyId::new("policy0")]
);
assert_eq!(
response.true_forbids().collect::<Vec<_>>(),
vec![&PolicyId::new("policy0")]
);
}

#[test]
Expand Down Expand Up @@ -2278,6 +2340,10 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
Vec::<&PolicyId>::new()
);
}

#[test]
Expand Down Expand Up @@ -2345,6 +2411,10 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
Vec::<&PolicyId>::new()
);
}

#[test]
Expand Down Expand Up @@ -2877,6 +2947,14 @@ when { principal in resource.admins };
let response = policies.tpe(&request, &es, &schema).unwrap();

assert_eq!(response.decision(), Some(Decision::Allow));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
vec![&PolicyId::new("l")]
);
assert_eq!(
response.true_permits().collect::<Vec<_>>(),
vec![&PolicyId::new("l")]
);
}

#[test]
Expand All @@ -2889,6 +2967,10 @@ when { principal in resource.admins };
let response = policies.tpe(&request, &es, &schema).unwrap();

assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
Vec::<&PolicyId>::new()
);
}

#[test]
Expand All @@ -2911,6 +2993,10 @@ when { principal in resource.admins };
let response = policies.tpe(&request, &es, &schema).unwrap();

assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(
response.reason().unwrap().collect::<Vec<_>>(),
Vec::<&PolicyId>::new()
);
}

#[test]
Expand Down Expand Up @@ -2943,6 +3029,7 @@ when { principal in resource.admins };
let residuals: Vec<_> = response.nontrivial_residual_policies().collect();
assert_eq!(residuals[0].to_pst().unwrap().body(), expected.body());
assert_eq!(response.decision(), None);
assert!(response.reason().is_none());
assert_eq!(residuals.len(), 1);
}
}
Expand Down
Loading