starred/openobserve
1
0
Fork 0
mirror of https://github.com/openobserve/openobserve.git synced 2026-11-05 08:09:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: alert move rbac issues (#6352)

Browse Source 
- 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
This commit is contained in:
Subhra264
2025-03-21 00:19:00 +05:30
committed by GitHub
parent 4ad2865ddb
commit 6926f67e08
9 changed files with 192 additions and 29 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/common/utils/auth.rs
+13 -4
View File
@@ -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,
src/handler/http/request/actions/action.rs
+1
View File
@@ -539,6 +539,7 @@ pub async fn upload_zipped_action(
&user_email.user_id,
"actions",
method,
"",
)
.await
{
src/handler/http/request/actions/operations.rs
+1
View File
@@ -62,6 +62,7 @@ pub async fn test_action(
&user_email.user_id,
"actions",
"POST",
"",
)
.await
{
src/handler/http/request/alerts/mod.rs
+4
View File
@@ -75,6 +75,8 @@ impl From<AlertError> 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<String>,
req_body: web::Json<MoveAlertsRequestBody>,
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
{
src/handler/http/request/service_accounts/mod.rs
+5 -3
View File
@@ -52,16 +52,17 @@ use crate::{
)
)]
#[get("/{org_id}/service_accounts")]
pub async fn list(org_id: web::Path<String>, _req: HttpRequest) -> Result<HttpResponse, Error> {
pub async fn list(org_id: web::Path<String>, req: HttpRequest) -> Result<HttpResponse, Error> {
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<String>, _req: HttpRequest) -> Result<HttpRe
// Get List of allowed objects ends
}
users::list_users(
user_id,
&org_id,
Some(UserRole::ServiceAccount),
_user_list_from_rbac,
src/handler/http/request/users/mod.rs
+21 -2
View File
@@ -29,9 +29,11 @@ use serde::Serialize;
use strum::IntoEnumIterator;
#[cfg(feature = "enterprise")]
use {
crate::common::utils::auth::check_permissions,
crate::service::self_reporting::audit,
o2_dex::config::get_config as get_dex_config,
o2_enterprise::enterprise::common::auditor::{AuditMessage, HttpMeta, Protocol},
o2_openfga::config::get_config as get_openfga_config,
};
use crate::{
@@ -66,9 +68,26 @@ pub mod service_accounts;
)
)]
#[get("/{org_id}/users")]
pub async fn list(org_id: web::Path<String>) -> Result<HttpResponse, Error> {
pub async fn list(org_id: web::Path<String>, user_email: UserEmail) -> Result<HttpResponse, Error> {
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
src/service/alerts/alert.rs
+97 -11
View File
@@ -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<C: ConnectionTrait + TransactionTrait>(
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<C: ConnectionTrait>(
user_id: Option<&str>,
params: ListAlertsParams,
) -> Result<Vec<(Folder, Alert)>, AlertError> {
let (permissions, is_all_permitted) = match permitted_alerts(&params.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(&params.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<Option<Vec<String>>, 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<Option<Vec<String>>, 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)]
src/service/folders.rs
+28 -8
View File
@@ -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<Vec<Folder>, 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::<Vec<_>>()
}
@@ -270,6 +280,7 @@ pub async fn delete_folder(
async fn permitted_folders(
_org_id: &str,
_user_id: Option<&str>,
_folder_type: FolderType,
) -> Result<Option<Vec<String>>, 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<Option<Vec<String>>, 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() {
src/service/users.rs
+22 -1
View File
@@ -556,10 +556,31 @@ pub async fn get_user_by_token(org_id: &str, token: &str) -> Option<User> {
}
pub async fn list_users(
_user_id: &str,
org_id: &str,
role: Option<UserRole>,
permitted: Option<Vec<String>>,
) -> Result<HttpResponse, Error> {
// 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<UserResponse> = 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]