From 6926f67e080ea6cc944a170ee0538f391abeba7f Mon Sep 17 00:00:00 2001 From: Subhra264 Date: Fri, 21 Mar 2025 00:19:00 +0530 Subject: [PATCH] fix: alert move rbac issues (#6352) - Fixes 403 error when alert move api is called - 403 error when trying to create alert in default folder even after having permission - Fixes # Regular user without `GET` user permission should not be able to get list of all users --- src/common/utils/auth.rs | 17 ++- src/handler/http/request/actions/action.rs | 1 + .../http/request/actions/operations.rs | 1 + src/handler/http/request/alerts/mod.rs | 4 + .../http/request/service_accounts/mod.rs | 8 +- src/handler/http/request/users/mod.rs | 23 +++- src/service/alerts/alert.rs | 108 ++++++++++++++++-- src/service/folders.rs | 36 ++++-- src/service/users.rs | 23 +++- 9 files changed, 192 insertions(+), 29 deletions(-) diff --git a/src/common/utils/auth.rs b/src/common/utils/auth.rs index 618cdf23c3..5930df9ce1 100644 --- a/src/common/utils/auth.rs +++ b/src/common/utils/auth.rs @@ -403,9 +403,6 @@ impl FromRequest for AuthExtractor { // Handle /v2 alert apis if path_columns[0].eq(V2_API_PREFIX) { if path_columns[2].eq("alerts") { - if method.eq("PATCH") { - method = "PUT".to_string(); - } format!( "{}:{}", OFGA_MODELS @@ -656,6 +653,17 @@ impl FromRequest for AuthExtractor { })); } + if method.eq("PATCH") && object_type.eq("alert:move") { + return ready(Ok(AuthExtractor { + auth: auth_str.to_owned(), + method: "".to_string(), + o2_type: "".to_string(), + org_id: "".to_string(), + bypass_check: true, // bypass check permissions + parent_id: folder, + })); + } + return ready(Ok(AuthExtractor { auth: auth_str.to_owned(), method, @@ -807,6 +815,7 @@ pub async fn check_permissions( user_id: &str, object_type: &str, method: &str, + parent_id: &str, ) -> bool { if !is_root_user(user_id) { let user: meta::user::User = match USERS.get(&format!("{org_id}/{}", user_id)) { @@ -834,7 +843,7 @@ pub async fn check_permissions( ), org_id: org_id.to_string(), bypass_check: false, - parent_id: "".to_string(), + parent_id: parent_id.to_string(), }, user.role, user.is_external, diff --git a/src/handler/http/request/actions/action.rs b/src/handler/http/request/actions/action.rs index 6081ed737a..cce028f652 100644 --- a/src/handler/http/request/actions/action.rs +++ b/src/handler/http/request/actions/action.rs @@ -539,6 +539,7 @@ pub async fn upload_zipped_action( &user_email.user_id, "actions", method, + "", ) .await { diff --git a/src/handler/http/request/actions/operations.rs b/src/handler/http/request/actions/operations.rs index 31bb20f32c..181253a235 100644 --- a/src/handler/http/request/actions/operations.rs +++ b/src/handler/http/request/actions/operations.rs @@ -62,6 +62,7 @@ pub async fn test_action( &user_email.user_id, "actions", "POST", + "", ) .await { diff --git a/src/handler/http/request/alerts/mod.rs b/src/handler/http/request/alerts/mod.rs index c669accff8..497dec5ebb 100644 --- a/src/handler/http/request/alerts/mod.rs +++ b/src/handler/http/request/alerts/mod.rs @@ -75,6 +75,8 @@ impl From for HttpResponse { AlertError::PermittedAlertsMissingUser => MetaHttpResponse::forbidden(""), AlertError::PermittedAlertsValidator(err) => MetaHttpResponse::forbidden(err), AlertError::NotSupportedAlertDestinationType(err) => MetaHttpResponse::forbidden(err), + AlertError::PermissionDenied => MetaHttpResponse::forbidden("Unauthorized access"), + AlertError::UserNotFound => MetaHttpResponse::forbidden("Unauthorized access"), } } } @@ -369,6 +371,7 @@ async fn trigger_alert(path: web::Path<(String, Ksuid)>) -> HttpResponse { async fn move_alerts( path: web::Path, req_body: web::Json, + user_email: UserEmail, ) -> HttpResponse { let org_id = path.into_inner(); let client = ORM_CLIENT.get_or_init(connect_to_orm).await; @@ -377,6 +380,7 @@ async fn move_alerts( &org_id, &req_body.alert_ids, &req_body.dst_folder_id, + &user_email.user_id, ) .await { diff --git a/src/handler/http/request/service_accounts/mod.rs b/src/handler/http/request/service_accounts/mod.rs index 6632f29c03..02d027227a 100644 --- a/src/handler/http/request/service_accounts/mod.rs +++ b/src/handler/http/request/service_accounts/mod.rs @@ -52,16 +52,17 @@ use crate::{ ) )] #[get("/{org_id}/service_accounts")] -pub async fn list(org_id: web::Path, _req: HttpRequest) -> Result { +pub async fn list(org_id: web::Path, req: HttpRequest) -> Result { let org_id = org_id.into_inner(); + let user_id = req.headers().get("user_id").unwrap(); + let user_id = user_id.to_str().unwrap(); let mut _user_list_from_rbac = None; // Get List of allowed objects #[cfg(feature = "enterprise")] { - let user_id = _req.headers().get("user_id").unwrap(); match crate::handler::http::auth::validator::list_objects_for_user( &org_id, - user_id.to_str().unwrap(), + user_id, "GET", "service_accounts", ) @@ -79,6 +80,7 @@ pub async fn list(org_id: web::Path, _req: HttpRequest) -> Result) -> Result { +pub async fn list(org_id: web::Path, user_email: UserEmail) -> Result { let org_id = org_id.into_inner(); - users::list_users(&org_id, None, None).await + let mut _user_list_from_rbac = None; + + #[cfg(feature = "enterprise")] + // Check if user has access to get users + if get_openfga_config().enabled + && check_permissions( + Some(format!("_all_{}", org_id)), + &org_id, + &user_email.user_id, + "users", + "GET", + "", + ) + .await + { + _user_list_from_rbac = Some(vec![]); + } + users::list_users(&user_email.user_id, &org_id, None, _user_list_from_rbac).await } /// CreateUser diff --git a/src/service/alerts/alert.rs b/src/service/alerts/alert.rs index ad39c69705..45a7ab9730 100644 --- a/src/service/alerts/alert.rs +++ b/src/service/alerts/alert.rs @@ -52,6 +52,8 @@ use o2_openfga::{ use sea_orm::{ConnectionTrait, TransactionTrait}; use svix_ksuid::Ksuid; +#[cfg(feature = "enterprise")] +use crate::common::utils::auth::check_permissions; use crate::{ common::{ meta::authz::Authz, @@ -155,6 +157,12 @@ pub enum AlertError { #[error("PermittedAlertsValidator# {0}")] PermittedAlertsValidator(String), + #[error("Permission denied")] + PermissionDenied, + + #[error("User not found")] + UserNotFound, + /// Not support save destination remote pipeline for alert so far #[error("Not support save destination {0} type for alert so far")] NotSupportedAlertDestinationType(Module), @@ -462,15 +470,34 @@ pub async fn move_to_folder( org_id: &str, alert_ids: &[Ksuid], dst_folder_id: &str, + _user_id: &str, ) -> Result<(), AlertError> { for alert_id in alert_ids { let _alert_id_str = alert_id.to_string(); + let Some((curr_folder, alert)) = db::alerts::alert::get_by_id(conn, org_id, *alert_id).await? else { return Err(AlertError::AlertNotFound); }; + #[cfg(feature = "enterprise")] + if get_openfga_config().enabled { + // TODO: Try to make a single call for all alerts + if !check_permissions( + Some(_alert_id_str.clone()), + org_id, + _user_id, + "alerts", + "PUT", + &curr_folder.folder_id, + ) + .await + { + return Err(AlertError::PermissionDenied); + } + } + update( conn, org_id, @@ -619,22 +646,24 @@ pub async fn list_v2( user_id: Option<&str>, params: ListAlertsParams, ) -> Result, AlertError> { - let (permissions, is_all_permitted) = match permitted_alerts(¶ms.org_id, user_id).await? { - Some(ps) => { - let org_all_permitted = ps.contains(&format!("alert:_all_{}", params.org_id)); - (ps, org_all_permitted) - } - None => (vec![], true), - }; + let (permissions, is_all_permitted) = + match permitted_alerts(¶ms.org_id, user_id, params.folder_id.as_deref()).await? { + Some(ps) => { + let org_all_permitted = ps.contains(&format!("alert:_all_{}", params.org_id)); + (ps, org_all_permitted) + } + None => (vec![], true), + }; let alerts = db::alerts::alert::list_with_folders(conn, params) .await? .into_iter() - .filter(|(_f, a)| { + .filter(|(f, a)| { // Include the alert if all alerts are permitted. is_all_permitted // Include the alert if the alert is permitted with the old OpenFGA identifier. || permissions.contains(&format!("alert:{}", a.name)) + || permissions.contains(&format!("alert:{}/{}", f.folder_id, a.id.as_ref().unwrap())) // Include the alert if the alert is permitted with the new OpenFGA identifier. || a.id .filter(|id| permissions.contains(&format!("alert:{id}"))) @@ -1549,6 +1578,7 @@ impl VarValue<'_> { async fn permitted_alerts( _org_id: &str, _user_id: Option<&str>, + _folder_id: Option<&str>, ) -> Result>, AlertError> { Ok(None) } @@ -1557,16 +1587,72 @@ async fn permitted_alerts( async fn permitted_alerts( org_id: &str, user_id: Option<&str>, + folder_id: Option<&str>, ) -> Result>, AlertError> { let Some(user_id) = user_id else { return Err(AlertError::PermittedAlertsMissingUser); }; - let stream_list = crate::handler::http::auth::validator::list_objects_for_user( - org_id, user_id, "GET", "alert", + + // If the list_only_permitted is true, then we will only return the alerts that the user has + // `GET` permission on. + if !get_openfga_config().list_only_permitted { + return Ok(None); + } + + // This function assumes the user already has `LIST` permission on the folder. + // Otherwise, the user will not be able to see the folder in the first place. + + // So, we check for the `GET` permission on the folder. + // If the user has `GET` permission on the folder, then they will be able to see the folder and + // all its contents. This includes the dashboards inside the folder. + + use o2_openfga::meta::mapping::OFGA_MODELS; + + use crate::{common::utils::auth::AuthExtractor, service::db::user::get as get_user}; + + if let Some(folder_id) = folder_id { + let user_role = match get_user(Some(org_id), user_id).await { + Ok(Some(user)) => user.role, + _ => return Err(AlertError::UserNotFound), + }; + let permitted = crate::handler::http::auth::validator::check_permissions( + user_id, + AuthExtractor { + org_id: org_id.to_string(), + o2_type: format!( + "{}:{folder_id}", + OFGA_MODELS.get("alert_folders").unwrap().key, + ), + method: "GET".to_string(), + bypass_check: false, + parent_id: "".to_string(), + auth: "".to_string(), // We don't need to pass the auth token here. + }, + user_role, + false, + ) + .await; + if permitted { + // The user has `GET` permission on the folder. + // So, they will be able to see all the dashboards inside the folder. + return Ok(None); + } + } + + // We also check for the `GET_INDIVIDUAL` permission on the dashboards. + // If the user has `GET_INDIVIDUAL` permission on a dashboard, then they will be able to see the + // dashboard. This is used to check if the user has permission to see a specific dashboard. + + let permitted_objects = crate::handler::http::auth::validator::list_objects_for_user( + org_id, + user_id, + "GET_INDIVIDUAL", + OFGA_MODELS.get("alerts").unwrap().key, ) .await .map_err(|err| AlertError::PermittedAlertsValidator(err.to_string()))?; - Ok(stream_list) + + Ok(permitted_objects) } #[cfg(test)] diff --git a/src/service/folders.rs b/src/service/folders.rs index 9d3c1f586a..d5deb9abcb 100644 --- a/src/service/folders.rs +++ b/src/service/folders.rs @@ -25,6 +25,8 @@ use infra::{ db::{ORM_CLIENT, connect_to_orm}, table, }; +#[cfg(feature = "enterprise")] +use o2_openfga::meta::mapping::OFGA_MODELS; use crate::common::{ meta::authz::Authz, @@ -172,18 +174,26 @@ pub async fn list_folders( user_id: Option<&str>, folder_type: FolderType, ) -> Result, FolderError> { - let permitted_folders = permitted_folders(org_id, user_id).await?; + let permitted_folders = permitted_folders(org_id, user_id, folder_type).await?; let folders = table::folders::list_folders(org_id, folder_type).await?; + #[cfg(feature = "enterprise")] + let folder_ofga_model = match folder_type { + FolderType::Dashboards => OFGA_MODELS.get("folders").unwrap().key, + FolderType::Alerts => OFGA_MODELS.get("alert_folders").unwrap().key, + }; + #[cfg(not(feature = "enterprise"))] + let folder_ofga_model = ""; + let filtered = match permitted_folders { Some(permitted_folders) => { - if permitted_folders.contains(&format!("{}:_all_{}", "dfolder", org_id)) { + if permitted_folders.contains(&format!("{}:_all_{}", folder_ofga_model, org_id)) { folders } else { folders .into_iter() .filter(|folder_loc| { permitted_folders - .contains(&format!("{}:{}", "dfolder", folder_loc.folder_id)) + .contains(&format!("{}:{}", folder_ofga_model, folder_loc.folder_id)) }) .collect::>() } @@ -270,6 +280,7 @@ pub async fn delete_folder( async fn permitted_folders( _org_id: &str, _user_id: Option<&str>, + _folder_type: FolderType, ) -> Result>, FolderError> { Ok(None) } @@ -278,9 +289,18 @@ async fn permitted_folders( async fn permitted_folders( org_id: &str, user_id: Option<&str>, + folder_type: FolderType, ) -> Result>, FolderError> { - use o2_openfga::meta::mapping::OFGA_MODELS; - let folder_ofga_model = OFGA_MODELS.get("folders").unwrap().key; + let (folder_ofga_model, child_ofga_model) = match folder_type { + FolderType::Dashboards => ( + OFGA_MODELS.get("folders").unwrap().key, + OFGA_MODELS.get("dashboards").unwrap().key, + ), + FolderType::Alerts => ( + OFGA_MODELS.get("alert_folders").unwrap().key, + OFGA_MODELS.get("alerts").unwrap().key, + ), + }; let Some(user_id) = user_id else { return Err(FolderError::PermittedFoldersMissingUser); @@ -291,7 +311,7 @@ async fn permitted_folders( org_id, user_id, "GET", - OFGA_MODELS.get("folders").unwrap().key, + folder_ofga_model, ) .await .map_err(|err| FolderError::PermittedFoldersValidator(err.to_string()))?; @@ -304,12 +324,12 @@ async fn permitted_folders( org_id, user_id, "GET_INDIVIDUAL_FROM_ROLE", - OFGA_MODELS.get("dashboards").unwrap().key, + child_ofga_model, ) .await .map_err(|err| FolderError::PermittedFoldersValidator(err.to_string()))?; - log::info!("permitted_dashboards: {:?}", permitted_dashboards); + log::debug!("permitted_dashboards: {:?}", permitted_dashboards); if permitted_dashboards.is_some() { let mut folder_list_with_roles = vec![]; for dashboard in permitted_dashboards.unwrap() { diff --git a/src/service/users.rs b/src/service/users.rs index 3c354a0b21..76751ef4eb 100644 --- a/src/service/users.rs +++ b/src/service/users.rs @@ -556,10 +556,31 @@ pub async fn get_user_by_token(org_id: &str, token: &str) -> Option { } pub async fn list_users( + _user_id: &str, org_id: &str, role: Option, permitted: Option>, ) -> Result { + // If the role is not + #[cfg(feature = "enterprise")] + if get_openfga_config().enabled { + let need_check_permission = !matches!(role, Some(UserRole::ServiceAccount)); + + if need_check_permission && permitted.is_none() { + let mut user_list = vec![]; + if let Some(user) = USERS.get(&format!("{org_id}/{_user_id}")) { + user_list.push(UserResponse { + email: user.value().email.clone(), + role: user.value().role.clone(), + first_name: user.value().first_name.clone(), + last_name: user.value().last_name.clone(), + is_external: user.value().is_external, + }); + } + return Ok(HttpResponse::Ok().json(UserList { data: user_list })); + } + } + let mut user_list: Vec = USERS .iter() .filter(|user| user.key().starts_with(&format!("{org_id}/"))) // Filter by organization ID @@ -827,7 +848,7 @@ mod tests { #[tokio::test] async fn test_list_users() { set_up().await; - assert!(list_users("dummy", None, None).await.is_ok()) + assert!(list_users("", "dummy", None, None).await.is_ok()) } #[tokio::test]