refactor(roles): Add more checks in create, update role APIs and change the response type (#3896)

crates/api_models/src/events/user_role.rs

@@ -3,7 +3,8 @@ use common_utils::events::{ApiEventMetric, ApiEventsType};
 use crate::user_role::{
     role::{
         CreateRoleRequest, GetRoleFromTokenResponse, GetRoleRequest, ListRolesResponse,
-        RoleInfoResponse, RoleInfoWithPermissionsResponse, UpdateRoleRequest,
+        RoleInfoResponse, RoleInfoWithGroupsResponse, RoleInfoWithPermissionsResponse,
+        UpdateRoleRequest,
     },
     AcceptInvitationRequest, AuthorizationInfoResponse, DeleteUserRoleRequest,
     TransferOrgOwnershipRequest, UpdateUserRoleRequest,
@@ -21,5 +22,6 @@ common_utils::impl_misc_api_event_type!(
     UpdateRoleRequest,
     ListRolesResponse,
     RoleInfoResponse,
-    GetRoleFromTokenResponse
+    GetRoleFromTokenResponse,
+    RoleInfoWithGroupsResponse
 );

crates/router/src/core/user_role/role.rs

@@ -57,14 +57,18 @@ pub async fn create_role(
     state: AppState,
     user_from_token: UserFromToken,
     req: role_api::CreateRoleRequest,
-) -> UserResponse<()> {
+) -> UserResponse<role_api::RoleInfoWithGroupsResponse> {
     let now = common_utils::date_time::now();
-    let role_name = RoleName::new(req.role_name)?.get_role_name();
+    let role_name = RoleName::new(req.role_name)?;
 
-    if req.groups.is_empty() {
+    utils::user_role::validate_role_groups(&req.groups)?;
-        return Err(UserErrors::InvalidRoleOperation.into())
+    utils::user_role::validate_role_name(
-            .attach_printable("Role groups cannot be empty");
+        &state,
-    }
+        &role_name,
+        &user_from_token.merchant_id,
+        &user_from_token.org_id,
+    )
+    .await?;
 
     if matches!(req.role_scope, RoleScope::Organization)
         && user_from_token.role_id != consts::user_role::ROLE_ID_ORGANIZATION_ADMIN
@@ -73,19 +77,11 @@ pub async fn create_role(
             .attach_printable("Non org admin user creating org level role");
     }
 
-    utils::user_role::is_role_name_already_present_for_merchant(
+    let role = state
-        &state,
-        &role_name,
-        &user_from_token.merchant_id,
-        &user_from_token.org_id,
-    )
-    .await?;
-
-    state
         .store
         .insert_role(RoleNew {
             role_id: generate_id_with_default_len("role"),
-            role_name,
+            role_name: role_name.get_role_name(),
             merchant_id: user_from_token.merchant_id,
             org_id: user_from_token.org_id,
             groups: req.groups,
@@ -98,7 +94,14 @@ pub async fn create_role(
         .await
         .to_duplicate_response(UserErrors::RoleNameAlreadyExists)?;
 
-    Ok(ApplicationResponse::StatusOk)
+    Ok(ApplicationResponse::Json(
+        role_api::RoleInfoWithGroupsResponse {
+            groups: role.groups,
+            role_id: role.role_id,
+            role_name: role.role_name,
+            role_scope: role.scope,
+        },
+    ))
 }
 
 // TODO: To be deprecated once groups are stable
@@ -260,15 +263,11 @@ pub async fn update_role(
     user_from_token: UserFromToken,
     req: role_api::UpdateRoleRequest,
     role_id: &str,
-) -> UserResponse<()> {
+) -> UserResponse<role_api::RoleInfoWithGroupsResponse> {
-    let role_name = req
+    let role_name = req.role_name.map(RoleName::new).transpose()?;
-        .role_name
-        .map(RoleName::new)
-        .transpose()?
-        .map(RoleName::get_role_name);
 
     if let Some(ref role_name) = role_name {
-        utils::user_role::is_role_name_already_present_for_merchant(
+        utils::user_role::validate_role_name(
             &state,
             role_name,
             &user_from_token.merchant_id,
@@ -277,6 +276,10 @@ pub async fn update_role(
         .await?;
     }
 
+    if let Some(ref groups) = req.groups {
+        utils::user_role::validate_role_groups(groups)?;
+    }
+
     let role_info = roles::RoleInfo::from_role_id(
         &state,
         role_id,
@@ -290,23 +293,16 @@ pub async fn update_role(
         && user_from_token.role_id != consts::user_role::ROLE_ID_ORGANIZATION_ADMIN
     {
         return Err(UserErrors::InvalidRoleOperation.into())
-            .attach_printable("Non org admin user creating org level role");
+            .attach_printable("Non org admin user changing org level role");
     }
 
-    if let Some(ref groups) = req.groups {
+    let updated_role = state
-        if groups.is_empty() {
-            return Err(UserErrors::InvalidRoleOperation.into())
-                .attach_printable("role groups cannot be empty");
-        }
-    }
-
-    state
         .store
         .update_role_by_role_id(
             role_id,
             RoleUpdate::UpdateDetails {
                 groups: req.groups,
-                role_name,
+                role_name: role_name.map(RoleName::get_role_name),
                 last_modified_at: common_utils::date_time::now(),
                 last_modified_by: user_from_token.user_id,
             },
@@ -316,5 +312,12 @@ pub async fn update_role(
 
     blacklist::insert_role_in_blacklist(&state, role_id).await?;
 
-    Ok(ApplicationResponse::StatusOk)
+    Ok(ApplicationResponse::Json(
+        role_api::RoleInfoWithGroupsResponse {
+            groups: updated_role.groups,
+            role_id: updated_role.role_id,
+            role_name: updated_role.role_name,
+            role_scope: updated_role.scope,
+        },
+    ))
 }

crates/router/src/services/authorization/roles/predefined_roles.rs

@@ -26,7 +26,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::OrganizationManage,
             ],
             role_id: consts::user_role::ROLE_ID_INTERNAL_ADMIN.to_string(),
-            role_name: "Internal Admin".to_string(),
+            role_name: "internal_admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -46,7 +46,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_INTERNAL_VIEW_ONLY_USER.to_string(),
-            role_name: "Internal View Only".to_string(),
+            role_name: "internal_view_only".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -73,7 +73,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::OrganizationManage,
             ],
             role_id: consts::user_role::ROLE_ID_ORGANIZATION_ADMIN.to_string(),
-            role_name: "Organization Admin".to_string(),
+            role_name: "organization_admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: false,
             is_deletable: false,
@@ -100,7 +100,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsManage,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_ADMIN.to_string(),
-            role_name: "Admin".to_string(),
+            role_name: "admin".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -120,7 +120,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_VIEW_ONLY.to_string(),
-            role_name: "View Only".to_string(),
+            role_name: "view_only".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -139,7 +139,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_IAM_ADMIN.to_string(),
-            role_name: "IAM".to_string(),
+            role_name: "iam".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -159,7 +159,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsManage,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_DEVELOPER.to_string(),
-            role_name: "Developer".to_string(),
+            role_name: "developer".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -180,7 +180,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_OPERATOR.to_string(),
-            role_name: "Operator".to_string(),
+            role_name: "operator".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,
@@ -198,7 +198,7 @@ pub static PREDEFINED_ROLES: Lazy<HashMap<&'static str, RoleInfo>> = Lazy::new(|
                 PermissionGroup::MerchantDetailsView,
             ],
             role_id: consts::user_role::ROLE_ID_MERCHANT_CUSTOMER_SUPPORT.to_string(),
-            role_name: "Customer Support".to_string(),
+            role_name: "customer_support".to_string(),
             scope: RoleScope::Organization,
             is_invitable: true,
             is_deletable: true,

crates/router/src/utils/user_role.rs

@@ -1,10 +1,12 @@
 use api_models::user_role as user_role_api;
+use common_enums::PermissionGroup;
 use error_stack::ResultExt;
 
 use crate::{
     core::errors::{UserErrors, UserResult},
     routes::AppState,
-    services::authorization::permissions::Permission,
+    services::authorization::{permissions::Permission, roles},
+    types::domain,
 };
 
 impl From<Permission> for user_role_api::Permission {
@@ -40,23 +42,44 @@ impl From<Permission> for user_role_api::Permission {
     }
 }
 
-pub async fn is_role_name_already_present_for_merchant(
+pub fn validate_role_groups(groups: &[PermissionGroup]) -> UserResult<()> {
+    if groups.is_empty() {
+        return Err(UserErrors::InvalidRoleOperation.into())
+            .attach_printable("Role groups cannot be empty");
+    }
+
+    if groups.contains(&PermissionGroup::OrganizationManage) {
+        return Err(UserErrors::InvalidRoleOperation.into())
+            .attach_printable("Organization manage group cannot be added to role");
+    }
+
+    Ok(())
+}
+
+pub async fn validate_role_name(
     state: &AppState,
-    role_name: &str,
+    role_name: &domain::RoleName,
     merchant_id: &str,
     org_id: &str,
 ) -> UserResult<()> {
-    let role_name_list: Vec<String> = state
+    let role_name_str = role_name.clone().get_role_name();
+
+    let is_present_in_predefined_roles = roles::predefined_roles::PREDEFINED_ROLES
+        .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
         .store
         .list_all_roles(merchant_id, org_id)
         .await
         .change_context(UserErrors::InternalServerError)?
         .iter()
-        .map(|role| role.role_name.to_owned())
+        .any(|role| role.role_name == role_name_str);
-        .collect();
 
-    if role_name_list.contains(&role_name.to_string()) {
+    if is_present_in_predefined_roles || is_present_in_custom_roles {
         return Err(UserErrors::RoleNameAlreadyExists.into());
     }
+
     Ok(())
 }