From 31473f8d5533f6637b213ccee2a755e62faac496 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 16:29:46 -0700 Subject: [PATCH] feat: Enhance user authorization checks and refactor related functions - Added user authorization checks in `list_attributes`, `list_dataset_groups`, `list_datasets`, `list_permission_groups`, and `list_teams` functions to ensure only users with appropriate roles can access these resources. - Refactored the `list_teams_handler` to accept `user_id` as a parameter, improving clarity and consistency across user-related functions. - Updated SQL queries to utilize the new authorization checks, enhancing security and data integrity. - Removed redundant column allowances in `list_teams` permissions, streamlining the codebase. --- api/src/database/models.rs | 5 ++ .../routes/users/assets/list_attributes.rs | 18 +++--- .../users/assets/list_dataset_groups.rs | 11 +++- .../rest/routes/users/assets/list_datasets.rs | 7 ++- .../users/assets/list_permission_groups.rs | 5 ++ .../rest/routes/users/assets/list_teams.rs | 61 +++++++++++++------ api/src/routes/ws/permissions/list_teams.rs | 8 --- 7 files changed, 75 insertions(+), 40 deletions(-) diff --git a/api/src/database/models.rs b/api/src/database/models.rs index dac2767e8..09f216462 100644 --- a/api/src/database/models.rs +++ b/api/src/database/models.rs @@ -11,6 +11,11 @@ allow_columns_to_appear_in_same_group_by_clause!( dataset_groups::name, dataset_permissions::id, dataset_groups_permissions::id, + teams::id, + teams::name, + permission_groups_to_identities::permission_group_id, + teams_to_users::user_id, + teams_to_users::role ); #[derive(Queryable, Insertable, Identifiable, Associations, Debug)] diff --git a/api/src/routes/rest/routes/users/assets/list_attributes.rs b/api/src/routes/rest/routes/users/assets/list_attributes.rs index 476a2321d..b61e67fcf 100644 --- a/api/src/routes/rest/routes/users/assets/list_attributes.rs +++ b/api/src/routes/rest/routes/users/assets/list_attributes.rs @@ -12,6 +12,7 @@ use crate::database::lib::get_pg_pool; use crate::database::models::User; use crate::database::schema::{users, users_to_organizations}; use crate::routes::rest::ApiResponse; +use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; #[derive(Debug, Serialize)] @@ -42,14 +43,13 @@ pub async fn list_attributes( async fn list_attributes_handler(user: User, user_id: Uuid) -> Result> { let mut conn = get_pg_pool().get().await?; - let user_orgnazation_id = match get_user_organization_id(&user_id).await { - Ok(id) => id, - Err(e) => { - tracing::error!("Error getting user organization id: {:?}", e); - return Err(anyhow::anyhow!("Error getting user organization id")); - } - }; + let organization_id = get_user_organization_id(&user_id).await?; + if !is_user_workspace_admin_or_data_admin(&user, &organization_id).await? { + return Err(anyhow::anyhow!( + "User is not authorized to list dataset groups" + )); + } let auth_user_orgnazation_id = match user.attributes.get("organization_id") { Some(Value::String(id)) => Uuid::parse_str(id).unwrap(), Some(_) => return Err(anyhow::anyhow!("User organization id not found")), @@ -66,14 +66,14 @@ async fn list_attributes_handler(user: User, user_id: Uuid) -> Result(&mut *conn) .await diff --git a/api/src/routes/rest/routes/users/assets/list_dataset_groups.rs b/api/src/routes/rest/routes/users/assets/list_dataset_groups.rs index b395be8c2..efde7e465 100644 --- a/api/src/routes/rest/routes/users/assets/list_dataset_groups.rs +++ b/api/src/routes/rest/routes/users/assets/list_dataset_groups.rs @@ -11,6 +11,7 @@ use crate::database::lib::get_pg_pool; use crate::database::models::User; use crate::database::schema::{dataset_groups, dataset_groups_permissions, dataset_permissions}; use crate::routes::rest::ApiResponse; +use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; #[derive(Debug, Serialize)] @@ -39,16 +40,20 @@ pub async fn list_dataset_groups( Ok(ApiResponse::JsonData(dataset_groups)) } -async fn list_dataset_groups_handler(user: User, id: Uuid) -> Result> { +async fn list_dataset_groups_handler(user: User, user_id: Uuid) -> Result> { let mut conn = get_pg_pool().get().await?; - let organization_id = get_user_organization_id(&user.id).await?; + let organization_id = get_user_organization_id(&user_id).await?; + + if !is_user_workspace_admin_or_data_admin(&user, &organization_id).await? { + return Err(anyhow::anyhow!("User is not authorized to list dataset groups")); + } let groups = dataset_groups::table .left_join( dataset_groups_permissions::table.on(dataset_groups_permissions::dataset_group_id .eq(dataset_groups::id) .and(dataset_groups_permissions::permission_type.eq("user")) - .and(dataset_groups_permissions::permission_id.eq(id)) + .and(dataset_groups_permissions::permission_id.eq(user_id)) .and(dataset_groups_permissions::deleted_at.is_null())), ) .left_join( diff --git a/api/src/routes/rest/routes/users/assets/list_datasets.rs b/api/src/routes/rest/routes/users/assets/list_datasets.rs index e6a3136e5..d5449be34 100644 --- a/api/src/routes/rest/routes/users/assets/list_datasets.rs +++ b/api/src/routes/rest/routes/users/assets/list_datasets.rs @@ -11,6 +11,7 @@ use crate::database::lib::get_pg_pool; use crate::database::models::User; use crate::database::schema::{dataset_permissions, datasets}; use crate::routes::rest::ApiResponse; +use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; #[derive(Debug, Serialize)] @@ -37,7 +38,11 @@ pub async fn list_datasets( async fn list_datasets_handler(user: User, user_id: Uuid) -> Result> { let mut conn = get_pg_pool().get().await?; - let organization_id = get_user_organization_id(&user.id).await?; + let organization_id = get_user_organization_id(&user_id).await?; + + if !is_user_workspace_admin_or_data_admin(&user, &organization_id).await? { + return Err(anyhow::anyhow!("User is not authorized to list datasets")); + } let datasets = match datasets::table .left_join( diff --git a/api/src/routes/rest/routes/users/assets/list_permission_groups.rs b/api/src/routes/rest/routes/users/assets/list_permission_groups.rs index b7022b522..cd3415072 100644 --- a/api/src/routes/rest/routes/users/assets/list_permission_groups.rs +++ b/api/src/routes/rest/routes/users/assets/list_permission_groups.rs @@ -11,6 +11,7 @@ use crate::database::lib::get_pg_pool; use crate::database::models::{PermissionGroup, User}; use crate::database::schema::permission_groups; use crate::routes::rest::ApiResponse; +use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; #[derive(Debug, Serialize)] @@ -43,6 +44,10 @@ async fn list_permission_groups_handler(user: User) -> Result = permission_groups::table .filter(permission_groups::organization_id.eq(organization_id)) .filter(permission_groups::deleted_at.is_null()) diff --git a/api/src/routes/rest/routes/users/assets/list_teams.rs b/api/src/routes/rest/routes/users/assets/list_teams.rs index 5ff590a99..eff7beed2 100644 --- a/api/src/routes/rest/routes/users/assets/list_teams.rs +++ b/api/src/routes/rest/routes/users/assets/list_teams.rs @@ -1,31 +1,32 @@ use anyhow::Result; +use axum::extract::Path; use axum::http::StatusCode; use axum::Extension; -use chrono::{DateTime, Utc}; use diesel::prelude::*; use diesel_async::RunQueryDsl; use serde::Serialize; use uuid::Uuid; use crate::database::lib::get_pg_pool; -use crate::database::models::{Team, User}; -use crate::database::schema::teams; +use crate::database::models::User; +use crate::database::schema::{teams, teams_to_users}; use crate::routes::rest::ApiResponse; +use crate::utils::security::checks::is_user_workspace_admin_or_data_admin; use crate::utils::user::user_info::get_user_organization_id; #[derive(Debug, Serialize)] pub struct TeamInfo { pub id: Uuid, pub name: String, - pub organization_id: Uuid, - pub created_at: DateTime, - pub updated_at: DateTime, + pub user_count: i64, + pub assigned: bool, } pub async fn list_teams( Extension(user): Extension, + Path(id): Path, ) -> Result>, (StatusCode, &'static str)> { - let teams = match list_teams_handler(user).await { + let teams = match list_teams_handler(user, id).await { Ok(teams) => teams, Err(e) => { tracing::error!("Error listing teams: {:?}", e); @@ -36,26 +37,48 @@ pub async fn list_teams( Ok(ApiResponse::JsonData(teams)) } -async fn list_teams_handler(user: User) -> Result> { +async fn list_teams_handler(user: User, user_id: Uuid) -> Result> { let mut conn = get_pg_pool().get().await?; - let organization_id = get_user_organization_id(&user.id).await?; + let organization_id = get_user_organization_id(&user_id).await?; - let teams: Vec = teams::table + if !is_user_workspace_admin_or_data_admin(&user, &organization_id).await? { + return Err(anyhow::anyhow!("User is not authorized to list teams")); + } + + let teams = match teams::table + .left_join( + teams_to_users::table.on(teams::id + .eq(teams_to_users::team_id) + .and(teams_to_users::deleted_at.is_null()) + .and(teams_to_users::user_id.eq(user_id))), + ) + .select(( + teams::id, + teams::name, + diesel::dsl::sql::("COALESCE(count(teams_to_users.user_id), 0)"), + diesel::dsl::sql::("teams_to_users.user_id IS NOT NULL"), + )) + .group_by((teams::id, teams::name, teams_to_users::user_id)) .filter(teams::organization_id.eq(organization_id)) .filter(teams::deleted_at.is_null()) .order_by(teams::created_at.desc()) - .load(&mut *conn) - .await?; - + .load::<(Uuid, String, i64, bool)>(&mut *conn) + .await + { + Ok(teams) => teams, + Err(e) => { + tracing::error!("Error listing teams: {:?}", e); + return Err(anyhow::anyhow!("Error listing teams")); + } + }; Ok(teams .into_iter() - .map(|team| TeamInfo { - id: team.id, - name: team.name, - organization_id: team.organization_id, - created_at: team.created_at, - updated_at: team.updated_at, + .map(|(id, name, count, assigned)| TeamInfo { + id, + name, + user_count: count, + assigned, }) .collect()) } diff --git a/api/src/routes/ws/permissions/list_teams.rs b/api/src/routes/ws/permissions/list_teams.rs index a618ad6a2..1b868a053 100644 --- a/api/src/routes/ws/permissions/list_teams.rs +++ b/api/src/routes/ws/permissions/list_teams.rs @@ -51,14 +51,6 @@ pub struct TeamPermissionInfo { pub team_role: Option, } -allow_columns_to_appear_in_same_group_by_clause!( - teams::id, - teams::name, - permission_groups_to_identities::permission_group_id, - teams_to_users::user_id, - teams_to_users::role, -); - pub async fn list_teams(user: &User, req: ListTeamPermissionsRequest) -> Result<()> { let page = req.page.unwrap_or(0); let page_size = req.page_size.unwrap_or(25);