Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 8 additions & 8 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 @@ -130,20 +130,20 @@ 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);
satisfied_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);
satisfied_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());
}
}
}
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::decision`, `TpeResponse::satisfied_permits`, and `TpeResponse::satisfied_forbids`)
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated

### Changed

Expand Down
64 changes: 57 additions & 7 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,44 @@ impl TpeResponse<'_> {
self.0.decision()
}

/// Get the determing policies for the partial authorization decision.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
/// These are a subset of the determining polices for any subsequent
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
/// concrete reauthorization.
///
/// When [`TpeResponse::decision`] returns a concrete allow or deny, the
/// determining policies are the satisfied permits or forbids respectivly.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
/// If TPE did not reach a concrete decision, then this will always return
/// an empty set.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
pub fn reason(&self) -> HashSet<&PolicyId> {
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
match self.0.decision() {
None => HashSet::new(),
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
Some(Decision::Allow) => self.satisfied_permits().collect(),
Some(Decision::Deny) => self.satisfied_forbids().collect(),
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
}
}

Comment thread
john-h-kastner-aws marked this conversation as resolved.
/// Get the permit policies that are concretly satisfied by the partial request.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
///
/// If the eventual concrete authorization decision is allow, then these
/// will be a subset of the determining policies. However, they may still be
/// overriden by a non-trivial residual forbid policy.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
pub fn satisfied_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 concretly satisfied by the partial request.
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
///
/// Presence of any satisfied forbids guarenteees that they are exactly the
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
/// policies returned by [`TpeResponse::reason`] and that [`TpeResponse::decision`]
/// must return `Deny`.
pub fn satisfied_forbids(&self) -> impl Iterator<Item = &PolicyId> {
Comment thread
john-h-kastner-aws marked this conversation as resolved.
Outdated
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 @@ -818,7 +857,7 @@ mod tpe_tests {
}

mod streaming_service {
use std::{collections::BTreeMap, str::FromStr};
use std::{collections::{BTreeMap, HashSet}, str::FromStr};

use cedar_policy_core::{authorizer::Decision, tpe::err::EntitiesError};
use cool_asserts::assert_matches;
Expand Down Expand Up @@ -1259,6 +1298,7 @@ unless
);

assert_eq!(response.decision(), None);
assert_eq!(response.reason(), HashSet::new());

let request = Request::new(
EntityUid::from_type_name_and_id(
Expand Down Expand Up @@ -2115,10 +2155,9 @@ 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};
use std::{collections::HashSet, i64, str::FromStr};

fn schema() -> Schema {
Schema::from_str("entity P, R; action A appliesTo { principal: P, resource: R };")
Expand Down Expand Up @@ -2153,6 +2192,7 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Allow));
assert_eq!(response.reason(), HashSet::from([&PolicyId::new("policy0")]));
}

#[test]
Expand Down Expand Up @@ -2214,6 +2254,7 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(response.reason(), HashSet::from([&PolicyId::new("policy0")]));
}

#[test]
Expand Down Expand Up @@ -2278,6 +2319,7 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(response.reason(), HashSet::new());
}

#[test]
Expand Down Expand Up @@ -2345,6 +2387,7 @@ when { principal in resource.admins };
.tpe(&req, &partial_entities, &schema)
.unwrap();
assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(response.reason(), HashSet::new());
}

#[test]
Expand Down Expand Up @@ -2815,7 +2858,10 @@ when { principal in resource.admins };
}

mod template_links {
use std::{collections::HashMap, str::FromStr};
use std::{
collections::{HashMap, HashSet},
str::FromStr,
};

use crate::{
pst, Decision, EntityUid, PartialEntities, PartialEntityUid, PartialRequest, Policy,
Expand Down Expand Up @@ -2877,6 +2923,7 @@ when { principal in resource.admins };
let response = policies.tpe(&request, &es, &schema).unwrap();

assert_eq!(response.decision(), Some(Decision::Allow));
assert_eq!(response.reason(), HashSet::from([&PolicyId::new("l")]));
}

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

assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(response.reason(), HashSet::new());
}

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

assert_eq!(response.decision(), Some(Decision::Deny));
assert_eq!(response.reason(), HashSet::new());
}

#[test]
Expand Down Expand Up @@ -2943,6 +2992,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_eq!(response.reason(), HashSet::new());
assert_eq!(residuals.len(), 1);
}
}
Expand Down
Loading