From e3cdccc331762f585bfc3fd980b975ad8e451495 Mon Sep 17 00:00:00 2001 From: gtema Date: Tue, 26 May 2026 14:54:23 +0200 Subject: [PATCH] feat: Expand role info in `expand_implied_roles` Sometimes it is necessary to get all effective roles knowing only the list of original role_ids. Update `expand_implied_roles` method to lookup for the role information when `name` attribute is not set (the name is not nullable in the database, so it is a perfect candidate for detecting). --- .../core-types/src/assignment/assignment.rs | 12 +- crates/core-types/src/role/role.rs | 47 +-- crates/core/src/role/backend.rs | 22 +- crates/core/src/role/provider_api.rs | 22 +- crates/role-sql/src/implied_role.rs | 32 +- crates/role-sql/src/lib.rs | 290 ++++++++++++++++-- 6 files changed, 338 insertions(+), 87 deletions(-) diff --git a/crates/core-types/src/assignment/assignment.rs b/crates/core-types/src/assignment/assignment.rs index 4ae3819b..a6a290b9 100644 --- a/crates/core-types/src/assignment/assignment.rs +++ b/crates/core-types/src/assignment/assignment.rs @@ -225,6 +225,7 @@ pub struct RoleAssignmentListParameters { #[validate(length(max = 64))] pub role_id: Option, + // Actors /// Get role assignments for the user. #[builder(default)] #[validate(length(max = 64))] @@ -235,14 +236,17 @@ pub struct RoleAssignmentListParameters { #[validate(length(max = 64))] pub group_id: Option, - /// Query role assignments on the project. - #[builder(default)] - #[validate(length(max = 64))] - pub project_id: Option, + // Targets /// Query role assignments on the domain. #[builder(default)] #[validate(length(max = 64))] pub domain_id: Option, + + /// Query role assignments on the project. + #[builder(default)] + #[validate(length(max = 64))] + pub project_id: Option, + /// Query role assignments on the system. #[builder(default)] #[validate(length(max = 64))] diff --git a/crates/core-types/src/role/role.rs b/crates/core-types/src/role/role.rs index 498ef035..14d4d790 100644 --- a/crates/core-types/src/role/role.rs +++ b/crates/core-types/src/role/role.rs @@ -25,24 +25,28 @@ use crate::error::BuilderError; #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct Role { - /// The role ID. - #[validate(length(min = 1, max = 64))] - pub id: String, - /// The role name. + /// The role description. + #[builder(default)] #[validate(length(min = 1, max = 255))] - pub name: String, + pub description: Option, + /// The role domain_id. #[builder(default)] #[validate(length(min = 1, max = 64))] pub domain_id: Option, - /// The role description. - #[builder(default)] - #[validate(length(min = 1, max = 255))] - pub description: Option, + /// Additional role properties. #[builder(default)] #[serde(flatten)] pub extra: HashMap, + + /// The role ID. + #[validate(length(min = 1, max = 64))] + pub id: String, + + /// The role name. + #[validate(length(min = 1, max = 255))] + pub name: String, } /// Short role representation (reference). @@ -95,6 +99,7 @@ pub struct RoleListParameters { #[builder(default)] #[validate(length(min = 1, max = 64))] pub domain_id: Option>, + /// Filter roles by the name attribute. #[builder(default)] #[validate(length(min = 1, max = 255))] @@ -106,22 +111,26 @@ pub struct RoleListParameters { #[builder(build_fn(error = "BuilderError"))] #[builder(setter(strip_option, into))] pub struct RoleCreate { - /// The role ID. + /// The role description. #[builder(default)] - #[validate(length(min = 1, max = 64))] - pub id: Option, - /// The role name. - #[validate(length(min = 1, max = 255))] - pub name: String, + #[validate(length(max = 255))] + pub description: Option, + /// The role domain_id. #[builder(default)] #[validate(length(min = 1, max = 64))] pub domain_id: Option, - /// The role description. - #[builder(default)] - #[validate(length(max = 255))] - pub description: Option, + /// Additional role properties. #[builder(default)] pub extra: HashMap, + + /// The role ID. + #[builder(default)] + #[validate(length(min = 1, max = 64))] + pub id: Option, + + /// The role name. + #[validate(length(min = 1, max = 255))] + pub name: String, } diff --git a/crates/core/src/role/backend.rs b/crates/core/src/role/backend.rs index d37c1a7f..f0f46414 100644 --- a/crates/core/src/role/backend.rs +++ b/crates/core/src/role/backend.rs @@ -45,6 +45,17 @@ pub trait RoleBackend: Send + Sync { id: &'a str, ) -> Result<(), RoleProviderError>; + /// Expand implied roles. + /// + /// # Arguments + /// * `state` - The current service state. + /// * `roles` - The list of roles to expand. + async fn expand_implied_roles( + &self, + state: &ServiceState, + roles: &mut Vec, + ) -> Result<(), RoleProviderError>; + /// Get single role by ID. /// /// * `state` - The current service state. @@ -58,17 +69,6 @@ pub trait RoleBackend: Send + Sync { id: &'a str, ) -> Result, RoleProviderError>; - /// Expand implied roles. - /// - /// # Arguments - /// * `state` - The current service state. - /// * `roles` - The list of roles to expand. - async fn expand_implied_roles( - &self, - state: &ServiceState, - roles: &mut Vec, - ) -> Result<(), RoleProviderError>; - /// List role imply rules. /// /// # Arguments diff --git a/crates/core/src/role/provider_api.rs b/crates/core/src/role/provider_api.rs index e0450ee6..4fde34d1 100644 --- a/crates/core/src/role/provider_api.rs +++ b/crates/core/src/role/provider_api.rs @@ -48,6 +48,17 @@ pub trait RoleApi: Send + Sync { id: &'a str, ) -> Result<(), RoleProviderError>; + /// Expand implied roles. + /// + /// # Arguments + /// * `state` - The current service state. + /// * `roles` - The list of roles to expand. + async fn expand_implied_roles( + &self, + state: &ServiceState, + roles: &mut Vec, + ) -> Result<(), RoleProviderError>; + /// Get a single role. /// /// * `state` - The current service state. @@ -61,17 +72,6 @@ pub trait RoleApi: Send + Sync { role_id: &'a str, ) -> Result, RoleProviderError>; - /// Expand implied roles. - /// - /// # Arguments - /// * `state` - The current service state. - /// * `roles` - The list of roles to expand. - async fn expand_implied_roles( - &self, - state: &ServiceState, - roles: &mut Vec, - ) -> Result<(), RoleProviderError>; - /// List role imply rules. /// /// # Arguments diff --git a/crates/role-sql/src/implied_role.rs b/crates/role-sql/src/implied_role.rs index 4b16bf47..1a405c01 100644 --- a/crates/role-sql/src/implied_role.rs +++ b/crates/role-sql/src/implied_role.rs @@ -97,7 +97,7 @@ pub async fn list_rules( } #[cfg(test)] -mod tests { +pub(crate) mod tests { use sea_orm::{DatabaseBackend, MockDatabase, Transaction}; use crate::entity::implied_role; @@ -144,10 +144,10 @@ mod tests { ); } - fn get_implied_role_mock(id: String, implied_id: String) -> implied_role::Model { + pub fn get_implied_role_mock>(id: S, implied_id: S) -> implied_role::Model { implied_role::Model { - prior_role_id: id.clone(), - implied_role_id: implied_id.clone(), + prior_role_id: id.into(), + implied_role_id: implied_id.into(), } } @@ -155,20 +155,20 @@ mod tests { async fn test_list_rules() { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![ - get_implied_role_mock("1".into(), "2".into()), - get_implied_role_mock("1".into(), "3".into()), - get_implied_role_mock("2".into(), "4".into()), - get_implied_role_mock("4".into(), "7".into()), - get_implied_role_mock("4".into(), "8".into()), - get_implied_role_mock("5".into(), "6".into()), + get_implied_role_mock("1", "2"), + get_implied_role_mock("1", "3"), + get_implied_role_mock("2", "4"), + get_implied_role_mock("4", "7"), + get_implied_role_mock("4", "8"), + get_implied_role_mock("5", "6"), ]]) .append_query_results([vec![ - get_implied_role_mock("1".into(), "2".into()), - get_implied_role_mock("1".into(), "3".into()), - get_implied_role_mock("2".into(), "4".into()), - get_implied_role_mock("4".into(), "7".into()), - get_implied_role_mock("4".into(), "8".into()), - get_implied_role_mock("5".into(), "6".into()), + get_implied_role_mock("1", "2"), + get_implied_role_mock("1", "3"), + get_implied_role_mock("2", "4"), + get_implied_role_mock("4", "7"), + get_implied_role_mock("4", "8"), + get_implied_role_mock("5", "6"), ]]) .into_connection(); assert_eq!( diff --git a/crates/role-sql/src/lib.rs b/crates/role-sql/src/lib.rs index e119a4cf..635e1b08 100644 --- a/crates/role-sql/src/lib.rs +++ b/crates/role-sql/src/lib.rs @@ -39,6 +39,57 @@ inventory::submit! { SqlDriverRegistration { driver: &PLUGIN } } +/// Expand implied roles by resolving role inheritance and populating +/// missing role metadata. +/// +/// # Parameters +/// - `db`: The database connection. +/// - `roles`: The list of roles to expand. +/// +/// # Returns +/// A `Result` indicating success or an `Error`. +pub async fn expand_implied_roles( + db: &DatabaseConnection, + roles: &mut Vec, +) -> Result<(), RoleProviderError> { + let rules = implied_role::list_rules(db, true).await?; + let mut role_ids: HashSet = + HashSet::from_iter(roles.iter().map(|role| role.id.clone())); + let mut implied_roles: Vec = Vec::new(); + // iterate over all implied role ids for every role in the initial list + for implied_role_id in roles + .iter() + .filter_map(|role| rules.get(&role.id)) + .flat_map(|val| val.iter()) + { + // Add the role that was not processed yet (present in the `role_ids` into the + // temporary list and save the processed id. + if !role_ids.contains(implied_role_id) { + implied_roles.push( + role::get(db, implied_role_id) + .await? + .ok_or(RoleProviderError::RoleNotFound(implied_role_id.clone()))? + .into(), + ); + role_ids.insert(implied_role_id.clone()); + } + } + roles.extend(implied_roles); + // The request list may only contain role IDs. In the response we need to make + // sure name and domain_id are populated. + for role in roles.iter_mut() { + if role.name.is_none() { + // The role was not resolved and only has the ID. Re-fetch it + let full_role = role::get(db, &role.id) + .await? + .ok_or(RoleProviderError::RoleNotFound(role.id.clone()))?; + role.name = Some(full_role.name.clone()); + role.domain_id = full_role.domain_id; + } + } + Ok(()) +} + #[async_trait] impl RoleBackend for SqlBackend { /// Create role. @@ -108,30 +159,7 @@ impl RoleBackend for SqlBackend { state: &ServiceState, roles: &mut Vec, ) -> Result<(), RoleProviderError> { - let rules = implied_role::list_rules(&state.db, true).await?; - let mut role_ids: HashSet = - HashSet::from_iter(roles.iter().map(|role| role.id.clone())); - let mut implied_roles: Vec = Vec::new(); - // iterate over all implied role ids for every role in the initial list - for implied_role_id in roles - .iter_mut() - .filter_map(|role| rules.get(&role.id)) - .flat_map(|val| val.iter()) - { - // Add the role that was not processed yet (present in the `role_ids` into the - // temporary list and save the processed id. - if !role_ids.contains(implied_role_id) { - implied_roles.push( - self.get_role(state, implied_role_id) - .await? - .ok_or(RoleProviderError::RoleNotFound(implied_role_id.clone()))? - .into(), - ); - role_ids.insert(implied_role_id.clone()); - } - } - roles.extend(implied_roles); - Ok(()) + expand_implied_roles(&state.db, roles).await } /// List role imply rules. @@ -165,8 +193,9 @@ impl RoleBackend for SqlBackend { state: &ServiceState, params: &RoleListParameters, ) -> Result, RoleProviderError> { - // TODO: Add possibility to list roles with expansion and filter (e.g., token_restriction - // has list of roles that need to be returned resolved) + // TODO: Add possibility to list roles with expansion and filter (e.g., + // token_restriction has list of roles that need to be returned + // resolved) Ok(role::list(&state.db, params).await?) } } @@ -192,3 +221,212 @@ impl SqlDriver for SqlBackend { Ok(()) } } + +#[cfg(test)] +mod tests { + use sea_orm::{DatabaseBackend, MockDatabase}; + + use crate::entity::implied_role; + use crate::implied_role::tests::get_implied_role_mock; + use crate::role::tests::get_role_mock; + + use super::*; + use openstack_keystone_core_types::role::RoleRefBuilder; + + fn mock_role_with_domain(id: &str, name: &str, domain: &str) -> crate::entity::role::Model { + crate::entity::role::Model { + id: id.into(), + name: name.into(), + extra: None, + domain_id: domain.into(), + description: None, + } + } + + #[tokio::test] + async fn test_expand_no_implies_no_domain_populates_name() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .append_query_results([vec![get_role_mock("r1", "admin")]]) + .into_connection(); + + let mut roles = vec![RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 1); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[0].domain_id.as_deref(), Some("foo_domain")); + } + + #[tokio::test] + async fn test_expand_adds_implied_roles_and_resolves_names() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![get_implied_role_mock("r1", "r2")]]) + .append_query_results([vec![mock_role_with_domain("r2", "reader", "d1")]]) + .append_query_results([vec![get_role_mock("r1", "admin")]]) + .into_connection(); + + let mut roles = vec![RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 2); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[0].domain_id.as_deref(), Some("foo_domain")); + assert_eq!(roles[1].id, "r2"); + assert_eq!(roles[1].name.as_deref(), Some("reader")); + assert_eq!(roles[1].domain_id.as_deref(), Some("d1")); + } + + #[tokio::test] + async fn test_expand_recursive_implied_roles_no_duplicates() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![ + get_implied_role_mock("r1", "r2"), + get_implied_role_mock("r2", "r3"), + ]]) + .append_query_results([vec![mock_role_with_domain("r2", "member", "d1")]]) + .append_query_results([vec![mock_role_with_domain("r3", "reader", "d1")]]) + .append_query_results([vec![get_role_mock("r1", "admin")]]) + .into_connection(); + + let mut roles = vec![RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 3); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[1].id, "r2"); + assert_eq!(roles[1].name.as_deref(), Some("member")); + assert_eq!(roles[2].id, "r3"); + assert_eq!(roles[2].name.as_deref(), Some("reader")); + } + + #[tokio::test] + async fn test_expand_preserves_existing_name() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .into_connection(); + + let mut roles = vec![ + RoleRefBuilder::default() + .id("r1") + .name("admin") + .domain_id("d1") + .build() + .unwrap(), + ]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 1); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[0].domain_id.as_deref(), Some("d1")); + } + + #[tokio::test] + async fn test_expand_empty_roles_list() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .into_connection(); + + let mut roles: Vec = vec![]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert!(roles.is_empty()); + } + + #[tokio::test] + async fn test_expand_implied_role_not_in_rules_skipped() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![get_implied_role_mock("r2", "r3")]]) + .append_query_results([vec![get_role_mock("r1", "admin")]]) + .into_connection(); + + let mut roles = vec![RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 1); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + } + + #[tokio::test] + async fn test_expand_global_role_has_no_domain_in_output() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .append_query_results([vec![mock_role_with_domain("r1", "admin", "<>")]]) + .into_connection(); + + let mut roles = vec![RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 1); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[0].domain_id, None); + } + + #[tokio::test] + async fn test_expand_multiple_roles_with_mixed_name_states() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![get_implied_role_mock("r1", "r3")]]) + .append_query_results([vec![mock_role_with_domain("r3", "viewer", "d1")]]) + .append_query_results([vec![get_role_mock("r1", "admin")]]) + .into_connection(); + + let mut roles = vec![ + RoleRef { + id: "r1".into(), + name: None, + domain_id: None, + }, + RoleRefBuilder::default() + .id("r2") + .name("member") + .domain_id("d1") + .build() + .unwrap(), + ]; + + expand_implied_roles(&db, &mut roles).await.unwrap(); + + assert_eq!(roles.len(), 3); + assert_eq!(roles[0].id, "r1"); + assert_eq!(roles[0].name.as_deref(), Some("admin")); + assert_eq!(roles[0].domain_id.as_deref(), Some("foo_domain")); + assert_eq!(roles[1].id, "r2"); + assert_eq!(roles[1].name.as_deref(), Some("member")); + assert_eq!(roles[1].domain_id.as_deref(), Some("d1")); + assert_eq!(roles[2].id, "r3"); + assert_eq!(roles[2].name.as_deref(), Some("viewer")); + assert_eq!(roles[2].domain_id.as_deref(), Some("d1")); + } +}