From b925f953c5b15c15aac57ca133a2212e308f9aa4 Mon Sep 17 00:00:00 2001 From: Artem Goncharov Date: Tue, 4 Nov 2025 18:21:22 +0100 Subject: [PATCH] feat: Enforce context for DB operations in identity Remove From trait implementation for the DB operations in the identity provider to enforce using the helper method with specifying the context information. This dramatically improves the error finding since the error contain the reference to the operation that caused it. --- src/identity/backends/error.rs | 6 -- src/identity/backends/sql.rs | 63 +++++++++++++++------ src/identity/backends/sql/federated_user.rs | 11 +++- src/identity/backends/sql/group/create.rs | 7 ++- src/identity/backends/sql/group/delete.rs | 7 ++- src/identity/backends/sql/group/get.rs | 5 +- src/identity/backends/sql/group/list.rs | 7 ++- src/identity/backends/sql/local_user.rs | 23 +++++--- src/identity/backends/sql/password.rs | 7 ++- src/identity/backends/sql/user.rs | 17 ++++-- src/identity/backends/sql/user_option.rs | 7 ++- 11 files changed, 109 insertions(+), 51 deletions(-) diff --git a/src/identity/backends/error.rs b/src/identity/backends/error.rs index 9d48ec3f..0958bf90 100644 --- a/src/identity/backends/error.rs +++ b/src/identity/backends/error.rs @@ -100,9 +100,3 @@ pub fn db_err(e: sea_orm::DbErr, context: &str) -> IdentityDatabaseError { }, ) } - -impl From for IdentityDatabaseError { - fn from(err: sea_orm::DbErr) -> Self { - db_err(err, "unknown") - } -} diff --git a/src/identity/backends/sql.rs b/src/identity/backends/sql.rs index 644f066b..c374a364 100644 --- a/src/identity/backends/sql.rs +++ b/src/identity/backends/sql.rs @@ -364,7 +364,10 @@ async fn list_users( federated_user_select.filter(db_federated_user::Column::DisplayName.eq(name)); } - let db_users: Vec = user_select.all(db).await?; + let db_users: Vec = user_select + .all(db) + .await + .map_err(|err| db_err(err, "fetching users data"))?; let (user_opts, local_users, nonlocal_users, federated_users) = tokio::join!( db_users.load_many(UserOption, db), @@ -373,7 +376,7 @@ async fn list_users( db_users.load_many(federated_user_select, db) ); - let locals = local_users?; + let locals = local_users.map_err(|err| db_err(err, "fetching local users data"))?; let local_users_passwords: Vec>> = local_user::load_local_users_passwords(db, locals.iter().cloned().map(|u| u.map(|x| x.id))) @@ -381,15 +384,23 @@ async fn list_users( let mut results: Vec = Vec::new(); for (u, (o, (l, (p, (n, f))))) in db_users.into_iter().zip( - user_opts?.into_iter().zip( - locals.into_iter().zip( - local_users_passwords.into_iter().zip( - nonlocal_users? - .into_iter() - .zip(federated_users?.into_iter()), + user_opts + .map_err(|err| db_err(err, "fetching user options"))? + .into_iter() + .zip( + locals.into_iter().zip( + local_users_passwords.into_iter().zip( + nonlocal_users + .map_err(|err| db_err(err, "fetching nonlocal users data"))? + .into_iter() + .zip( + federated_users + .map_err(|err| db_err(err, "fetching federated users data"))? + .into_iter(), + ), + ), ), ), - ), ) { if l.is_none() && n.is_none() && f.is_empty() { continue; @@ -430,7 +441,10 @@ pub async fn get_user( ) -> Result, IdentityDatabaseError> { let user_select = DbUser::find_by_id(user_id); - let user_entry: Option = user_select.one(db).await?; + let user_entry: Option = user_select + .one(db) + .await + .map_err(|err| db_err(err, "fetching the user data"))?; if let Some(user) = user_entry { let (user_opts, local_user_with_passwords) = tokio::join!( @@ -449,16 +463,31 @@ pub async fn get_user( &user, local_user_with_passwords.0, Some(local_user_with_passwords.1), - user_opts?, + user_opts.map_err(|err| db_err(err, "fetching user options"))?, ), - _ => match user.find_related(NonlocalUser).one(db).await? { - Some(nonlocal_user) => { - common::get_nonlocal_user_builder(&user, nonlocal_user, user_opts?) - } + _ => match user + .find_related(NonlocalUser) + .one(db) + .await + .map_err(|err| db_err(err, "fetching nonlocal user data"))? + { + Some(nonlocal_user) => common::get_nonlocal_user_builder( + &user, + nonlocal_user, + user_opts.map_err(|err| db_err(err, "fetching user options"))?, + ), _ => { - let federated_user = user.find_related(FederatedUser).all(db).await?; + let federated_user = user + .find_related(FederatedUser) + .all(db) + .await + .map_err(|err| db_err(err, "fetching federated user data"))?; if !federated_user.is_empty() { - common::get_federated_user_builder(&user, federated_user, user_opts?) + common::get_federated_user_builder( + &user, + federated_user, + user_opts.map_err(|err| db_err(err, "fetching user options"))?, + ) } else { return Err(IdentityDatabaseError::MalformedUser(user_id.to_string()))?; } diff --git a/src/identity/backends/sql/federated_user.rs b/src/identity/backends/sql/federated_user.rs index c870d834..66378b97 100644 --- a/src/identity/backends/sql/federated_user.rs +++ b/src/identity/backends/sql/federated_user.rs @@ -18,7 +18,7 @@ use sea_orm::query::*; use crate::config::Config; use crate::db::entity::{federated_user, prelude::FederatedUser}; -use crate::identity::backends::error::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; pub async fn create( _conf: &Config, @@ -28,7 +28,11 @@ pub async fn create( where A: Into, { - let db_user: federated_user::Model = federation.into().insert(db).await?; + let db_user: federated_user::Model = federation + .into() + .insert(db) + .await + .map_err(|err| db_err(err, "persisting federated user data"))?; Ok(db_user) } @@ -44,7 +48,8 @@ pub async fn find_by_idp_and_unique_id, U: AsRef>( .filter(federated_user::Column::IdpId.eq(idp_id.as_ref())) .filter(federated_user::Column::UniqueId.eq(unique_id.as_ref())) .all(db) - .await? + .await + .map_err(|err| db_err(err, "searching federated user by the idp and unique id"))? .first() .cloned()) } diff --git a/src/identity/backends/sql/group/create.rs b/src/identity/backends/sql/group/create.rs index 58a8c0ec..ae305bf7 100644 --- a/src/identity/backends/sql/group/create.rs +++ b/src/identity/backends/sql/group/create.rs @@ -18,7 +18,7 @@ use serde_json::json; use crate::db::entity::group; use crate::identity::Config; -use crate::identity::backends::sql::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::{Group, GroupCreate}; pub async fn create( @@ -36,7 +36,10 @@ pub async fn create( )?)), }; - let db_entry: group::Model = entry.insert(db).await?; + let db_entry: group::Model = entry + .insert(db) + .await + .map_err(|err| db_err(err, "persisting new group record"))?; Ok(db_entry.into()) } diff --git a/src/identity/backends/sql/group/delete.rs b/src/identity/backends/sql/group/delete.rs index 835b80fc..419ebe43 100644 --- a/src/identity/backends/sql/group/delete.rs +++ b/src/identity/backends/sql/group/delete.rs @@ -17,14 +17,17 @@ use sea_orm::entity::*; use crate::db::entity::prelude::Group as DbGroup; use crate::identity::Config; -use crate::identity::backends::sql::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; pub async fn delete>( _conf: &Config, db: &DatabaseConnection, group_id: S, ) -> Result<(), IdentityDatabaseError> { - let res = DbGroup::delete_by_id(group_id.as_ref()).exec(db).await?; + let res = DbGroup::delete_by_id(group_id.as_ref()) + .exec(db) + .await + .map_err(|err| db_err(err, "removing group record"))?; if res.rows_affected == 1 { Ok(()) } else { diff --git a/src/identity/backends/sql/group/get.rs b/src/identity/backends/sql/group/get.rs index 5ca2edad..9389758f 100644 --- a/src/identity/backends/sql/group/get.rs +++ b/src/identity/backends/sql/group/get.rs @@ -17,7 +17,7 @@ use sea_orm::entity::*; use crate::db::entity::prelude::Group as DbGroup; use crate::identity::Config; -use crate::identity::backends::sql::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::Group; pub async fn get>( @@ -27,7 +27,8 @@ pub async fn get>( ) -> Result, IdentityDatabaseError> { Ok(DbGroup::find_by_id(group_id.as_ref()) .one(db) - .await? + .await + .map_err(|err| db_err(err, "fetching group data"))? .map(Into::into)) } diff --git a/src/identity/backends/sql/group/list.rs b/src/identity/backends/sql/group/list.rs index 306e1c78..4e26a10d 100644 --- a/src/identity/backends/sql/group/list.rs +++ b/src/identity/backends/sql/group/list.rs @@ -18,7 +18,7 @@ use sea_orm::query::*; use crate::db::entity::{group, prelude::Group as DbGroup}; use crate::identity::Config; -use crate::identity::backends::sql::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::{Group, GroupListParameters}; pub async fn list( @@ -36,7 +36,10 @@ pub async fn list( group_select = group_select.filter(group::Column::Name.eq(name)); } - let db_groups: Vec = group_select.all(db).await?; + let db_groups: Vec = group_select + .all(db) + .await + .map_err(|err| db_err(err, "listing groups"))?; let results: Vec = db_groups.into_iter().map(Into::into).collect(); Ok(results) diff --git a/src/identity/backends/sql/local_user.rs b/src/identity/backends/sql/local_user.rs index 368b31ea..b04983aa 100644 --- a/src/identity/backends/sql/local_user.rs +++ b/src/identity/backends/sql/local_user.rs @@ -22,7 +22,7 @@ use crate::db::entity::{ local_user, password, prelude::{LocalUser, Password}, }; -use crate::identity::backends::error::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::UserCreate; /// Load local user record with passwords from database @@ -55,7 +55,8 @@ pub async fn load_local_user_with_passwords, S2: AsRef, S3: .find_with_related(Password) .order_by(password::Column::CreatedAtInt, Order::Desc) .all(db) - .await?; + .await + .map_err(|err| db_err(err, "fetching user with passwords"))?; Ok(results.first().cloned()) } @@ -76,7 +77,8 @@ pub async fn load_local_users_passwords>>( .filter(password::Column::LocalUserId.is_in(keys.clone())) .order_by(password::Column::CreatedAtInt, Order::Desc) .all(db) - .await?; + .await + .map_err(|err| db_err(err, "fetching user passwords"))?; // Prepare hashmap of passwords per local_user_id from requested users let mut hashmap: HashMap> = @@ -126,7 +128,10 @@ pub async fn create( entry.failed_auth_count = Set(Some(0)); } - let db_user: local_user::Model = entry.insert(db).await?; + let db_user: local_user::Model = entry + .insert(db) + .await + .map_err(|err| db_err(err, "inserting new user record"))?; Ok(db_user) } @@ -137,11 +142,12 @@ pub async fn get_by_name_and_domain, D: AsRef>( name: N, domain_id: D, ) -> Result, IdentityDatabaseError> { - Ok(LocalUser::find() + LocalUser::find() .filter(local_user::Column::Name.eq(name.as_ref())) .filter(local_user::Column::DomainId.eq(domain_id.as_ref())) .one(db) - .await?) + .await + .map_err(|err| db_err(err, "searching user by name and domain")) } pub async fn get_by_user_id>( @@ -149,8 +155,9 @@ pub async fn get_by_user_id>( db: &DatabaseConnection, user_id: U, ) -> Result, IdentityDatabaseError> { - Ok(LocalUser::find() + LocalUser::find() .filter(local_user::Column::UserId.eq(user_id.as_ref())) .one(db) - .await?) + .await + .map_err(|err| db_err(err, "fetching the user by ID")) } diff --git a/src/identity/backends/sql/password.rs b/src/identity/backends/sql/password.rs index f1983023..631c0a17 100644 --- a/src/identity/backends/sql/password.rs +++ b/src/identity/backends/sql/password.rs @@ -17,7 +17,7 @@ use sea_orm::DatabaseConnection; use sea_orm::entity::*; use crate::db::entity::password; -use crate::identity::backends::error::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; pub async fn create>( db: &DatabaseConnection, @@ -40,6 +40,9 @@ pub async fn create>( entry.expires_at = Set(Some(expire.naive_utc())); entry.expires_at_int = Set(Some(expire.timestamp_micros())); } - let db_entry: password::Model = entry.insert(db).await?; + let db_entry: password::Model = entry + .insert(db) + .await + .map_err(|err| db_err(err, "inserting new password record"))?; Ok(db_entry) } diff --git a/src/identity/backends/sql/user.rs b/src/identity/backends/sql/user.rs index 1bb9266c..8f35a06f 100644 --- a/src/identity/backends/sql/user.rs +++ b/src/identity/backends/sql/user.rs @@ -19,14 +19,17 @@ use serde_json::json; use crate::config::Config; use crate::db::entity::{prelude::User as DbUser, user}; -use crate::identity::backends::error::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::UserCreate; pub async fn get>( db: &DatabaseConnection, user_id: U, ) -> Result, IdentityDatabaseError> { - Ok(DbUser::find_by_id(user_id.as_ref()).one(db).await?) + DbUser::find_by_id(user_id.as_ref()) + .one(db) + .await + .map_err(|err| db_err(err, "fetching user by ID")) } pub(super) async fn create( @@ -62,7 +65,10 @@ pub(super) async fn create( created_at: Set(Some(now)), domain_id: Set(user.domain_id.clone()), }; - let db_user: user::Model = entry.insert(db).await?; + let db_user: user::Model = entry + .insert(db) + .await + .map_err(|err| db_err(err, "inserting user entry"))?; Ok(db_user) } @@ -71,7 +77,10 @@ pub async fn delete>( db: &DatabaseConnection, user_id: U, ) -> Result<(), IdentityDatabaseError> { - let res = DbUser::delete_by_id(user_id.as_ref()).exec(db).await?; + let res = DbUser::delete_by_id(user_id.as_ref()) + .exec(db) + .await + .map_err(|err| db_err(err, "deleting the user record"))?; if res.rows_affected == 1 { Ok(()) } else { diff --git a/src/identity/backends/sql/user_option.rs b/src/identity/backends/sql/user_option.rs index 0122cd44..3cec8016 100644 --- a/src/identity/backends/sql/user_option.rs +++ b/src/identity/backends/sql/user_option.rs @@ -17,17 +17,18 @@ use sea_orm::entity::*; use sea_orm::query::*; use crate::db::entity::{prelude::UserOption as DbUserOptions, user_option}; -use crate::identity::backends::sql::IdentityDatabaseError; +use crate::identity::backends::sql::{IdentityDatabaseError, db_err}; use crate::identity::types::*; pub async fn get>( db: &DatabaseConnection, user_id: S, ) -> Result, IdentityDatabaseError> { - Ok(DbUserOptions::find() + DbUserOptions::find() .filter(user_option::Column::UserId.eq(user_id.as_ref())) .all(db) - .await?) + .await + .map_err(|err| db_err(err, "fetching options of the user")) } impl FromIterator for UserOptions {