From 9c04ad6f584398a5117b12ab5c16a4ab00adb996 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Thu, 28 Nov 2024 15:44:12 +0530 Subject: [PATCH 01/19] chore: rename find_role_by_role_id_in_org_scopr --- crates/diesel_models/src/query/role.rs | 2 +- crates/router/src/analytics.rs | 4 ++-- crates/router/src/core/user.rs | 2 +- crates/router/src/core/user_role.rs | 2 +- crates/router/src/db/kafka_store.rs | 4 ++-- crates/router/src/db/role.rs | 10 ++++++---- crates/router/src/services/authorization/roles.rs | 4 ++-- 7 files changed, 15 insertions(+), 13 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 065a5b6e114e..0dd9ca99ce89 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -43,7 +43,7 @@ impl Role { .await } - pub async fn find_by_role_id_in_org_scope( + pub async fn find_by_role_id_and_org_id( conn: &PgPooledConn, role_id: &str, org_id: &id_type::OrganizationId, diff --git a/crates/router/src/analytics.rs b/crates/router/src/analytics.rs index 96f41f75ee07..cd882e263db0 100644 --- a/crates/router/src/analytics.rs +++ b/crates/router/src/analytics.rs @@ -1886,7 +1886,7 @@ pub mod routes { let role_id = user_role.role_id.clone(); let org_id = user_role.org_id.clone().unwrap_or_default(); async move { - RoleInfo::from_role_id_in_org_scope(&state, &role_id, &org_id) + RoleInfo::from_role_id_and_org_id(&state, &role_id, &org_id) .await .change_context(UserErrors::InternalServerError) .change_context(OpenSearchError::UnknownError) @@ -2008,7 +2008,7 @@ pub mod routes { let role_id = user_role.role_id.clone(); let org_id = user_role.org_id.clone().unwrap_or_default(); async move { - RoleInfo::from_role_id_in_org_scope(&state, &role_id, &org_id) + RoleInfo::from_role_id_and_org_id(&state, &role_id, &org_id) .await .change_context(UserErrors::InternalServerError) .change_context(OpenSearchError::UnknownError) diff --git a/crates/router/src/core/user.rs b/crates/router/src/core/user.rs index 2087d01dbb4b..61499ef5c6d3 100644 --- a/crates/router/src/core/user.rs +++ b/crates/router/src/core/user.rs @@ -1390,7 +1390,7 @@ pub async fn list_user_roles_details( .collect::>() .into_iter() .map(|role_id| async { - let role_info = roles::RoleInfo::from_role_id_in_org_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &role_id, &user_from_token.org_id, diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index 6641e553fd80..efad8ee15670 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -718,7 +718,7 @@ pub async fn list_users_in_lineage( let role_info_map = futures::future::try_join_all(user_roles_set.iter().map(|user_role| async { - roles::RoleInfo::from_role_id_in_org_scope( + roles::RoleInfo::from_role_id_and_org_id( &state, &user_role.role_id, &user_from_token.org_id, diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index a9df1abbc083..ca9ddbe479de 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3513,13 +3513,13 @@ impl RoleInterface for KafkaStore { .await } - async fn find_role_by_role_id_in_org_scope( + async fn find_by_role_id_and_org_id( &self, role_id: &str, org_id: &id_type::OrganizationId, ) -> CustomResult { self.diesel_store - .find_role_by_role_id_in_org_scope(role_id, org_id) + .find_by_role_id_and_org_id(role_id, org_id) .await } diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index d13508356e5a..f3eb647bdb32 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -23,6 +23,7 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; + // Replace this everywhere to find_by_role_id_and_org_id async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, @@ -30,7 +31,8 @@ pub trait RoleInterface { org_id: &id_type::OrganizationId, ) -> CustomResult; - async fn find_role_by_role_id_in_org_scope( + // Change this everywhere the func name + async fn find_by_role_id_and_org_id( &self, role_id: &str, org_id: &id_type::OrganizationId, @@ -100,13 +102,13 @@ impl RoleInterface for Store { } #[instrument(skip_all)] - async fn find_role_by_role_id_in_org_scope( + async fn find_by_role_id_and_org_id( &self, role_id: &str, org_id: &id_type::OrganizationId, ) -> CustomResult { let conn = connection::pg_connection_read(self).await?; - storage::Role::find_by_role_id_in_org_scope(&conn, role_id, org_id) + storage::Role::find_by_role_id_and_org_id(&conn, role_id, org_id) .await .map_err(|error| report!(errors::StorageError::from(error))) } @@ -241,7 +243,7 @@ impl RoleInterface for MockDb { ) } - async fn find_role_by_role_id_in_org_scope( + async fn find_by_role_id_and_org_id( &self, role_id: &str, org_id: &id_type::OrganizationId, diff --git a/crates/router/src/services/authorization/roles.rs b/crates/router/src/services/authorization/roles.rs index bf66eb924669..a1884699f5f2 100644 --- a/crates/router/src/services/authorization/roles.rs +++ b/crates/router/src/services/authorization/roles.rs @@ -95,7 +95,7 @@ impl RoleInfo { } } - pub async fn from_role_id_in_org_scope( + pub async fn from_role_id_and_org_id( state: &SessionState, role_id: &str, org_id: &id_type::OrganizationId, @@ -105,7 +105,7 @@ impl RoleInfo { } else { state .store - .find_role_by_role_id_in_org_scope(role_id, org_id) + .find_by_role_id_and_org_id(role_id, org_id) .await .map(Self::from) } From 35b89e80f7f0c3c9e65ba549eb5df0fadeb9cac4 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Fri, 29 Nov 2024 11:39:50 +0530 Subject: [PATCH 02/19] refactor(users): refactor find_by_role_id_in_merchant_scope query for custom roles --- crates/diesel_models/src/query/role.rs | 21 ++++++++ crates/router/src/analytics.rs | 26 +++------- crates/router/src/core/user.rs | 41 +++++---------- crates/router/src/core/user_role.rs | 36 +++++-------- crates/router/src/core/user_role/role.rs | 36 +++++-------- crates/router/src/db/kafka_store.rs | 12 +++++ crates/router/src/db/role.rs | 51 ++++++++++++++++++- crates/router/src/services/authorization.rs | 6 +-- .../src/services/authorization/roles.rs | 4 +- crates/router/src/utils/user.rs | 11 ++-- crates/router/src/utils/user_role.rs | 32 ++++-------- 11 files changed, 144 insertions(+), 132 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 0dd9ca99ce89..85b84afd9ad3 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -26,6 +26,7 @@ impl Role { .await } + // TODO:remove once find_by_role_id_in_lineage is stable pub async fn find_by_role_id_in_merchant_scope( conn: &PgPooledConn, role_id: &str, @@ -43,6 +44,26 @@ impl Role { .await } + pub async fn find_by_role_id_in_lineage( + conn: &PgPooledConn, + role_id: &str, + merchant_id: &id_type::MerchantId, + org_id: &id_type::OrganizationId, + ) -> StorageResult { + generics::generic_find_one::<::Table, _, _>( + conn, + dsl::role_id + .eq(role_id.to_owned()) + .and(dsl::org_id.eq(org_id.to_owned())) + .and( + dsl::scope.eq(RoleScope::Organization).or(dsl::merchant_id + .eq(merchant_id.to_owned()) + .and(dsl::scope.eq(RoleScope::Merchant))), + ), + ) + .await + } + pub async fn find_by_role_id_and_org_id( conn: &PgPooledConn, role_id: &str, diff --git a/crates/router/src/analytics.rs b/crates/router/src/analytics.rs index cd882e263db0..aaecd417f1b4 100644 --- a/crates/router/src/analytics.rs +++ b/crates/router/src/analytics.rs @@ -1847,15 +1847,10 @@ pub mod routes { json_payload.into_inner(), |state, auth: UserFromToken, req, _| async move { let role_id = auth.role_id; - let role_info = RoleInfo::from_role_id_in_merchant_scope( - &state, - &role_id, - &auth.merchant_id, - &auth.org_id, - ) - .await - .change_context(UserErrors::InternalServerError) - .change_context(OpenSearchError::UnknownError)?; + let role_info = RoleInfo::from_role_id_and_org_id(&state, &role_id, &auth.org_id) + .await + .change_context(UserErrors::InternalServerError) + .change_context(OpenSearchError::UnknownError)?; let permission_groups = role_info.get_permission_groups(); if !permission_groups.contains(&common_enums::PermissionGroup::OperationsView) { return Err(OpenSearchError::AccessForbiddenError)?; @@ -1970,15 +1965,10 @@ pub mod routes { indexed_req, |state, auth: UserFromToken, req, _| async move { let role_id = auth.role_id; - let role_info = RoleInfo::from_role_id_in_merchant_scope( - &state, - &role_id, - &auth.merchant_id, - &auth.org_id, - ) - .await - .change_context(UserErrors::InternalServerError) - .change_context(OpenSearchError::UnknownError)?; + let role_info = RoleInfo::from_role_id_and_org_id(&state, &role_id, &auth.org_id) + .await + .change_context(UserErrors::InternalServerError) + .change_context(OpenSearchError::UnknownError)?; let permission_groups = role_info.get_permission_groups(); if !permission_groups.contains(&common_enums::PermissionGroup::OperationsView) { return Err(OpenSearchError::AccessForbiddenError)?; diff --git a/crates/router/src/core/user.rs b/crates/router/src/core/user.rs index 61499ef5c6d3..dcebc3d60462 100644 --- a/crates/router/src/core/user.rs +++ b/crates/router/src/core/user.rs @@ -99,10 +99,9 @@ pub async fn get_user_details( ) -> UserResponse { let user = user_from_token.get_user_from_db(&state).await?; let verification_days_left = utils::user::get_verification_days_left(&state, &user)?; - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -549,7 +548,7 @@ async fn handle_invitation( .into()); } - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_in_lineage( state, &request.role_id, &user_from_token.merchant_id, @@ -1239,10 +1238,9 @@ pub async fn list_user_roles_details( .await .to_not_found_response(UserErrors::InvalidRoleOperation)?; - let requestor_role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let requestor_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2415,10 +2413,9 @@ pub async fn list_orgs_for_user( state: SessionState, user_from_token: auth::UserFromToken, ) -> UserResponse> { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2473,10 +2470,9 @@ pub async fn list_merchants_for_user_in_org( state: SessionState, user_from_token: auth::UserFromToken, ) -> UserResponse> { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2545,10 +2541,9 @@ pub async fn list_profiles_for_user_in_org_and_merchant_account( state: SessionState, user_from_token: auth::UserFromToken, ) -> UserResponse> { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2634,10 +2629,9 @@ pub async fn switch_org_for_user( .into()); } - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2685,10 +2679,9 @@ pub async fn switch_org_for_user( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_merchant_id_org_id( + utils::user_role::set_role_permissions_in_cache_by_role_id_org_id( &state, &user_role.role_id, - &merchant_id, &request.org_id, ) .await; @@ -2714,10 +2707,9 @@ pub async fn switch_merchant_for_user_in_org( } let key_manager_state = &(&state).into(); - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2870,13 +2862,8 @@ pub async fn switch_merchant_for_user_in_org( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_merchant_id_org_id( - &state, - &role_id, - &merchant_id, - &org_id, - ) - .await; + utils::user_role::set_role_permissions_in_cache_by_role_id_org_id(&state, &role_id, &org_id) + .await; let response = user_api::TokenResponse { token: token.clone(), @@ -2899,10 +2886,9 @@ pub async fn switch_profile_for_user_in_org_and_merchant( } let key_manager_state = &(&state).into(); - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -2976,10 +2962,9 @@ pub async fn switch_profile_for_user_in_org_and_merchant( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_merchant_id_org_id( + utils::user_role::set_role_permissions_in_cache_by_role_id_org_id( &state, &role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await; diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index efad8ee15670..c0b15269b9ce 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -83,10 +83,9 @@ pub async fn get_parent_group_info( state: SessionState, user_from_token: auth::UserFromToken, ) -> UserResponse> { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -119,14 +118,10 @@ pub async fn update_user_role( req: user_role_api::UpdateUserRoleRequest, _req_state: ReqState, ) -> UserResponse<()> { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( - &state, - &req.role_id, - &user_from_token.merchant_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleId)?; + let role_info = + roles::RoleInfo::from_role_id_and_org_id(&state, &req.role_id, &user_from_token.org_id) + .await + .to_not_found_response(UserErrors::InvalidRoleId)?; if !role_info.is_updatable() { return Err(report!(UserErrors::InvalidRoleOperation)) @@ -144,10 +139,9 @@ pub async fn update_user_role( .attach_printable("User Changing their own role"); } - let updator_role = roles::RoleInfo::from_role_id_in_merchant_scope( + let updator_role = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -177,10 +171,9 @@ pub async fn update_user_role( }; if let Some(user_role) = v2_user_role_to_be_updated { - let role_to_be_updated = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_to_be_updated = roles::RoleInfo::from_role_id_and_org_id( &state, &user_role.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -250,10 +243,9 @@ pub async fn update_user_role( }; if let Some(user_role) = v1_user_role_to_be_updated { - let role_to_be_updated = roles::RoleInfo::from_role_id_in_merchant_scope( + let role_to_be_updated = roles::RoleInfo::from_role_id_and_org_id( &state, &user_role.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -457,10 +449,9 @@ pub async fn delete_user_role( .attach_printable("User deleting himself"); } - let deletion_requestor_role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let deletion_requestor_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -491,10 +482,9 @@ pub async fn delete_user_role( }; if let Some(role_to_be_deleted) = user_role_v2 { - let target_role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let target_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &role_to_be_deleted.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -553,10 +543,9 @@ pub async fn delete_user_role( }; if let Some(role_to_be_deleted) = user_role_v1 { - let target_role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let target_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &role_to_be_deleted.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -632,10 +621,9 @@ pub async fn list_users_in_lineage( user_from_token: auth::UserFromToken, request: user_role_api::ListUsersInEntityRequest, ) -> UserResponse> { - let requestor_role_info = roles::RoleInfo::from_role_id_in_merchant_scope( + let requestor_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 0250415d4fda..4eee04de1228 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -116,14 +116,10 @@ pub async fn get_role_with_groups( user_from_token: UserFromToken, role: role_api::GetRoleRequest, ) -> UserResponse { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( - &state, - &role.role_id, - &user_from_token.merchant_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleId)?; + let role_info = + roles::RoleInfo::from_role_id_and_org_id(&state, &role.role_id, &user_from_token.org_id) + .await + .to_not_found_response(UserErrors::InvalidRoleId)?; if role_info.is_internal() { return Err(UserErrors::InvalidRoleId.into()); @@ -144,14 +140,10 @@ pub async fn get_parent_info_for_role( user_from_token: UserFromToken, role: role_api::GetRoleRequest, ) -> UserResponse { - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( - &state, - &role.role_id, - &user_from_token.merchant_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleId)?; + let role_info = + roles::RoleInfo::from_role_id_and_org_id(&state, &role.role_id, &user_from_token.org_id) + .await + .to_not_found_response(UserErrors::InvalidRoleId)?; if role_info.is_internal() { return Err(UserErrors::InvalidRoleId.into()); @@ -207,14 +199,10 @@ pub async fn update_role( utils::user_role::validate_role_groups(groups)?; } - let role_info = roles::RoleInfo::from_role_id_in_merchant_scope( - &state, - role_id, - &user_from_token.merchant_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleOperation)?; + let role_info = + roles::RoleInfo::from_role_id_and_org_id(&state, role_id, &user_from_token.org_id) + .await + .to_not_found_response(UserErrors::InvalidRoleOperation)?; if matches!(role_info.get_scope(), RoleScope::Organization) && user_from_token.role_id != common_utils::consts::ROLE_ID_ORGANIZATION_ADMIN diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index ca9ddbe479de..655e91174123 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3502,6 +3502,7 @@ impl RoleInterface for KafkaStore { self.diesel_store.find_role_by_role_id(role_id).await } + //TODO:remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, @@ -3513,6 +3514,17 @@ impl RoleInterface for KafkaStore { .await } + async fn find_role_by_role_id_in_lineage( + &self, + role_id: &str, + merchant_id: &id_type::MerchantId, + org_id: &id_type::OrganizationId, + ) -> CustomResult { + self.diesel_store + .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id) + .await + } + async fn find_by_role_id_and_org_id( &self, role_id: &str, diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index f3eb647bdb32..2d9076a908e6 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -23,7 +23,7 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; - // Replace this everywhere to find_by_role_id_and_org_id + //TODO:remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, @@ -31,7 +31,13 @@ pub trait RoleInterface { org_id: &id_type::OrganizationId, ) -> CustomResult; - // Change this everywhere the func name + async fn find_role_by_role_id_in_lineage( + &self, + role_id: &str, + merchant_id: &id_type::MerchantId, + org_id: &id_type::OrganizationId, + ) -> CustomResult; + async fn find_by_role_id_and_org_id( &self, role_id: &str, @@ -88,6 +94,7 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } + //TODO:remove once find_by_role_id_in_lineage is stable #[instrument(skip_all)] async fn find_role_by_role_id_in_merchant_scope( &self, @@ -101,6 +108,19 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } + #[instrument(skip_all)] + async fn find_role_by_role_id_in_lineage( + &self, + role_id: &str, + merchant_id: &id_type::MerchantId, + org_id: &id_type::OrganizationId, + ) -> CustomResult { + let conn = connection::pg_connection_read(self).await?; + storage::Role::find_by_role_id_in_lineage(&conn, role_id, merchant_id, org_id) + .await + .map_err(|error| report!(errors::StorageError::from(error))) + } + #[instrument(skip_all)] async fn find_by_role_id_and_org_id( &self, @@ -219,6 +239,7 @@ impl RoleInterface for MockDb { ) } + // TODO:remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, @@ -243,6 +264,32 @@ impl RoleInterface for MockDb { ) } + async fn find_role_by_role_id_in_lineage( + &self, + role_id: &str, + merchant_id: &id_type::MerchantId, + org_id: &id_type::OrganizationId, + ) -> CustomResult { + let roles = self.roles.lock().await; + roles + .iter() + .find(|role| { + role.role_id == role_id + && role.org_id == *org_id + && ((role.scope == enums::RoleScope::Organization) + || (role.merchant_id == *merchant_id + && role.scope == enums::RoleScope::Merchant)) + }) + .cloned() + .ok_or( + errors::StorageError::ValueNotFound(format!( + "No role available in merchant scope for role_id = {role_id}, \ + merchant_id = {merchant_id:?} and org_id = {org_id:?}" + )) + .into(), + ) + } + async fn find_by_role_id_and_org_id( &self, role_id: &str, diff --git a/crates/router/src/services/authorization.rs b/crates/router/src/services/authorization.rs index 87ff9f6abd59..adb220ee0213 100644 --- a/crates/router/src/services/authorization.rs +++ b/crates/router/src/services/authorization.rs @@ -33,8 +33,7 @@ where return Ok(role_info.clone()); } - let role_info = - get_role_info_from_db(state, &token.role_id, &token.merchant_id, &token.org_id).await?; + let role_info = get_role_info_from_db(state, &token.role_id, &token.org_id).await?; let token_expiry = i64::try_from(token.exp).change_context(ApiErrorResponse::InternalServerError)?; @@ -66,7 +65,6 @@ pub fn get_cache_key_from_role_id(role_id: &str) -> String { async fn get_role_info_from_db( state: &A, role_id: &str, - merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, ) -> RouterResult where @@ -74,7 +72,7 @@ where { state .store() - .find_role_by_role_id_in_merchant_scope(role_id, merchant_id, org_id) + .find_by_role_id_and_org_id(role_id, org_id) .await .map(roles::RoleInfo::from) .to_not_found_response(ApiErrorResponse::InvalidJwtToken) diff --git a/crates/router/src/services/authorization/roles.rs b/crates/router/src/services/authorization/roles.rs index a1884699f5f2..c200ad70f820 100644 --- a/crates/router/src/services/authorization/roles.rs +++ b/crates/router/src/services/authorization/roles.rs @@ -78,7 +78,7 @@ impl RoleInfo { }) } - pub async fn from_role_id_in_merchant_scope( + pub async fn from_role_id_in_lineage( state: &SessionState, role_id: &str, merchant_id: &id_type::MerchantId, @@ -89,7 +89,7 @@ impl RoleInfo { } else { state .store - .find_role_by_role_id_in_merchant_scope(role_id, merchant_id, org_id) + .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id) .await .map(Self::from) } diff --git a/crates/router/src/utils/user.rs b/crates/router/src/utils/user.rs index f115a16c0622..02722f151eb0 100644 --- a/crates/router/src/utils/user.rs +++ b/crates/router/src/utils/user.rs @@ -74,14 +74,9 @@ impl UserFromToken { } pub async fn get_role_info_from_db(&self, state: &SessionState) -> UserResult { - RoleInfo::from_role_id_in_merchant_scope( - state, - &self.role_id, - &self.merchant_id, - &self.org_id, - ) - .await - .change_context(UserErrors::InternalServerError) + RoleInfo::from_role_id_and_org_id(state, &self.role_id, &self.org_id) + .await + .change_context(UserErrors::InternalServerError) } } diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index aaf313a196e5..a0e1699a354b 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -72,31 +72,21 @@ pub async fn set_role_permissions_in_cache_by_user_role( state: &SessionState, user_role: &UserRole, ) -> bool { - let Some(ref merchant_id) = user_role.merchant_id else { - return false; - }; - let Some(ref org_id) = user_role.org_id else { return false; }; - set_role_permissions_in_cache_if_required( - state, - user_role.role_id.as_str(), - merchant_id, - org_id, - ) - .await - .map_err(|e| logger::error!("Error setting permissions in cache {:?}", e)) - .is_ok() + set_role_permissions_in_cache_if_required(state, user_role.role_id.as_str(), org_id) + .await + .map_err(|e| logger::error!("Error setting permissions in cache {:?}", e)) + .is_ok() } -pub async fn set_role_permissions_in_cache_by_role_id_merchant_id_org_id( +pub async fn set_role_permissions_in_cache_by_role_id_org_id( state: &SessionState, role_id: &str, - merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, ) -> bool { - set_role_permissions_in_cache_if_required(state, role_id, merchant_id, org_id) + set_role_permissions_in_cache_if_required(state, role_id, org_id) .await .map_err(|e| logger::error!("Error setting permissions in cache {:?}", e)) .is_ok() @@ -105,18 +95,16 @@ pub async fn set_role_permissions_in_cache_by_role_id_merchant_id_org_id( pub async fn set_role_permissions_in_cache_if_required( state: &SessionState, role_id: &str, - merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, ) -> UserResult<()> { if roles::predefined_roles::PREDEFINED_ROLES.contains_key(role_id) { return Ok(()); } - let role_info = - roles::RoleInfo::from_role_id_in_merchant_scope(state, role_id, merchant_id, org_id) - .await - .change_context(UserErrors::InternalServerError) - .attach_printable("Error getting role_info from role_id")?; + let role_info = roles::RoleInfo::from_role_id_and_org_id(state, role_id, org_id) + .await + .change_context(UserErrors::InternalServerError) + .attach_printable("Error getting role_info from role_id")?; authz::set_role_info_in_cache( state, From 5cb5954c1d5945775e8c4f58d4ea5839f5323c62 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Fri, 29 Nov 2024 12:46:55 +0530 Subject: [PATCH 03/19] fix: changes for list roles and list roles at entity-level --- crates/diesel_models/src/query/role.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 85b84afd9ad3..236302cc907b 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -109,9 +109,11 @@ impl Role { merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, ) -> StorageResult> { - let predicate = dsl::merchant_id.eq(merchant_id.to_owned()).or(dsl::org_id - .eq(org_id.to_owned()) - .and(dsl::scope.eq(RoleScope::Organization))); + let predicate = dsl::org_id.eq(org_id.to_owned()).and( + dsl::scope.eq(RoleScope::Organization).or(dsl::merchant_id + .eq(merchant_id.to_owned()) + .and(dsl::scope.eq(RoleScope::Merchant))), + ); generics::generic_filter::<::Table, _, _, _>( conn, @@ -136,9 +138,10 @@ impl Role { if let Some(merchant_id) = merchant_id { query = query.filter( - dsl::merchant_id + (dsl::merchant_id .eq(merchant_id) - .or(dsl::scope.eq(RoleScope::Organization)), + .and(dsl::scope.eq(RoleScope::Merchant))) + .or(dsl::scope.eq(RoleScope::Organization)), ); } From 113788691b351a50ad6b9c40d676ff89f2aa6d50 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 2 Dec 2024 16:34:15 +0530 Subject: [PATCH 04/19] fix: changes for backfilling user-roles --- crates/diesel_models/src/user_role.rs | 22 +++++++++++-------- crates/router/src/core/user.rs | 7 +++--- crates/router/src/core/user_role.rs | 15 ++++++++----- crates/router/src/core/user_role/role.rs | 22 ++++++++++++++----- crates/router/src/utils/user_role.rs | 2 +- .../down.sql | 2 ++ .../up.sql | 10 +++++++++ 7 files changed, 56 insertions(+), 24 deletions(-) create mode 100644 migrations/2024-12-02-110129_update-user-role-entity-type/down.sql create mode 100644 migrations/2024-12-02-110129_update-user-role-entity-type/up.sql diff --git a/crates/diesel_models/src/user_role.rs b/crates/diesel_models/src/user_role.rs index 04f3264b45e3..89fb80d8a52c 100644 --- a/crates/diesel_models/src/user_role.rs +++ b/crates/diesel_models/src/user_role.rs @@ -29,16 +29,20 @@ pub struct UserRole { impl UserRole { pub fn get_entity_id_and_type(&self) -> Option<(String, EntityType)> { - match (self.version, self.role_id.as_str()) { - (enums::UserRoleVersion::V1, consts::ROLE_ID_ORGANIZATION_ADMIN) => { - let org_id = self.org_id.clone()?.get_string_repr().to_string(); - Some((org_id, EntityType::Organization)) + match self.version { + enums::UserRoleVersion::V1 if self.entity_type.is_none() => { + match self.role_id.as_str() { + consts::ROLE_ID_ORGANIZATION_ADMIN => { + let org_id = self.org_id.clone()?.get_string_repr().to_string(); + Some((org_id, EntityType::Organization)) + } + _ => { + let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string(); + Some((merchant_id, EntityType::Merchant)) + } + } } - (enums::UserRoleVersion::V1, _) => { - let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string(); - Some((merchant_id, EntityType::Merchant)) - } - (enums::UserRoleVersion::V2, _) => self.entity_id.clone().zip(self.entity_type), + _ => self.entity_id.clone().zip(self.entity_type), } } } diff --git a/crates/router/src/core/user.rs b/crates/router/src/core/user.rs index dcebc3d60462..da01ff174ebc 100644 --- a/crates/router/src/core/user.rs +++ b/crates/router/src/core/user.rs @@ -2679,7 +2679,7 @@ pub async fn switch_org_for_user( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_org_id( + utils::user_role::set_role_info_in_cache_by_role_id_org_id( &state, &user_role.role_id, &request.org_id, @@ -2862,8 +2862,7 @@ pub async fn switch_merchant_for_user_in_org( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_org_id(&state, &role_id, &org_id) - .await; + utils::user_role::set_role_info_in_cache_by_role_id_org_id(&state, &role_id, &org_id).await; let response = user_api::TokenResponse { token: token.clone(), @@ -2962,7 +2961,7 @@ pub async fn switch_profile_for_user_in_org_and_merchant( ) .await?; - utils::user_role::set_role_permissions_in_cache_by_role_id_org_id( + utils::user_role::set_role_info_in_cache_by_role_id_org_id( &state, &role_id, &user_from_token.org_id, diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index c0b15269b9ce..9fcc5bbc7613 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -118,10 +118,14 @@ pub async fn update_user_role( req: user_role_api::UpdateUserRoleRequest, _req_state: ReqState, ) -> UserResponse<()> { - let role_info = - roles::RoleInfo::from_role_id_and_org_id(&state, &req.role_id, &user_from_token.org_id) - .await - .to_not_found_response(UserErrors::InvalidRoleId)?; + let role_info = roles::RoleInfo::from_role_id_in_lineage( + &state, + &req.role_id, + &user_from_token.merchant_id, + &user_from_token.org_id, + ) + .await + .to_not_found_response(UserErrors::InvalidRoleId)?; if !role_info.is_updatable() { return Err(report!(UserErrors::InvalidRoleOperation)) @@ -449,9 +453,10 @@ pub async fn delete_user_role( .attach_printable("User deleting himself"); } - let deletion_requestor_role_info = roles::RoleInfo::from_role_id_and_org_id( + let deletion_requestor_role_info = roles::RoleInfo::from_role_id_in_lineage( &state, &user_from_token.role_id, + &user_from_token.merchant_id, &user_from_token.org_id, ) .await diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 4eee04de1228..90ad4e77cd52 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -199,13 +199,25 @@ pub async fn update_role( utils::user_role::validate_role_groups(groups)?; } - let role_info = - roles::RoleInfo::from_role_id_and_org_id(&state, role_id, &user_from_token.org_id) - .await - .to_not_found_response(UserErrors::InvalidRoleOperation)?; + let role_info = roles::RoleInfo::from_role_id_in_lineage( + &state, + role_id, + &user_from_token.merchant_id, + &user_from_token.org_id, + ) + .await + .to_not_found_response(UserErrors::InvalidRoleOperation)?; + + let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id( + &state, + &user_from_token.role_id, + &user_from_token.org_id, + ) + .await + .to_not_found_response(UserErrors::InvalidRoleId)?; if matches!(role_info.get_scope(), RoleScope::Organization) - && user_from_token.role_id != common_utils::consts::ROLE_ID_ORGANIZATION_ADMIN + && user_from_token_role_info.get_entity_type() != EntityType::Organization { return Err(report!(UserErrors::InvalidRoleOperation)) .attach_printable("Non org admin user changing org level role"); diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index a0e1699a354b..96b0c84e46ca 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -81,7 +81,7 @@ pub async fn set_role_permissions_in_cache_by_user_role( .is_ok() } -pub async fn set_role_permissions_in_cache_by_role_id_org_id( +pub async fn set_role_info_in_cache_by_role_id_org_id( state: &SessionState, role_id: &str, org_id: &id_type::OrganizationId, diff --git a/migrations/2024-12-02-110129_update-user-role-entity-type/down.sql b/migrations/2024-12-02-110129_update-user-role-entity-type/down.sql new file mode 100644 index 000000000000..c7c9cbeb4017 --- /dev/null +++ b/migrations/2024-12-02-110129_update-user-role-entity-type/down.sql @@ -0,0 +1,2 @@ +-- This file should undo anything in `up.sql` +SELECT 1; \ No newline at end of file diff --git a/migrations/2024-12-02-110129_update-user-role-entity-type/up.sql b/migrations/2024-12-02-110129_update-user-role-entity-type/up.sql new file mode 100644 index 000000000000..f2759f030d50 --- /dev/null +++ b/migrations/2024-12-02-110129_update-user-role-entity-type/up.sql @@ -0,0 +1,10 @@ +-- Your SQL goes here +UPDATE user_roles +SET + entity_type = CASE + WHEN role_id = 'org_admin' THEN 'organization' + ELSE 'merchant' + END +WHERE + version = 'v1' + AND entity_type IS NULL; \ No newline at end of file From 1cd045f72f99f01ed4c84435f157dacb5a1076af Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Tue, 3 Dec 2024 12:32:01 +0530 Subject: [PATCH 05/19] fix: addressed comments --- crates/diesel_models/src/query/role.rs | 2 +- crates/router/src/core/user_role.rs | 9 +++++---- crates/router/src/core/user_role/role.rs | 10 ++-------- crates/router/src/db/kafka_store.rs | 2 +- crates/router/src/db/role.rs | 6 +++--- .../router/src/types/domain/user/decision_manager.rs | 6 ++---- crates/router/src/utils/user_role.rs | 8 ++++---- 7 files changed, 18 insertions(+), 25 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 236302cc907b..c94edb54a05a 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -26,7 +26,7 @@ impl Role { .await } - // TODO:remove once find_by_role_id_in_lineage is stable + // TODO:Remove once find_by_role_id_in_lineage is stable pub async fn find_by_role_id_in_merchant_scope( conn: &PgPooledConn, role_id: &str, diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index 49ffa2546a0e..1f4d2880573b 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -485,10 +485,9 @@ pub async fn delete_user_role( .attach_printable("User deleting himself"); } - let deletion_requestor_role_info = roles::RoleInfo::from_role_id_in_lineage( + let deletion_requestor_role_info = roles::RoleInfo::from_role_id_and_org_id( &state, &user_from_token.role_id, - &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -523,9 +522,10 @@ pub async fn delete_user_role( }; if let Some(role_to_be_deleted) = user_role_v2 { - let target_role_info = roles::RoleInfo::from_role_id_and_org_id( + let target_role_info = roles::RoleInfo::from_role_id_in_lineage( &state, &role_to_be_deleted.role_id, + &user_from_token.merchant_id, &user_from_token.org_id, ) .await @@ -592,9 +592,10 @@ pub async fn delete_user_role( }; if let Some(role_to_be_deleted) = user_role_v1 { - let target_role_info = roles::RoleInfo::from_role_id_and_org_id( + let target_role_info = roles::RoleInfo::from_role_id_in_lineage( &state, &role_to_be_deleted.role_id, + &user_from_token.merchant_id, &user_from_token.org_id, ) .await diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 90ad4e77cd52..3b997751b85e 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -208,16 +208,10 @@ pub async fn update_role( .await .to_not_found_response(UserErrors::InvalidRoleOperation)?; - let user_from_token_role_info = roles::RoleInfo::from_role_id_and_org_id( - &state, - &user_from_token.role_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleId)?; + let user_role_info = user_from_token.get_role_info_from_db(&state).await?; if matches!(role_info.get_scope(), RoleScope::Organization) - && user_from_token_role_info.get_entity_type() != EntityType::Organization + && user_role_info.get_entity_type() != EntityType::Organization { return Err(report!(UserErrors::InvalidRoleOperation)) .attach_printable("Non org admin user changing org level role"); diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index fd7914f47c4f..9bb9b56ad9e6 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3508,7 +3508,7 @@ impl RoleInterface for KafkaStore { self.diesel_store.find_role_by_role_id(role_id).await } - //TODO:remove once find_by_role_id_in_lineage is stable + //TODO:Remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 2d9076a908e6..30b5e01607ed 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -23,7 +23,7 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; - //TODO:remove once find_by_role_id_in_lineage is stable + //TODO:Remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, @@ -94,7 +94,7 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } - //TODO:remove once find_by_role_id_in_lineage is stable + //TODO:Remove once find_by_role_id_in_lineage is stable #[instrument(skip_all)] async fn find_role_by_role_id_in_merchant_scope( &self, @@ -239,7 +239,7 @@ impl RoleInterface for MockDb { ) } - // TODO:remove once find_by_role_id_in_lineage is stable + // TODO:Remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, diff --git a/crates/router/src/types/domain/user/decision_manager.rs b/crates/router/src/types/domain/user/decision_manager.rs index 10990da6ccbd..5a92a6c9d59f 100644 --- a/crates/router/src/types/domain/user/decision_manager.rs +++ b/crates/router/src/types/domain/user/decision_manager.rs @@ -337,8 +337,7 @@ impl NextFlow { .change_context(UserErrors::InternalServerError)? .pop() .ok_or(UserErrors::InternalServerError)?; - utils::user_role::set_role_permissions_in_cache_by_user_role(state, &user_role) - .await; + utils::user_role::set_role_info_in_cache_by_user_role(state, &user_role).await; jwt_flow.generate_jwt(state, self, &user_role).await } @@ -357,8 +356,7 @@ impl NextFlow { { self.user.get_verification_days_left(state)?; } - utils::user_role::set_role_permissions_in_cache_by_user_role(state, user_role) - .await; + utils::user_role::set_role_info_in_cache_by_user_role(state, user_role).await; jwt_flow.generate_jwt(state, self, user_role).await } diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index ab9b3ac9df59..f140a30ea440 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -68,14 +68,14 @@ pub async fn validate_role_name( Ok(()) } -pub async fn set_role_permissions_in_cache_by_user_role( +pub async fn set_role_info_in_cache_by_user_role( state: &SessionState, user_role: &UserRole, ) -> bool { let Some(ref org_id) = user_role.org_id else { return false; }; - set_role_permissions_in_cache_if_required(state, user_role.role_id.as_str(), org_id) + set_role_info_in_cache_if_required(state, user_role.role_id.as_str(), org_id) .await .map_err(|e| logger::error!("Error setting permissions in cache {:?}", e)) .is_ok() @@ -86,13 +86,13 @@ pub async fn set_role_info_in_cache_by_role_id_org_id( role_id: &str, org_id: &id_type::OrganizationId, ) -> bool { - set_role_permissions_in_cache_if_required(state, role_id, org_id) + set_role_info_in_cache_if_required(state, role_id, org_id) .await .map_err(|e| logger::error!("Error setting permissions in cache {:?}", e)) .is_ok() } -pub async fn set_role_permissions_in_cache_if_required( +pub async fn set_role_info_in_cache_if_required( state: &SessionState, role_id: &str, org_id: &id_type::OrganizationId, From 2b24810fa004627edde82430f2f1e257f2fcc1e0 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Tue, 3 Dec 2024 14:51:09 +0530 Subject: [PATCH 06/19] refactor: code refactoring --- crates/diesel_models/src/user_role.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/crates/diesel_models/src/user_role.rs b/crates/diesel_models/src/user_role.rs index 89fb80d8a52c..81541dd81165 100644 --- a/crates/diesel_models/src/user_role.rs +++ b/crates/diesel_models/src/user_role.rs @@ -29,18 +29,14 @@ pub struct UserRole { impl UserRole { pub fn get_entity_id_and_type(&self) -> Option<(String, EntityType)> { - match self.version { - enums::UserRoleVersion::V1 if self.entity_type.is_none() => { - match self.role_id.as_str() { - consts::ROLE_ID_ORGANIZATION_ADMIN => { - let org_id = self.org_id.clone()?.get_string_repr().to_string(); - Some((org_id, EntityType::Organization)) - } - _ => { - let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string(); - Some((merchant_id, EntityType::Merchant)) - } - } + match (self.version, self.entity_type, self.role_id.as_str()) { + (enums::UserRoleVersion::V1, None, consts::ROLE_ID_ORGANIZATION_ADMIN) => { + let org_id = self.org_id.clone()?.get_string_repr().to_string(); + Some((org_id, EntityType::Organization)) + } + (enums::UserRoleVersion::V1, None, _) => { + let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string(); + Some((merchant_id, EntityType::Merchant)) } _ => self.entity_id.clone().zip(self.entity_type), } From f70f3cf57b9b0c6acb1047941494f7f6f0c1cb98 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Thu, 5 Dec 2024 15:08:56 +0530 Subject: [PATCH 07/19] fix: addressed comments --- crates/diesel_models/src/query/role.rs | 2 +- crates/diesel_models/src/user_role.rs | 5 ++++- crates/router/src/core/user_role/role.rs | 4 +++- crates/router/src/db/role.rs | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index c94edb54a05a..6f6a1404ee2c 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -26,7 +26,7 @@ impl Role { .await } - // TODO:Remove once find_by_role_id_in_lineage is stable + // TODO: Remove once find_by_role_id_in_lineage is stable pub async fn find_by_role_id_in_merchant_scope( conn: &PgPooledConn, role_id: &str, diff --git a/crates/diesel_models/src/user_role.rs b/crates/diesel_models/src/user_role.rs index 81541dd81165..08449685b291 100644 --- a/crates/diesel_models/src/user_role.rs +++ b/crates/diesel_models/src/user_role.rs @@ -38,7 +38,10 @@ impl UserRole { let merchant_id = self.merchant_id.clone()?.get_string_repr().to_string(); Some((merchant_id, EntityType::Merchant)) } - _ => self.entity_id.clone().zip(self.entity_type), + (enums::UserRoleVersion::V1, Some(_), _) => { + self.entity_id.clone().zip(self.entity_type) + } + (enums::UserRoleVersion::V2, _, _) => self.entity_id.clone().zip(self.entity_type), } } } diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index a0fc82502ee1..0a9b7d74a4b8 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -76,8 +76,10 @@ pub async fn create_role( ) .await?; + let user_role_info = user_from_token.get_role_info_from_db(&state).await?; + if matches!(req.role_scope, RoleScope::Organization) - && user_from_token.role_id != common_utils::consts::ROLE_ID_ORGANIZATION_ADMIN + && user_role_info.get_entity_type() != EntityType::Organization { return Err(report!(UserErrors::InvalidRoleOperation)) .attach_printable("Non org admin user creating org level role"); diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 30b5e01607ed..877a4c540774 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -239,7 +239,7 @@ impl RoleInterface for MockDb { ) } - // TODO:Remove once find_by_role_id_in_lineage is stable + // TODO: Remove once find_by_role_id_in_lineage is stable async fn find_role_by_role_id_in_merchant_scope( &self, role_id: &str, From abeac9cfad190074d786e86cd8a0b68f5c53d02c Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Wed, 11 Dec 2024 16:22:05 +0530 Subject: [PATCH 08/19] changes-for-profile-read --- crates/api_models/src/user_role/role.rs | 2 + crates/common_enums/src/enums.rs | 18 +- crates/diesel_models/src/query/role.rs | 157 ++++++++++++- crates/diesel_models/src/role.rs | 37 +++ crates/diesel_models/src/schema.rs | 2 + crates/diesel_models/src/schema_v2.rs | 2 + crates/router/src/core/user_role/role.rs | 173 ++++++++++----- crates/router/src/db/kafka_store.rs | 23 ++ crates/router/src/db/role.rs | 210 +++++++++++++++++- crates/router/src/utils/user_role.rs | 60 ++++- .../down.sql | 2 + .../up.sql | 2 + .../down.sql | 2 + .../up.sql | 3 + 14 files changed, 622 insertions(+), 71 deletions(-) create mode 100644 migrations/2024-10-17-073555_add-profile-id-to-roles/down.sql create mode 100644 migrations/2024-10-17-073555_add-profile-id-to-roles/up.sql create mode 100644 migrations/2024-10-17-123943_add-profile-enum-in-role-scope/down.sql create mode 100644 migrations/2024-10-17-123943_add-profile-enum-in-role-scope/up.sql diff --git a/crates/api_models/src/user_role/role.rs b/crates/api_models/src/user_role/role.rs index 7c877cd74777..6b8736d3e764 100644 --- a/crates/api_models/src/user_role/role.rs +++ b/crates/api_models/src/user_role/role.rs @@ -7,6 +7,7 @@ pub struct CreateRoleRequest { pub role_name: String, pub groups: Vec, pub role_scope: RoleScope, + pub entity_type: Option, } #[derive(Debug, serde::Deserialize, serde::Serialize)] @@ -21,6 +22,7 @@ pub struct RoleInfoWithGroupsResponse { pub groups: Vec, pub role_name: String, pub role_scope: RoleScope, + pub entity_type: EntityType, } #[derive(Debug, serde::Serialize)] diff --git a/crates/common_enums/src/enums.rs b/crates/common_enums/src/enums.rs index 7d8359800560..c4b427476585 100644 --- a/crates/common_enums/src/enums.rs +++ b/crates/common_enums/src/enums.rs @@ -2736,6 +2736,8 @@ pub enum TransactionType { Debug, Eq, PartialEq, + Ord, + PartialOrd, serde::Deserialize, serde::Serialize, strum::Display, @@ -2745,8 +2747,19 @@ pub enum TransactionType { #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] pub enum RoleScope { - Merchant, - Organization, + Organization = 2, + Merchant = 1, + Profile = 0, +} + +impl From for EntityType { + fn from(role_scope: RoleScope) -> Self { + match role_scope { + RoleScope::Organization => Self::Organization, + RoleScope::Merchant => Self::Merchant, + RoleScope::Profile => Self::Profile, + } + } } /// Indicates the transaction status @@ -3202,6 +3215,7 @@ pub enum ApiVersion { serde::Serialize, strum::Display, strum::EnumString, + strum::EnumIter, ToSchema, Hash, )] diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 6f6a1404ee2c..09c049897a25 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -1,4 +1,5 @@ use async_bb8_diesel::AsyncRunQueryDsl; +use common_enums::EntityType; use common_utils::id_type; use diesel::{ associations::HasTable, debug_query, pg::Pg, result::Error as DieselError, @@ -104,6 +105,7 @@ impl Role { .await } + // TODO: Change as it will break pub async fn list_roles( conn: &PgPooledConn, merchant_id: &id_type::MerchantId, @@ -125,11 +127,72 @@ impl Role { .await } + pub async fn find_role_by_role_name_in_lineage( + conn: &PgPooledConn, + role_name: &str, + entity_type: ListRolesByEntityPayload, + ) -> StorageResult { + let mut query = ::table().into_boxed(); + + if let Some(org_id) = entity_type.get_organization_id() { + query = query.filter( + dsl::org_id + .eq(org_id) + .and(dsl::scope.eq(RoleScope::Organization)) + .and(dsl::entity_type.eq_any(vec![ + EntityType::Profile, + EntityType::Merchant, + EntityType::Organization, + ])), + ); + }; + + if let Some(merchant_id) = entity_type.get_merchant_id() { + query = query.or_filter( + dsl::merchant_id + .eq(merchant_id) + .and(dsl::scope.eq(RoleScope::Merchant)) + .and(dsl::entity_type.eq_any(vec![EntityType::Profile, EntityType::Merchant])), + ); + }; + + if let Some(profile_id) = entity_type.get_profile_id() { + query = query.or_filter( + dsl::profile_id + .eq(profile_id) + .and(dsl::scope.eq(RoleScope::Profile)) + .and(dsl::entity_type.eq(EntityType::Profile)), + ); + }; + + query = query.filter(dsl::role_name.eq(role_name.to_owned())); + + router_env::logger::debug!(query = %debug_query::(&query).to_string()); + + match generics::db_metrics::track_database_call::( + query.get_results_async(conn), + generics::db_metrics::DatabaseOperation::Filter, + ) + .await + { + Err(err) => match err { + DieselError::NotFound => { + Err(report!(err)).change_context(errors::DatabaseError::NotFound) + } + _ => Err(report!(err)).change_context(errors::DatabaseError::Others), + }, + Ok(mut role_info) => role_info + .pop() + .ok_or(error_stack::report!(errors::DatabaseError::NotFound)), + } + } + + //TODO: Remove once generic_list_roles_by_entity_type is stable pub async fn generic_roles_list_for_org( conn: &PgPooledConn, org_id: id_type::OrganizationId, merchant_id: Option, - entity_type: Option, + entity_type: Option, limit: Option, ) -> StorageResult> { let mut query = ::table() @@ -170,4 +233,96 @@ impl Role { }, } } + + pub async fn generic_list_roles_by_entity_type( + conn: &PgPooledConn, + entity_type: ListRolesByEntityPayload, + is_lineage_data_required: bool, + limit: Option, + ) -> StorageResult> { + let mut query = ::table().into_boxed(); + + match entity_type { + ListRolesByEntityPayload::Organization(org_id) => { + let entity_in_vec = if is_lineage_data_required { + vec![ + EntityType::Organization, + EntityType::Merchant, + EntityType::Profile, + ] + } else { + vec![EntityType::Organization] + }; + query = query + .filter(dsl::org_id.eq(org_id)) + .filter( + dsl::scope + .eq(RoleScope::Organization) + .or(dsl::scope.eq(RoleScope::Merchant)) + .or(dsl::scope.eq(RoleScope::Profile)), + ) + .filter(dsl::entity_type.eq_any(entity_in_vec)) + } + + ListRolesByEntityPayload::Merchant(org_id, merchant_id) => { + let entity_in_vec = if is_lineage_data_required { + vec![EntityType::Merchant, EntityType::Profile] + } else { + vec![EntityType::Merchant] + }; + query = query + .filter(dsl::org_id.eq(org_id)) + .filter( + dsl::scope + .eq(RoleScope::Organization) + .or(dsl::scope + .eq(RoleScope::Merchant) + .and(dsl::merchant_id.eq(merchant_id.clone()))) + .or(dsl::scope + .eq(RoleScope::Profile) + .and(dsl::merchant_id.eq(merchant_id))), + ) + .filter(dsl::entity_type.eq_any(entity_in_vec)) + } + + ListRolesByEntityPayload::Profile(org_id, merchant_id, profile_id) => { + let entity_in_vec = vec![EntityType::Profile]; + query = query + .filter(dsl::org_id.eq(org_id)) + .filter( + dsl::scope + .eq(RoleScope::Organization) + .or(dsl::scope + .eq(RoleScope::Merchant) + .and(dsl::merchant_id.eq(merchant_id.clone()))) + .or(dsl::scope + .eq(RoleScope::Profile) + .and(dsl::merchant_id.eq(merchant_id)) + .and(dsl::profile_id.eq(profile_id))), + ) + .filter(dsl::entity_type.eq_any(entity_in_vec)) + } + }; + + if let Some(limit) = limit { + query = query.limit(limit.into()); + } + + router_env::logger::debug!(query = %debug_query::(&query).to_string()); + + match generics::db_metrics::track_database_call::( + query.get_results_async(conn), + generics::db_metrics::DatabaseOperation::Filter, + ) + .await + { + Ok(value) => Ok(value), + Err(err) => match err { + DieselError::NotFound => { + Err(report!(err)).change_context(errors::DatabaseError::NotFound) + } + _ => Err(report!(err)).change_context(errors::DatabaseError::Others), + }, + } + } } diff --git a/crates/diesel_models/src/role.rs b/crates/diesel_models/src/role.rs index 8199bd3979ce..20677064a2c2 100644 --- a/crates/diesel_models/src/role.rs +++ b/crates/diesel_models/src/role.rs @@ -19,6 +19,7 @@ pub struct Role { pub last_modified_at: PrimitiveDateTime, pub last_modified_by: String, pub entity_type: enums::EntityType, + pub profile_id: Option, } #[derive(router_derive::Setter, Clone, Debug, Insertable, router_derive::DebugAsDisplay)] @@ -36,6 +37,7 @@ pub struct RoleNew { pub last_modified_at: PrimitiveDateTime, pub last_modified_by: String, pub entity_type: enums::EntityType, + pub profile_id: Option, } #[derive(Clone, Debug, AsChangeset, router_derive::DebugAsDisplay)] @@ -73,3 +75,38 @@ impl From for RoleUpdateInternal { } } } + +#[derive(Clone, Debug)] +pub enum ListRolesByEntityPayload { + Profile( + id_type::OrganizationId, + id_type::MerchantId, + id_type::ProfileId, + ), + Merchant(id_type::OrganizationId, id_type::MerchantId), + Organization(id_type::OrganizationId), +} + +impl ListRolesByEntityPayload { + pub fn get_organization_id(&self) -> Option { + match self { + ListRolesByEntityPayload::Organization(org_id) + | ListRolesByEntityPayload::Merchant(org_id, _) + | ListRolesByEntityPayload::Profile(org_id, _, _) => Some(org_id.to_owned()), + } + } + pub fn get_merchant_id(&self) -> Option { + match self { + ListRolesByEntityPayload::Organization(_) => None, + ListRolesByEntityPayload::Merchant(_, merchant_id) + | ListRolesByEntityPayload::Profile(_, merchant_id, _) => Some(merchant_id.to_owned()), + } + } + pub fn get_profile_id(&self) -> Option { + match self { + ListRolesByEntityPayload::Organization(_) + | ListRolesByEntityPayload::Merchant(_, _) => None, + ListRolesByEntityPayload::Profile(_, _, profile_id) => Some(profile_id.to_owned()), + } + } +} diff --git a/crates/diesel_models/src/schema.rs b/crates/diesel_models/src/schema.rs index c30562de9913..f58f987bfcce 100644 --- a/crates/diesel_models/src/schema.rs +++ b/crates/diesel_models/src/schema.rs @@ -1270,6 +1270,8 @@ diesel::table! { last_modified_by -> Varchar, #[max_length = 64] entity_type -> Varchar, + #[max_length = 64] + profile_id -> Nullable, } } diff --git a/crates/diesel_models/src/schema_v2.rs b/crates/diesel_models/src/schema_v2.rs index 12bb19c0a7f1..6ffde7490280 100644 --- a/crates/diesel_models/src/schema_v2.rs +++ b/crates/diesel_models/src/schema_v2.rs @@ -1217,6 +1217,8 @@ diesel::table! { last_modified_by -> Varchar, #[max_length = 64] entity_type -> Varchar, + #[max_length = 64] + profile_id -> Nullable, } } diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 714bf9fed3cc..b2b4e7a57c58 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -1,9 +1,9 @@ -use std::collections::HashSet; +use std::{cmp, collections::HashSet}; use api_models::user_role::role as role_api; -use common_enums::{EntityType, ParentGroup, PermissionGroup, RoleScope}; +use common_enums::{EntityType, ParentGroup, PermissionGroup}; use common_utils::generate_id_with_default_len; -use diesel_models::role::{RoleNew, RoleUpdate}; +use diesel_models::role::{ListRolesByEntityPayload, RoleNew, RoleUpdate}; use error_stack::{report, ResultExt}; use crate::{ @@ -65,6 +65,45 @@ pub async fn create_role( _req_state: ReqState, ) -> UserResponse { let now = common_utils::date_time::now(); + + let user_entity_type = user_from_token + .get_role_info_from_db(&state) + .await + .attach_printable("Invalid role_id in JWT")? + .get_entity_type(); + + let role_entity_type = req.entity_type.unwrap_or(EntityType::Merchant); + + if matches!(role_entity_type, EntityType::Organization) { + return Err(report!(UserErrors::InvalidRoleOperation)) + .attach_printable("User trying to create org level custom role"); + } + + // TODO: Remove in PR custom-role-write-pr + if matches!(role_entity_type, EntityType::Profile) { + return Err(report!(UserErrors::InvalidRoleOperation)) + .attach_printable("User trying to create profile level custom role"); + } + + let requestor_entity_from_role_scope = EntityType::from(req.role_scope); + + if user_entity_type < role_entity_type { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "{} is trying to create {} ", + user_entity_type, role_entity_type + )); + } else if user_entity_type < requestor_entity_from_role_scope { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "{} is trying to create role of scope {} ", + user_entity_type, requestor_entity_from_role_scope + )); + } else if requestor_entity_from_role_scope < role_entity_type { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "User is trying to create role of type {} and scope {}", + requestor_entity_from_role_scope, role_entity_type + )); + } + let role_name = RoleName::new(req.role_name)?; utils::user_role::validate_role_groups(&req.groups)?; @@ -73,17 +112,13 @@ pub async fn create_role( &role_name, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, + &role_entity_type, ) .await?; - let user_role_info = user_from_token.get_role_info_from_db(&state).await?; - - if matches!(req.role_scope, RoleScope::Organization) - && user_role_info.get_entity_type() != EntityType::Organization - { - return Err(report!(UserErrors::InvalidRoleOperation)) - .attach_printable("Non org admin user creating org level role"); - } + let profile_id = + matches!(role_entity_type, EntityType::Profile).then_some(user_from_token.profile_id); let role = state .store @@ -94,11 +129,12 @@ pub async fn create_role( org_id: user_from_token.org_id, groups: req.groups, scope: req.role_scope, - entity_type: EntityType::Merchant, + entity_type: role_entity_type, created_by: user_from_token.user_id.clone(), last_modified_by: user_from_token.user_id, created_at: now, last_modified_at: now, + profile_id, }) .await .to_duplicate_response(UserErrors::RoleNameAlreadyExists)?; @@ -109,6 +145,7 @@ pub async fn create_role( role_id: role.role_id, role_name: role.role_name, role_scope: role.scope, + entity_type: role.entity_type, }, )) } @@ -133,6 +170,7 @@ pub async fn get_role_with_groups( role_id: role.role_id, role_name: role_info.get_role_name().to_string(), role_scope: role_info.get_scope(), + entity_type: role_info.get_entity_type(), }, )) } @@ -186,6 +224,28 @@ pub async fn update_role( role_id: &str, ) -> UserResponse { let role_name = req.role_name.map(RoleName::new).transpose()?; + let role_info = roles::RoleInfo::from_role_id_in_lineage( + &state, + role_id, + &user_from_token.merchant_id, + &user_from_token.org_id, + ) + .await + .to_not_found_response(UserErrors::InvalidRoleOperation)?; + + let user_role_info = user_from_token.get_role_info_from_db(&state).await?; + let max_from_scope_and_entity = cmp::max( + user_role_info.get_entity_type(), + EntityType::from(role_info.get_scope()), + ); + + if user_role_info.get_entity_type() < max_from_scope_and_entity { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "{} is trying to update {} ", + user_role_info.get_entity_type(), + role_info.get_entity_type() + )); + } if let Some(ref role_name) = role_name { utils::user_role::validate_role_name( @@ -193,6 +253,8 @@ pub async fn update_role( role_name, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, + &role_info.get_entity_type(), ) .await?; } @@ -201,24 +263,6 @@ pub async fn update_role( utils::user_role::validate_role_groups(groups)?; } - let role_info = roles::RoleInfo::from_role_id_in_lineage( - &state, - role_id, - &user_from_token.merchant_id, - &user_from_token.org_id, - ) - .await - .to_not_found_response(UserErrors::InvalidRoleOperation)?; - - let user_role_info = user_from_token.get_role_info_from_db(&state).await?; - - if matches!(role_info.get_scope(), RoleScope::Organization) - && user_role_info.get_entity_type() != EntityType::Organization - { - return Err(report!(UserErrors::InvalidRoleOperation)) - .attach_printable("Non org admin user changing org level role"); - } - let updated_role = state .store .update_role_by_role_id( @@ -241,6 +285,7 @@ pub async fn update_role( role_id: updated_role.role_id, role_name: updated_role.role_name, role_scope: updated_role.scope, + entity_type: updated_role.entity_type, }, )) } @@ -272,10 +317,9 @@ pub async fn list_roles_with_info( match utils::user_role::get_min_entity(user_role_entity, request.entity_type)? { EntityType::Tenant | EntityType::Organization => state .store - .list_roles_for_org_by_parameters( - &user_from_token.org_id, - None, - request.entity_type, + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Organization(user_from_token.org_id), + request.entity_type.is_none(), None, ) .await @@ -283,17 +327,32 @@ pub async fn list_roles_with_info( .attach_printable("Failed to get roles")?, EntityType::Merchant => state .store - .list_roles_for_org_by_parameters( - &user_from_token.org_id, - Some(&user_from_token.merchant_id), - request.entity_type, + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Merchant( + user_from_token.org_id, + user_from_token.merchant_id, + ), + request.entity_type.is_none(), + None, + ) + .await + .change_context(UserErrors::InternalServerError) + .attach_printable("Failed to get roles")?, + + EntityType::Profile => state + .store + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Profile( + user_from_token.org_id, + user_from_token.merchant_id, + user_from_token.profile_id, + ), + request.entity_type.is_none(), None, ) .await .change_context(UserErrors::InternalServerError) .attach_printable("Failed to get roles")?, - // TODO: Populate this from Db function when support for profile id and profile level custom roles is added - EntityType::Profile => Vec::new(), }; role_info_vec.extend(custom_roles.into_iter().map(roles::RoleInfo::from)); @@ -345,10 +404,9 @@ pub async fn list_roles_at_entity_level( let custom_roles = match req.entity_type { EntityType::Tenant | EntityType::Organization => state .store - .list_roles_for_org_by_parameters( - &user_from_token.org_id, - None, - Some(req.entity_type), + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Organization(user_from_token.org_id), + false, None, ) .await @@ -357,17 +415,32 @@ pub async fn list_roles_at_entity_level( EntityType::Merchant => state .store - .list_roles_for_org_by_parameters( - &user_from_token.org_id, - Some(&user_from_token.merchant_id), - Some(req.entity_type), + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Merchant( + user_from_token.org_id, + user_from_token.merchant_id, + ), + false, + None, + ) + .await + .change_context(UserErrors::InternalServerError) + .attach_printable("Failed to get roles")?, + + EntityType::Profile => state + .store + .generic_list_roles_by_entity_type( + ListRolesByEntityPayload::Profile( + user_from_token.org_id, + user_from_token.merchant_id, + user_from_token.profile_id, + ), + false, None, ) .await .change_context(UserErrors::InternalServerError) .attach_printable("Failed to get roles")?, - // TODO: Populate this from Db function when support for profile id and profile level custom roles is added - EntityType::Profile => Vec::new(), }; role_info_vec.extend(custom_roles.into_iter().map(roles::RoleInfo::from)); diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index 2fd30a3610b5..e1b3742c97e4 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3571,6 +3571,7 @@ impl RoleInterface for KafkaStore { self.diesel_store.delete_role_by_role_id(role_id).await } + //TODO: Remove once find_by_role_name is stable async fn list_all_roles( &self, merchant_id: &id_type::MerchantId, @@ -3579,6 +3580,17 @@ impl RoleInterface for KafkaStore { self.diesel_store.list_all_roles(merchant_id, org_id).await } + async fn find_role_by_role_name_in_lineage( + &self, + role_name: &str, + entity_type: storage::ListRolesByEntityPayload, + ) -> CustomResult { + self.diesel_store + .find_role_by_role_name_in_lineage(role_name, entity_type) + .await + } + + //TODO: Remove once generic_list_roles_by_entity_type is stable async fn list_roles_for_org_by_parameters( &self, org_id: &id_type::OrganizationId, @@ -3590,6 +3602,17 @@ impl RoleInterface for KafkaStore { .list_roles_for_org_by_parameters(org_id, merchant_id, entity_type, limit) .await } + + async fn generic_list_roles_by_entity_type( + &self, + entity_type: diesel_models::role::ListRolesByEntityPayload, + is_lineage_data_required: bool, + limit: Option, + ) -> CustomResult, errors::StorageError> { + self.diesel_store + .generic_list_roles_by_entity_type(entity_type, is_lineage_data_required, limit) + .await + } } #[async_trait::async_trait] diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 877a4c540774..47541864cecf 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -1,6 +1,8 @@ -use common_enums::enums; use common_utils::id_type; -use diesel_models::role as storage; +use diesel_models::{ + enums::{EntityType, RoleScope}, + role as storage, +}; use error_stack::report; use router_env::{instrument, tracing}; @@ -55,17 +57,32 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; + //TODO: Remove once find_by_role_name is stable async fn list_all_roles( &self, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, ) -> CustomResult, errors::StorageError>; + async fn find_role_by_role_name_in_lineage( + &self, + role_name: &str, + entity_type: storage::ListRolesByEntityPayload, + ) -> CustomResult; + + //TODO: Remove once generic_list_roles_by_entity_type is stable async fn list_roles_for_org_by_parameters( &self, org_id: &id_type::OrganizationId, merchant_id: Option<&id_type::MerchantId>, - entity_type: Option, + entity_type: Option, + limit: Option, + ) -> CustomResult, errors::StorageError>; + + async fn generic_list_roles_by_entity_type( + &self, + entity_type: storage::ListRolesByEntityPayload, + is_lineage_data_required: bool, limit: Option, ) -> CustomResult, errors::StorageError>; } @@ -156,6 +173,7 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } + //TODO: Remove once find_by_role_name is stable #[instrument(skip_all)] async fn list_all_roles( &self, @@ -168,12 +186,25 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } + #[instrument(skip_all)] + async fn find_role_by_role_name_in_lineage( + &self, + role_name: &str, + entity_type: storage::ListRolesByEntityPayload, + ) -> CustomResult { + let conn = connection::pg_connection_read(self).await?; + storage::Role::find_role_by_role_name_in_lineage(&conn, role_name, entity_type) + .await + .map_err(|error| report!(errors::StorageError::from(error))) + } + + //TODO: Remove once generic_list_roles_by_entity_type is stable #[instrument(skip_all)] async fn list_roles_for_org_by_parameters( &self, org_id: &id_type::OrganizationId, merchant_id: Option<&id_type::MerchantId>, - entity_type: Option, + entity_type: Option, limit: Option, ) -> CustomResult, errors::StorageError> { let conn = connection::pg_connection_read(self).await?; @@ -187,6 +218,24 @@ impl RoleInterface for Store { .await .map_err(|error| report!(errors::StorageError::from(error))) } + + #[instrument(skip_all)] + async fn generic_list_roles_by_entity_type( + &self, + entity_type: storage::ListRolesByEntityPayload, + is_lineage_data_required: bool, + limit: Option, + ) -> CustomResult, errors::StorageError> { + let conn = connection::pg_connection_read(self).await?; + storage::Role::generic_list_roles_by_entity_type( + &conn, + entity_type, + is_lineage_data_required, + limit, + ) + .await + .map_err(|error| report!(errors::StorageError::from(error))) + } } #[async_trait::async_trait] @@ -217,6 +266,7 @@ impl RoleInterface for MockDb { created_at: role.created_at, last_modified_at: role.last_modified_at, last_modified_by: role.last_modified_by, + profile_id: role.profile_id, }; roles.push(role.clone()); Ok(role) @@ -252,7 +302,7 @@ impl RoleInterface for MockDb { .find(|role| { role.role_id == role_id && (role.merchant_id == *merchant_id - || (role.org_id == *org_id && role.scope == enums::RoleScope::Organization)) + || (role.org_id == *org_id && role.scope == RoleScope::Organization)) }) .cloned() .ok_or( @@ -276,9 +326,8 @@ impl RoleInterface for MockDb { .find(|role| { role.role_id == role_id && role.org_id == *org_id - && ((role.scope == enums::RoleScope::Organization) - || (role.merchant_id == *merchant_id - && role.scope == enums::RoleScope::Merchant)) + && ((role.scope == RoleScope::Organization) + || (role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant)) }) .cloned() .ok_or( @@ -357,6 +406,7 @@ impl RoleInterface for MockDb { Ok(roles.remove(role_index)) } + //TODO: Remove once find_by_role_name is stable async fn list_all_roles( &self, merchant_id: &id_type::MerchantId, @@ -368,8 +418,7 @@ impl RoleInterface for MockDb { .iter() .filter(|role| { role.merchant_id == *merchant_id - || (role.org_id == *org_id - && role.scope == diesel_models::enums::RoleScope::Organization) + || (role.org_id == *org_id && role.scope == RoleScope::Organization) }) .cloned() .collect(); @@ -385,12 +434,72 @@ impl RoleInterface for MockDb { Ok(roles_list) } + async fn find_role_by_role_name_in_lineage( + &self, + role_name: &str, + entity_type: storage::ListRolesByEntityPayload, + ) -> CustomResult { + let roles = self.roles.lock().await; + + let roles_list = roles + .iter() + .find(|role| { + let org_id = entity_type.get_organization_id(); + let merchant_id = entity_type.get_merchant_id(); + let profile_id = entity_type.get_profile_id(); + + let condition = org_id + .as_ref() + .map(|org_id| { + *org_id == role.org_id + && role.entity_type == EntityType::Organization + && role.scope == RoleScope::Organization + }) + .unwrap_or(true) + || merchant_id + .as_ref() + .map(|merchant_id| { + let entity_in_vec = + vec![EntityType::Merchant, EntityType::Organization]; + *merchant_id == role.merchant_id + && role.scope == RoleScope::Merchant + && entity_in_vec.contains(&role.entity_type) + }) + .unwrap_or(true) + || profile_id + .as_ref() + .zip(role.profile_id.as_ref()) + .map(|(profile_id, role_profile_id)| { + let entity_in_vec = vec![ + EntityType::Profile, + EntityType::Merchant, + EntityType::Organization, + ]; + profile_id == role_profile_id + && role.scope == RoleScope::Profile + && entity_in_vec.contains(&role.entity_type) + }) + .unwrap_or(true); + + role.role_name == role_name && condition + }) + .cloned(); + + roles_list.ok_or( + errors::StorageError::ValueNotFound( + "No role available with role_name={role_name} in org_id = {org_id:?}".to_string(), + ) + .into(), + ) + } + + //TODO: Remove once generic_list_roles_by_entity_type is stable #[instrument(skip_all)] async fn list_roles_for_org_by_parameters( &self, org_id: &id_type::OrganizationId, merchant_id: Option<&id_type::MerchantId>, - entity_type: Option, + entity_type: Option, limit: Option, ) -> CustomResult, errors::StorageError> { let roles = self.roles.lock().await; @@ -411,4 +520,83 @@ impl RoleInterface for MockDb { Ok(roles_list) } + + #[instrument(skip_all)] + async fn generic_list_roles_by_entity_type( + &self, + entity_type: storage::ListRolesByEntityPayload, + is_lineage_data_required: bool, + limit: Option, + ) -> CustomResult, errors::StorageError> { + let roles = self.roles.lock().await; + let limit_usize = limit.unwrap_or(u32::MAX).try_into().unwrap_or(usize::MAX); + let roles_list: Vec<_> = roles + .iter() + .filter(|role| match &entity_type { + storage::ListRolesByEntityPayload::Organization(org_id) => { + let entity_in_vec = if is_lineage_data_required { + vec![ + EntityType::Organization, + EntityType::Merchant, + EntityType::Profile, + ] + } else { + vec![EntityType::Organization] + }; + + let matches_scope = role.scope == RoleScope::Organization + || role.scope == RoleScope::Merchant + || role.scope == RoleScope::Profile; + + role.org_id == *org_id + && (matches_scope) + && entity_in_vec.contains(&role.entity_type) + } + storage::ListRolesByEntityPayload::Merchant(org_id, merchant_id) => { + let entity_in_vec = if is_lineage_data_required { + vec![EntityType::Merchant, EntityType::Profile] + } else { + vec![EntityType::Merchant] + }; + + let matches_merchant = + role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant; + let matches_profile = + role.merchant_id == *merchant_id && role.scope == RoleScope::Profile; + + role.org_id == *org_id + && (role.scope == RoleScope::Organization + || matches_merchant + || matches_profile) + && entity_in_vec.contains(&role.entity_type) + } + storage::ListRolesByEntityPayload::Profile(org_id, merchant_id, profile_id) => { + let entity_in_vec = [EntityType::Profile]; + + let matches_merchant = + role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant; + + let matches_profile = role.merchant_id == *merchant_id + && role + .profile_id + .as_ref() + .map(|profile_id_from_role| { + profile_id_from_role == profile_id + && role.scope == RoleScope::Profile + }) + .unwrap_or(true); + + role.org_id == *org_id + && (role.scope == RoleScope::Organization + || matches_merchant + || matches_profile) + && entity_in_vec.contains(&role.entity_type) + } + }) + .take(limit_usize) + .cloned() + .collect(); + + Ok(roles_list) + } } diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index b8f93bff943c..69e3b4298d1d 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -4,8 +4,10 @@ use common_enums::{EntityType, PermissionGroup}; use common_utils::id_type; use diesel_models::{ enums::{UserRoleVersion, UserStatus}, + role::ListRolesByEntityPayload, user_role::{UserRole, UserRoleUpdate}, }; + use error_stack::{report, Report, ResultExt}; use router_env::logger; use storage_impl::errors::StorageError; @@ -48,6 +50,8 @@ pub async fn validate_role_name( role_name: &domain::RoleName, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, + entity_type: &EntityType, ) -> UserResult<()> { let role_name_str = role_name.clone().get_role_name(); @@ -55,16 +59,58 @@ pub async fn validate_role_name( .iter() .any(|(_, role_info)| role_info.get_role_name() == role_name_str); - // TODO: Create and use find_by_role_name to make this efficient - let is_present_in_custom_roles = state + let entity_type_for_role = match entity_type { + EntityType::Tenant | EntityType::Organization => { + ListRolesByEntityPayload::Organization(org_id.to_owned()) + } + EntityType::Merchant => { + ListRolesByEntityPayload::Merchant(org_id.to_owned(), merchant_id.to_owned()) + } + EntityType::Profile => ListRolesByEntityPayload::Profile( + org_id.to_owned(), + merchant_id.to_owned(), + profile_id.to_owned(), + ), + }; + + let is_present_in_custom_roles_in_my_lineage = match state .store - .list_all_roles(merchant_id, org_id) + .find_role_by_role_name_in_lineage( + role_name.clone().get_role_name().as_str(), + entity_type_for_role.clone(), + ) .await - .change_context(UserErrors::InternalServerError)? - .iter() - .any(|role| role.role_name == role_name_str); + { + Ok(_) => true, + Err(e) => { + if e.current_context().is_db_not_found() { + false + } else { + return Err(UserErrors::InternalServerError.into()); + } + } + }; + let is_present_in_custom_roles_below_me = match state + .store + .generic_list_roles_by_entity_type(entity_type_for_role, true, None) + .await + { + Ok(roles_list) => roles_list + .iter() + .any(|role| role.role_name == role_name_str), + Err(e) => { + if e.current_context().is_db_not_found() { + false + } else { + return Err(UserErrors::InternalServerError.into()); + } + } + }; - if is_present_in_predefined_roles || is_present_in_custom_roles { + if is_present_in_predefined_roles + || is_present_in_custom_roles_in_my_lineage + || is_present_in_custom_roles_below_me + { return Err(UserErrors::RoleNameAlreadyExists.into()); } diff --git a/migrations/2024-10-17-073555_add-profile-id-to-roles/down.sql b/migrations/2024-10-17-073555_add-profile-id-to-roles/down.sql new file mode 100644 index 000000000000..d611be2c3da6 --- /dev/null +++ b/migrations/2024-10-17-073555_add-profile-id-to-roles/down.sql @@ -0,0 +1,2 @@ +-- This file should undo anything in `up.sql` +ALTER TABLE roles DROP COLUMN IF EXISTS profile_id; \ No newline at end of file diff --git a/migrations/2024-10-17-073555_add-profile-id-to-roles/up.sql b/migrations/2024-10-17-073555_add-profile-id-to-roles/up.sql new file mode 100644 index 000000000000..b3873266fec7 --- /dev/null +++ b/migrations/2024-10-17-073555_add-profile-id-to-roles/up.sql @@ -0,0 +1,2 @@ +-- Your SQL goes here +ALTER TABLE roles ADD COLUMN IF NOT EXISTS profile_id VARCHAR(64); \ No newline at end of file diff --git a/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/down.sql b/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/down.sql new file mode 100644 index 000000000000..c7c9cbeb4017 --- /dev/null +++ b/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/down.sql @@ -0,0 +1,2 @@ +-- This file should undo anything in `up.sql` +SELECT 1; \ No newline at end of file diff --git a/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/up.sql b/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/up.sql new file mode 100644 index 000000000000..6fd9b07fd508 --- /dev/null +++ b/migrations/2024-10-17-123943_add-profile-enum-in-role-scope/up.sql @@ -0,0 +1,3 @@ +-- Your SQL goes here +ALTER TYPE "RoleScope" +ADD VALUE IF NOT EXISTS 'profile'; \ No newline at end of file From 335b35970e5a23776cbb827984bcb06954008d53 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Wed, 18 Dec 2024 11:10:02 +0530 Subject: [PATCH 09/19] refactor: removed unused code --- crates/diesel_models/src/query/role.rs | 52 ++------ crates/diesel_models/src/role.rs | 18 +-- crates/router/src/core/user.rs | 1 + crates/router/src/core/user_role.rs | 3 + crates/router/src/core/user_role/role.rs | 1 + crates/router/src/db/kafka_store.rs | 24 +--- crates/router/src/db/role.rs | 115 +++--------------- .../src/services/authorization/roles.rs | 3 +- 8 files changed, 43 insertions(+), 174 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 09c049897a25..78c64f80b588 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -27,29 +27,12 @@ impl Role { .await } - // TODO: Remove once find_by_role_id_in_lineage is stable - pub async fn find_by_role_id_in_merchant_scope( - conn: &PgPooledConn, - role_id: &str, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> StorageResult { - generics::generic_find_one::<::Table, _, _>( - conn, - dsl::role_id.eq(role_id.to_owned()).and( - dsl::merchant_id.eq(merchant_id.to_owned()).or(dsl::org_id - .eq(org_id.to_owned()) - .and(dsl::scope.eq(RoleScope::Organization))), - ), - ) - .await - } - pub async fn find_by_role_id_in_lineage( conn: &PgPooledConn, role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> StorageResult { generics::generic_find_one::<::Table, _, _>( conn, @@ -57,9 +40,14 @@ impl Role { .eq(role_id.to_owned()) .and(dsl::org_id.eq(org_id.to_owned())) .and( - dsl::scope.eq(RoleScope::Organization).or(dsl::merchant_id - .eq(merchant_id.to_owned()) - .and(dsl::scope.eq(RoleScope::Merchant))), + dsl::scope + .eq(RoleScope::Organization) + .or(dsl::merchant_id + .eq(merchant_id.to_owned()) + .and(dsl::scope.eq(RoleScope::Merchant))) + .or(dsl::profile_id + .eq(profile_id.to_owned()) + .and(dsl::scope.eq(RoleScope::Profile))), ), ) .await @@ -105,27 +93,7 @@ impl Role { .await } - // TODO: Change as it will break - pub async fn list_roles( - conn: &PgPooledConn, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> StorageResult> { - let predicate = dsl::org_id.eq(org_id.to_owned()).and( - dsl::scope.eq(RoleScope::Organization).or(dsl::merchant_id - .eq(merchant_id.to_owned()) - .and(dsl::scope.eq(RoleScope::Merchant))), - ); - - generics::generic_filter::<::Table, _, _, _>( - conn, - predicate, - None, - None, - Some(dsl::last_modified_at.asc()), - ) - .await - } + pub async fn find_role_by_role_name_in_lineage( conn: &PgPooledConn, diff --git a/crates/diesel_models/src/role.rs b/crates/diesel_models/src/role.rs index 20677064a2c2..8adad4e63a05 100644 --- a/crates/diesel_models/src/role.rs +++ b/crates/diesel_models/src/role.rs @@ -90,23 +90,23 @@ pub enum ListRolesByEntityPayload { impl ListRolesByEntityPayload { pub fn get_organization_id(&self) -> Option { match self { - ListRolesByEntityPayload::Organization(org_id) - | ListRolesByEntityPayload::Merchant(org_id, _) - | ListRolesByEntityPayload::Profile(org_id, _, _) => Some(org_id.to_owned()), + Self::Organization(org_id) + | Self::Merchant(org_id, _) + | Self::Profile(org_id, _, _) => Some(org_id.to_owned()), } } pub fn get_merchant_id(&self) -> Option { match self { - ListRolesByEntityPayload::Organization(_) => None, - ListRolesByEntityPayload::Merchant(_, merchant_id) - | ListRolesByEntityPayload::Profile(_, merchant_id, _) => Some(merchant_id.to_owned()), + Self::Organization(_) => None, + Self::Merchant(_, merchant_id) | Self::Profile(_, merchant_id, _) => { + Some(merchant_id.to_owned()) + } } } pub fn get_profile_id(&self) -> Option { match self { - ListRolesByEntityPayload::Organization(_) - | ListRolesByEntityPayload::Merchant(_, _) => None, - ListRolesByEntityPayload::Profile(_, _, profile_id) => Some(profile_id.to_owned()), + Self::Organization(_) | Self::Merchant(_, _) => None, + Self::Profile(_, _, profile_id) => Some(profile_id.to_owned()), } } } diff --git a/crates/router/src/core/user.rs b/crates/router/src/core/user.rs index 629476b2591e..9c00ba628449 100644 --- a/crates/router/src/core/user.rs +++ b/crates/router/src/core/user.rs @@ -557,6 +557,7 @@ async fn handle_invitation( &request.role_id, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, ) .await .to_not_found_response(UserErrors::InvalidRoleId)?; diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index d8fdff0e6233..da2efe4404c0 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -123,6 +123,7 @@ pub async fn update_user_role( &req.role_id, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, ) .await .to_not_found_response(UserErrors::InvalidRoleId)?; @@ -527,6 +528,7 @@ pub async fn delete_user_role( &role_to_be_deleted.role_id, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, ) .await .change_context(UserErrors::InternalServerError)?; @@ -597,6 +599,7 @@ pub async fn delete_user_role( &role_to_be_deleted.role_id, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, ) .await .change_context(UserErrors::InternalServerError)?; diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 75dc2039f537..319d641f7ed5 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -230,6 +230,7 @@ pub async fn update_role( role_id, &user_from_token.merchant_id, &user_from_token.org_id, + &user_from_token.profile_id, ) .await .to_not_found_response(UserErrors::InvalidRoleOperation)?; diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index e1b3742c97e4..65cad1791dca 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3521,26 +3521,15 @@ impl RoleInterface for KafkaStore { self.diesel_store.find_role_by_role_id(role_id).await } - //TODO:Remove once find_by_role_id_in_lineage is stable - async fn find_role_by_role_id_in_merchant_scope( - &self, - role_id: &str, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult { - self.diesel_store - .find_role_by_role_id_in_merchant_scope(role_id, merchant_id, org_id) - .await - } - async fn find_role_by_role_id_in_lineage( &self, role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> CustomResult { self.diesel_store - .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id) + .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id, profile_id) .await } @@ -3571,15 +3560,6 @@ impl RoleInterface for KafkaStore { self.diesel_store.delete_role_by_role_id(role_id).await } - //TODO: Remove once find_by_role_name is stable - async fn list_all_roles( - &self, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult, errors::StorageError> { - self.diesel_store.list_all_roles(merchant_id, org_id).await - } - async fn find_role_by_role_name_in_lineage( &self, role_name: &str, diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 47541864cecf..48b6313de91c 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -25,19 +25,12 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; - //TODO:Remove once find_by_role_id_in_lineage is stable - async fn find_role_by_role_id_in_merchant_scope( - &self, - role_id: &str, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult; - async fn find_role_by_role_id_in_lineage( &self, role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> CustomResult; async fn find_by_role_id_and_org_id( @@ -57,13 +50,6 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; - //TODO: Remove once find_by_role_name is stable - async fn list_all_roles( - &self, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult, errors::StorageError>; - async fn find_role_by_role_name_in_lineage( &self, role_name: &str, @@ -111,29 +97,16 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } - //TODO:Remove once find_by_role_id_in_lineage is stable - #[instrument(skip_all)] - async fn find_role_by_role_id_in_merchant_scope( - &self, - role_id: &str, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult { - let conn = connection::pg_connection_read(self).await?; - storage::Role::find_by_role_id_in_merchant_scope(&conn, role_id, merchant_id, org_id) - .await - .map_err(|error| report!(errors::StorageError::from(error))) - } - #[instrument(skip_all)] async fn find_role_by_role_id_in_lineage( &self, role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> CustomResult { let conn = connection::pg_connection_read(self).await?; - storage::Role::find_by_role_id_in_lineage(&conn, role_id, merchant_id, org_id) + storage::Role::find_by_role_id_in_lineage(&conn, role_id, merchant_id, org_id, profile_id) .await .map_err(|error| report!(errors::StorageError::from(error))) } @@ -173,19 +146,6 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } - //TODO: Remove once find_by_role_name is stable - #[instrument(skip_all)] - async fn list_all_roles( - &self, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult, errors::StorageError> { - let conn = connection::pg_connection_read(self).await?; - storage::Role::list_roles(&conn, merchant_id, org_id) - .await - .map_err(|error| report!(errors::StorageError::from(error))) - } - #[instrument(skip_all)] async fn find_role_by_role_name_in_lineage( &self, @@ -289,36 +249,12 @@ impl RoleInterface for MockDb { ) } - // TODO: Remove once find_by_role_id_in_lineage is stable - async fn find_role_by_role_id_in_merchant_scope( - &self, - role_id: &str, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult { - let roles = self.roles.lock().await; - roles - .iter() - .find(|role| { - role.role_id == role_id - && (role.merchant_id == *merchant_id - || (role.org_id == *org_id && role.scope == RoleScope::Organization)) - }) - .cloned() - .ok_or( - errors::StorageError::ValueNotFound(format!( - "No role available in merchant scope for role_id = {role_id}, \ - merchant_id = {merchant_id:?} and org_id = {org_id:?}" - )) - .into(), - ) - } - async fn find_role_by_role_id_in_lineage( &self, role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> CustomResult { let roles = self.roles.lock().await; roles @@ -327,7 +263,15 @@ impl RoleInterface for MockDb { role.role_id == role_id && role.org_id == *org_id && ((role.scope == RoleScope::Organization) - || (role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant)) + || (role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant) + || (role + .profile_id + .as_ref() + .map(|profile_id_from_role| { + profile_id_from_role == profile_id + && role.scope == RoleScope::Profile + }) + .unwrap_or(true))) }) .cloned() .ok_or( @@ -406,34 +350,6 @@ impl RoleInterface for MockDb { Ok(roles.remove(role_index)) } - //TODO: Remove once find_by_role_name is stable - async fn list_all_roles( - &self, - merchant_id: &id_type::MerchantId, - org_id: &id_type::OrganizationId, - ) -> CustomResult, errors::StorageError> { - let roles = self.roles.lock().await; - - let roles_list: Vec<_> = roles - .iter() - .filter(|role| { - role.merchant_id == *merchant_id - || (role.org_id == *org_id && role.scope == RoleScope::Organization) - }) - .cloned() - .collect(); - - if roles_list.is_empty() { - return Err(errors::StorageError::ValueNotFound(format!( - "No role found for merchant id = {:?} and org_id = {:?}", - merchant_id, org_id - )) - .into()); - } - - Ok(roles_list) - } - async fn find_role_by_role_name_in_lineage( &self, role_name: &str, @@ -459,8 +375,7 @@ impl RoleInterface for MockDb { || merchant_id .as_ref() .map(|merchant_id| { - let entity_in_vec = - vec![EntityType::Merchant, EntityType::Organization]; + let entity_in_vec = [EntityType::Merchant, EntityType::Organization]; *merchant_id == role.merchant_id && role.scope == RoleScope::Merchant && entity_in_vec.contains(&role.entity_type) @@ -470,7 +385,7 @@ impl RoleInterface for MockDb { .as_ref() .zip(role.profile_id.as_ref()) .map(|(profile_id, role_profile_id)| { - let entity_in_vec = vec![ + let entity_in_vec = [ EntityType::Profile, EntityType::Merchant, EntityType::Organization, diff --git a/crates/router/src/services/authorization/roles.rs b/crates/router/src/services/authorization/roles.rs index dcffa3107c91..ad567549fcc8 100644 --- a/crates/router/src/services/authorization/roles.rs +++ b/crates/router/src/services/authorization/roles.rs @@ -121,13 +121,14 @@ impl RoleInfo { role_id: &str, merchant_id: &id_type::MerchantId, org_id: &id_type::OrganizationId, + profile_id: &id_type::ProfileId, ) -> CustomResult { if let Some(role) = predefined_roles::PREDEFINED_ROLES.get(role_id) { Ok(role.clone()) } else { state .store - .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id) + .find_role_by_role_id_in_lineage(role_id, merchant_id, org_id, profile_id) .await .map(Self::from) } From 26eaa33ed2e3700d07c56cde82e3dc002167e9fa Mon Sep 17 00:00:00 2001 From: "hyperswitch-bot[bot]" <148525504+hyperswitch-bot[bot]@users.noreply.github.com> Date: Wed, 18 Dec 2024 07:42:43 +0000 Subject: [PATCH 10/19] chore: run formatter --- crates/diesel_models/src/query/role.rs | 2 -- crates/router/src/utils/user_role.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 78c64f80b588..7548c5396f2e 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -93,8 +93,6 @@ impl Role { .await } - - pub async fn find_role_by_role_name_in_lineage( conn: &PgPooledConn, role_name: &str, diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index 69e3b4298d1d..22bc60990512 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -7,7 +7,6 @@ use diesel_models::{ role::ListRolesByEntityPayload, user_role::{UserRole, UserRoleUpdate}, }; - use error_stack::{report, Report, ResultExt}; use router_env::logger; use storage_impl::errors::StorageError; From cfbeeaff3bafe59e3c8481e025febdf9355b79de Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Wed, 18 Dec 2024 14:46:22 +0530 Subject: [PATCH 11/19] fix: added indexes --- crates/common_enums/src/enums.rs | 1 - .../2024-12-18-061400_change-roles-index/down.sql | 10 ++++++++++ .../2024-12-18-061400_change-roles-index/up.sql | 13 +++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 migrations/2024-12-18-061400_change-roles-index/down.sql create mode 100644 migrations/2024-12-18-061400_change-roles-index/up.sql diff --git a/crates/common_enums/src/enums.rs b/crates/common_enums/src/enums.rs index 348cbf76c630..f0e00661f37b 100644 --- a/crates/common_enums/src/enums.rs +++ b/crates/common_enums/src/enums.rs @@ -3214,7 +3214,6 @@ pub enum ApiVersion { serde::Serialize, strum::Display, strum::EnumString, - strum::EnumIter, ToSchema, Hash, )] diff --git a/migrations/2024-12-18-061400_change-roles-index/down.sql b/migrations/2024-12-18-061400_change-roles-index/down.sql new file mode 100644 index 000000000000..f59b8c876691 --- /dev/null +++ b/migrations/2024-12-18-061400_change-roles-index/down.sql @@ -0,0 +1,10 @@ +-- This file should undo anything in `up.sql` +CREATE UNIQUE INDEX role_name_org_id_org_scope_index ON roles (org_id, role_name) +WHERE + scope = 'organization'; + +CREATE UNIQUE INDEX role_name_merchant_id_merchant_scope_index ON roles (merchant_id, role_name) +WHERE + scope = 'merchant'; + +DROP INDEX IF EXISTS roles_merchant_org_index; \ No newline at end of file diff --git a/migrations/2024-12-18-061400_change-roles-index/up.sql b/migrations/2024-12-18-061400_change-roles-index/up.sql new file mode 100644 index 000000000000..08203559eb9a --- /dev/null +++ b/migrations/2024-12-18-061400_change-roles-index/up.sql @@ -0,0 +1,13 @@ +-- Your SQL goes here + +DROP INDEX IF EXISTS role_name_org_id_org_scope_index; + +DROP INDEX IF EXISTS role_name_merchant_id_merchant_scope_index; + +DROP INDEX IF EXISTS roles_merchant_org_index; + +CREATE INDEX roles_merchant_org_index ON roles ( + org_id, + merchant_id, + profile_id +); \ No newline at end of file From 04703e32f74b4acbd7516f774326e6f188584587 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Wed, 18 Dec 2024 15:04:11 +0530 Subject: [PATCH 12/19] fix: variable name changes --- crates/router/src/utils/user_role.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index 22bc60990512..fd08e87d8d10 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -72,7 +72,7 @@ pub async fn validate_role_name( ), }; - let is_present_in_custom_roles_in_my_lineage = match state + let is_present_in_lineage = match state .store .find_role_by_role_name_in_lineage( role_name.clone().get_role_name().as_str(), @@ -89,7 +89,7 @@ pub async fn validate_role_name( } } }; - let is_present_in_custom_roles_below_me = match state + let is_present_below_me = match state .store .generic_list_roles_by_entity_type(entity_type_for_role, true, None) .await @@ -106,10 +106,7 @@ pub async fn validate_role_name( } }; - if is_present_in_predefined_roles - || is_present_in_custom_roles_in_my_lineage - || is_present_in_custom_roles_below_me - { + if is_present_in_predefined_roles || is_present_in_lineage || is_present_below_me { return Err(UserErrors::RoleNameAlreadyExists.into()); } From 6a35c22af95c8959d430b89dcebf1d71e643d68e Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 23 Dec 2024 13:21:13 +0530 Subject: [PATCH 13/19] fix: query changes --- crates/common_enums/src/enums.rs | 1 + crates/diesel_models/src/query/role.rs | 122 ++++++------------------- crates/router/src/db/kafka_store.rs | 10 -- crates/router/src/db/role.rs | 76 --------------- crates/router/src/utils/user_role.rs | 23 +---- 5 files changed, 30 insertions(+), 202 deletions(-) diff --git a/crates/common_enums/src/enums.rs b/crates/common_enums/src/enums.rs index f0e00661f37b..348cbf76c630 100644 --- a/crates/common_enums/src/enums.rs +++ b/crates/common_enums/src/enums.rs @@ -3214,6 +3214,7 @@ pub enum ApiVersion { serde::Serialize, strum::Display, strum::EnumString, + strum::EnumIter, ToSchema, Hash, )] diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 7548c5396f2e..050dc17a311c 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -1,3 +1,7 @@ +use crate::{ + enums::RoleScope, errors, query::generics, role::*, schema::roles::dsl, PgPooledConn, + StorageResult, +}; use async_bb8_diesel::AsyncRunQueryDsl; use common_enums::EntityType; use common_utils::id_type; @@ -6,11 +10,7 @@ use diesel::{ BoolExpressionMethods, ExpressionMethods, QueryDsl, }; use error_stack::{report, ResultExt}; - -use crate::{ - enums::RoleScope, errors, query::generics, role::*, schema::roles::dsl, PgPooledConn, - StorageResult, -}; +use strum::IntoEnumIterator; impl RoleNew { pub async fn insert(self, conn: &PgPooledConn) -> StorageResult { @@ -19,6 +19,19 @@ impl RoleNew { } impl Role { + fn get_enitity_list( + current_entity: EntityType, + is_lineage_data_required: bool, + ) -> Vec { + is_lineage_data_required + .then(|| { + EntityType::iter() + .filter(|variant| *variant <= current_entity) + .collect() + }) + .unwrap_or_else(|| vec![current_entity]) + } + pub async fn find_by_role_id(conn: &PgPooledConn, role_id: &str) -> StorageResult { generics::generic_find_one::<::Table, _, _>( conn, @@ -93,66 +106,6 @@ impl Role { .await } - pub async fn find_role_by_role_name_in_lineage( - conn: &PgPooledConn, - role_name: &str, - entity_type: ListRolesByEntityPayload, - ) -> StorageResult { - let mut query = ::table().into_boxed(); - - if let Some(org_id) = entity_type.get_organization_id() { - query = query.filter( - dsl::org_id - .eq(org_id) - .and(dsl::scope.eq(RoleScope::Organization)) - .and(dsl::entity_type.eq_any(vec![ - EntityType::Profile, - EntityType::Merchant, - EntityType::Organization, - ])), - ); - }; - - if let Some(merchant_id) = entity_type.get_merchant_id() { - query = query.or_filter( - dsl::merchant_id - .eq(merchant_id) - .and(dsl::scope.eq(RoleScope::Merchant)) - .and(dsl::entity_type.eq_any(vec![EntityType::Profile, EntityType::Merchant])), - ); - }; - - if let Some(profile_id) = entity_type.get_profile_id() { - query = query.or_filter( - dsl::profile_id - .eq(profile_id) - .and(dsl::scope.eq(RoleScope::Profile)) - .and(dsl::entity_type.eq(EntityType::Profile)), - ); - }; - - query = query.filter(dsl::role_name.eq(role_name.to_owned())); - - router_env::logger::debug!(query = %debug_query::(&query).to_string()); - - match generics::db_metrics::track_database_call::( - query.get_results_async(conn), - generics::db_metrics::DatabaseOperation::Filter, - ) - .await - { - Err(err) => match err { - DieselError::NotFound => { - Err(report!(err)).change_context(errors::DatabaseError::NotFound) - } - _ => Err(report!(err)).change_context(errors::DatabaseError::Others), - }, - Ok(mut role_info) => role_info - .pop() - .ok_or(error_stack::report!(errors::DatabaseError::NotFound)), - } - } - //TODO: Remove once generic_list_roles_by_entity_type is stable pub async fn generic_roles_list_for_org( conn: &PgPooledConn, @@ -210,49 +163,29 @@ impl Role { match entity_type { ListRolesByEntityPayload::Organization(org_id) => { - let entity_in_vec = if is_lineage_data_required { - vec![ - EntityType::Organization, - EntityType::Merchant, - EntityType::Profile, - ] - } else { - vec![EntityType::Organization] - }; + let entity_in_vec = + Self::get_enitity_list(EntityType::Organization, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) - .filter( - dsl::scope - .eq(RoleScope::Organization) - .or(dsl::scope.eq(RoleScope::Merchant)) - .or(dsl::scope.eq(RoleScope::Profile)), - ) .filter(dsl::entity_type.eq_any(entity_in_vec)) } ListRolesByEntityPayload::Merchant(org_id, merchant_id) => { - let entity_in_vec = if is_lineage_data_required { - vec![EntityType::Merchant, EntityType::Profile] - } else { - vec![EntityType::Merchant] - }; + let entity_in_vec = + Self::get_enitity_list(EntityType::Merchant, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) .filter( dsl::scope .eq(RoleScope::Organization) - .or(dsl::scope - .eq(RoleScope::Merchant) - .and(dsl::merchant_id.eq(merchant_id.clone()))) - .or(dsl::scope - .eq(RoleScope::Profile) - .and(dsl::merchant_id.eq(merchant_id))), + .or(dsl::merchant_id.eq(merchant_id)), ) .filter(dsl::entity_type.eq_any(entity_in_vec)) } ListRolesByEntityPayload::Profile(org_id, merchant_id, profile_id) => { - let entity_in_vec = vec![EntityType::Profile]; + let entity_in_vec = + Self::get_enitity_list(EntityType::Profile, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) .filter( @@ -261,10 +194,7 @@ impl Role { .or(dsl::scope .eq(RoleScope::Merchant) .and(dsl::merchant_id.eq(merchant_id.clone()))) - .or(dsl::scope - .eq(RoleScope::Profile) - .and(dsl::merchant_id.eq(merchant_id)) - .and(dsl::profile_id.eq(profile_id))), + .or(dsl::profile_id.eq(profile_id)), ) .filter(dsl::entity_type.eq_any(entity_in_vec)) } diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index 65cad1791dca..3df1d2a05c0d 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3560,16 +3560,6 @@ impl RoleInterface for KafkaStore { self.diesel_store.delete_role_by_role_id(role_id).await } - async fn find_role_by_role_name_in_lineage( - &self, - role_name: &str, - entity_type: storage::ListRolesByEntityPayload, - ) -> CustomResult { - self.diesel_store - .find_role_by_role_name_in_lineage(role_name, entity_type) - .await - } - //TODO: Remove once generic_list_roles_by_entity_type is stable async fn list_roles_for_org_by_parameters( &self, diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 48b6313de91c..e6f4c7040df9 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -50,12 +50,6 @@ pub trait RoleInterface { role_id: &str, ) -> CustomResult; - async fn find_role_by_role_name_in_lineage( - &self, - role_name: &str, - entity_type: storage::ListRolesByEntityPayload, - ) -> CustomResult; - //TODO: Remove once generic_list_roles_by_entity_type is stable async fn list_roles_for_org_by_parameters( &self, @@ -146,18 +140,6 @@ impl RoleInterface for Store { .map_err(|error| report!(errors::StorageError::from(error))) } - #[instrument(skip_all)] - async fn find_role_by_role_name_in_lineage( - &self, - role_name: &str, - entity_type: storage::ListRolesByEntityPayload, - ) -> CustomResult { - let conn = connection::pg_connection_read(self).await?; - storage::Role::find_role_by_role_name_in_lineage(&conn, role_name, entity_type) - .await - .map_err(|error| report!(errors::StorageError::from(error))) - } - //TODO: Remove once generic_list_roles_by_entity_type is stable #[instrument(skip_all)] async fn list_roles_for_org_by_parameters( @@ -350,64 +332,6 @@ impl RoleInterface for MockDb { Ok(roles.remove(role_index)) } - async fn find_role_by_role_name_in_lineage( - &self, - role_name: &str, - entity_type: storage::ListRolesByEntityPayload, - ) -> CustomResult { - let roles = self.roles.lock().await; - - let roles_list = roles - .iter() - .find(|role| { - let org_id = entity_type.get_organization_id(); - let merchant_id = entity_type.get_merchant_id(); - let profile_id = entity_type.get_profile_id(); - - let condition = org_id - .as_ref() - .map(|org_id| { - *org_id == role.org_id - && role.entity_type == EntityType::Organization - && role.scope == RoleScope::Organization - }) - .unwrap_or(true) - || merchant_id - .as_ref() - .map(|merchant_id| { - let entity_in_vec = [EntityType::Merchant, EntityType::Organization]; - *merchant_id == role.merchant_id - && role.scope == RoleScope::Merchant - && entity_in_vec.contains(&role.entity_type) - }) - .unwrap_or(true) - || profile_id - .as_ref() - .zip(role.profile_id.as_ref()) - .map(|(profile_id, role_profile_id)| { - let entity_in_vec = [ - EntityType::Profile, - EntityType::Merchant, - EntityType::Organization, - ]; - profile_id == role_profile_id - && role.scope == RoleScope::Profile - && entity_in_vec.contains(&role.entity_type) - }) - .unwrap_or(true); - - role.role_name == role_name && condition - }) - .cloned(); - - roles_list.ok_or( - errors::StorageError::ValueNotFound( - "No role available with role_name={role_name} in org_id = {org_id:?}".to_string(), - ) - .into(), - ) - } - //TODO: Remove once generic_list_roles_by_entity_type is stable #[instrument(skip_all)] async fn list_roles_for_org_by_parameters( diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index fd08e87d8d10..eb1342644f8e 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -72,26 +72,9 @@ pub async fn validate_role_name( ), }; - let is_present_in_lineage = match state + let is_present_in_custom_role = match state .store - .find_role_by_role_name_in_lineage( - role_name.clone().get_role_name().as_str(), - entity_type_for_role.clone(), - ) - .await - { - Ok(_) => true, - Err(e) => { - if e.current_context().is_db_not_found() { - false - } else { - return Err(UserErrors::InternalServerError.into()); - } - } - }; - let is_present_below_me = match state - .store - .generic_list_roles_by_entity_type(entity_type_for_role, true, None) + .generic_list_roles_by_entity_type(entity_type_for_role, false, None) .await { Ok(roles_list) => roles_list @@ -106,7 +89,7 @@ pub async fn validate_role_name( } }; - if is_present_in_predefined_roles || is_present_in_lineage || is_present_below_me { + if is_present_in_predefined_roles || is_present_in_custom_role { return Err(UserErrors::RoleNameAlreadyExists.into()); } From f8f59367c9e8028ba6c536d79e0edff6b630aba0 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 23 Dec 2024 14:55:05 +0530 Subject: [PATCH 14/19] refactor: addressed comments --- crates/diesel_models/src/query/role.rs | 9 +--- crates/router/src/core/user_role/role.rs | 60 ++++++++++++------------ crates/router/src/db/kafka_store.rs | 5 +- crates/router/src/db/role.rs | 24 +++------- crates/router/src/utils/user_role.rs | 2 +- 5 files changed, 43 insertions(+), 57 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 050dc17a311c..f4e6d1014c2a 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -155,13 +155,12 @@ impl Role { pub async fn generic_list_roles_by_entity_type( conn: &PgPooledConn, - entity_type: ListRolesByEntityPayload, + payload: ListRolesByEntityPayload, is_lineage_data_required: bool, - limit: Option, ) -> StorageResult> { let mut query = ::table().into_boxed(); - match entity_type { + match payload { ListRolesByEntityPayload::Organization(org_id) => { let entity_in_vec = Self::get_enitity_list(EntityType::Organization, is_lineage_data_required); @@ -200,10 +199,6 @@ impl Role { } }; - if let Some(limit) = limit { - query = query.limit(limit.into()); - } - router_env::logger::debug!(query = %debug_query::(&query).to_string()); match generics::db_metrics::track_database_call::( diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 319d641f7ed5..996bb6b37f3c 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -87,22 +87,20 @@ pub async fn create_role( let requestor_entity_from_role_scope = EntityType::from(req.role_scope); - if user_entity_type < role_entity_type { - return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( - "{} is trying to create {} ", - user_entity_type, role_entity_type - )); - } else if user_entity_type < requestor_entity_from_role_scope { - return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( - "{} is trying to create role of scope {} ", - user_entity_type, requestor_entity_from_role_scope - )); - } else if requestor_entity_from_role_scope < role_entity_type { + if requestor_entity_from_role_scope < role_entity_type { return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( "User is trying to create role of type {} and scope {}", requestor_entity_from_role_scope, role_entity_type )); } + let max_from_scope_and_entity = cmp::max(requestor_entity_from_role_scope, role_entity_type); + + if user_entity_type < max_from_scope_and_entity { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "{} is trying to create of scope {} and of type {}", + user_entity_type, requestor_entity_from_role_scope, role_entity_type + )); + } let role_name = RoleName::new(req.role_name)?; @@ -237,16 +235,24 @@ pub async fn update_role( let user_role_info = user_from_token.get_role_info_from_db(&state).await?; - let max_from_scope_and_entity = cmp::max( - user_role_info.get_entity_type(), - EntityType::from(role_info.get_scope()), - ); + let requested_entity_from_role_scope = EntityType::from(role_info.get_scope()); + let requested_role_entity_type = role_info.get_entity_type(); + + if requested_entity_from_role_scope < requested_role_entity_type { + return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( + "User is trying to create role of type {} and scope {}", + requested_entity_from_role_scope, requested_role_entity_type + )); + } + let max_from_scope_and_entity = + cmp::max(requested_entity_from_role_scope, requested_role_entity_type); if user_role_info.get_entity_type() < max_from_scope_and_entity { return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( - "{} is trying to update {} ", + "{} is trying to create of scope {} and of type {}", user_role_info.get_entity_type(), - role_info.get_entity_type() + requested_entity_from_role_scope, + requested_role_entity_type )); } @@ -316,14 +322,14 @@ pub async fn list_roles_with_info( .collect::>(); let user_role_entity = user_role_info.get_entity_type(); + let is_lineage_data_required = request.entity_type.is_none(); let custom_roles = match utils::user_role::get_min_entity(user_role_entity, request.entity_type)? { EntityType::Tenant | EntityType::Organization => state .store .generic_list_roles_by_entity_type( ListRolesByEntityPayload::Organization(user_from_token.org_id), - request.entity_type.is_none(), - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) @@ -335,8 +341,7 @@ pub async fn list_roles_with_info( user_from_token.org_id, user_from_token.merchant_id, ), - request.entity_type.is_none(), - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) @@ -350,8 +355,7 @@ pub async fn list_roles_with_info( user_from_token.merchant_id, user_from_token.profile_id, ), - request.entity_type.is_none(), - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) @@ -404,13 +408,13 @@ pub async fn list_roles_at_entity_level( .map(|(_, role_info)| role_info.clone()) .collect::>(); + let is_lineage_data_required = false; let custom_roles = match req.entity_type { EntityType::Tenant | EntityType::Organization => state .store .generic_list_roles_by_entity_type( ListRolesByEntityPayload::Organization(user_from_token.org_id), - false, - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) @@ -423,8 +427,7 @@ pub async fn list_roles_at_entity_level( user_from_token.org_id, user_from_token.merchant_id, ), - false, - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) @@ -438,8 +441,7 @@ pub async fn list_roles_at_entity_level( user_from_token.merchant_id, user_from_token.profile_id, ), - false, - None, + is_lineage_data_required, ) .await .change_context(UserErrors::InternalServerError) diff --git a/crates/router/src/db/kafka_store.rs b/crates/router/src/db/kafka_store.rs index 3df1d2a05c0d..4155baab1d82 100644 --- a/crates/router/src/db/kafka_store.rs +++ b/crates/router/src/db/kafka_store.rs @@ -3575,12 +3575,11 @@ impl RoleInterface for KafkaStore { async fn generic_list_roles_by_entity_type( &self, - entity_type: diesel_models::role::ListRolesByEntityPayload, + payload: diesel_models::role::ListRolesByEntityPayload, is_lineage_data_required: bool, - limit: Option, ) -> CustomResult, errors::StorageError> { self.diesel_store - .generic_list_roles_by_entity_type(entity_type, is_lineage_data_required, limit) + .generic_list_roles_by_entity_type(payload, is_lineage_data_required) .await } } diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index e6f4c7040df9..6ae51f9c6ecf 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -61,9 +61,8 @@ pub trait RoleInterface { async fn generic_list_roles_by_entity_type( &self, - entity_type: storage::ListRolesByEntityPayload, + payload: storage::ListRolesByEntityPayload, is_lineage_data_required: bool, - limit: Option, ) -> CustomResult, errors::StorageError>; } @@ -164,19 +163,13 @@ impl RoleInterface for Store { #[instrument(skip_all)] async fn generic_list_roles_by_entity_type( &self, - entity_type: storage::ListRolesByEntityPayload, + payload: storage::ListRolesByEntityPayload, is_lineage_data_required: bool, - limit: Option, ) -> CustomResult, errors::StorageError> { let conn = connection::pg_connection_read(self).await?; - storage::Role::generic_list_roles_by_entity_type( - &conn, - entity_type, - is_lineage_data_required, - limit, - ) - .await - .map_err(|error| report!(errors::StorageError::from(error))) + storage::Role::generic_list_roles_by_entity_type(&conn, payload, is_lineage_data_required) + .await + .map_err(|error| report!(errors::StorageError::from(error))) } } @@ -363,15 +356,13 @@ impl RoleInterface for MockDb { #[instrument(skip_all)] async fn generic_list_roles_by_entity_type( &self, - entity_type: storage::ListRolesByEntityPayload, + payload: storage::ListRolesByEntityPayload, is_lineage_data_required: bool, - limit: Option, ) -> CustomResult, errors::StorageError> { let roles = self.roles.lock().await; - let limit_usize = limit.unwrap_or(u32::MAX).try_into().unwrap_or(usize::MAX); let roles_list: Vec<_> = roles .iter() - .filter(|role| match &entity_type { + .filter(|role| match &payload { storage::ListRolesByEntityPayload::Organization(org_id) => { let entity_in_vec = if is_lineage_data_required { vec![ @@ -432,7 +423,6 @@ impl RoleInterface for MockDb { && entity_in_vec.contains(&role.entity_type) } }) - .take(limit_usize) .cloned() .collect(); diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index eb1342644f8e..12f778704c7d 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -74,7 +74,7 @@ pub async fn validate_role_name( let is_present_in_custom_role = match state .store - .generic_list_roles_by_entity_type(entity_type_for_role, false, None) + .generic_list_roles_by_entity_type(entity_type_for_role, false) .await { Ok(roles_list) => roles_list From 0f7acd6bf8d7782cd9c10f3812c4340c8b30f85e Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 23 Dec 2024 15:39:27 +0530 Subject: [PATCH 15/19] fix: addressed comments --- crates/common_enums/src/enums.rs | 8 +++----- crates/diesel_models/src/query/role.rs | 7 +------ crates/router/src/core/user_role/role.rs | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/crates/common_enums/src/enums.rs b/crates/common_enums/src/enums.rs index 348cbf76c630..ef2bbc5ea392 100644 --- a/crates/common_enums/src/enums.rs +++ b/crates/common_enums/src/enums.rs @@ -2736,8 +2736,6 @@ pub enum TransactionType { Debug, Eq, PartialEq, - Ord, - PartialOrd, serde::Deserialize, serde::Serialize, strum::Display, @@ -2747,9 +2745,9 @@ pub enum TransactionType { #[serde(rename_all = "snake_case")] #[strum(serialize_all = "snake_case")] pub enum RoleScope { - Organization = 2, - Merchant = 1, - Profile = 0, + Organization, + Merchant, + Profile, } impl From for EntityType { diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index f4e6d1014c2a..bfe957f6b132 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -208,12 +208,7 @@ impl Role { .await { Ok(value) => Ok(value), - Err(err) => match err { - DieselError::NotFound => { - Err(report!(err)).change_context(errors::DatabaseError::NotFound) - } - _ => Err(report!(err)).change_context(errors::DatabaseError::Others), - }, + Err(err) => Err(report!(err)).change_context(errors::DatabaseError::Others), } } } diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 996bb6b37f3c..c16d532902e4 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -115,16 +115,25 @@ pub async fn create_role( ) .await?; - let profile_id = - matches!(role_entity_type, EntityType::Profile).then_some(user_from_token.profile_id); + let (org_id, merchant_id, profile_id) = match role_entity_type { + EntityType::Organization | EntityType::Tenant => { + (user_from_token.org_id, user_from_token.merchant_id, None) + } + EntityType::Merchant => (user_from_token.org_id, user_from_token.merchant_id, None), + EntityType::Profile => ( + user_from_token.org_id, + user_from_token.merchant_id, + Some(user_from_token.profile_id), + ), + }; let role = state .store .insert_role(RoleNew { role_id: generate_id_with_default_len("role"), role_name: role_name.get_role_name(), - merchant_id: user_from_token.merchant_id, - org_id: user_from_token.org_id, + merchant_id, + org_id, groups: req.groups, scope: req.role_scope, entity_type: role_entity_type, From 142ff926dd434370c2b32631a3d191dddcf9a382 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 23 Dec 2024 16:06:08 +0530 Subject: [PATCH 16/19] refactor: refactor code --- crates/router/src/db/role.rs | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/crates/router/src/db/role.rs b/crates/router/src/db/role.rs index 6ae51f9c6ecf..b185f6f1c53a 100644 --- a/crates/router/src/db/role.rs +++ b/crates/router/src/db/role.rs @@ -242,11 +242,10 @@ impl RoleInterface for MockDb { || (role .profile_id .as_ref() - .map(|profile_id_from_role| { + .is_some_and(|profile_id_from_role| { profile_id_from_role == profile_id && role.scope == RoleScope::Profile - }) - .unwrap_or(true))) + }))) }) .cloned() .ok_or( @@ -374,13 +373,7 @@ impl RoleInterface for MockDb { vec![EntityType::Organization] }; - let matches_scope = role.scope == RoleScope::Organization - || role.scope == RoleScope::Merchant - || role.scope == RoleScope::Profile; - - role.org_id == *org_id - && (matches_scope) - && entity_in_vec.contains(&role.entity_type) + role.org_id == *org_id && entity_in_vec.contains(&role.entity_type) } storage::ListRolesByEntityPayload::Merchant(org_id, merchant_id) => { let entity_in_vec = if is_lineage_data_required { @@ -389,15 +382,9 @@ impl RoleInterface for MockDb { vec![EntityType::Merchant] }; - let matches_merchant = - role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant; - let matches_profile = - role.merchant_id == *merchant_id && role.scope == RoleScope::Profile; - role.org_id == *org_id && (role.scope == RoleScope::Organization - || matches_merchant - || matches_profile) + || role.merchant_id == *merchant_id) && entity_in_vec.contains(&role.entity_type) } storage::ListRolesByEntityPayload::Profile(org_id, merchant_id, profile_id) => { @@ -406,15 +393,13 @@ impl RoleInterface for MockDb { let matches_merchant = role.merchant_id == *merchant_id && role.scope == RoleScope::Merchant; - let matches_profile = role.merchant_id == *merchant_id - && role - .profile_id + let matches_profile = + role.profile_id .as_ref() - .map(|profile_id_from_role| { + .is_some_and(|profile_id_from_role| { profile_id_from_role == profile_id && role.scope == RoleScope::Profile - }) - .unwrap_or(true); + }); role.org_id == *org_id && (role.scope == RoleScope::Organization From b1be688ef277316aeb99877d8036f4b98235a9b3 Mon Sep 17 00:00:00 2001 From: "hyperswitch-bot[bot]" <148525504+hyperswitch-bot[bot]@users.noreply.github.com> Date: Mon, 23 Dec 2024 11:53:55 +0000 Subject: [PATCH 17/19] chore: run formatter --- crates/diesel_models/src/query/role.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index bfe957f6b132..7d49ab5d64e2 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -1,7 +1,3 @@ -use crate::{ - enums::RoleScope, errors, query::generics, role::*, schema::roles::dsl, PgPooledConn, - StorageResult, -}; use async_bb8_diesel::AsyncRunQueryDsl; use common_enums::EntityType; use common_utils::id_type; @@ -12,6 +8,11 @@ use diesel::{ use error_stack::{report, ResultExt}; use strum::IntoEnumIterator; +use crate::{ + enums::RoleScope, errors, query::generics, role::*, schema::roles::dsl, PgPooledConn, + StorageResult, +}; + impl RoleNew { pub async fn insert(self, conn: &PgPooledConn) -> StorageResult { generics::generic_insert(conn, self).await From 7fd671b6824d720a6db62a4c66d7b377fa4f629f Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Mon, 23 Dec 2024 17:30:51 +0530 Subject: [PATCH 18/19] refactor: spell correction --- crates/diesel_models/src/query/role.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index bfe957f6b132..06dd827a9ff1 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -19,7 +19,7 @@ impl RoleNew { } impl Role { - fn get_enitity_list( + fn get_entity_list( current_entity: EntityType, is_lineage_data_required: bool, ) -> Vec { @@ -163,7 +163,7 @@ impl Role { match payload { ListRolesByEntityPayload::Organization(org_id) => { let entity_in_vec = - Self::get_enitity_list(EntityType::Organization, is_lineage_data_required); + Self::get_entity_list(EntityType::Organization, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) .filter(dsl::entity_type.eq_any(entity_in_vec)) @@ -171,7 +171,7 @@ impl Role { ListRolesByEntityPayload::Merchant(org_id, merchant_id) => { let entity_in_vec = - Self::get_enitity_list(EntityType::Merchant, is_lineage_data_required); + Self::get_entity_list(EntityType::Merchant, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) .filter( @@ -184,7 +184,7 @@ impl Role { ListRolesByEntityPayload::Profile(org_id, merchant_id, profile_id) => { let entity_in_vec = - Self::get_enitity_list(EntityType::Profile, is_lineage_data_required); + Self::get_entity_list(EntityType::Profile, is_lineage_data_required); query = query .filter(dsl::org_id.eq(org_id)) .filter( From 0ad7d4d478959b841265850f65a924ab14522b69 Mon Sep 17 00:00:00 2001 From: Riddhiagrawal001 Date: Tue, 24 Dec 2024 15:07:19 +0530 Subject: [PATCH 19/19] refactor: removed unused code --- crates/diesel_models/src/query/role.rs | 2 +- crates/diesel_models/src/role.rs | 24 ------------------------ crates/router/src/core/user_role/role.rs | 11 ++--------- 3 files changed, 3 insertions(+), 34 deletions(-) diff --git a/crates/diesel_models/src/query/role.rs b/crates/diesel_models/src/query/role.rs index 278750503d36..2ab22a16473c 100644 --- a/crates/diesel_models/src/query/role.rs +++ b/crates/diesel_models/src/query/role.rs @@ -30,7 +30,7 @@ impl Role { .filter(|variant| *variant <= current_entity) .collect() }) - .unwrap_or_else(|| vec![current_entity]) + .unwrap_or(vec![current_entity]) } pub async fn find_by_role_id(conn: &PgPooledConn, role_id: &str) -> StorageResult { diff --git a/crates/diesel_models/src/role.rs b/crates/diesel_models/src/role.rs index 8adad4e63a05..19d56d2b9984 100644 --- a/crates/diesel_models/src/role.rs +++ b/crates/diesel_models/src/role.rs @@ -86,27 +86,3 @@ pub enum ListRolesByEntityPayload { Merchant(id_type::OrganizationId, id_type::MerchantId), Organization(id_type::OrganizationId), } - -impl ListRolesByEntityPayload { - pub fn get_organization_id(&self) -> Option { - match self { - Self::Organization(org_id) - | Self::Merchant(org_id, _) - | Self::Profile(org_id, _, _) => Some(org_id.to_owned()), - } - } - pub fn get_merchant_id(&self) -> Option { - match self { - Self::Organization(_) => None, - Self::Merchant(_, merchant_id) | Self::Profile(_, merchant_id, _) => { - Some(merchant_id.to_owned()) - } - } - } - pub fn get_profile_id(&self) -> Option { - match self { - Self::Organization(_) | Self::Merchant(_, _) => None, - Self::Profile(_, _, profile_id) => Some(profile_id.to_owned()), - } - } -} diff --git a/crates/router/src/core/user_role/role.rs b/crates/router/src/core/user_role/role.rs index 05299f52d66b..edfbc4f9b705 100644 --- a/crates/router/src/core/user_role/role.rs +++ b/crates/router/src/core/user_role/role.rs @@ -90,7 +90,7 @@ pub async fn create_role( if requestor_entity_from_role_scope < role_entity_type { return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( "User is trying to create role of type {} and scope {}", - requestor_entity_from_role_scope, role_entity_type + role_entity_type, requestor_entity_from_role_scope )); } let max_from_scope_and_entity = cmp::max(requestor_entity_from_role_scope, role_entity_type); @@ -246,19 +246,12 @@ pub async fn update_role( let requested_entity_from_role_scope = EntityType::from(role_info.get_scope()); let requested_role_entity_type = role_info.get_entity_type(); - - if requested_entity_from_role_scope < requested_role_entity_type { - return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( - "User is trying to create role of type {} and scope {}", - requested_entity_from_role_scope, requested_role_entity_type - )); - } let max_from_scope_and_entity = cmp::max(requested_entity_from_role_scope, requested_role_entity_type); if user_role_info.get_entity_type() < max_from_scope_and_entity { return Err(report!(UserErrors::InvalidRoleOperation)).attach_printable(format!( - "{} is trying to create of scope {} and of type {}", + "{} is trying to update of scope {} and of type {}", user_role_info.get_entity_type(), requested_entity_from_role_scope, requested_role_entity_type