From 92a05ac0f7562722d85c6cc3678970abf2e8a046 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Fri, 2 Jan 2026 18:38:30 +0100 Subject: [PATCH] refactor: Improve conditional parallel tasks Refactor the code to better support conditional parallel tasks (role assignments and listing users). This allows making the code more flexible with fewer if-else conditions and code duplication. --- Cargo.toml | 1 + src/assignment/backend/sql/assignment/list.rs | 394 ++++++++++-------- src/identity/backends/sql/authenticate.rs | 40 +- src/identity/backends/sql/federated_user.rs | 16 + src/identity/backends/sql/local_user.rs | 6 +- src/identity/backends/sql/local_user/load.rs | 2 +- src/identity/backends/sql/nonlocal_user.rs | 13 + src/identity/backends/sql/user/list.rs | 161 +++++-- src/identity/backends/sql/user_option.rs | 24 +- 9 files changed, 426 insertions(+), 231 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c381e34f..89b22ced 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -46,6 +46,7 @@ derive_builder = { version = "0.20" } dyn-clone = { version = "1.0" } eyre = { version = "0.6" } fernet = { version = "0.2", default-features = false, features = ["rustcrypto"] } +# futures = { version = "0.3" } futures-util = { version = "0.3" } itertools = { version = "0.14" } mockall_double = { version = "0.3" } diff --git a/src/assignment/backend/sql/assignment/list.rs b/src/assignment/backend/sql/assignment/list.rs index 226a26cd..a33976f1 100644 --- a/src/assignment/backend/sql/assignment/list.rs +++ b/src/assignment/backend/sql/assignment/list.rs @@ -139,87 +139,24 @@ pub async fn list_for_multiple_actors_and_targets( db: &DatabaseConnection, params: &RoleAssignmentListForMultipleActorTargetParameters, ) -> Result, AssignmentDatabaseError> { - let mut select = DbAssignment::find(); - let mut select_system = DbSystemAssignment::find(); - let mut include_system = false; // Track if we should query role assignments table - let mut include_regular = false; // Track if we should query system assignments table - - if !params.actors.is_empty() { - select = select.filter(db_assignment::Column::ActorId.is_in(params.actors.clone())); - select_system = select_system - .filter(db_system_assignment::Column::ActorId.is_in(params.actors.clone())); - } - if let Some(rid) = ¶ms.role_id { - select = select.filter(db_assignment::Column::RoleId.eq(rid)); - select_system = select_system.filter(db_system_assignment::Column::RoleId.eq(rid)); - } - if !params.targets.is_empty() { - let mut cond = Condition::any(); - let mut system_cond = Condition::any(); - for target in params.targets.iter() { - match target.r#type { - RoleAssignmentTargetType::Domain | RoleAssignmentTargetType::Project => { - cond = cond.add( - Condition::all() - .add(db_assignment::Column::TargetId.eq(&target.id)) - .add_option( - target - .inherited - .map(|x| db_assignment::Column::Inherited.eq(x)), - ), - ); - include_regular = true; - } - RoleAssignmentTargetType::System => { - system_cond = system_cond.add( - Condition::all() - .add(db_system_assignment::Column::TargetId.eq(&target.id)) - .add_option( - target - .inherited - .map(|x| db_system_assignment::Column::Inherited.eq(x)), - ), - ); - include_system = true; - } - }; - } - select = select.filter(cond); - select_system = select_system.filter(system_cond); - } - - // Get all implied rules - let imply_rules = implied_role::list_rules(db, true).await?; - - // Query both tables in parallel (if needed) - let assignments: Vec = match (include_regular, include_system) { - (true, false) => select - .all(db) - .await - .context("fetching role assignments")? - .into_iter() - .map(Into::into) - .collect(), - (false, true) => select_system - .all(db) - .await - .context("fetching system role assignments")? - .into_iter() - .map(TryInto::::try_into) - .collect::, _>>()?, - (false, false) | (true, true) => { - let (a, b) = tokio::join!(select.all(db), select_system.all(db)); - a.context("fetching role assignments")? - .into_iter() - .map(|val| Ok(Into::into(val))) - .chain( - b.context("fetching system role assignments")? - .into_iter() - .map(TryInto::::try_into), - ) - .collect::, _>>()? - } - }; + // Query both assignment tables in parallel and imply rules + let db_res = tokio::try_join!( + // Result assignments + list_for_multiple_actors_and_targets_regular(db, params), + // System assignments + list_for_multiple_actors_and_targets_system(db, params), + // Get all implied rules + implied_role::list_rules(db, true) + )?; + + let assignments: Vec = [db_res.0, db_res.1] + .into_iter() + // discard None + .flatten() + // convert iter of items to items themselves + .flatten() + .collect(); + let imply_rules = db_res.2; // Merge and apply role implications let mut result_map: BTreeMap = BTreeMap::new(); @@ -259,6 +196,118 @@ pub async fn list_for_multiple_actors_and_targets( Ok(result_map.into_values().collect()) } +/// Select regular assignments. +/// +/// Return Vec for the regular role assignments or `None` when no corresponding targets +/// were given in the query parameters. +async fn list_for_multiple_actors_and_targets_regular( + db: &DatabaseConnection, + params: &RoleAssignmentListForMultipleActorTargetParameters, +) -> Result>, AssignmentDatabaseError> { + let mut select = DbAssignment::find(); + let mut should_return = false; + + if !params.actors.is_empty() { + select = select.filter(db_assignment::Column::ActorId.is_in(params.actors.clone())); + } + if let Some(rid) = ¶ms.role_id { + select = select.filter(db_assignment::Column::RoleId.eq(rid)); + } + if !params.targets.is_empty() { + let mut cond = Condition::any(); + for target in params.targets.iter() { + match target.r#type { + RoleAssignmentTargetType::Domain | RoleAssignmentTargetType::Project => { + cond = cond.add( + Condition::all() + .add(db_assignment::Column::TargetId.eq(&target.id)) + .add_option( + target + .inherited + .map(|x| db_assignment::Column::Inherited.eq(x)), + ), + ); + should_return = true; + } + _ => {} + }; + } + select = select.filter(cond); + } else { + // When no targets requested we still query assignments. + should_return = true; + } + + if should_return { + Ok(Some( + select + .all(db) + .await + .context("fetching role assignments")? + .into_iter() + .map(Into::into) + .collect(), + )) + } else { + Ok(None) + } +} + +/// Select system assignments. +/// +/// Return Vec for the regular role assignments or `None` when no corresponding targets +/// were given in the query parameters. +async fn list_for_multiple_actors_and_targets_system( + db: &DatabaseConnection, + params: &RoleAssignmentListForMultipleActorTargetParameters, +) -> Result>, AssignmentDatabaseError> { + let mut select_system = DbSystemAssignment::find(); + let mut should_return = false; + + if !params.actors.is_empty() { + select_system = select_system + .filter(db_system_assignment::Column::ActorId.is_in(params.actors.clone())); + } + if let Some(rid) = ¶ms.role_id { + select_system = select_system.filter(db_system_assignment::Column::RoleId.eq(rid)); + } + if !params.targets.is_empty() { + let mut system_cond = Condition::any(); + for target in params.targets.iter() { + if let RoleAssignmentTargetType::System = target.r#type { + system_cond = system_cond.add( + Condition::all() + .add(db_system_assignment::Column::TargetId.eq(&target.id)) + .add_option( + target + .inherited + .map(|x| db_system_assignment::Column::Inherited.eq(x)), + ), + ); + should_return = true; + }; + } + select_system = select_system.filter(system_cond); + } else { + // When no targets requested we still query assignments. + should_return = true; + } + + if should_return { + Ok(Some( + select_system + .all(db) + .await + .context("fetching system role assignments")? + .into_iter() + .map(TryInto::::try_into) + .collect::, _>>()?, + )) + } else { + Ok(None) + } +} + #[cfg(test)] mod tests { use sea_orm::{DatabaseBackend, MockDatabase, Transaction}; @@ -477,14 +526,14 @@ mod tests { async fn test_list_for_multiple_actor_targets_multiple_actors_single_target() { // Create MockDatabase with mock query results let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![get_role_assignment_mock("1")]]) + .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![ get_role_mock("1", "rname"), get_role_mock("2", "rname2"), ]]) - .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![get_role_system_assignment_mock("1")]]) + .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![ get_role_mock("1", "rname"), get_role_mock("2", "rname2"), @@ -566,11 +615,6 @@ mod tests { assert_eq!( db.into_transaction_log(), [ - Transaction::from_sql_and_values( - DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, - [] - ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE "assignment"."actor_id" IN ($1, $2, $3) AND "assignment"."role_id" = $4 AND "assignment"."target_id" = $5"#, @@ -584,13 +628,13 @@ mod tests { ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "role"."id", "role"."name" FROM "role" WHERE "id" IN ($1, $2)"#, - ["1".into(), "2".into(),] + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + [] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, - [] + r#"SELECT "role"."id", "role"."name" FROM "role" WHERE "id" IN ($1, $2)"#, + ["1".into(), "2".into(),] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, @@ -603,6 +647,11 @@ mod tests { "system".into() ] ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + [] + ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT "role"."id", "role"."name" FROM "role" WHERE "id" IN ($1, $2)"#, @@ -615,10 +664,9 @@ mod tests { #[tokio::test] async fn test_list_for_multiple_actor_targets_multiple_complex_targets() { // Create MockDatabase with mock query results - let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![get_role_assignment_mock("1")]]) + .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![ get_role_mock("1", "rname"), get_role_mock("2", "rname2"), @@ -626,40 +674,33 @@ mod tests { .into_connection(); let config = Config::default(); // multiple actors multiple complex targets - assert!( - list_for_multiple_actors_and_targets( - &config, - &db, - &RoleAssignmentListForMultipleActorTargetParameters { - actors: vec!["uid1".into(), "gid1".into(), "gid2".into()], - targets: vec![ - RoleAssignmentTarget { - id: "pid1".into(), - r#type: RoleAssignmentTargetType::Project, - inherited: None - }, - RoleAssignmentTarget { - id: "pid2".into(), - r#type: RoleAssignmentTargetType::Project, - inherited: Some(true) - } - ], - role_id: None - } - ) - .await - .is_ok() - ); + list_for_multiple_actors_and_targets( + &config, + &db, + &RoleAssignmentListForMultipleActorTargetParameters { + actors: vec!["uid1".into(), "gid1".into(), "gid2".into()], + targets: vec![ + RoleAssignmentTarget { + id: "pid1".into(), + r#type: RoleAssignmentTargetType::Project, + inherited: None, + }, + RoleAssignmentTarget { + id: "pid2".into(), + r#type: RoleAssignmentTargetType::Project, + inherited: Some(true), + }, + ], + role_id: None, + }, + ) + .await + .unwrap(); // Checking transaction log assert_eq!( db.into_transaction_log(), [ - Transaction::from_sql_and_values( - DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, - [] - ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE "assignment"."actor_id" IN ($1, $2, $3) AND ("assignment"."target_id" = $4 OR ("assignment"."target_id" = $5 AND "assignment"."inherited" = $6))"#, @@ -672,6 +713,11 @@ mod tests { true.into() ] ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + [] + ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT "role"."id", "role"."name" FROM "role" WHERE "id" IN ($1, $2)"#, @@ -686,9 +732,9 @@ mod tests { // Create MockDatabase with mock query results let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![get_role_assignment_mock("1")]]) .append_query_results([vec![get_role_system_assignment_mock("2")]]) + .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![ get_role_mock("1", "rname"), get_role_mock("2", "rname2"), @@ -696,19 +742,20 @@ mod tests { .into_connection(); let config = Config::default(); //// empty actors and targets - assert!( - list_for_multiple_actors_and_targets( - &config, - &db, - &RoleAssignmentListForMultipleActorTargetParameters { - actors: vec![], - targets: vec![], - role_id: None - } - ) - .await - .is_ok() - ); + //assert!( + list_for_multiple_actors_and_targets( + &config, + &db, + &RoleAssignmentListForMultipleActorTargetParameters { + actors: vec![], + targets: vec![], + role_id: None, + }, + ) + .await + .unwrap(); + // .is_ok() + //); // Checking transaction log assert_eq!( @@ -716,17 +763,17 @@ mod tests { [ Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment""#, [] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment""#, + r#"SELECT "system_assignment"."type", "system_assignment"."actor_id", "system_assignment"."target_id", "system_assignment"."role_id", "system_assignment"."inherited" FROM "system_assignment""#, [] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "system_assignment"."type", "system_assignment"."actor_id", "system_assignment"."target_id", "system_assignment"."role_id", "system_assignment"."inherited" FROM "system_assignment""#, + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, [] ), Transaction::from_sql_and_values( @@ -743,8 +790,8 @@ mod tests { // Create MockDatabase with mock query results let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![get_role_assignment_mock("1")]]) + .append_query_results([get_implied_rules_mock()]) .append_query_results([vec![ get_role_mock("1", "rname"), get_role_mock("2", "rname2"), @@ -784,13 +831,13 @@ mod tests { [ Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, - [] + r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE "assignment"."target_id" = $1 OR ("assignment"."target_id" = $2 AND "assignment"."inherited" = $3)"#, + ["pid1".into(), "pid2".into(), true.into()] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE "assignment"."target_id" = $1 OR ("assignment"."target_id" = $2 AND "assignment"."inherited" = $3)"#, - ["pid1".into(), "pid2".into(), true.into()] + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + [] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, @@ -804,38 +851,35 @@ mod tests { #[tokio::test] async fn test_list_for_multiple_actor_targets_complex_targets() { // Create MockDatabase with mock query results - let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([get_implied_rules_mock()]) .append_query_results([Vec::::new()]) + .append_query_results([get_implied_rules_mock()]) .into_connection(); let config = Config::default(); //// only complex targets - assert!( - list_for_multiple_actors_and_targets( - &config, - &db, - &RoleAssignmentListForMultipleActorTargetParameters { - actors: vec![], - targets: vec![ - RoleAssignmentTarget { - id: "pid1".into(), - r#type: RoleAssignmentTargetType::Project, - inherited: Some(false) - }, - RoleAssignmentTarget { - id: "pid2".into(), - r#type: RoleAssignmentTargetType::Project, - inherited: Some(true) - } - ], - role_id: None - } - ) - .await - .is_ok() - ); + list_for_multiple_actors_and_targets( + &config, + &db, + &RoleAssignmentListForMultipleActorTargetParameters { + actors: vec![], + targets: vec![ + RoleAssignmentTarget { + id: "pid1".into(), + r#type: RoleAssignmentTargetType::Project, + inherited: Some(false), + }, + RoleAssignmentTarget { + id: "pid2".into(), + r#type: RoleAssignmentTargetType::Project, + inherited: Some(true), + }, + ], + role_id: None, + }, + ) + .await + .unwrap(); // Checking transaction log assert_eq!( @@ -843,13 +887,13 @@ mod tests { [ Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, - [] + r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE ("assignment"."target_id" = $1 AND "assignment"."inherited" = $2) OR ("assignment"."target_id" = $3 AND "assignment"."inherited" = $4)"#, + ["pid1".into(), false.into(), "pid2".into(), true.into()] ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT CAST("assignment"."type" AS "text"), "assignment"."actor_id", "assignment"."target_id", "assignment"."role_id", "assignment"."inherited" FROM "assignment" WHERE ("assignment"."target_id" = $1 AND "assignment"."inherited" = $2) OR ("assignment"."target_id" = $3 AND "assignment"."inherited" = $4)"#, - ["pid1".into(), false.into(), "pid2".into(), true.into()] + r#"SELECT "implied_role"."prior_role_id", "implied_role"."implied_role_id" FROM "implied_role""#, + [] ), ] ); diff --git a/src/identity/backends/sql/authenticate.rs b/src/identity/backends/sql/authenticate.rs index 25f487c6..d096059f 100644 --- a/src/identity/backends/sql/authenticate.rs +++ b/src/identity/backends/sql/authenticate.rs @@ -182,7 +182,7 @@ mod tests { let db = MockDatabase::new(DatabaseBackend::Postgres).into_connection(); let config = Config::default(); assert!( - !should_lock(&config, &db, &get_local_user_mock()) + !should_lock(&config, &db, &get_local_user_mock("user_id")) .await .unwrap(), "Default config does not request any validation and user is not considered locked" @@ -233,7 +233,7 @@ mod tests { #[tokio::test] async fn test_should_lock_no_failed_auth_at() { let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![get_local_user_mock()]]) + .append_query_results([vec![get_local_user_mock("user_id")]]) .into_connection(); let mut config = Config::default(); config.security_compliance.lockout_failure_attempts = Some(5); @@ -273,7 +273,7 @@ mod tests { #[tokio::test] async fn test_should_lock_expired() { let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![get_local_user_mock()]]) + .append_query_results([vec![get_local_user_mock("user_id")]]) .into_connection(); let mut config = Config::default(); config.security_compliance.lockout_failure_attempts = Some(5); @@ -378,7 +378,7 @@ mod tests { password_hash: String, ) -> (db_local_user::Model, db_password::Model) { ( - get_local_user_mock(), + get_local_user_mock("user_id"), db_password::ModelBuilder::default() .password_hash(password_hash) .build() @@ -397,6 +397,7 @@ mod tests { .unwrap(), )]]) .append_query_results([user_option::tests::get_user_options_mock( + "user_id", &UserOptions::default(), )]) .append_query_results([vec![user::tests::get_user_mock("user_id")]]) @@ -465,6 +466,7 @@ mod tests { .unwrap(), )]]) .append_query_results([user_option::tests::get_user_options_mock( + "user_id", &UserOptions::default(), )]) .into_connection(); @@ -515,10 +517,13 @@ mod tests { .build() .unwrap(), )]]) - .append_query_results([user_option::tests::get_user_options_mock(&UserOptions { - ignore_lockout_failure_attempts: Some(true), - ..Default::default() - })]) + .append_query_results([user_option::tests::get_user_options_mock( + "user_id", + &UserOptions { + ignore_lockout_failure_attempts: Some(true), + ..Default::default() + }, + )]) .append_query_results([vec![user::tests::get_user_mock("user_id")]]) .append_query_results([vec![user::tests::get_user_mock("user_id")]]) .into_connection(); @@ -544,13 +549,14 @@ mod tests { let config = Config::default(); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![( - get_local_user_mock(), + get_local_user_mock("user_id"), db_password::ModelBuilder::default() .password_hash("wrong_password") .build() .unwrap(), )]]) .append_query_results([user_option::tests::get_user_options_mock( + "user_id", &UserOptions::default(), )]) .into_connection(); @@ -582,7 +588,7 @@ mod tests { let password = String::from("foo_pass"); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![( - get_local_user_mock(), + get_local_user_mock("user_id"), db_password::ModelBuilder::default() .password_hash( password_hashing::hash_password(&config, &password) @@ -594,6 +600,7 @@ mod tests { .unwrap(), )]]) .append_query_results([user_option::tests::get_user_options_mock( + "user_id", &UserOptions::default(), )]) .into_connection(); @@ -626,7 +633,7 @@ mod tests { let password = String::from("foo_pass"); let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([vec![( - get_local_user_mock(), + get_local_user_mock("user_id"), db_password::ModelBuilder::expired() .password_hash( password_hashing::hash_password(&config, &password) @@ -636,10 +643,13 @@ mod tests { .build() .unwrap(), )]]) - .append_query_results([user_option::tests::get_user_options_mock(&UserOptions { - ignore_password_expiry: Some(true), - ..Default::default() - })]) + .append_query_results([user_option::tests::get_user_options_mock( + "user_id", + &UserOptions { + ignore_password_expiry: Some(true), + ..Default::default() + }, + )]) .append_query_results([vec![user::tests::get_user_mock("user_id")]]) .append_query_results([vec![user::tests::get_user_mock("user_id")]]) .into_connection(); diff --git a/src/identity/backends/sql/federated_user.rs b/src/identity/backends/sql/federated_user.rs index 8b1e377c..9200e0cf 100644 --- a/src/identity/backends/sql/federated_user.rs +++ b/src/identity/backends/sql/federated_user.rs @@ -48,3 +48,19 @@ impl UserResponseBuilder { self } } + +#[cfg(test)] +pub(crate) mod tests { + use crate::db::entity::federated_user as db_federated_user; + + pub fn get_federated_user_mock>(user_id: UID) -> db_federated_user::Model { + db_federated_user::Model { + id: 1, + user_id: user_id.into(), + idp_id: "idp_id".into(), + protocol_id: "protocol_id".into(), + unique_id: "uid".into(), + display_name: Some("foo".into()), + } + } +} diff --git a/src/identity/backends/sql/local_user.rs b/src/identity/backends/sql/local_user.rs index 131102c8..a42bdff4 100644 --- a/src/identity/backends/sql/local_user.rs +++ b/src/identity/backends/sql/local_user.rs @@ -32,15 +32,15 @@ impl UserResponseBuilder { } #[cfg(test)] -pub(super) mod tests { +pub(crate) mod tests { use chrono::Utc; use crate::db::entity::{local_user as db_local_user, password as db_password}; - pub fn get_local_user_mock() -> db_local_user::Model { + pub fn get_local_user_mock>(user_id: UID) -> db_local_user::Model { db_local_user::Model { id: 1, - user_id: "user_id".into(), + user_id: user_id.into(), domain_id: "foo_domain".into(), name: "foo_domain".into(), failed_auth_count: Some(0), diff --git a/src/identity/backends/sql/local_user/load.rs b/src/identity/backends/sql/local_user/load.rs index 72c07287..4e88796b 100644 --- a/src/identity/backends/sql/local_user/load.rs +++ b/src/identity/backends/sql/local_user/load.rs @@ -63,7 +63,7 @@ pub async fn load_local_user_with_passwords, S2: AsRef, S3: /// /// Returns vector of optional vectors with passwords in the same order as /// requested keeping None in place where local_user was empty. -pub async fn load_local_users_passwords>>( +pub async fn load_local_users_passwords> + std::fmt::Debug>( db: &DatabaseConnection, user_ids: L, ) -> Result>>, IdentityDatabaseError> { diff --git a/src/identity/backends/sql/nonlocal_user.rs b/src/identity/backends/sql/nonlocal_user.rs index eb600042..0d442409 100644 --- a/src/identity/backends/sql/nonlocal_user.rs +++ b/src/identity/backends/sql/nonlocal_user.rs @@ -21,3 +21,16 @@ impl UserResponseBuilder { self } } + +#[cfg(test)] +pub(crate) mod tests { + use crate::db::entity::nonlocal_user as db_nonlocal_user; + + pub fn get_nonlocal_user_mock>(user_id: UID) -> db_nonlocal_user::Model { + db_nonlocal_user::Model { + user_id: user_id.into(), + domain_id: "foo_domain".into(), + name: "foo".into(), + } + } +} diff --git a/src/identity/backends/sql/user/list.rs b/src/identity/backends/sql/user/list.rs index 01c71541..5ec2408b 100644 --- a/src/identity/backends/sql/user/list.rs +++ b/src/identity/backends/sql/user/list.rs @@ -21,13 +21,19 @@ use crate::config::Config; use crate::db::entity::{ federated_user as db_federated_user, local_user as db_local_user, nonlocal_user as db_nonlocal_user, password as db_password, - prelude::{FederatedUser, LocalUser, NonlocalUser, User as DbUser, UserOption}, + prelude::{FederatedUser, LocalUser, NonlocalUser, User as DbUser, UserOption as DbUserOption}, user as db_user, }; use crate::error::DbContextExt; use crate::identity::backends::error::IdentityDatabaseError; use crate::identity::types::*; +/// List users. +/// +/// List users in the database. Fetch matching `user` table entries first. Afterwards fetch in +/// parallel `local_user`, `nonlocal_user`, `federated_user`, `user_option` entries merging results +/// to the proper entry. For the local users additionally passwords are being retrieved to identify +/// the password expiration date. pub async fn list( conf: &Config, db: &DatabaseConnection, @@ -49,21 +55,54 @@ pub async fn list( federated_user_select.filter(db_federated_user::Column::DisplayName.eq(name)); } + // Fetch main `user` entries let db_users: Vec = user_select.all(db).await.context("fetching users data")?; + let count_of_users_selected = db_users.len(); let (user_opts, local_users, nonlocal_users, federated_users) = tokio::join!( - db_users.load_many(UserOption, db), - db_users.load_one(local_user_select, db), - db_users.load_one(nonlocal_user_select, db), - db_users.load_many(federated_user_select, db) + db_users.load_many(DbUserOption, db), + // Load local users when requested, otherwise return empty results list + async { + if true { + db_users.load_one(local_user_select, db).await + } else { + Ok(vec![None; count_of_users_selected]) + } + }, + // Load nonlocal users when requested + async { + if true { + db_users.load_one(nonlocal_user_select, db).await + } else { + Ok(vec![None; count_of_users_selected]) + } + }, + // Load federated users when requested + async { + if true { + db_users.load_many(federated_user_select, db).await + } else { + Ok(vec![Vec::new(); count_of_users_selected]) + } + }, ); let locals = local_users.context("fetching local users data")?; + // For local users fetch passwords to determine password expiration let local_users_passwords: Vec>> = - local_user::load_local_users_passwords(db, locals.iter().cloned().map(|u| u.map(|x| x.id))) - .await?; + local_user::load_local_users_passwords( + db, + locals + .iter() + .cloned() + .map(|u| u.map(|x| x.id)) + .collect::>(), + ) + .await?; + // Determine the date for which users with the last activity earlier than are determined as + // inactive. let last_activity_cutof_date = conf.get_user_last_activity_cutof_date(); let mut results: Vec = Vec::new(); @@ -83,17 +122,12 @@ pub async fn list( ), ), ) { - if l.is_none() && n.is_none() && f.is_empty() { - continue; - } - let mut user_builder = UserResponseBuilder::default(); user_builder.merge_user_data( &u, &UserOptions::from_iter(o), last_activity_cutof_date.as_ref(), ); - //user_builder.merge_options(&UserOptions::from_iter(o)); if let Some(local) = l { user_builder.merge_local_user_data(&local); if let Some(pass) = p { @@ -104,24 +138,97 @@ pub async fn list( } else if !f.is_empty() { user_builder.merge_federated_user_data(f); } else { - return Err(IdentityDatabaseError::MalformedUser(u.id))?; + // No matching user details found (maybe due to the filters) + continue; }; results.push(user_builder.build()?); } - //let select: Vec<(String, Option, )> = DbUser::find() - //let select = DbUser::find(); - //let select = Prefixer::new(DbUser::find().select_only()) - // .add_columns(DbUser) - // .add_columns(LocalUser) - // .add_columns(NonlocalUser) - // .selector - // .left_join(LocalUser) - // .left_join(NonlocalUser) - // //.left_join(FederatedUser) - // .into_model::() - // .all(db) - // .await - // .unwrap(); Ok(results) } + +#[cfg(test)] +mod tests { + use sea_orm::{DatabaseBackend, MockDatabase, Transaction}; + + use crate::config::Config; + use crate::db::entity::password as db_password; + + use super::super::super::federated_user::tests::*; + use super::super::super::local_user::tests::*; + use super::super::super::nonlocal_user::tests::*; + use super::super::super::user_option::tests::*; + use super::super::tests::*; + use super::*; + + #[tokio::test] + async fn test_list() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![ + // local user + get_user_mock("1"), + // nonlocal user + get_user_mock("2"), + // federated user + get_user_mock("3"), + // a "bad" user with no user detail records + get_user_mock("4"), + ]]) + .append_query_results([[ + get_user_options_mock("1", &UserOptions::default()), + get_user_options_mock("2", &UserOptions::default()), + get_user_options_mock("3", &UserOptions::default()), + ] + .into_iter() + .flatten()]) + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_nonlocal_user_mock("2")]]) + .append_query_results([vec![get_federated_user_mock("3")]]) + .append_query_results([vec![db_password::Model::default()]]) + .into_connection(); + + let config = Config::default(); + let res = list(&config, &db, &UserListParameters::default()) + .await + .unwrap(); + assert_eq!(res.len(), 3, "3 users found"); + + for (l,r) in db.into_transaction_log().iter().zip([ + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "user"."id", "user"."extra", "user"."enabled", "user"."default_project_id", "user"."created_at", "user"."last_active_at", "user"."domain_id" FROM "user""#, + [] + ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "user_option"."user_id", "user_option"."option_id", "user_option"."option_value" FROM "user_option" WHERE "user_option"."user_id" IN ($1, $2, $3, $4)"#, + [] + ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "local_user"."id", "local_user"."user_id", "local_user"."domain_id", "local_user"."name", "local_user"."failed_auth_count", "local_user"."failed_auth_at" FROM "local_user" WHERE ("local_user"."user_id", "local_user"."domain_id") IN (($1, $2), ($3, $4), ($5, $6), ($7, $8))"#, + [] + ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "nonlocal_user"."domain_id", "nonlocal_user"."name", "nonlocal_user"."user_id" FROM "nonlocal_user" WHERE ("nonlocal_user"."user_id", "nonlocal_user"."domain_id") IN (($1, $2), ($3, $4), ($5, $6), ($7, $8))"#, + [] + ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "federated_user"."id", "federated_user"."user_id", "federated_user"."idp_id", "federated_user"."protocol_id", "federated_user"."unique_id", "federated_user"."display_name" FROM "federated_user" WHERE "federated_user"."user_id" IN ($1, $2, $3, $4)"#, + [] + ), + Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT "password"."id", "password"."local_user_id", "password"."self_service", "password"."created_at", "password"."expires_at", "password"."password_hash", "password"."created_at_int", "password"."expires_at_int" FROM "password" WHERE "password"."local_user_id" IN ($1) ORDER BY "password"."created_at_int" DESC"#, + [] + ), + ]) { + assert_eq!( + l.statements().iter().map(|x| x.sql.clone()).collect::>(), + r.statements().iter().map(|x| x.sql.clone()).collect::>() + ); + } + } +} diff --git a/src/identity/backends/sql/user_option.rs b/src/identity/backends/sql/user_option.rs index 95afad21..2aee18bf 100644 --- a/src/identity/backends/sql/user_option.rs +++ b/src/identity/backends/sql/user_option.rs @@ -51,49 +51,50 @@ impl FromIterator for UserOptions { } #[allow(unused)] -fn get_user_options_db_entries>( +fn get_user_options_db_entries>( user_id: U, options: &UserOptions, ) -> Result, IdentityProviderError> { let mut res: Vec = Vec::new(); + let uid = user_id.into(); if let Some(val) = &options.ignore_change_password_upon_first_use { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "1000".into(), option_value: Some(val.to_string()), }); } if let Some(val) = &options.ignore_password_expiry { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "1001".into(), option_value: Some(val.to_string()), }); } if let Some(val) = &options.ignore_lockout_failure_attempts { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "1002".into(), option_value: Some(val.to_string()), }); } if let Some(val) = &options.lock_password { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "1003".into(), option_value: Some(val.to_string()), }); } if let Some(val) = &options.multi_factor_auth_rules { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "MFAR".into(), option_value: Some(serde_json::to_string(val)?), }); } if let Some(val) = &options.multi_factor_auth_enabled { res.push(user_option::Model { - user_id: user_id.as_ref().to_string(), + user_id: uid.clone(), option_id: "MFAE".into(), option_value: Some(val.to_string()), }); @@ -102,7 +103,7 @@ fn get_user_options_db_entries>( } #[cfg(test)] -pub(super) mod tests { +pub(crate) mod tests { use crate::db::entity::user_option; use crate::identity::types::UserOptions; @@ -118,8 +119,11 @@ pub(super) mod tests { } } - pub fn get_user_options_mock(options: &UserOptions) -> Vec { - get_user_options_db_entries("1", options) + pub fn get_user_options_mock>( + user_id: U, + options: &UserOptions, + ) -> Vec { + get_user_options_db_entries(user_id, options) .unwrap() .into_iter() .collect()