-
Notifications
You must be signed in to change notification settings - Fork 95
H-4345: Prepare evaluation of policies when calling from API #6880
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
H-4345: Prepare evaluation of policies when calling from API #6880
Conversation
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.
Copilot reviewed 28 out of 29 changed files in this pull request and generated no comments.
Files not reviewed (1)
- libs/@local/graph/authorization/schemas/policies.cedarschema: Language not supported
Comments suppressed due to low confidence (1)
libs/@local/graph/postgres-store/src/permissions/mod.rs:1004
- The new 'register_action' implementation removes the explicit parent parameter and relies on 'action.parent()'. Please verify that actions (other than the root 'All') which do not explicitly provide a parent are correctly handled and that an error is raised when an action lacks a defined parent.
pub async fn register_action(&mut self, action: ActionName) -> Result<(), Report<ActionError>> {
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.
This PR successfully implements the groundwork for using Cedar policies for permission evaluation. The implementation creates a solid foundation by making several key changes:
- Action hierarchy modeling with proper parent-child relationships
- Improved context building for policy evaluation that includes teams, roles, and actors
- Refactoring of database queries to include more complete information about actors and their roles
- Introduction of a PrincipalStore trait that allows for the creation of webs with appropriate authorization checks
- Consistent schema between Cedar and the Rust implementation
The code is generally well-structured and follows good practices. I particularly appreciate:
- The use of iterators for parent-child relationships in ActionName
- The comprehensive database queries that collect all necessary information for policy evaluation
- The thoughtful trait designs that maintain backward compatibility while adding new functionality
- The Cedar schema validation test ensuring consistency between different parts of the system
I do see some areas for potential future optimization around the database access patterns in the context builder, but as mentioned in the PR description, this is intended to be a straightforward initial implementation that can be optimized later.
Overall, this is a solid implementation that achieves the goal of preparing the evaluation of policies when calling from the API layer.
} | ||
|
||
pub fn parents(self) -> impl Iterator<Item = Self> { | ||
iter::successors(self.parent(), |&action| action.parent()) |
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.
Nice implementation of the parent/child relationships for actions. Using iterators for the parents()
method is a clean approach. Consider adding a test case that explicitly tests the is_parent_of
and is_child_of
methods directly, as they're only indirectly tested through the action_ids()
test.
@@ -55,6 +55,10 @@ impl ContextBuilder { | |||
self.entities.push(role.to_cedar_entity()); | |||
} | |||
|
|||
pub fn add_team(&mut self, team: &Team) { |
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.
The Context implementation nicely centralizes the building of Cedar entities. Consider adding some documentation for the add_team
method explaining its purpose, similar to the other methods in this class.
id: UserId::new(ActorEntityUuid::new(row.get(0))), | ||
roles: HashSet::new(), | ||
.map(|row| { | ||
let role_ids = row.get::<_, Option<Vec<Uuid>>>(1).unwrap_or_default(); |
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.
It's good that you've updated all the query implementations to properly include the roles when fetching actors. One optimization to consider for the future would be to extract this common query pattern (which appears in get_user, get_machine, and get_ai with nearly identical logic) into a shared utility function to reduce code duplication.
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.
It's expected that the implementation will diverge and unifying the implementation would just add more work later.
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.
@TimDiekmann I understand your reasoning. If the implementations are expected to diverge, then keeping them separate makes sense to avoid refactoring work later. This approach gives you more flexibility to customize each actor type's query as needed in the future.
/// | ||
/// [`PrincipalNotFound`]: PrincipalError::PrincipalNotFound | ||
/// [`StoreError`]: PrincipalError::StoreError | ||
pub async fn build_principal_context( |
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.
The implementation of build_principal_context
is quite thorough in collecting all the necessary entities for a proper Cedar evaluation. Be careful with the potentially large number of database queries here - this might become a performance bottleneck. Consider adding a comment about future optimization possibilities, such as combining some of these queries or implementing caching strategies.
@@ -195,4 +227,24 @@ | |||
|
|||
Ok(()) | |||
} | |||
|
|||
#[test] |
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.
Your implementation of the Cedar schema validation test is valuable to ensure consistency between the Rust code and Cedar schema. However, consider adding a test comment explaining what the test is verifying specifically, as it might not be immediately obvious to other developers.
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.
This PR introduces a solid foundation for the new policy evaluation system by exposing the Cedar policy interface to the Graph API layer and implementing principal context creation. The changes are well-structured and the implementation is thorough.
The addition of parent-child relationships for actions using iterators is particularly elegant, and the thorough testing of this functionality (including validation against the Cedar schema) gives confidence in the implementation. This will make policy evaluation more flexible and maintainable.
The implementation of the context building functionality is comprehensive, ensuring all necessary entities are available for Cedar policy evaluation. While there are potentially some performance considerations when fetching all context data, this provides a solid starting point that can be optimized in the future.
I've suggested some minor improvements to the documentation and a couple of small cleanups for unused imports. These are relatively minor issues that don't impact the overall quality of the implementation.
/// | ||
/// This allows policies associated with the team to be considered during authorization, | ||
/// making the team available as a potential principal in the Cedar evaluation context. | ||
pub fn add_team(&mut self, team: &Team) { |
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.
Please add documentation for the add_team
method, similar to the style used for the other methods in this class. This would help maintain consistency in the documentation and make it clear what this method is intended for.
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.
This literally was added this commit!
@@ -103,14 +142,18 @@ | |||
#[expect(clippy::panic_in_result_fn, reason = "Assertions in test are expected")] | |||
mod tests { | |||
use core::error::Error; | |||
use std::collections::{HashMap, HashSet}; |
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.
This import of HashSet
is unused and can be removed.
@@ -9,7 +9,7 @@ | |||
|
|||
use super::{ | |||
PolicyValidator, | |||
principal::{Actor, actor::Machine, role::Role}, | |||
principal::{Actor, actor::Machine, role::Role, team::Team}, |
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.
The actor::Machine
import is unused after removing the add_machine
method and can be safely removed.
/// - Each action defined in the Cedar schema can be mapped to a Rust `ActionName` | ||
/// - The parent-child relationships defined by `action.parents()` match the hierarchical | ||
/// relationships in the Cedar schema (descendants) | ||
#[test] |
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.
Please add a more detailed doc comment that explains what this test is specifically verifying. It would be helpful to clarify that it's validating consistency between the Rust ActionName
enum hierarchy and the Cedar schema structure.
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.
This literally was added this commit!
/// - Combining some queries into a single more complex query | ||
/// - Implementing caching strategies for frequently accessed contexts | ||
/// - Prefetching contexts for related actors in batch operations | ||
pub async fn build_principal_context( |
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.
This method performs multiple database queries to build the context, which could become a performance bottleneck for frequent requests. Consider adding a comment about potential future optimizations such as:
- Combining some queries into a single more complex query
- Implementing caching strategies for frequently accessed contexts
- Prefetching contexts for related actors in batch operations
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.
This literally was added this commit!
50f9702
to
e0b4dc2
Compare
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
scaling_read_entity_linkless
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10000 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
representative_read_multiple_entities
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph |
scaling_read_entity_complete_one_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph |
🌟 What is the purpose of this PR?
To allow the usage of the new policies we need to bubble up the interface to the Graph API layer. Also, it has to be possible to create a principal context which can be used in Cedar.
🔍 What does this change?
PrincipalStore
PrincipalStore
up to therest
module (but with commented out functionality to not interfere with the current permission system)Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🛡 What tests cover this?
create_web
function signature