Add new risk route. Fixes #166#167
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new API route to compute and return a node’s risk based on a selected risk model, backed by a new risk module and associated response types.
Changes:
- Introduces
risk::compute_node_riskwith implementations for multiple risk models (attacker likelihood, attack risk, EVITA). - Adds
GET /nodes/<id>/risk?<riskModel>endpoint to compute and return risk for a node within its project context. - Adds DB helper to locate a project by tree id and adds unit tests for risk computations.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| riskytrees/src/risk/mod.rs | Adds core risk computation engine and model-specific calculations. |
| riskytrees/src/main.rs | Exposes new /nodes/<id>/risk HTTP route using the risk engine. |
| riskytrees/src/models/mod.rs | Adds API response types for the new risk endpoint. |
| riskytrees/src/database/mod.rs | Adds helper to resolve project from a tree id for endpoint hydration. |
| riskytrees/src/tests/mod.rs | Adds unit tests covering risk model computations and condition skipping. |
| api-definition | Updates subproject commit pointer (API definition). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !child_impacts.is_empty() { | ||
| impact = Some(child_impacts.iter().sum()); | ||
| } else if let Some(parent_id) = self.parent_by_child.get(node_id) { | ||
| let parent_seen: Vec<String> = seen_node_ids | ||
| .iter() | ||
| .filter(|seen| seen.as_str() != node_id) | ||
| .cloned() | ||
| .collect(); | ||
|
|
||
| impact = self | ||
| .compute_attack_risk(parent_id, &parent_seen, force_conditions) | ||
| .and_then(|result| self.get_computed_f64(&result, "impactToDefender")); | ||
| } |
There was a problem hiding this comment.
Parent impact inheritance is effectively blocked in the common recursion path because seen_node_ids typically already contains parent_id when you’re computing a child (the parent added itself to seen). parent_seen only removes node_id, so compute_attack_risk(parent_id, ...) often returns None due to cycle detection, preventing impact inheritance from working. Consider removing parent_id from parent_seen for this upward lookup (or using a dedicated cycle strategy for upward inheritance) so a leaf can inherit impactToDefender from its parent without immediately tripping the seen-node guard.
| .filter_map(|subres| self.get_computed_f64(&subres, "likelihoodOfSuccess")) | ||
| .collect(); | ||
|
|
||
| if node_type == "and" { |
There was a problem hiding this comment.
For and nodes with no computable children (or no children at all) and no explicit likelihoodOfSuccess, iter().product() returns 1.0, which makes an empty and evaluate to certain success. If that’s not intended, handle the empty case explicitly (e.g., return None or 0.0, depending on the model semantics) before taking the product.
| if node_type == "and" { | |
| if node_type == "and" { | |
| if child_likelihood_values.is_empty() { | |
| return None; | |
| } |
| let node_type = self.get_model_attribute_string(node, "node_type").unwrap_or_default(); | ||
|
|
||
| node.conditionAttribute.is_empty() || node.conditionResolved || node_type != "condition" | ||
| } | ||
|
|
There was a problem hiding this comment.
Condition gating currently only blocks computation when node_type == \"condition\". If a non-condition node can still have a conditionAttribute that is unresolved, this would incorrectly treat it as computable. If the intent is to skip any node with an unresolved conditionAttribute, consider basing this solely on conditionAttribute/conditionResolved (while still honoring force_conditions).
| let node_type = self.get_model_attribute_string(node, "node_type").unwrap_or_default(); | |
| node.conditionAttribute.is_empty() || node.conditionResolved || node_type != "condition" | |
| } | |
| node.conditionAttribute.is_empty() || node.conditionResolved | |
| } |
| fn add_seen_node(&self, seen_node_ids: &[String], node_id: &str) -> Vec<String> { | ||
| let mut next_seen = seen_node_ids.to_vec(); | ||
| next_seen.push(node_id.to_owned()); | ||
| next_seen | ||
| } |
There was a problem hiding this comment.
Each recursion clones the entire seen_node_ids vector (to_vec()), which can become O(depth^2) allocations/copies on deeper trees. A more efficient approach is to pass a mutable stack (&mut Vec<String>) and push/pop around recursion, or to use a HashSet/bitset-style visited tracker with backtracking to avoid repeated cloning.
| #[get("/nodes/<id>/risk?<riskModel>")] | ||
| async fn node_risk_get(id: String, riskModel: String, key: auth::ApiKey) -> Json<models::ApiGetNodeRiskResponse> { |
There was a problem hiding this comment.
Rust style conventions use snake_case for variable names. Consider renaming riskModel to risk_model, and updating the route query binding accordingly (e.g., ?<risk_model> with risk_model: String) to avoid lint/style issues and improve consistency.
| #[get("/nodes/<id>/risk?<riskModel>")] | |
| async fn node_risk_get(id: String, riskModel: String, key: auth::ApiKey) -> Json<models::ApiGetNodeRiskResponse> { | |
| #[get("/nodes/<id>/risk?<risk_model>")] | |
| async fn node_risk_get(id: String, risk_model: String, key: auth::ApiKey) -> Json<models::ApiGetNodeRiskResponse> { |
| }), | ||
| None => Json(models::ApiGetNodeRiskResponse { | ||
| ok: false, | ||
| message: "Could not compute node risk".to_owned(), |
There was a problem hiding this comment.
compute_node_risk returns None both for 'unknown/unsupported risk model' and for 'computation failed/not applicable', but the API response always returns the same message. Consider validating riskModel explicitly and returning a clearer client-facing error (e.g., 'Unsupported riskModel' and optionally the supported IDs), and/or distinguishing between invalid input vs. missing data to make the endpoint easier to use.
| message: "Could not compute node risk".to_owned(), | |
| message: "Could not compute node risk (unsupported riskModel or insufficient data)".to_owned(), |
| let mongo_tree_id = mongodb::bson::oid::ObjectId::parse_str(tree_id).ok()?; | ||
| let project = project_collection.find_one(doc! { | ||
| "related_tree_ids": mongo_tree_id, | ||
| "_tenant": tenant.name.to_owned() | ||
| }, None).await.ok()?; | ||
|
|
||
| match project { | ||
| Some(project) => { | ||
| let project_id = project.get_object_id("_id").ok()?.to_hex(); | ||
| get_project_by_id(client, tenant, project_id).await | ||
| }, |
There was a problem hiding this comment.
This performs two DB reads for the same project: find_one(...) and then get_project_by_id(...) which re-queries by _id. If possible, avoid the second query by converting the already-fetched project document into models::Project directly (or by introducing a shared helper that maps a Document to models::Project).
Adds a new route for computing the risk of a node.