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.

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

Update TinyTodo to use templates #82

Merged
merged 11 commits into from
Jan 1, 2024
23 changes: 12 additions & 11 deletions tinytodo/policies.cedar
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,26 @@ permit (
permit (principal, action, resource)
when { resource is List && resource.owner == principal };

// Policy 2: A User can see a List if they are either a reader or editor
// Policy 2: Users who are members of [?principal] are readers of [?resource]
@id("reader-template")
permit (
principal,
principal in ?principal,
action == Action::"GetList",
resource
)
when { principal in resource.readers || principal in resource.editors };
resource == ?resource
);

// Policy 3: A User can update a List and its tasks if they are an editor
// Policy 3: Users who are members of [?principal] are editors of [?resource]
@id("editor-template")
permit (
principal,
principal in ?principal,
action in
[Action::"UpdateList",
[Action::"GetList",
Action::"UpdateList",
Action::"CreateTask",
Action::"UpdateTask",
Action::"DeleteTask"],
resource
)
when { principal in resource.editors };
resource == ?resource
);

// Policy 4: Admins can perform any action on any resource
// @id("admin-omnipotence")
Expand Down
109 changes: 92 additions & 17 deletions tinytodo/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@

use itertools::Itertools;
use lazy_static::lazy_static;
use std::path::PathBuf;
use tracing::{info, trace};
use std::{collections::HashMap, path::PathBuf, str::FromStr};
use tracing::{error, info, trace};

use cedar_policy::{
Authorizer, Context, Decision, Diagnostics, EntityTypeName, ParseErrors, PolicySet,
PolicySetError, Request, Schema, SchemaError, ValidationMode, Validator,
Authorizer, Context, Decision, Diagnostics, EntityTypeName, ParseErrors, PolicyId, PolicySet,
PolicySetError, Request, Schema, SchemaError, SlotId, ValidationMode, Validator,
};
use thiserror::Error;
use tokio::sync::{
Expand All @@ -32,7 +32,7 @@ use tokio::sync::{
use crate::{
api::{
AddShare, CreateList, CreateTask, DeleteList, DeleteShare, DeleteTask, Empty, GetList,
GetLists, UpdateList, UpdateTask,
GetLists, ShareRole, UpdateList, UpdateTask,
},
entitystore::{EntityDecodeError, EntityStore},
objects::List,
Expand Down Expand Up @@ -308,6 +308,7 @@ impl AppContext {
let tx = send.clone();
tokio::spawn(async move {
info!("Serving application server!");
// FIXME: probably should pass in [schema] not [schema_path]
policy_store::spawn_watcher(policies_path, schema_path, tx).await;
let c = Self {
entities,
Expand Down Expand Up @@ -355,27 +356,101 @@ impl AppContext {

#[tracing::instrument(skip(policy_set))]
fn update_policy_set(&mut self, policy_set: PolicySet) -> Result<AppResponse> {
let policies = rename_from_id_annotation(policy_set)?;
self.policies = policies;
info!("Reloaded policy set");
let mut new_policies = rename_from_id_annotation(policy_set)?;
let mut err = false;
let mut updated = false;
// for each existing template-linked policy,
// link against the new version of the template in the new policy set if present
for p in self.policies.policies() {
match p.template_id() {
None => (), // not a template-linked policy
Some(tid) => {
// template-linked policy
match new_policies.template(tid) {
None => {
// template not in new policy set
let tidx = tid.clone();
let pidx = p.id().clone();
mwhicks1 marked this conversation as resolved.
Show resolved Hide resolved
err = true;
error!("Error when reloading policies: Could not find policy template {tidx} to link {pidx}")
}
Some(_) => {
// found template in new policy set
match p.template_links() {
None => error!("Error when reloading policies: Template with no matching links"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this set err = true;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an internal error, which should never happen. I.e., you should not have template_id() return Some(...) but then have template_links() return None. I'll change this to use expect and produce a panic! if it ever happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought: We could change the Policy::template_links() API to return both the linked values and the template ID, together in an Option. Then we wouldn't need multiple calls. You could just call template_links() as a way to test whether a policy is template-linked, and if it is to return Some(tid,links). That would simplify a bunch of logic here, and avoid the need for app code to assume the Cedar-internal invariant.

It would be changing the API, but we haven't released the PolicySet::template_links() API yet, so it won't be a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cedar-policy/cedar#548 -- I'm of a mixed mind about whether doing this is worth it.

Some(vals) => {
// link against new template, using the same policy ID as the old one
updated = true;
new_policies.link(tid.clone(), p.id().clone(), vals)?
}
}
}
}
}
}
}
// no error during relinking
if !err {
// check that re-linked policies validate properly
if updated {
let validator = Validator::new(self.schema.clone());
let output = validator.validate(&new_policies, ValidationMode::default());
if !output.validation_passed() {
for e in output.validation_errors() {
error!("Error validating linked policies: {e}")
}
} else {
self.policies = new_policies;
info!("Reloaded policy set")
}
}
};
Ok(AppResponse::Unit(()))
}

fn add_share(&mut self, r: AddShare) -> Result<AppResponse> {
self.is_authorized(&r.uid, &*ACTION_EDIT_SHARE, &r.list)?;
let list = self.entities.get_list(&r.list)?;
let team_uid = list.get_team(r.role).clone();
let target_entity = self.entities.get_user_or_team_mut(&r.share_with)?;
target_entity.insert_parent(team_uid);
// Confirm that the identified list and sharer are known
let _list = self.entities.get_list(&r.list)?;
let _target_entity = self.entities.get_user_or_team_mut(&r.share_with)?;
// Link a template to register the new permission
let (tid, pid_prefix) = match r.role {
ShareRole::Reader => (PolicyId::from_str("reader-template")?, "reader"),
ShareRole::Editor => (PolicyId::from_str("editor-template")?, "editor"),
};
// Construct template linking values
let target_euid: &cedar_policy::EntityUid = r.share_with.as_ref();
let list_euid: &cedar_policy::EntityUid = r.list.as_ref();
let env: HashMap<SlotId, cedar_policy::EntityUid> = [
(SlotId::principal(), target_euid.clone()),
(SlotId::resource(), list_euid.clone()),
]
.into_iter()
.collect();
// Construct policy ID; assumes no policy in the set has it already
let target_eid = target_euid.id();
let list_eid = list_euid.id();
let pid = PolicyId::from_str(&format!("{pid_prefix}[{target_eid}][{list_eid}]"))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use a UUID here

Copy link
Contributor Author

@mwhicks1 mwhicks1 Dec 19, 2023

Choose a reason for hiding this comment

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

I think it is. The list ID is under the app's control, so the last part of the ID is always [N] where N is a number. The first part always reader[ or editor[. The stuff in the middle is always the target user or team's ID with a trailing ]. Is this reasoning flawed? If not, IDs are sure to be an injective function of the three inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment that makes this clear.

// Link it!
self.policies.link(tid, pid.clone(), env)?;
info!("Created policy {pid}");
Ok(AppResponse::Unit(()))
}

fn delete_share(&mut self, r: DeleteShare) -> Result<AppResponse> {
self.is_authorized(&r.uid, &*ACTION_EDIT_SHARE, &r.list)?;
let list = self.entities.get_list(&r.list)?;
let team_uid = list.get_team(r.role).clone();
let target_entity = self.entities.get_user_or_team_mut(&r.unshare_with)?;
target_entity.delete_parent(&team_uid);
// Confirm that the identified list and un-sharer are known
let _list = self.entities.get_list(&r.list)?;
let _target_entity = self.entities.get_user_or_team_mut(&r.unshare_with)?;
// Unlink the policy that provided the permission
let pid_prefix = match r.role {
ShareRole::Reader => "reader",
ShareRole::Editor => "editor",
};
let target_eid = r.unshare_with.as_ref().id();
let list_eid = r.list.as_ref().id();
let pid = PolicyId::from_str(&format!("{pid_prefix}[{target_eid}][{list_eid}]"))?;
self.policies.unlink(pid)?;
Ok(AppResponse::Unit(()))
}

Expand Down Expand Up @@ -431,7 +506,7 @@ impl AppContext {
.entities
.fresh_euid::<ListUid>(TYPE_LIST.clone())
.unwrap();
let l = List::new(&mut self.entities, euid.clone(), r.uid, r.name);
let l = List::new(euid.clone(), r.uid, r.name);
self.entities.insert_list(l);

Ok(AppResponse::euid(euid))
Expand Down
1 change: 1 addition & 0 deletions tinytodo/src/entitystore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ pub struct EntityStore {
uid: usize,
}

#[allow(dead_code)]
impl EntityStore {
pub fn euids(&self) -> impl Iterator<Item = &EntityUid> {
self.users
Expand Down
32 changes: 3 additions & 29 deletions tinytodo/src/objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ use cedar_policy::{Entity, EvalResult, RestrictedExpression};
use serde::{Deserialize, Serialize};

use crate::{
api::ShareRole,
context::APPLICATION_TINY_TODO,
entitystore::{EntityDecodeError, EntityStore},
util::{EntityUid, ListUid, TeamUid, UserUid, TYPE_TEAM},
entitystore::EntityDecodeError,
util::{EntityUid, ListUid, TeamUid, UserUid},
};

#[derive(Debug, Clone, Deserialize, Serialize)]
Expand Down Expand Up @@ -142,25 +141,15 @@ pub struct List {
owner: UserUid,
name: String,
tasks: Vec<Task>, // Invariant, `tasks` must be sorted
readers: TeamUid,
editors: TeamUid,
}

impl List {
pub fn new(store: &mut EntityStore, uid: ListUid, owner: UserUid, name: String) -> Self {
let readers_uid = store.fresh_euid::<TeamUid>(TYPE_TEAM.clone()).unwrap();
let readers = Team::new(readers_uid.clone());
let writers_uid = store.fresh_euid::<TeamUid>(TYPE_TEAM.clone()).unwrap();
let writers = Team::new(writers_uid.clone());
store.insert_team(readers);
store.insert_team(writers);
pub fn new(uid: ListUid, owner: UserUid, name: String) -> Self {
Self {
uid,
owner,
name,
tasks: vec![],
readers: readers_uid,
editors: writers_uid,
}
}

Expand Down Expand Up @@ -192,13 +181,6 @@ impl List {
pub fn update_name(&mut self, name: String) {
self.name = name;
}

pub fn get_team(&self, role: ShareRole) -> &TeamUid {
match role {
ShareRole::Reader => &self.readers,
ShareRole::Editor => &self.editors,
}
}
}

impl From<List> for Entity {
Expand All @@ -213,14 +195,6 @@ impl From<List> for Entity {
"tasks",
RestrictedExpression::new_set(value.tasks.into_iter().map(|t| t.into())),
),
(
"readers",
format!("{}", value.readers.as_ref()).parse().unwrap(),
),
(
"editors",
format!("{}", value.editors.as_ref()).parse().unwrap(),
),
]
.into_iter()
.map(|(x, v)| (x.into(), v))
Expand Down
8 changes: 0 additions & 8 deletions tinytodo/tinytodo.cedarschema.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,6 @@
"name": {
"type": "String"
},
"readers": {
"type": "Entity",
"name": "Team"
},
"editors": {
"type": "Entity",
"name": "Team"
},
"tasks": {
"type": "Set",
"element": {
Expand Down
2 changes: 1 addition & 1 deletion tinytodo/tinytodo.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def process_response(name, resp, f, args):
tup = (current_user, name, args)
print('Access denied. User %s is not authorized to %s on [%s]' % tup )
else:
print('Error: %s' % resp['error'])
print('Error: %s' % body['error'])
else:
print(f(body))
else:
Expand Down
Loading