From 8dc308d1bdaf09c6284796fd56ee95fd6a3d6831 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Tue, 28 Jul 2026 14:51:51 +0200 Subject: [PATCH] perf(db): 3 optimizations for DB queries Token validation fetched the scope's domain separately from the already-resolved user domain, even when both point at the same row (the common case: project and user in the same domain). Skip the second `get_domain` lookup when domain_ids match. resolve_implied_roles pulled the entire implied_role table on every call, regardless of how many roles were actually being resolved. Walk the imply graph breadth-first from the assignments' own role IDs via list_role_imply_rules_by_prior instead, since effective-role sets are typically a handful of roles. Also select last local user password only when showing single or listing users. Signed-off-by: Artem Goncharov --- crates/assignment-driver-sql/src/lib.rs | 128 +++++++++------ crates/core/src/token/service.rs | 93 ++++------- .../identity-driver-sql/src/authenticate.rs | 135 +++++++++------- crates/identity-driver-sql/src/local_user.rs | 1 + .../src/local_user/load.rs | 153 +++++++++++++++++- crates/identity-driver-sql/src/user/get.rs | 23 ++- crates/identity-driver-sql/src/user/list.rs | 4 +- crates/identity-driver-sql/src/user/update.rs | 36 +++-- 8 files changed, 380 insertions(+), 193 deletions(-) diff --git a/crates/assignment-driver-sql/src/lib.rs b/crates/assignment-driver-sql/src/lib.rs index a2f183c97..4c53a744c 100644 --- a/crates/assignment-driver-sql/src/lib.rs +++ b/crates/assignment-driver-sql/src/lib.rs @@ -59,9 +59,15 @@ inventory::submit! { impl SqlBackend { /// Resolve implied roles for a set of assignments. /// - /// Fetches role imply rules, computes transitive closure, and generates - /// assignment entries for each implied role. Does NOT resolve role names - /// (that's the provider's responsibility). + /// Walks the imply-rule graph breadth-first, starting only from the + /// role IDs present in `assignments` and fetching each frontier role's + /// direct rules via `list_role_imply_rules_by_prior`, instead of pulling + /// the entire `implied_role` table. Effective-role sets are typically a + /// handful of roles, so this trades one full-table scan for a few + /// narrow, indexed lookups. Computes transitive closure over the + /// resulting (small) subgraph, and generates assignment entries for + /// each implied role. Does NOT resolve role names (that's the + /// provider's responsibility). /// /// Returns a `Vec` containing both the original assignments /// and any additionally generated implied role assignments. @@ -72,17 +78,33 @@ impl SqlBackend { assignments: Vec, ) -> Result, AssignmentProviderError> { let exec = ExecutionContext::internal(state); - let rules = state - .provider - .get_role_provider() - .list_role_imply_rules(&exec) - .await?; let mut imply_rules: BTreeMap> = BTreeMap::new(); - for rule in &rules { - imply_rules - .entry(rule.prior_role.id.clone()) - .or_default() - .insert(rule.implied_role.id.clone()); + let mut visited: HashSet = HashSet::new(); + let mut frontier: BTreeSet = + assignments.iter().map(|a| a.role_id.clone()).collect(); + while !frontier.is_empty() { + let mut next_frontier: BTreeSet = BTreeSet::new(); + for role_id in frontier { + if !visited.insert(role_id.clone()) { + continue; + } + let rules = state + .provider + .get_role_provider() + .list_role_imply_rules_by_prior(&exec, &role_id) + .await?; + for rule in rules { + let implied_id = rule.implied_role.id.clone(); + imply_rules + .entry(rule.prior_role.id.clone()) + .or_default() + .insert(implied_id.clone()); + if !visited.contains(&implied_id) { + next_frontier.insert(implied_id); + } + } + } + frontier = next_frontier; } // Transitive expansion let mut changed = true; @@ -415,23 +437,29 @@ mod tests { .into_connection(); let mut role_mock = MockRoleProvider::default(); - role_mock.expect_list_role_imply_rules().returning(|_e| { - Ok(vec![ - RoleImplyBuilder::default() - .prior_role(RoleRef { - id: "1".into(), - name: Some("r1".into()), - domain_id: None, - }) - .implied_role(RoleRef { - id: "2".into(), - name: Some("r2".into()), - domain_id: None, - }) - .build() - .unwrap(), - ]) - }); + role_mock + .expect_list_role_imply_rules_by_prior() + .returning(|_e, prior_role_id| { + if prior_role_id == "1" { + Ok(vec![ + RoleImplyBuilder::default() + .prior_role(RoleRef { + id: "1".into(), + name: Some("r1".into()), + domain_id: None, + }) + .implied_role(RoleRef { + id: "2".into(), + name: Some("r2".into()), + domain_id: None, + }) + .build() + .unwrap(), + ]) + } else { + Ok(vec![]) + } + }); let provider = Provider::mocked_builder() .mock_role(role_mock) .build() @@ -649,23 +677,29 @@ mod tests { .into_connection(); let mut role_mock = MockRoleProvider::default(); - role_mock.expect_list_role_imply_rules().returning(|_e| { - Ok(vec![ - RoleImplyBuilder::default() - .prior_role(RoleRef { - id: "1".into(), - name: Some("r1".into()), - domain_id: None, - }) - .implied_role(RoleRef { - id: "2".into(), - name: Some("r2".into()), - domain_id: None, - }) - .build() - .unwrap(), - ]) - }); + role_mock + .expect_list_role_imply_rules_by_prior() + .returning(|_e, prior_role_id| { + if prior_role_id == "1" { + Ok(vec![ + RoleImplyBuilder::default() + .prior_role(RoleRef { + id: "1".into(), + name: Some("r1".into()), + domain_id: None, + }) + .implied_role(RoleRef { + id: "2".into(), + name: Some("r2".into()), + domain_id: None, + }) + .build() + .unwrap(), + ]) + } else { + Ok(vec![]) + } + }); let provider = Provider::mocked_builder() .mock_role(role_mock) .build() diff --git a/crates/core/src/token/service.rs b/crates/core/src/token/service.rs index 3f77165f3..bed8f0709 100644 --- a/crates/core/src/token/service.rs +++ b/crates/core/src/token/service.rs @@ -34,7 +34,7 @@ use openstack_keystone_core_types::auth::{ AuthzInfo, AuthzInfoBuilder, ScopeInfo, SecurityContext, TrustProjectInfo, }; use openstack_keystone_core_types::events::{Event, EventPayload, Operation}; -use openstack_keystone_core_types::resource::ResourceProviderError; +use openstack_keystone_core_types::resource::{Domain, ResourceProviderError}; use openstack_keystone_core_types::role::{Role, RoleListParameters, RoleRef}; use openstack_keystone_core_types::token::{ FernetToken, TokenRestriction, TokenRestrictionCreate, TokenRestrictionListParameters, @@ -107,12 +107,28 @@ impl TokenService { /// Build [`AuthzInfo`] from a token by fetching scope objects /// from DB. + /// + /// `known_domain` is the caller's already-resolved user domain: when a + /// project scope's `domain_id` matches it, that `Domain` is reused + /// instead of issuing a second `get_domain` lookup for the same row. async fn build_authz_info_from_fernet_token( &self, state: &ServiceState, token: &FernetToken, + known_domain: &Domain, ) -> Result { let ctx = ExecutionContext::internal(state); + let get_project_domain = async |domain_id: &str| -> Result { + if domain_id == known_domain.id { + return Ok(known_domain.clone()); + } + Ok(state + .provider + .get_resource_provider() + .get_domain(&ctx, domain_id) + .await? + .ok_or(ResourceProviderError::DomainNotFound(domain_id.to_owned()))?) + }; let scope = match token { FernetToken::ApplicationCredential(data) => { let project = state @@ -123,29 +139,15 @@ impl TokenService { .ok_or(ResourceProviderError::ProjectNotFound( data.project_id.clone(), ))?; - let project_domain = state - .provider - .get_resource_provider() - .get_domain(&ctx, &project.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - project.domain_id.clone(), - ))?; + let project_domain = get_project_domain(&project.domain_id).await?; ScopeInfo::Project { project, project_domain, } } - FernetToken::DomainScope(data) => ScopeInfo::Domain( - state - .provider - .get_resource_provider() - .get_domain(&ctx, &data.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - data.domain_id.clone(), - ))?, - ), + FernetToken::DomainScope(data) => { + ScopeInfo::Domain(get_project_domain(&data.domain_id).await?) + } FernetToken::ProjectScope(data) => { let project = state .provider @@ -155,29 +157,15 @@ impl TokenService { .ok_or(ResourceProviderError::ProjectNotFound( data.project_id.clone(), ))?; - let project_domain = state - .provider - .get_resource_provider() - .get_domain(&ctx, &project.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - project.domain_id.clone(), - ))?; + let project_domain = get_project_domain(&project.domain_id).await?; ScopeInfo::Project { project, project_domain, } } - FernetToken::FederationDomainScope(data) => ScopeInfo::Domain( - state - .provider - .get_resource_provider() - .get_domain(&ctx, &data.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - data.domain_id.clone(), - ))?, - ), + FernetToken::FederationDomainScope(data) => { + ScopeInfo::Domain(get_project_domain(&data.domain_id).await?) + } FernetToken::FederationProjectScope(data) => { let project = state .provider @@ -187,14 +175,7 @@ impl TokenService { .ok_or(ResourceProviderError::ProjectNotFound( data.project_id.clone(), ))?; - let project_domain = state - .provider - .get_resource_provider() - .get_domain(&ctx, &project.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - project.domain_id.clone(), - ))?; + let project_domain = get_project_domain(&project.domain_id).await?; ScopeInfo::Project { project, project_domain, @@ -210,14 +191,7 @@ impl TokenService { .ok_or(ResourceProviderError::ProjectNotFound( data.project_id.clone(), ))?; - let project_domain = state - .provider - .get_resource_provider() - .get_domain(&ctx, &project.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - project.domain_id.clone(), - ))?; + let project_domain = get_project_domain(&project.domain_id).await?; let trust = state .provider .get_trust_provider() @@ -239,14 +213,7 @@ impl TokenService { .ok_or(ResourceProviderError::ProjectNotFound( data.project_id.clone(), ))?; - let project_domain = state - .provider - .get_resource_provider() - .get_domain(&ctx, &project.domain_id) - .await? - .ok_or(ResourceProviderError::DomainNotFound( - project.domain_id.clone(), - ))?; + let project_domain = get_project_domain(&project.domain_id).await?; ScopeInfo::Project { project, project_domain, @@ -371,14 +338,14 @@ impl TokenService { UserIdentityInfoBuilder::default() .user_id(token.user_id()) .user(user.clone()) - .user_domain(user_domain) + .user_domain(user_domain.clone()) .build()?, ), }) .expires_at(*token.expires_at()) .authorization( // populate scope info - self.build_authz_info_from_fernet_token(state, &token) + self.build_authz_info_from_fernet_token(state, &token, &user_domain) .await?, ); if let FernetToken::Restricted(restriction) = &token { diff --git a/crates/identity-driver-sql/src/authenticate.rs b/crates/identity-driver-sql/src/authenticate.rs index f0cc6692a..f345fe795 100644 --- a/crates/identity-driver-sql/src/authenticate.rs +++ b/crates/identity-driver-sql/src/authenticate.rs @@ -22,7 +22,9 @@ use openstack_keystone_core::identity::IdentityProviderError; use openstack_keystone_core_types::identity::{UserPasswordAuthRequest, UserResponseBuilder}; use openstack_keystone_password_hashing as password_hashing; -use crate::entity::{local_user as db_local_user, password as db_password}; +use crate::entity::local_user as db_local_user; +#[cfg(test)] +use crate::entity::password as db_password; use crate::local_user; use crate::local_user::MergeLocalUserData; use crate::password; @@ -58,7 +60,7 @@ pub async fn authenticate_by_password( db: &DatabaseConnection, auth: &UserPasswordAuthRequest, ) -> Result { - let user_with_passwords = local_user::load_local_user_with_passwords( + let user_with_passwords = local_user::load_local_user_with_latest_password( db, auth.id.as_ref(), auth.name.as_ref(), @@ -112,9 +114,8 @@ pub async fn authenticate_by_password( return Err(AuthenticationError::UserDisabled(local_user_entry.user_id.clone()).into()); } - let passwords: Vec = password.into_iter().collect(); - let latest_password = passwords - .first() + let latest_password = password + .as_ref() .ok_or(IdentityProviderError::NoPasswordsForUser( local_user_entry.user_id.clone(), ))?; @@ -158,7 +159,7 @@ pub async fn authenticate_by_password( .as_ref(), ) .merge_local_user_data(&local_user_entry) - .merge_passwords_data(passwords) + .merge_passwords_data(password) .build()?; Ok(AuthenticationResultBuilder::default() @@ -454,12 +455,14 @@ mod tests { let config = Config::default(); let password = String::from("pass"); let password_secret = SecretString::from(password.clone()); + let (lu, pw) = get_local_user_with_password_mock( + password_hashing::hash_password(&config, &password_secret) + .await + .unwrap(), + ); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![get_local_user_with_password_mock( - password_hashing::hash_password(&config, &password_secret) - .await - .unwrap(), - )]]) + .append_query_results([vec![lu]]) + .append_query_results([vec![pw]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions::default(), @@ -486,17 +489,25 @@ mod tests { // Checking transaction log let log = db.into_transaction_log(); - assert_eq!(log.len(), 4); + assert_eq!(log.len(), 5); assert_eq!( log[0], Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "local_user"."id" AS "A_id", "local_user"."user_id" AS "A_user_id", "local_user"."domain_id" AS "A_domain_id", "local_user"."name" AS "A_name", "local_user"."failed_auth_count" AS "A_failed_auth_count", "local_user"."failed_auth_at" AS "A_failed_auth_at", "password"."id" AS "B_id", "password"."local_user_id" AS "B_local_user_id", "password"."self_service" AS "B_self_service", "password"."created_at" AS "B_created_at", "password"."expires_at" AS "B_expires_at", "password"."password_hash" AS "B_password_hash", "password"."created_at_int" AS "B_created_at_int", "password"."expires_at_int" AS "B_expires_at_int" FROM "local_user" LEFT JOIN "password" ON "local_user"."id" = "password"."local_user_id" WHERE "local_user"."user_id" = $1 ORDER BY "local_user"."id" ASC, "password"."created_at_int" DESC"#, - ["user_id".into()] + 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" = $1 LIMIT $2"#, + ["user_id".into(), 1u64.into()] ) ); assert_eq!( log[1], + 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" = $1 ORDER BY "password"."created_at_int" DESC LIMIT $2"#, + [1i32.into(), 1u64.into()] + ) + ); + assert_eq!( + log[2], 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" = $1"#, @@ -504,7 +515,7 @@ mod tests { ) ); assert_eq!( - log[2], + log[3], Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT "user"."created_at", "user"."default_project_id", "user"."domain_id", "user"."enabled", "user"."extra", "user"."id", "user"."last_active_at" FROM "user" WHERE "user"."id" = $1 LIMIT $2"#, @@ -513,7 +524,7 @@ mod tests { ); // Verify the UPDATE statement for successful authentication - let update_debug = format!("{:?}", log[3]); + let update_debug = format!("{:?}", log[4]); assert!( update_debug.contains("UPDATE \\\"user\\\" SET \\\"last_active_at\\\"") && update_debug.contains("user_id"), @@ -531,20 +542,20 @@ mod tests { let mut config = Config::default(); config.security_compliance.lockout_failure_attempts = Some(5); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - db_local_user::Model { - id: 1, - user_id: "user_id".into(), - domain_id: "foo_domain".into(), - name: "foo_domain".into(), - failed_auth_count: Some(10), - failed_auth_at: Some(Utc::now().naive_utc()), - }, + .append_query_results([vec![db_local_user::Model { + id: 1, + user_id: "user_id".into(), + domain_id: "foo_domain".into(), + name: "foo_domain".into(), + failed_auth_count: Some(10), + failed_auth_at: Some(Utc::now().naive_utc()), + }]]) + .append_query_results([vec![ db_password::ModelBuilder::default() .local_user_id(1) .build() .unwrap(), - )]]) + ]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions::default(), @@ -578,15 +589,15 @@ mod tests { let password = "foo_pass"; config.security_compliance.lockout_failure_attempts = Some(5); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - db_local_user::Model { - id: 1, - user_id: "user_id".into(), - domain_id: "foo_domain".into(), - name: "foo_domain".into(), - failed_auth_count: Some(10), - failed_auth_at: Some(Utc::now().naive_utc()), - }, + .append_query_results([vec![db_local_user::Model { + id: 1, + user_id: "user_id".into(), + domain_id: "foo_domain".into(), + name: "foo_domain".into(), + failed_auth_count: Some(10), + failed_auth_at: Some(Utc::now().naive_utc()), + }]]) + .append_query_results([vec![ db_password::ModelBuilder::default() .local_user_id(1) .password_hash( @@ -596,7 +607,7 @@ mod tests { ) .build() .unwrap(), - )]]) + ]]) .append_exec_results([MockExecResult { rows_affected: 1, ..Default::default() @@ -636,13 +647,13 @@ mod tests { async fn test_authenticate_wrong_password() { let config = Config::default(); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - get_local_user_mock("user_id"), + .append_query_results([vec![get_local_user_mock("user_id")]]) + .append_query_results([vec![ db_password::ModelBuilder::default() .password_hash("wrong_password") .build() .unwrap(), - )]]) + ]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions::default(), @@ -673,19 +684,27 @@ mod tests { // Verify that the failure was logged let log = db.into_transaction_log(); - assert_eq!(log.len(), 4); + assert_eq!(log.len(), 5); - // Verify first 3 transactions exactly + // Verify first transactions exactly assert_eq!( log[0], Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "local_user"."id" AS "A_id", "local_user"."user_id" AS "A_user_id", "local_user"."domain_id" AS "A_domain_id", "local_user"."name" AS "A_name", "local_user"."failed_auth_count" AS "A_failed_auth_count", "local_user"."failed_auth_at" AS "A_failed_auth_at", "password"."id" AS "B_id", "password"."local_user_id" AS "B_local_user_id", "password"."self_service" AS "B_self_service", "password"."created_at" AS "B_created_at", "password"."expires_at" AS "B_expires_at", "password"."password_hash" AS "B_password_hash", "password"."created_at_int" AS "B_created_at_int", "password"."expires_at_int" AS "B_expires_at_int" FROM "local_user" LEFT JOIN "password" ON "local_user"."id" = "password"."local_user_id" WHERE "local_user"."user_id" = $1 ORDER BY "local_user"."id" ASC, "password"."created_at_int" DESC"#, - ["user_id".into()] + 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" = $1 LIMIT $2"#, + ["user_id".into(), 1u64.into()] ) ); assert_eq!( log[1], + 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" = $1 ORDER BY "password"."created_at_int" DESC LIMIT $2"#, + [1i32.into(), 1u64.into()] + ) + ); + assert_eq!( + log[2], 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" = $1"#, @@ -693,7 +712,7 @@ mod tests { ) ); assert_eq!( - log[2], + log[3], Transaction::from_sql_and_values( DatabaseBackend::Postgres, r#"SELECT "user"."created_at", "user"."default_project_id", "user"."domain_id", "user"."enabled", "user"."extra", "user"."id", "user"."last_active_at" FROM "user" WHERE "user"."id" = $1 LIMIT $2"#, @@ -703,7 +722,7 @@ mod tests { // Verify the UPDATE statement for failed auth logging // timestamp (ChronoDateTime) is dynamic so we check via debug string - let update_debug = format!("{:?}", log[3]); + let update_debug = format!("{:?}", log[4]); assert!( update_debug.contains("UPDATE \\\"local_user\\\" SET \\\"failed_auth_count\\\"") && update_debug.contains("\\\"failed_auth_at\\\""), @@ -728,7 +747,7 @@ mod tests { async fn test_authenticate_user_not_found() { let config = Config::default(); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([Vec::<(db_local_user::Model, db_password::Model)>::new()]) + .append_query_results([Vec::::new()]) .into_connection(); match authenticate_by_password( &config, @@ -757,8 +776,8 @@ mod tests { log[0], Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "local_user"."id" AS "A_id", "local_user"."user_id" AS "A_user_id", "local_user"."domain_id" AS "A_domain_id", "local_user"."name" AS "A_name", "local_user"."failed_auth_count" AS "A_failed_auth_count", "local_user"."failed_auth_at" AS "A_failed_auth_at", "password"."id" AS "B_id", "password"."local_user_id" AS "B_local_user_id", "password"."self_service" AS "B_self_service", "password"."created_at" AS "B_created_at", "password"."expires_at" AS "B_expires_at", "password"."password_hash" AS "B_password_hash", "password"."created_at_int" AS "B_created_at_int", "password"."expires_at_int" AS "B_expires_at_int" FROM "local_user" LEFT JOIN "password" ON "local_user"."id" = "password"."local_user_id" WHERE "local_user"."user_id" = $1 ORDER BY "local_user"."id" ASC, "password"."created_at_int" DESC"#, - ["nonexistent_user".into()] + 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" = $1 LIMIT $2"#, + ["nonexistent_user".into(), 1u64.into()] ) ); } @@ -779,7 +798,7 @@ mod tests { let mut not_found_total = std::time::Duration::ZERO; for _ in 0..iterations { let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([Vec::<(db_local_user::Model, db_password::Model)>::new()]) + .append_query_results([Vec::::new()]) .into_connection(); let start = std::time::Instant::now(); let _ = authenticate_by_password( @@ -799,13 +818,13 @@ mod tests { let mut wrong_password_total = std::time::Duration::ZERO; for _ in 0..iterations { let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - get_local_user_mock("user_id"), + .append_query_results([vec![get_local_user_mock("user_id")]]) + .append_query_results([vec![ db_password::ModelBuilder::default() .password_hash("wrong_hash") .build() .unwrap(), - )]]) + ]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions::default(), @@ -857,8 +876,8 @@ mod tests { let password = String::from("foo_pass"); let password_secret = SecretString::from(password.clone()); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - get_local_user_mock("user_id"), + .append_query_results([vec![get_local_user_mock("user_id")]]) + .append_query_results([vec![ db_password::ModelBuilder::default() .password_hash( password_hashing::hash_password(&config, &password_secret) @@ -868,7 +887,7 @@ mod tests { .expires(DateTime::::MIN_UTC) .build() .unwrap(), - )]]) + ]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions::default(), @@ -904,8 +923,8 @@ mod tests { let password = String::from("foo_pass"); let password_secret = SecretString::from(password.clone()); let db = MockDatabase::new(DatabaseBackend::Postgres) - .append_query_results([vec![( - get_local_user_mock("user_id"), + .append_query_results([vec![get_local_user_mock("user_id")]]) + .append_query_results([vec![ db_password::ModelBuilder::expired() .password_hash( password_hashing::hash_password(&config, &password_secret) @@ -914,7 +933,7 @@ mod tests { ) .build() .unwrap(), - )]]) + ]]) .append_query_results([user_option::tests::get_user_options_mock( "user_id", &UserOptions { diff --git a/crates/identity-driver-sql/src/local_user.rs b/crates/identity-driver-sql/src/local_user.rs index 063ba46be..0ef493cab 100644 --- a/crates/identity-driver-sql/src/local_user.rs +++ b/crates/identity-driver-sql/src/local_user.rs @@ -28,6 +28,7 @@ mod load; mod set; pub use create::create; +pub use load::load_local_user_with_latest_password; pub use load::load_local_user_with_passwords; pub use load::load_local_users_passwords; pub use set::*; diff --git a/crates/identity-driver-sql/src/local_user/load.rs b/crates/identity-driver-sql/src/local_user/load.rs index 6e5c34588..90008033a 100644 --- a/crates/identity-driver-sql/src/local_user/load.rs +++ b/crates/identity-driver-sql/src/local_user/load.rs @@ -76,7 +76,66 @@ pub async fn load_local_user_with_passwords< Ok(results.first().cloned()) } -/// Fetch passwords for list of optional local user ids. +/// Load local user record with only its latest password from database. +/// +/// Same lookup as [`load_local_user_with_passwords`] but for callers that +/// only ever consult the newest password (auth, display) and don't need the +/// full history, avoiding pulling every password row for the user. +/// +/// # Parameters +/// - `db`: The database connection. +/// - `user_id`: The optional user ID. +/// - `name`: The optional user name. +/// - `domain_id`: The optional domain ID. +/// +/// # Returns +/// A `Result` containing an `Option` with the `(local_user::Model, +/// Option)` if found, or an `Error`. +#[tracing::instrument(skip_all)] +pub async fn load_local_user_with_latest_password< + C: ConnectionTrait, + S1: AsRef, + S2: AsRef, + S3: AsRef, +>( + db: &C, + user_id: Option, + name: Option, + domain_id: Option, +) -> Result)>, IdentityProviderError> { + let mut select = LocalUser::find(); + if let Some(user_id) = user_id { + select = select.filter(local_user::Column::UserId.eq(user_id.as_ref())) + } else { + select = select + .filter( + local_user::Column::Name.eq(name + .ok_or(IdentityProviderError::UserIdOrNameWithDomain)? + .as_ref()), + ) + .filter( + local_user::Column::DomainId.eq(domain_id + .ok_or(IdentityProviderError::UserIdOrNameWithDomain)? + .as_ref()), + ); + } + let Some(local_user) = select.one(db).await.context("fetching local user")? else { + return Ok(None); + }; + let latest_password = Password::find() + .filter(password::Column::LocalUserId.eq(local_user.id)) + .order_by(password::Column::CreatedAtInt, Order::Desc) + .one(db) + .await + .context("fetching latest user password")?; + Ok(Some((local_user, latest_password))) +} + +/// Fetch the latest password for a list of optional local user ids. +/// +/// Only the newest password per user is needed by callers (password +/// expiration display), so the query is restricted to one row per +/// `local_user_id` instead of the full history. /// /// Returns vector of optional vectors with passwords in the same order as /// requested keeping None in place where local_user was empty. @@ -99,9 +158,11 @@ pub async fn load_local_users_passwords< // Collect local user IDs that we need to query let keys: Vec = ids.iter().filter_map(Option::as_ref).copied().collect(); - // Fetch passwords for the local users by keys + // Fetch only the latest password per local user by keys let passwords: Vec = Password::find() .filter(password::Column::LocalUserId.is_in(keys.clone())) + .distinct_on([password::Column::LocalUserId]) + .order_by(password::Column::LocalUserId, Order::Asc) .order_by(password::Column::CreatedAtInt, Order::Desc) .all(db) .await @@ -219,4 +280,92 @@ mod tests { ),] ); } + + #[tokio::test] + async fn test_load_local_user_with_latest_password_found() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![get_local_user_mock("user_id")]]) + .append_query_results([vec![password::Model { + id: 1, + local_user_id: 1, + self_service: false, + expires_at: None, + password_hash: Some("hash".into()), + created_at: chrono::Utc::now().naive_utc(), + created_at_int: 1, + expires_at_int: None, + }]]) + .into_connection(); + let result = + load_local_user_with_latest_password(&db, Some("user_id"), None::<&str>, None::<&str>) + .await + .unwrap(); + assert!(result.is_some()); + assert!(result.unwrap().1.is_some()); + + // Checking transaction log + assert_eq!( + db.into_transaction_log(), + [ + 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" = $1 LIMIT $2"#, + ["user_id".into(), 1u64.into()] + ), + 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" = $1 ORDER BY "password"."created_at_int" DESC LIMIT $2"#, + [1i32.into(), 1u64.into()] + ), + ] + ); + } + + #[tokio::test] + async fn test_load_local_user_with_latest_password_no_local_user() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([Vec::::new()]) + .into_connection(); + let result = + load_local_user_with_latest_password(&db, Some("user_id"), None::<&str>, None::<&str>) + .await + .unwrap(); + assert!(result.is_none()); + + // Only the local_user lookup should run: no password query issued + // once the user is not found. + assert_eq!(db.into_transaction_log().len(), 1); + } + + #[tokio::test] + async fn test_load_local_users_passwords_only_latest_per_user() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_query_results([vec![password::Model { + id: 2, + local_user_id: 1, + self_service: false, + expires_at: None, + password_hash: Some("latest".into()), + created_at: chrono::Utc::now().naive_utc(), + created_at_int: 2, + expires_at_int: None, + }]]) + .into_connection(); + let result = load_local_users_passwords(&db, [Some(1)]).await.unwrap(); + assert_eq!(result.len(), 1); + assert_eq!(result[0].as_ref().unwrap().len(), 1); + assert_eq!( + result[0].as_ref().unwrap()[0].password_hash, + Some("latest".into()) + ); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"SELECT DISTINCT ON ("local_user_id") "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"."local_user_id" ASC, "password"."created_at_int" DESC"#, + [1i32.into()] + ),] + ); + } } diff --git a/crates/identity-driver-sql/src/user/get.rs b/crates/identity-driver-sql/src/user/get.rs index e9ae6d39e..3fdb4ac29 100644 --- a/crates/identity-driver-sql/src/user/get.rs +++ b/crates/identity-driver-sql/src/user/get.rs @@ -77,7 +77,7 @@ pub async fn get( if let Some(user) = user_entry { let (user_opts, local_user_with_passwords) = tokio::join!( user.find_related(UserOption).all(db), - local_user::load_local_user_with_passwords( + local_user::load_local_user_with_latest_password( db, Some(&user_id), None::<&str>, @@ -277,6 +277,10 @@ mod tests { #[tokio::test] async fn test_get_user_local() { + let (lu, pw) = local_user::tests::get_local_user_with_password_mock("1", 1) + .into_iter() + .next() + .unwrap(); // Create MockDatabase with mock query results let db = MockDatabase::new(DatabaseBackend::Postgres) .append_query_results([ @@ -292,8 +296,12 @@ mod tests { }], ]) .append_query_results([ - // Third query result - local user with passwords - local_user::tests::get_local_user_with_password_mock("1", 1), + // Third query result - local user + vec![lu], + ]) + .append_query_results([ + // Fourth query result - latest password for the local user + vec![pw], ]) .into_connection(); let config = Config::default(); @@ -331,8 +339,13 @@ mod tests { ), Transaction::from_sql_and_values( DatabaseBackend::Postgres, - r#"SELECT "local_user"."id" AS "A_id", "local_user"."user_id" AS "A_user_id", "local_user"."domain_id" AS "A_domain_id", "local_user"."name" AS "A_name", "local_user"."failed_auth_count" AS "A_failed_auth_count", "local_user"."failed_auth_at" AS "A_failed_auth_at", "password"."id" AS "B_id", "password"."local_user_id" AS "B_local_user_id", "password"."self_service" AS "B_self_service", "password"."created_at" AS "B_created_at", "password"."expires_at" AS "B_expires_at", "password"."password_hash" AS "B_password_hash", "password"."created_at_int" AS "B_created_at_int", "password"."expires_at_int" AS "B_expires_at_int" FROM "local_user" LEFT JOIN "password" ON "local_user"."id" = "password"."local_user_id" WHERE "local_user"."user_id" = $1 ORDER BY "local_user"."id" ASC, "password"."created_at_int" DESC"#, - ["1".into()] + 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" = $1 LIMIT $2"#, + ["1".into(), 1u64.into()] + ), + 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" = $1 ORDER BY "password"."created_at_int" DESC LIMIT $2"#, + [1i32.into(), 1u64.into()] ), ] ); diff --git a/crates/identity-driver-sql/src/user/list.rs b/crates/identity-driver-sql/src/user/list.rs index f7c00bfe6..848472ad2 100644 --- a/crates/identity-driver-sql/src/user/list.rs +++ b/crates/identity-driver-sql/src/user/list.rs @@ -412,7 +412,7 @@ mod tests { ), 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"#, + r#"SELECT DISTINCT ON ("local_user_id") "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"."local_user_id" ASC, "password"."created_at_int" DESC"#, [] ), ]) { @@ -474,7 +474,7 @@ mod tests { ), 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"#, + r#"SELECT DISTINCT ON ("local_user_id") "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"."local_user_id" ASC, "password"."created_at_int" DESC"#, [] ), ]) { diff --git a/crates/identity-driver-sql/src/user/update.rs b/crates/identity-driver-sql/src/user/update.rs index 33542cc04..c84a08123 100644 --- a/crates/identity-driver-sql/src/user/update.rs +++ b/crates/identity-driver-sql/src/user/update.rs @@ -204,8 +204,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 4. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 5. Fetch local user with passwords - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 5. Fetch local user, 6. Fetch its latest password + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate { @@ -235,8 +236,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 6. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 7. Fetch local user with passwords - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 7. Fetch local user, 8. Fetch its latest password + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate { @@ -279,10 +281,8 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 8. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 9. Fetch local user with passwords (empty - no local user) - .append_query_results([ - Vec::<(crate::entity::local_user::Model, db_password::Model)>::new(), - ]) + // 9. Fetch local user (empty - no local user, no password query follows) + .append_query_results([Vec::::new()]) // 10. Fetch nonlocal user data (user::get falls back to this when no local_user) .append_query_results([vec![db_nonlocal_user::Model { domain_id: "foo_domain".into(), @@ -328,8 +328,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 7. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 8. Fetch local user with passwords - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 8. Fetch local user, 9. Fetch its latest password + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate { @@ -402,8 +403,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 8. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 9. Fetch local user with passwords - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 9. Fetch local user, 10. Fetch its latest password + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate { @@ -448,8 +450,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 7. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 8. Fetch local user with passwords (1 password with expiry info) - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 8. Fetch local user, 9. Fetch its latest password (with expiry info) + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate { @@ -487,8 +490,9 @@ mod tests { .append_query_results([vec![get_user_mock("1")]]) // 8. Fetch user options (empty) .append_query_results([Vec::::new()]) - // 9. Fetch local user with passwords - .append_query_results([get_local_user_with_password_mock("1", 1)]) + // 9. Fetch local user, 10. Fetch its latest password + .append_query_results([vec![get_local_user_mock("1")]]) + .append_query_results([vec![get_local_user_with_password_mock("1", 1).remove(0).1]]) .into_connection(); let req = UserUpdate {