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

@@ -142,7 +142,6 @@ pub struct GetUsersResponse(pub Vec<UserDetails>);
 
 #[derive(Debug, serde::Serialize)]
 pub struct UserDetails {
-    pub user_id: String,
     pub email: pii::Email,
     pub name: Secret<String>,
     pub role_id: String,

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,
 }
 

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<api_models::errors::types::ApiErrorResponse> for UserErrors {
@@ -103,9 +105,12 @@ impl common_utils::errors::ErrorSwitch<api_models::errors::types::ApiErrorRespon
             Self::CompanyNameParsingError => {
                 AER::BadRequest(ApiError::new(sub_code, 14, self.get_error_message(), None))
             }
-            Self::MerchantAccountCreationError(error_message) => {
+            Self::MerchantAccountCreationError(_) => AER::InternalServerError(ApiError::new(
-                AER::InternalServerError(ApiError::new(sub_code, 15, error_message, None))
+                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<api_models::errors::types::ApiErrorRespon
             Self::RoleNotFound => {
                 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,
         }
     }
 }

crates/router/src/core/errors/utils.rs

@@ -509,3 +509,34 @@ impl RedisErrorExt for error_stack::Report<errors::RedisError> {
         }
     }
 }
+
+#[cfg(feature = "olap")]
+impl<T> StorageErrorExt<T, errors::UserErrors> for error_stack::Result<T, errors::StorageError> {
+    #[track_caller]
+    fn to_not_found_response(
+        self,
+        not_found_response: errors::UserErrors,
+    ) -> error_stack::Result<T, errors::UserErrors> {
+        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<T, errors::UserErrors> {
+        self.map_err(|e| {
+            if e.current_context().is_db_unique_violation() {
+                e.change_context(duplicate_response)
+            } else {
+                e.change_context(errors::UserErrors::InternalServerError)
+            }
+        })
+    }
+}

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())
+        return Err(UserErrors::InvalidRoleOperationWithMessage(
-            .attach_printable("User Inviting themself");
+            "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<InviteMultipleUserResponse> {
-    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())
+        return Err(UserErrors::InvalidRoleOperationWithMessage(
-            .attach_printable("User Inviting themself");
+            "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<user_api::SwitchMerchantResponse> {
     if user_from_token.merchant_id == request.merchant_id {
-        return Err(UserErrors::InvalidRoleOperation.into())
+        return Err(UserErrors::InvalidRoleOperationWithMessage(
-            .attach_printable("User switch to same merchant id.");
+            "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::<Vec<_>>();
 
-    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 =
+    let user_from_db: domain::UserFromStorage = user_from_token.get_user(&state).await?.into();
-        user_from_token.get_user(state.clone()).await?.into();
 
     let new_user = domain::NewUser::try_from((user_from_db, req, user_from_token))?;
     let new_merchant = new_user.get_new_merchant();

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<user_role_api::ListRol
     Ok(ApplicationResponse::Json(user_role_api::ListRolesResponse(
         predefined_permissions::PREDEFINED_PERMISSIONS
             .iter()
+            .filter(|(_, role_info)| role_info.is_invitable())
             .filter_map(|(role_id, role_info)| {
                 utils::user_role::get_role_name_and_permission_response(role_info).map(
                     |(permissions, role_name)| user_role_api::RoleInfoResponse {
@@ -87,34 +88,49 @@ pub async fn update_user_role(
     user_from_token: auth::UserFromToken,
     req: user_role_api::UpdateUserRoleRequest,
 ) -> UserResponse<()> {
-    let merchant_id = user_from_token.merchant_id;
+    if !predefined_permissions::is_role_updatable(&req.role_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 {
         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(),
+            user_to_be_updated.get_user_id(),
-            merchant_id.as_str(),
+            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| {
+        .to_not_found_response(UserErrors::InvalidRoleOperation)
-            if e.current_context().is_db_not_found() {
+        .attach_printable("User with given email is not found in the organization")?;
-                return e
+
-                    .change_context(UserErrors::InvalidRoleOperation)
+    auth::blacklist::insert_user_in_blacklist(&state, user_to_be_updated.get_user_id()).await?;
-                    .attach_printable("UserId MerchantId not found");
-            }
-            e.change_context(UserErrors::InternalServerError)
-        })?;
 
     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) {
+            if !predefined_permissions::is_role_deletable(&user_role.role_id)? {
-                return Err(UserErrors::InvalidRoleId.into())
+                return Err(UserErrors::InvalidDeleteOperation.into())
-                    .attach_printable("Deletion not allowed for users with specific role id");
+                    .attach_printable(format!("role_id = {} is not deletable", user_role.role_id));
             }
         }
         None => {

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<Permission>,
     name: Option<&'static str>,
     is_invitable: bool,
     is_deletable: bool,
+    is_updatable: bool,
 }
 
 impl RoleInfo {
@@ -65,6 +71,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: None,
             is_invitable: false,
             is_deletable: false,
+            is_updatable: false,
         },
     );
     roles.insert(
@@ -90,6 +97,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: None,
             is_invitable: false,
             is_deletable: false,
+            is_updatable: false,
         },
     );
 
@@ -130,6 +138,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: Some("Organization Admin"),
             is_invitable: false,
             is_deletable: false,
+            is_updatable: false,
         },
     );
 
@@ -170,6 +179,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: Some("Admin"),
             is_invitable: true,
             is_deletable: true,
+            is_updatable: true,
         },
     );
     roles.insert(
@@ -195,6 +205,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = 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<HashMap<&'static str, RoleInfo>> = Lazy:
             name: Some("IAM"),
             is_invitable: true,
             is_deletable: true,
+            is_updatable: true,
         },
     );
     roles.insert(
@@ -247,6 +259,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: Some("Developer"),
             is_invitable: true,
             is_deletable: true,
+            is_updatable: true,
         },
     );
     roles.insert(
@@ -278,6 +291,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = Lazy:
             name: Some("Operator"),
             is_invitable: true,
             is_deletable: true,
+            is_updatable: true,
         },
     );
     roles.insert(
@@ -301,6 +315,7 @@ pub static PREDEFINED_PERMISSIONS: Lazy<HashMap<&'static str, RoleInfo>> = 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<bool> {
     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<bool> {
     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<bool> {
+    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))
 }

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<UserRole> {
+    ) -> CustomResult<UserRole, errors::StorageError> {
         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<UserAndRoleJoined> 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,

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<diesel_models::user::User> {
+    pub async fn get_user(&self, state: &AppState) -> UserResult<diesel_models::user::User> {
         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<UserFromStorage, StorageError> {
+    state
+        .store
+        .find_user_by_email(email.get_secret().expose().as_str())
+        .await
+        .map(UserFromStorage::from)
+}

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::RoleInfo},
-    services::authorization::{
-        permissions::Permission,
-        predefined_permissions::{self, 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<user_role_api::Permission>, &'static str)> {