Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
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::reason`, `TpeResponse::satisfied_permits`, and `TpeResponse::satisfied_forbids`).
Comment thread
Copilot marked this conversation as resolved.
Outdated

### Changed

Expand Down
74 changes: 67 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 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 are the satisfied permits or forbids respectively.
/// 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 concretely satisfied by the partial request.
Comment thread
luxas marked this conversation as resolved.
///
/// If the eventual concrete authorization decision is allow, then these
/// will be a subset of the determining policies. However, they may still be
/// overridden by a non-trivial residual forbid policy.
Comment thread
victornicolet 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 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 `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,10 @@ 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 +1301,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 +2158,10 @@ 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 +2196,10 @@ 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 +2261,10 @@ 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 +2329,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 +2397,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 +2868,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 +2933,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 +2946,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 +2969,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 +3002,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