-
Notifications
You must be signed in to change notification settings - Fork 160
adding support for linked policies in symcc #2338
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,7 +46,7 @@ pub mod type_abbrevs; | |
| pub mod verifier; | ||
|
|
||
| use cedar_policy::Schema; | ||
| use cedar_policy_core::ast::{Expr, ExprBuilder, Policy, PolicySet}; | ||
| use cedar_policy_core::ast::{Expr, ExprBuilder, Policy, PolicySet, Template}; | ||
| use cedar_policy_core::validator::{ | ||
| typecheck::Typechecker, types::RequestEnv, ValidationMode, Validator, | ||
| }; | ||
|
|
@@ -645,8 +645,11 @@ pub fn well_typed_policy( | |
| env: &cedar_policy::RequestEnv, | ||
| schema: &Schema, | ||
| ) -> Result<Policy> { | ||
| let env = to_validator_request_env(env, schema.as_ref()) | ||
| let mut env = to_validator_request_env(env, schema.as_ref()) | ||
| .ok_or_else(|| Error::ActionNotInSchema(env.action().to_string()))?; | ||
| if !policy.is_static() { | ||
| env = env.link_slot_env(policy.env()); | ||
| } | ||
| well_typed_policy_inner(policy, &env, schema) | ||
| } | ||
|
|
||
|
|
@@ -655,16 +658,35 @@ fn well_typed_policy_inner( | |
| env: &RequestEnv<'_>, | ||
| schema: &Schema, | ||
| ) -> Result<Policy> { | ||
| // For linked policies, create a static equivalent by resolving slots in the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we change the model to match this? IIRC, the lean FFI layer currently does this, but it could make sense to handle this is the corresponding lean function here, I think: might be a larger update than i'm thinking though
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes the Lean FFI layer is currently doing the slot replacement. Another difference between the Rust and Lean is that the Lean env doesn't get the extra information that the linked env in Rust passes. However, trying to integrate that alone in Lean doesn't seem trivial either.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you have a decent idea of what would be needed, can you type up an issue on cedar-spec? |
||
| // scope constraints. This ensures the typechecked expression has no slots. | ||
| let template_for_tc; | ||
| let template_ref = if policy.is_static() { | ||
| policy.template() | ||
| } else { | ||
| template_for_tc = Template::new_shared( | ||
| policy.id().clone(), | ||
| policy.loc().cloned(), | ||
| policy.template().annotations_arc().clone(), | ||
| policy.effect(), | ||
| policy.principal_constraint(), | ||
| policy.template().action_constraint().clone(), | ||
| policy.resource_constraint(), | ||
| policy.non_scope_constraints_arc().cloned(), | ||
| ); | ||
| &template_for_tc | ||
| }; | ||
|
|
||
| let validator_schema = schema.as_ref(); | ||
| // We need to perform these three checks like what the validator does here: https://github.com/cedar-policy/cedar/blob/82784864c01b5096cb73885dd2df5643074355ed/cedar-policy-core/src/validator.rs#L178-L185 | ||
| // We don't need `validate_template_action_application` because existence of `env` already serves as evidence | ||
| let errs: Vec<_> = | ||
| Validator::validate_entity_types_and_literals(schema.as_ref(), policy.template()).collect(); | ||
| Validator::validate_entity_types_and_literals(schema.as_ref(), template_ref).collect(); | ||
| if !errs.is_empty() { | ||
| return Err(Error::PolicyNotWellTyped { errs }); | ||
| } | ||
| let type_checker = Typechecker::new(validator_schema, ValidationMode::Strict); | ||
| let policy_check = type_checker.typecheck_by_single_request_env(policy.template(), env); | ||
| let policy_check = type_checker.typecheck_by_single_request_env(template_ref, env); | ||
|
|
||
| use cedar_policy_core::validator::typecheck::PolicyCheck::*; | ||
| match policy_check { | ||
|
|
@@ -711,17 +733,18 @@ pub fn well_typed_policies( | |
| env: &cedar_policy::RequestEnv, | ||
| schema: &Schema, | ||
| ) -> Result<PolicySet> { | ||
| if policies.policies().any(|p| !p.is_static()) { | ||
| return Err(CompileError::UnsupportedFeature( | ||
| "template-linked policies are not supported".to_string(), | ||
| ) | ||
| .into()); | ||
| } | ||
| let env = to_validator_request_env(env, schema.as_ref()) | ||
| let base_env = to_validator_request_env(env, schema.as_ref()) | ||
| .ok_or_else(|| Error::ActionNotInSchema(env.action().to_string()))?; | ||
| let typed_policies: Result<Vec<Policy>> = policies | ||
| .static_policies() | ||
| .map(|p| well_typed_policy_inner(p, &env, schema)) | ||
| .policies() | ||
| .map(|p| { | ||
| let linked_env = if p.is_static() { | ||
| base_env.clone() | ||
| } else { | ||
| base_env.clone().link_slot_env(p.env()) | ||
| }; | ||
| well_typed_policy_inner(p, &linked_env, schema) | ||
| }) | ||
| .collect(); | ||
| match typed_policies { | ||
| Ok(ps) => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a separate
SymccPoliciesArgnow that it accepts templates links? I think I remember @katherine-hough duplicated in order to not accept templates here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
John is correct see: afae911
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I should have remembered too.