From edd6806f97b8d400f1215d845023bb0d7c06aaca Mon Sep 17 00:00:00 2001 From: Mani Chandra <84711804+ThisIsMani@users.noreply.github.com> Date: Thu, 8 Feb 2024 14:19:44 +0530 Subject: [PATCH] refactor(user_role): Change update user role request to take `email` instead of `user_id` (#3530) Co-authored-by: hyperswitch-bot[bot] <148525504+hyperswitch-bot[bot]@users.noreply.github.com> --- crates/api_models/src/user.rs | 1 - crates/api_models/src/user_role.rs | 2 +- crates/router/src/core/errors/user.rs | 15 ++++- crates/router/src/core/errors/utils.rs | 31 ++++++++++ crates/router/src/core/user.rs | 53 ++++++++++------ crates/router/src/core/user_role.rs | 60 ++++++++++++------- .../authorization/predefined_permissions.rs | 38 ++++++++++-- crates/router/src/types/domain/user.rs | 15 +---- crates/router/src/utils/user.rs | 20 +++++-- crates/router/src/utils/user_role.rs | 13 +--- 10 files changed, 172 insertions(+), 76 deletions(-) diff --git a/crates/api_models/src/user.rs b/crates/api_models/src/user.rs index b29ce811be..c3e6908c73 100644 --- a/crates/api_models/src/user.rs +++ b/crates/api_models/src/user.rs @@ -142,7 +142,6 @@ pub struct GetUsersResponse(pub Vec); #[derive(Debug, serde::Serialize)] pub struct UserDetails { - pub user_id: String, pub email: pii::Email, pub name: Secret, pub role_id: String, diff --git a/crates/api_models/src/user_role.rs b/crates/api_models/src/user_role.rs index e8c9b777c7..2672293390 100644 --- a/crates/api_models/src/user_role.rs +++ b/crates/api_models/src/user_role.rs @@ -86,7 +86,7 @@ pub struct PermissionInfo { #[derive(Debug, serde::Deserialize, serde::Serialize)] pub struct UpdateUserRoleRequest { - pub user_id: String, + pub email: pii::Email, pub role_id: String, } diff --git a/crates/router/src/core/errors/user.rs b/crates/router/src/core/errors/user.rs index d3b1679378..c837fd7c20 100644 --- a/crates/router/src/core/errors/user.rs +++ b/crates/router/src/core/errors/user.rs @@ -60,6 +60,8 @@ pub enum UserErrors { MaxInvitationsError, #[error("RoleNotFound")] RoleNotFound, + #[error("InvalidRoleOperationWithMessage")] + InvalidRoleOperationWithMessage(String), } impl common_utils::errors::ErrorSwitch for UserErrors { @@ -103,9 +105,12 @@ impl common_utils::errors::ErrorSwitch { AER::BadRequest(ApiError::new(sub_code, 14, self.get_error_message(), None)) } - Self::MerchantAccountCreationError(error_message) => { - AER::InternalServerError(ApiError::new(sub_code, 15, error_message, None)) - } + Self::MerchantAccountCreationError(_) => AER::InternalServerError(ApiError::new( + sub_code, + 15, + self.get_error_message(), + None, + )), Self::InvalidEmailError => { AER::BadRequest(ApiError::new(sub_code, 16, self.get_error_message(), None)) } @@ -151,6 +156,9 @@ impl common_utils::errors::ErrorSwitch { AER::BadRequest(ApiError::new(sub_code, 32, self.get_error_message(), None)) } + Self::InvalidRoleOperationWithMessage(_) => { + AER::BadRequest(ApiError::new(sub_code, 33, self.get_error_message(), None)) + } } } } @@ -184,6 +192,7 @@ impl UserErrors { Self::InvalidDeleteOperation => "Delete Operation Not Supported", Self::MaxInvitationsError => "Maximum invite count per request exceeded", Self::RoleNotFound => "Role Not Found", + Self::InvalidRoleOperationWithMessage(error_message) => error_message, } } } diff --git a/crates/router/src/core/errors/utils.rs b/crates/router/src/core/errors/utils.rs index a54dae1c03..1f82132a45 100644 --- a/crates/router/src/core/errors/utils.rs +++ b/crates/router/src/core/errors/utils.rs @@ -509,3 +509,34 @@ impl RedisErrorExt for error_stack::Report { } } } + +#[cfg(feature = "olap")] +impl StorageErrorExt for error_stack::Result { + #[track_caller] + fn to_not_found_response( + self, + not_found_response: errors::UserErrors, + ) -> error_stack::Result { + self.map_err(|e| { + if e.current_context().is_db_not_found() { + e.change_context(not_found_response) + } else { + e.change_context(errors::UserErrors::InternalServerError) + } + }) + } + + #[track_caller] + fn to_duplicate_response( + self, + duplicate_response: errors::UserErrors, + ) -> error_stack::Result { + self.map_err(|e| { + if e.current_context().is_db_unique_violation() { + e.change_context(duplicate_response) + } else { + e.change_context(errors::UserErrors::InternalServerError) + } + }) + } +} diff --git a/crates/router/src/core/user.rs b/crates/router/src/core/user.rs index 70c3eb6673..7b37f529f8 100644 --- a/crates/router/src/core/user.rs +++ b/crates/router/src/core/user.rs @@ -11,13 +11,15 @@ use router_env::env; #[cfg(feature = "email")] use router_env::logger; -use super::errors::{UserErrors, UserResponse, UserResult}; +use super::errors::{StorageErrorExt, UserErrors, UserResponse, UserResult}; #[cfg(feature = "email")] use crate::services::email::types as email_types; use crate::{ consts, routes::AppState, - services::{authentication as auth, ApplicationResponse}, + services::{ + authentication as auth, authorization::predefined_permissions, ApplicationResponse, + }, types::domain, utils, }; @@ -150,7 +152,7 @@ pub async fn signin( let preferred_role = user_from_db .get_role_from_db_by_merchant_id(&state, preferred_merchant_id.as_str()) .await - .change_context(UserErrors::InternalServerError) + .to_not_found_response(UserErrors::InternalServerError) .attach_printable("User role with preferred_merchant_id not found")?; domain::SignInWithRoleStrategyType::SingleRole(domain::SignInWithSingleRoleStrategy { user: user_from_db, @@ -408,11 +410,17 @@ pub async fn invite_user( .change_context(UserErrors::InternalServerError)?; if inviter_user.email == request.email { - return Err(UserErrors::InvalidRoleOperation.into()) - .attach_printable("User Inviting themself"); + return Err(UserErrors::InvalidRoleOperationWithMessage( + "User Inviting themselves".to_string(), + ) + .into()); + } + + if !predefined_permissions::is_role_invitable(request.role_id.as_str())? { + return Err(UserErrors::InvalidRoleId.into()) + .attach_printable(format!("role_id = {} is not invitable", request.role_id)); } - utils::user_role::validate_role_id(request.role_id.as_str())?; let invitee_email = domain::UserEmail::from_pii_email(request.email.clone())?; let invitee_user = state @@ -561,14 +569,20 @@ async fn handle_invitation( user_from_token: &auth::UserFromToken, request: &user_api::InviteUserRequest, ) -> UserResult { - let inviter_user = user_from_token.get_user(state.clone()).await?; + let inviter_user = user_from_token.get_user(state).await?; if inviter_user.email == request.email { - return Err(UserErrors::InvalidRoleOperation.into()) - .attach_printable("User Inviting themself"); + return Err(UserErrors::InvalidRoleOperationWithMessage( + "User Inviting themselves".to_string(), + ) + .into()); + } + + if !predefined_permissions::is_role_invitable(request.role_id.as_str())? { + return Err(UserErrors::InvalidRoleId.into()) + .attach_printable(format!("role_id = {} is not invitable", request.role_id)); } - utils::user_role::validate_role_id(request.role_id.as_str())?; let invitee_email = domain::UserEmail::from_pii_email(request.email.clone())?; let invitee_user = state .store @@ -733,7 +747,11 @@ pub async fn resend_invite( .map_err(|e| { if e.current_context().is_db_not_found() { e.change_context(UserErrors::InvalidRoleOperation) - .attach_printable("User role with given UserId MerchantId not found") + .attach_printable(format!( + "User role with user_id = {} and org_id = {} is not found", + user.get_user_id(), + user_from_token.merchant_id + )) } else { e.change_context(UserErrors::InternalServerError) } @@ -741,7 +759,7 @@ pub async fn resend_invite( if !matches!(user_role.status, UserStatus::InvitationSent) { return Err(UserErrors::InvalidRoleOperation.into()) - .attach_printable("Invalid Status for Reinvitation"); + .attach_printable("User status is not InvitationSent".to_string()); } let email_contents = email_types::InviteUser { @@ -832,8 +850,10 @@ pub async fn switch_merchant_id( user_from_token: auth::UserFromToken, ) -> UserResponse { if user_from_token.merchant_id == request.merchant_id { - return Err(UserErrors::InvalidRoleOperation.into()) - .attach_printable("User switch to same merchant id."); + return Err(UserErrors::InvalidRoleOperationWithMessage( + "User switching to same merchant id".to_string(), + ) + .into()); } let user_roles = state @@ -847,7 +867,7 @@ pub async fn switch_merchant_id( .filter(|role| role.status == UserStatus::Active) .collect::>(); - let user = user_from_token.get_user(state.clone()).await?.into(); + let user = user_from_token.get_user(&state).await?.into(); let (token, role_id) = if utils::user_role::is_internal_role(&user_from_token.role_id) { let key_store = state @@ -916,8 +936,7 @@ pub async fn create_merchant_account( user_from_token: auth::UserFromToken, req: user_api::UserMerchantCreate, ) -> UserResponse<()> { - let user_from_db: domain::UserFromStorage = - user_from_token.get_user(state.clone()).await?.into(); + let user_from_db: domain::UserFromStorage = user_from_token.get_user(&state).await?.into(); let new_user = domain::NewUser::try_from((user_from_db, req, user_from_token))?; let new_merchant = new_user.get_new_merchant(); diff --git a/crates/router/src/core/user_role.rs b/crates/router/src/core/user_role.rs index 742c281b89..b48b39eea1 100644 --- a/crates/router/src/core/user_role.rs +++ b/crates/router/src/core/user_role.rs @@ -5,7 +5,7 @@ use masking::ExposeInterface; use router_env::logger; use crate::{ - core::errors::{UserErrors, UserResponse}, + core::errors::{StorageErrorExt, UserErrors, UserResponse}, routes::AppState, services::{ authentication::{self as auth}, @@ -33,6 +33,7 @@ pub async fn list_roles(_state: AppState) -> UserResponse UserResponse<()> { - let merchant_id = user_from_token.merchant_id; - let role_id = req.role_id.clone(); - utils::user_role::validate_role_id(role_id.as_str())?; - - if user_from_token.user_id == req.user_id { + if !predefined_permissions::is_role_updatable(&req.role_id)? { return Err(UserErrors::InvalidRoleOperation.into()) - .attach_printable("Admin User Changing their role"); + .attach_printable(format!("User role cannot be updated to {}", req.role_id)); + } + + let user_to_be_updated = + utils::user::get_user_from_db_by_email(&state, domain::UserEmail::try_from(req.email)?) + .await + .to_not_found_response(UserErrors::InvalidRoleOperation) + .attach_printable("User not found in our records".to_string())?; + + if user_from_token.user_id == user_to_be_updated.get_user_id() { + return Err(UserErrors::InvalidRoleOperation.into()) + .attach_printable("User Changing their own role"); + } + + let user_role_to_be_updated = user_to_be_updated + .get_role_from_db_by_merchant_id(&state, &user_from_token.merchant_id) + .await + .to_not_found_response(UserErrors::InvalidRoleOperation)?; + + if !predefined_permissions::is_role_updatable(&user_role_to_be_updated.role_id)? { + return Err(UserErrors::InvalidRoleOperation.into()).attach_printable(format!( + "User role cannot be updated from {}", + user_role_to_be_updated.role_id + )); } state .store .update_user_role_by_user_id_merchant_id( - req.user_id.as_str(), - merchant_id.as_str(), + user_to_be_updated.get_user_id(), + user_role_to_be_updated.merchant_id.as_str(), UserRoleUpdate::UpdateRole { - role_id, + role_id: req.role_id.clone(), modified_by: user_from_token.user_id, }, ) .await - .map_err(|e| { - if e.current_context().is_db_not_found() { - return e - .change_context(UserErrors::InvalidRoleOperation) - .attach_printable("UserId MerchantId not found"); - } - e.change_context(UserErrors::InternalServerError) - })?; + .to_not_found_response(UserErrors::InvalidRoleOperation) + .attach_printable("User with given email is not found in the organization")?; + + auth::blacklist::insert_user_in_blacklist(&state, user_to_be_updated.get_user_id()).await?; Ok(ApplicationResponse::StatusOk) } @@ -181,7 +197,7 @@ pub async fn delete_user_role( .map_err(|e| { if e.current_context().is_db_not_found() { e.change_context(UserErrors::InvalidRoleOperation) - .attach_printable("User not found in records") + .attach_printable("User not found in our records") } else { e.change_context(UserErrors::InternalServerError) } @@ -204,9 +220,9 @@ pub async fn delete_user_role( .find(|&role| role.merchant_id == user_from_token.merchant_id.as_str()) { Some(user_role) => { - if !predefined_permissions::is_role_deletable(&user_role.role_id) { - return Err(UserErrors::InvalidRoleId.into()) - .attach_printable("Deletion not allowed for users with specific role id"); + if !predefined_permissions::is_role_deletable(&user_role.role_id)? { + return Err(UserErrors::InvalidDeleteOperation.into()) + .attach_printable(format!("role_id = {} is not deletable", user_role.role_id)); } } None => { diff --git a/crates/router/src/services/authorization/predefined_permissions.rs b/crates/router/src/services/authorization/predefined_permissions.rs index 6fe0ddcc36..fd98d90c19 100644 --- a/crates/router/src/services/authorization/predefined_permissions.rs +++ b/crates/router/src/services/authorization/predefined_permissions.rs @@ -1,15 +1,21 @@ use std::collections::HashMap; +#[cfg(feature = "olap")] +use error_stack::ResultExt; use once_cell::sync::Lazy; use super::permissions::Permission; use crate::consts; +#[cfg(feature = "olap")] +use crate::core::errors::{UserErrors, UserResult}; +#[allow(dead_code)] pub struct RoleInfo { permissions: Vec, name: Option<&'static str>, is_invitable: bool, is_deletable: bool, + is_updatable: bool, } impl RoleInfo { @@ -65,6 +71,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: None, is_invitable: false, is_deletable: false, + is_updatable: false, }, ); roles.insert( @@ -90,6 +97,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: None, is_invitable: false, is_deletable: false, + is_updatable: false, }, ); @@ -130,6 +138,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("Organization Admin"), is_invitable: false, is_deletable: false, + is_updatable: false, }, ); @@ -170,6 +179,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("Admin"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles.insert( @@ -195,6 +205,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("View Only"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles.insert( @@ -221,6 +232,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("IAM"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles.insert( @@ -247,6 +259,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("Developer"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles.insert( @@ -278,6 +291,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("Operator"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles.insert( @@ -301,6 +315,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy> = Lazy: name: Some("Customer Support"), is_invitable: true, is_deletable: true, + is_updatable: true, }, ); roles @@ -312,14 +327,29 @@ pub fn get_role_name_from_id(role_id: &str) -> Option<&'static str> { .and_then(|role_info| role_info.name) } -pub fn is_role_invitable(role_id: &str) -> bool { +#[cfg(feature = "olap")] +pub fn is_role_invitable(role_id: &str) -> UserResult { PREDEFINED_PERMISSIONS .get(role_id) - .map_or(false, |role_info| role_info.is_invitable) + .map(|role_info| role_info.is_invitable) + .ok_or(UserErrors::InvalidRoleId.into()) + .attach_printable(format!("role_id = {} doesn't exist", role_id)) } -pub fn is_role_deletable(role_id: &str) -> bool { +#[cfg(feature = "olap")] +pub fn is_role_deletable(role_id: &str) -> UserResult { PREDEFINED_PERMISSIONS .get(role_id) - .map_or(false, |role_info| role_info.is_deletable) + .map(|role_info| role_info.is_deletable) + .ok_or(UserErrors::InvalidRoleId.into()) + .attach_printable(format!("role_id = {} doesn't exist", role_id)) +} + +#[cfg(feature = "olap")] +pub fn is_role_updatable(role_id: &str) -> UserResult { + PREDEFINED_PERMISSIONS + .get(role_id) + .map(|role_info| role_info.is_updatable) + .ok_or(UserErrors::InvalidRoleId.into()) + .attach_printable(format!("role_id = {} doesn't exist", role_id)) } diff --git a/crates/router/src/types/domain/user.rs b/crates/router/src/types/domain/user.rs index d3ea69ecd8..468fa8e4cd 100644 --- a/crates/router/src/types/domain/user.rs +++ b/crates/router/src/types/domain/user.rs @@ -3,7 +3,7 @@ use std::{collections::HashSet, ops, str::FromStr}; use api_models::{ admin as admin_api, organization as api_org, user as user_api, user_role as user_role_api, }; -use common_utils::pii; +use common_utils::{errors::CustomResult, pii}; use diesel_models::{ enums::UserStatus, organization as diesel_org, @@ -21,7 +21,7 @@ use crate::{ consts, core::{ admin, - errors::{UserErrors, UserResult}, + errors::{self, UserErrors, UserResult}, }, db::StorageInterface, routes::AppState, @@ -778,19 +778,11 @@ impl UserFromStorage { &self, state: &AppState, merchant_id: &str, - ) -> UserResult { + ) -> CustomResult { state .store .find_user_role_by_user_id_merchant_id(self.get_user_id(), merchant_id) .await - .map_err(|e| { - if e.current_context().is_db_not_found() { - UserErrors::RoleNotFound - } else { - UserErrors::InternalServerError - } - }) - .into_report() } } @@ -850,7 +842,6 @@ impl TryFrom for user_api::UserDetails { .to_string(); Ok(Self { - user_id: user_and_role.0.user_id, email: user_and_role.0.email, name: user_and_role.0.name, role_id, diff --git a/crates/router/src/utils/user.rs b/crates/router/src/utils/user.rs index 697d10f772..9c2d2c1fd3 100644 --- a/crates/router/src/utils/user.rs +++ b/crates/router/src/utils/user.rs @@ -1,15 +1,16 @@ use std::collections::HashMap; use api_models::user as user_api; +use common_utils::errors::CustomResult; use diesel_models::{enums::UserStatus, user_role::UserRole}; use error_stack::ResultExt; -use masking::Secret; +use masking::{ExposeInterface, Secret}; use crate::{ - core::errors::{UserErrors, UserResult}, + core::errors::{StorageError, UserErrors, UserResult}, routes::AppState, services::authentication::{AuthToken, UserFromToken}, - types::domain::{MerchantAccount, UserFromStorage}, + types::domain::{self, MerchantAccount, UserFromStorage}, }; pub mod dashboard_metadata; @@ -47,7 +48,7 @@ impl UserFromToken { Ok(merchant_account) } - pub async fn get_user(&self, state: AppState) -> UserResult { + pub async fn get_user(&self, state: &AppState) -> UserResult { let user = state .store .find_user_by_id(&self.user_id) @@ -146,3 +147,14 @@ pub fn get_multiple_merchant_details_with_status( }) .collect() } + +pub async fn get_user_from_db_by_email( + state: &AppState, + email: domain::UserEmail, +) -> CustomResult { + state + .store + .find_user_by_email(email.get_secret().expose().as_str()) + .await + .map(UserFromStorage::from) +} diff --git a/crates/router/src/utils/user_role.rs b/crates/router/src/utils/user_role.rs index 462d3d01d7..9c7150c08d 100644 --- a/crates/router/src/utils/user_role.rs +++ b/crates/router/src/utils/user_role.rs @@ -2,11 +2,7 @@ use api_models::user_role as user_role_api; use crate::{ consts, - core::errors::{UserErrors, UserResult}, - services::authorization::{ - permissions::Permission, - predefined_permissions::{self, RoleInfo}, - }, + services::authorization::{permissions::Permission, predefined_permissions::RoleInfo}, }; pub fn is_internal_role(role_id: &str) -> bool { @@ -14,13 +10,6 @@ pub fn is_internal_role(role_id: &str) -> bool { || role_id == consts::user_role::ROLE_ID_INTERNAL_VIEW_ONLY_USER } -pub fn validate_role_id(role_id: &str) -> UserResult<()> { - if predefined_permissions::is_role_invitable(role_id) { - return Ok(()); - } - Err(UserErrors::InvalidRoleId.into()) -} - pub fn get_role_name_and_permission_response( role_info: &RoleInfo, ) -> Option<(Vec, &'static str)> {