Update TinyTodo to use templates #82
Conversation
tinytodo/src/context.rs
Outdated
| // 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}]"))?; |
There was a problem hiding this comment.
Should use a UUID here
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll add a comment that makes this clear.
| ``` | ||
| The first line tries to authorize the request; for our example, it checks that `User::"andrew"` can perform `Action::"EditShare"` on resource `List::"0"`. The `is_authorized` call returns `Ok()` because *policy 1* evaluates to `true`, since `List::"0".owner == User::"andrew"` (see Figure 3 in the original tutorial). The call to `get_list` then retrieves the native `List` object for `List::"0"` from `self.entities`, our `EntityStore`. The next call `get_user_or_team_mut` gets the `User` or `Team` to share the list with, which in this case is `Team::"interns"`. If either call fails then the target objects don't exist in the entity store and an error is sent back. | ||
|
|
||
| Next, we construct the name of the template we will link: if `r.role` is `Reader`, as in our example, the template ID is `reader-template`. Now we construct the entity UIDs to link `?principal` and `?resource` against, and store them in `env`, a hashmap. Then we call the function `linked_policy_id` (not shown) to construct the policy ID from the sharing role, the target user/team, and the list. This function is injective, so we can be sure no two linked policies will have the same ID. In the case of our example, a policy ID is `reader[interns][0]`. Finally, we link the policy into the policy store. |
There was a problem hiding this comment.
This function is injective, so we can be sure no two linked policies will have the same ID.
This feels like a bit of a flaw in our template linking API. This shouldn't be the callers responsibility.
There was a problem hiding this comment.
I agree that it should not be a requirement, but it currently is.
As a concrete suggestion: How about adding an API that constructs the instance policy ID based on the template's ID and the IDs of its linked-in parameters, perhaps with some additional string prefix provided by the caller? E.g.,
templateID = reader
?principal = User::"Alice"
?resource = Photo::"vacation.jpg"
prefix = photo
yields linked policy ID
photo|reader|User::"Alice"|Photo::"vacation.jpg"
?
There was a problem hiding this comment.
After writing this up as an issue, I'm not sure if it's better than the status quo: cedar-policy/cedar#547
tinytodo/src/context.rs
Outdated
| Some(_) => { | ||
| // found template in new policy set | ||
| match p.template_links() { | ||
| None => error!("Error when reloading policies: Template with no matching links"), |
There was a problem hiding this comment.
Should this set err = true;?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
cedar-policy/cedar#548 -- I'm of a mixed mind about whether doing this is worth it.
Description of changes: Updated TinyTodo to use templates, rather than groups in attributes.
My plan is to make this a new directory, or a feature flag, rather than change the code. But I submit it as a PR this way so you can easily see the diffs.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.