From c4c7b753060fdabe6c83a4e5a062f7354d5dbc95 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 15:12:32 -0700 Subject: [PATCH 1/5] feat: Update dataset group listing to include permissions - Enhanced the `list_dataset_groups` function to join with the `dataset_permissions` table, allowing retrieval of permission details for each dataset group. - Modified the `DatasetGroupInfo` struct to include `permission_id` and `assigned` fields, reflecting the new data structure. - Refactored the SQL query to group by necessary fields and ensure accurate permission data is returned, improving the functionality and security of dataset group listings. --- api/src/database/models.rs | 6 +++ .../users/assets/list_dataset_groups.rs | 45 +++++++++++++------ 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/api/src/database/models.rs b/api/src/database/models.rs index 109e5b25f..d51b7a373 100644 --- a/api/src/database/models.rs +++ b/api/src/database/models.rs @@ -6,6 +6,12 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use uuid::Uuid; +allow_columns_to_appear_in_same_group_by_clause!( + dataset_groups::id, + dataset_groups::name, + dataset_permissions::id, +); + #[derive(Queryable, Insertable, Identifiable, Associations, Debug)] #[diesel(belongs_to(User, foreign_key = owner_id))] #[diesel(table_name = api_keys)] 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 8df778f83..d4d84b9db 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 @@ -1,15 +1,14 @@ use anyhow::Result; 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::{DatasetGroup, User}; -use crate::database::schema::dataset_groups; +use crate::database::models::User; +use crate::database::schema::{dataset_groups, dataset_permissions}; use crate::routes::rest::ApiResponse; use crate::utils::user::user_info::get_user_organization_id; @@ -17,9 +16,8 @@ use crate::utils::user::user_info::get_user_organization_id; pub struct DatasetGroupInfo { pub id: Uuid, pub name: String, - pub organization_id: Uuid, - pub created_at: DateTime, - pub updated_at: DateTime, + pub permission_id: i32, + pub assigned: bool, } pub async fn list_dataset_groups( @@ -43,21 +41,40 @@ async fn list_dataset_groups_handler(user: User) -> Result let mut conn = get_pg_pool().get().await?; let organization_id = get_user_organization_id(&user.id).await?; - let groups: Vec = dataset_groups::table + let groups = dataset_groups::table + .left_join( + dataset_permissions::table.on(dataset_permissions::permission_id + .eq(dataset_groups::id) + .and(dataset_permissions::permission_type.eq("dataset_group")) + .and(dataset_permissions::deleted_at.is_null()) + .and(dataset_permissions::organization_id.eq(organization_id))), + ) + .select(( + dataset_groups::id, + dataset_groups::name, + diesel::dsl::sql::( + "COALESCE(count(dataset_permissions.id), 0)", + ), + diesel::dsl::sql::("dataset_permissions.id IS NOT NULL"), + )) + .group_by(( + dataset_groups::id, + dataset_groups::name, + dataset_permissions::id, + )) .filter(dataset_groups::organization_id.eq(organization_id)) .filter(dataset_groups::deleted_at.is_null()) .order_by(dataset_groups::created_at.desc()) - .load(&mut *conn) + .load::<(Uuid, String, i32, bool)>(&mut *conn) .await?; Ok(groups .into_iter() - .map(|group| DatasetGroupInfo { - id: group.id, - name: group.name, - organization_id: group.organization_id, - created_at: group.created_at, - updated_at: group.updated_at, + .map(|(id, name, permission_id, assigned)| DatasetGroupInfo { + id, + name, + permission_id, + assigned, }) .collect()) } From b9b5146299f1e5055e16a410a273b39b9c2b55ff Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 15:24:34 -0700 Subject: [PATCH 2/5] feat: Add DatasetGroupPermission model and schema - Introduced a new `DatasetGroupPermission` struct in `models.rs` to represent permissions associated with dataset groups. - Updated the database schema in `schema.rs` to include the `dataset_groups_permissions` table, defining its structure and relationships. - Modified the `is_user_workspace_admin_or_data_admin` function in `checks.rs` to correctly reference the user's organization role, enhancing role validation logic. --- .../down.sql | 6 ++++++ .../up.sql | 15 +++++++++++++++ api/src/database/models.rs | 12 ++++++++++++ api/src/database/schema.rs | 16 ++++++++++++++++ api/src/utils/security/checks.rs | 2 +- 5 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/down.sql create mode 100644 api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/up.sql diff --git a/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/down.sql b/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/down.sql new file mode 100644 index 000000000..b21bff40b --- /dev/null +++ b/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/down.sql @@ -0,0 +1,6 @@ +-- This file should undo anything in `up.sql` +DROP TRIGGER IF EXISTS update_dataset_groups_permissions_updated_at ON dataset_groups_permissions; +DROP INDEX IF EXISTS dataset_groups_permissions_organization_id_idx; +DROP INDEX IF EXISTS dataset_groups_permissions_permission_id_idx; +DROP INDEX IF EXISTS dataset_groups_permissions_dataset_group_id_idx; +DROP TABLE IF EXISTS dataset_groups_permissions; diff --git a/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/up.sql b/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/up.sql new file mode 100644 index 000000000..d2d538ea8 --- /dev/null +++ b/api/migrations/2025-01-20-221752_add_dataset_gropus_to_permission_groups_and_users/up.sql @@ -0,0 +1,15 @@ +-- Your SQL goes here +CREATE TABLE dataset_groups_permissions ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + dataset_group_id UUID NOT NULL REFERENCES dataset_groups(id), + permission_id UUID NOT NULL, + permission_type VARCHAR NOT NULL, + organization_id UUID NOT NULL REFERENCES organizations(id), + created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), + updated_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), + deleted_at TIMESTAMP WITH TIME ZONE +); + +CREATE INDEX dataset_groups_permissions_dataset_group_id_idx ON dataset_groups_permissions(dataset_group_id); +CREATE INDEX dataset_groups_permissions_permission_id_idx ON dataset_groups_permissions(permission_id); +CREATE INDEX dataset_groups_permissions_organization_id_idx ON dataset_groups_permissions(organization_id); \ No newline at end of file diff --git a/api/src/database/models.rs b/api/src/database/models.rs index d51b7a373..fc0b54d55 100644 --- a/api/src/database/models.rs +++ b/api/src/database/models.rs @@ -523,3 +523,15 @@ pub struct DatasetPermission { pub updated_at: DateTime, pub deleted_at: Option>, } + +#[derive(Queryable, Insertable, Debug)] +#[diesel(table_name = dataset_groups_permissions)] +pub struct DatasetGroupPermission { + pub id: Uuid, + pub dataset_group_id: Uuid, + pub permission_id: Uuid, + pub permission_type: String, + pub created_at: DateTime, + pub updated_at: DateTime, + pub deleted_at: Option>, +} diff --git a/api/src/database/schema.rs b/api/src/database/schema.rs index 1f9314a15..8dac857e8 100644 --- a/api/src/database/schema.rs +++ b/api/src/database/schema.rs @@ -201,6 +201,19 @@ diesel::table! { } } +diesel::table! { + dataset_groups_permissions (id) { + id -> Uuid, + dataset_group_id -> Uuid, + permission_id -> Uuid, + permission_type -> Varchar, + organization_id -> Uuid, + created_at -> Timestamptz, + updated_at -> Timestamptz, + deleted_at -> Nullable, + } +} + diesel::table! { dataset_permissions (id) { id -> Uuid, @@ -506,6 +519,8 @@ diesel::joinable!(dashboard_versions -> dashboards (dashboard_id)); diesel::joinable!(dashboards -> organizations (organization_id)); diesel::joinable!(data_sources -> organizations (organization_id)); diesel::joinable!(dataset_groups -> organizations (organization_id)); +diesel::joinable!(dataset_groups_permissions -> dataset_groups (dataset_group_id)); +diesel::joinable!(dataset_groups_permissions -> organizations (organization_id)); diesel::joinable!(dataset_permissions -> datasets (dataset_id)); diesel::joinable!(dataset_permissions -> organizations (organization_id)); diesel::joinable!(datasets -> data_sources (data_source_id)); @@ -544,6 +559,7 @@ diesel::allow_tables_to_appear_in_same_query!( data_sources, dataset_columns, dataset_groups, + dataset_groups_permissions, dataset_permissions, datasets, datasets_to_dataset_groups, diff --git a/api/src/utils/security/checks.rs b/api/src/utils/security/checks.rs index ea17ddea3..9fe639de0 100644 --- a/api/src/utils/security/checks.rs +++ b/api/src/utils/security/checks.rs @@ -25,7 +25,7 @@ pub async fn is_user_workspace_admin_or_data_admin( None => return Err(anyhow::anyhow!("User organization id not found")), }; - let user_role = match user.attributes.get("role") { + let user_role = match user.attributes.get("organization_role") { Some(Value::String(role)) => role, Some(_) => return Err(anyhow::anyhow!("User role not found")), None => return Err(anyhow::anyhow!("User role not found")), From df231a81d907df661fae2aba9ba59d3418843210 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 15:40:50 -0700 Subject: [PATCH 3/5] feat: Update dataset group listing to include dataset group permissions - Modified the `list_dataset_groups` function to accept an additional `id` parameter for filtering dataset groups based on user permissions. - Updated the SQL query to join with the `dataset_groups_permissions` table, allowing retrieval of permission counts for each dataset group. - Refactored the `DatasetGroupInfo` struct to replace `permission_id` with `permission_count`, enhancing clarity and accuracy in the data representation. - Ensured that the query groups by the new permission structure, improving the functionality and security of dataset group listings. --- api/src/database/models.rs | 1 + .../users/assets/list_dataset_groups.rs | 29 ++++++++++++------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/api/src/database/models.rs b/api/src/database/models.rs index fc0b54d55..dac2767e8 100644 --- a/api/src/database/models.rs +++ b/api/src/database/models.rs @@ -10,6 +10,7 @@ allow_columns_to_appear_in_same_group_by_clause!( dataset_groups::id, dataset_groups::name, dataset_permissions::id, + dataset_groups_permissions::id, ); #[derive(Queryable, Insertable, Identifiable, Associations, Debug)] 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 d4d84b9db..b395be8c2 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 @@ -1,4 +1,5 @@ use anyhow::Result; +use axum::extract::Path; use axum::http::StatusCode; use axum::Extension; use diesel::prelude::*; @@ -8,7 +9,7 @@ use uuid::Uuid; use crate::database::lib::get_pg_pool; use crate::database::models::User; -use crate::database::schema::{dataset_groups, dataset_permissions}; +use crate::database::schema::{dataset_groups, dataset_groups_permissions, dataset_permissions}; use crate::routes::rest::ApiResponse; use crate::utils::user::user_info::get_user_organization_id; @@ -16,14 +17,15 @@ use crate::utils::user::user_info::get_user_organization_id; pub struct DatasetGroupInfo { pub id: Uuid, pub name: String, - pub permission_id: i32, + pub permission_count: i64, pub assigned: bool, } pub async fn list_dataset_groups( Extension(user): Extension, + Path(id): Path, ) -> Result>, (StatusCode, &'static str)> { - let dataset_groups = match list_dataset_groups_handler(user).await { + let dataset_groups = match list_dataset_groups_handler(user, id).await { Ok(groups) => groups, Err(e) => { tracing::error!("Error listing dataset groups: {:?}", e); @@ -37,11 +39,18 @@ pub async fn list_dataset_groups( Ok(ApiResponse::JsonData(dataset_groups)) } -async fn list_dataset_groups_handler(user: User) -> Result> { +async fn list_dataset_groups_handler(user: User, id: Uuid) -> Result> { let mut conn = get_pg_pool().get().await?; let organization_id = get_user_organization_id(&user.id).await?; 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::deleted_at.is_null())), + ) .left_join( dataset_permissions::table.on(dataset_permissions::permission_id .eq(dataset_groups::id) @@ -52,28 +61,28 @@ async fn list_dataset_groups_handler(user: User) -> Result .select(( dataset_groups::id, dataset_groups::name, - diesel::dsl::sql::( + diesel::dsl::sql::( "COALESCE(count(dataset_permissions.id), 0)", ), - diesel::dsl::sql::("dataset_permissions.id IS NOT NULL"), + diesel::dsl::sql::("dataset_groups_permissions.id IS NOT NULL"), )) .group_by(( dataset_groups::id, dataset_groups::name, - dataset_permissions::id, + dataset_groups_permissions::id, )) .filter(dataset_groups::organization_id.eq(organization_id)) .filter(dataset_groups::deleted_at.is_null()) .order_by(dataset_groups::created_at.desc()) - .load::<(Uuid, String, i32, bool)>(&mut *conn) + .load::<(Uuid, String, i64, bool)>(&mut *conn) .await?; Ok(groups .into_iter() - .map(|(id, name, permission_id, assigned)| DatasetGroupInfo { + .map(|(id, name, permission_count, assigned)| DatasetGroupInfo { id, name, - permission_id, + permission_count, assigned, }) .collect()) From 94be56e042fd5b8984f3b557fea6c5574e6ef6f6 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 15:47:01 -0700 Subject: [PATCH 4/5] feat: Refactor dataset listing to include user-specific permissions - Updated the `list_datasets` function to accept an additional `id` parameter for filtering datasets based on user permissions. - Enhanced the SQL query to join with the `dataset_permissions` table, allowing retrieval of permission details for each dataset. - Refactored the `DatasetInfo` struct to include an `assigned` field, improving clarity in the dataset representation. - Improved error handling for dataset retrieval, ensuring robust logging and response management. --- .../rest/routes/users/assets/list_datasets.rs | 55 ++++++++++--------- 1 file changed, 30 insertions(+), 25 deletions(-) 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 35d0a2c12..e6a3136e5 100644 --- a/api/src/routes/rest/routes/users/assets/list_datasets.rs +++ b/api/src/routes/rest/routes/users/assets/list_datasets.rs @@ -1,15 +1,15 @@ 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::{Dataset, User}; -use crate::database::schema::datasets; +use crate::database::models::User; +use crate::database::schema::{dataset_permissions, datasets}; use crate::routes::rest::ApiResponse; use crate::utils::user::user_info::get_user_organization_id; @@ -17,18 +17,14 @@ use crate::utils::user::user_info::get_user_organization_id; pub struct DatasetInfo { pub id: Uuid, pub name: String, - pub organization_id: Uuid, - pub data_source_id: Uuid, - pub enabled: bool, - pub imported: bool, - pub created_at: DateTime, - pub updated_at: DateTime, + pub assigned: bool, } pub async fn list_datasets( Extension(user): Extension, + Path(id): Path, ) -> Result>, (StatusCode, &'static str)> { - let datasets = match list_datasets_handler(user).await { + let datasets = match list_datasets_handler(user, id).await { Ok(datasets) => datasets, Err(e) => { tracing::error!("Error listing datasets: {:?}", e); @@ -39,28 +35,37 @@ pub async fn list_datasets( Ok(ApiResponse::JsonData(datasets)) } -async fn list_datasets_handler(user: User) -> Result> { +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 datasets: Vec = datasets::table + let datasets = match datasets::table + .left_join( + dataset_permissions::table.on(dataset_permissions::dataset_id + .eq(datasets::id) + .and(dataset_permissions::permission_type.eq("user")) + .and(dataset_permissions::permission_id.eq(user_id)) + .and(dataset_permissions::deleted_at.is_null())), + ) .filter(datasets::organization_id.eq(organization_id)) .filter(datasets::deleted_at.is_null()) - .order_by(datasets::created_at.desc()) - .load(&mut *conn) - .await?; + .select(( + datasets::id, + datasets::name, + diesel::dsl::sql::("dataset_permissions.id IS NOT NULL"), + )) + .load::<(Uuid, String, bool)>(&mut *conn) + .await + { + Ok(datasets) => datasets, + Err(e) => { + tracing::error!("Error listing datasets: {:?}", e); + return Err(anyhow::anyhow!("Error listing datasets")); + } + }; Ok(datasets .into_iter() - .map(|dataset| DatasetInfo { - id: dataset.id, - name: dataset.name, - organization_id: dataset.organization_id, - data_source_id: dataset.data_source_id, - enabled: dataset.enabled, - imported: dataset.imported, - created_at: dataset.created_at, - updated_at: dataset.updated_at, - }) + .map(|(id, name, assigned)| DatasetInfo { id, name, assigned }) .collect()) } From 8ae08b00e6f0f7fd3127cada8731f7cab4656be0 Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 20 Jan 2025 15:57:31 -0700 Subject: [PATCH 5/5] fix: Correct user role attribute and enhance read-only logic in list_attributes_handler - Updated the user role attribute key from "role" to "organization_role" for accurate role retrieval. - Introduced a read-only flag for specific user attributes, improving data integrity by clearly indicating which attributes should not be modified. - Enhanced error handling for user role retrieval, ensuring robust responses for missing or incorrect attributes. --- .../rest/routes/users/assets/list_attributes.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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 92508692e..476a2321d 100644 --- a/api/src/routes/rest/routes/users/assets/list_attributes.rs +++ b/api/src/routes/rest/routes/users/assets/list_attributes.rs @@ -56,7 +56,7 @@ async fn list_attributes_handler(user: User, user_id: Uuid) -> Result return Err(anyhow::anyhow!("User organization id not found")), }; - let auth_user_role = match user.attributes.get("role") { + let auth_user_role = match user.attributes.get("organization_role") { Some(Value::String(role)) => role, Some(_) => return Err(anyhow::anyhow!("User role not found")), None => return Err(anyhow::anyhow!("User role not found")), @@ -86,10 +86,17 @@ async fn list_attributes_handler(user: User, user_id: Uuid) -> Result