diff --git a/api/libs/database/tests/helpers/dashboard_files_test.rs b/api/libs/database/tests/helpers/dashboard_files_test.rs index d91f3ac6d..f73f03ff1 100644 --- a/api/libs/database/tests/helpers/dashboard_files_test.rs +++ b/api/libs/database/tests/helpers/dashboard_files_test.rs @@ -10,14 +10,13 @@ use diesel_async::RunQueryDsl; use tokio; use uuid::Uuid; -use super::test_utils::{cleanup_test_data, insert_test_dashboard_file, insert_test_permission, TestDb}; +use crate::helpers::test_utils::{TestDb, insert_test_dashboard_file, insert_test_permission, cleanup_test_data}; /// Tests the fetch_dashboard_file_with_permission function with direct permission #[tokio::test] async fn test_dashboard_file_direct_permission() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -26,7 +25,7 @@ async fn test_dashboard_file_direct_permission() -> Result<()> { let dashboard_id = dashboard_file.id; // Insert the test dashboard file - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Test cases with different permission roles for role in [ @@ -40,7 +39,7 @@ async fn test_dashboard_file_direct_permission() -> Result<()> { let permission = test_db .create_asset_permission(&dashboard_id, AssetType::DashboardFile, &owner_id, role) .await?; - insert_test_permission(&mut conn, &permission).await?; + insert_test_permission(&permission).await?; // Fetch file with permissions let result = fetch_dashboard_file_with_permission(&dashboard_id, &owner_id).await?; @@ -50,16 +49,10 @@ async fn test_dashboard_file_direct_permission() -> Result<()> { let file_with_permission = result.unwrap(); assert_eq!(file_with_permission.dashboard_file.id, dashboard_id); assert_eq!(file_with_permission.permission, Some(role)); - - // Clean up permission for next test - diesel::delete(database::schema::asset_permissions::table) - .filter(database::schema::asset_permissions::asset_id.eq(dashboard_id)) - .execute(&mut conn) - .await?; } // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; Ok(()) } @@ -69,7 +62,6 @@ async fn test_dashboard_file_direct_permission() -> Result<()> { async fn test_dashboard_file_no_permission() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -78,7 +70,7 @@ async fn test_dashboard_file_no_permission() -> Result<()> { let dashboard_id = dashboard_file.id; // Insert the test dashboard file - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Fetch file with permissions (no permission exists) let result = fetch_dashboard_file_with_permission(&dashboard_id, &Uuid::new_v4()).await?; @@ -90,7 +82,7 @@ async fn test_dashboard_file_no_permission() -> Result<()> { assert_eq!(file_with_permission.permission, None); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; Ok(()) } @@ -100,7 +92,6 @@ async fn test_dashboard_file_no_permission() -> Result<()> { async fn test_dashboard_file_public_access() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -115,7 +106,7 @@ async fn test_dashboard_file_public_access() -> Result<()> { let dashboard_id = dashboard_file.id; // Insert the test dashboard file - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Fetch file with permissions for a random user (no direct permission) let random_user_id = Uuid::new_v4(); @@ -128,7 +119,7 @@ async fn test_dashboard_file_public_access() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanView)); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; Ok(()) } @@ -138,7 +129,6 @@ async fn test_dashboard_file_public_access() -> Result<()> { async fn test_dashboard_file_expired_public_access() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -153,7 +143,7 @@ async fn test_dashboard_file_expired_public_access() -> Result<()> { let dashboard_id = dashboard_file.id; // Insert the test dashboard file - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Fetch file with permissions for a random user (no direct permission) let random_user_id = Uuid::new_v4(); @@ -166,7 +156,7 @@ async fn test_dashboard_file_expired_public_access() -> Result<()> { assert_eq!(file_with_permission.permission, None); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; Ok(()) } @@ -176,7 +166,6 @@ async fn test_dashboard_file_expired_public_access() -> Result<()> { async fn test_fetch_multiple_dashboard_files() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and files let user = test_db.create_test_user().await?; @@ -185,28 +174,28 @@ async fn test_fetch_multiple_dashboard_files() -> Result<()> { // Create and insert three test dashboard files with different permissions let dashboard_file1 = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id1 = dashboard_file1.id; - insert_test_dashboard_file(&mut conn, &dashboard_file1).await?; + insert_test_dashboard_file(&dashboard_file1).await?; let mut dashboard_file2 = test_db.create_test_dashboard_file(&owner_id).await?; dashboard_file2.publicly_accessible = true; dashboard_file2.public_expiry_date = Some(Utc::now() + chrono::Duration::days(1)); let dashboard_id2 = dashboard_file2.id; - insert_test_dashboard_file(&mut conn, &dashboard_file2).await?; + insert_test_dashboard_file(&dashboard_file2).await?; let dashboard_file3 = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id3 = dashboard_file3.id; - insert_test_dashboard_file(&mut conn, &dashboard_file3).await?; + insert_test_dashboard_file(&dashboard_file3).await?; // Create and insert permissions let permission1 = test_db .create_asset_permission(&dashboard_id1, AssetType::DashboardFile, &owner_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &permission1).await?; + insert_test_permission(&permission1).await?; let permission3 = test_db .create_asset_permission(&dashboard_id3, AssetType::DashboardFile, &owner_id, AssetPermissionRole::CanView) .await?; - insert_test_permission(&mut conn, &permission3).await?; + insert_test_permission(&permission3).await?; // Fetch multiple files with permissions let ids = vec![dashboard_id1, dashboard_id2, dashboard_id3]; @@ -229,7 +218,7 @@ async fn test_fetch_multiple_dashboard_files() -> Result<()> { } // Clean up - cleanup_test_data(&mut conn, &ids).await?; + cleanup_test_data(&ids).await?; Ok(()) } @@ -249,7 +238,7 @@ async fn test_dashboard_file_collection_access() -> Result<()> { // Create dashboard file let dashboard_file = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id = dashboard_file.id; - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create collection let collection_id = Uuid::new_v4(); @@ -293,7 +282,7 @@ async fn test_dashboard_file_collection_access() -> Result<()> { let collection_permission = test_db .create_asset_permission(&collection_id, AssetType::Collection, &viewer_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &collection_permission).await?; + insert_test_permission(&collection_permission).await?; // Fetch dashboard file with permissions as viewer let result = fetch_dashboard_file_with_permission(&dashboard_id, &viewer_id).await?; @@ -305,7 +294,7 @@ async fn test_dashboard_file_collection_access() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanEdit)); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; // Delete collection and associations diesel::delete(database::schema::collections_to_assets::table) @@ -340,7 +329,7 @@ async fn test_dashboard_file_permission_precedence() -> Result<()> { // Create dashboard file let dashboard_file = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id = dashboard_file.id; - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create collection let collection_id = Uuid::new_v4(); @@ -386,13 +375,13 @@ async fn test_dashboard_file_permission_precedence() -> Result<()> { let direct_permission = test_db .create_asset_permission(&dashboard_id, AssetType::DashboardFile, &owner_id, AssetPermissionRole::CanView) .await?; - insert_test_permission(&mut conn, &direct_permission).await?; + insert_test_permission(&direct_permission).await?; // Collection permission - CanEdit (higher) let collection_permission = test_db .create_asset_permission(&collection_id, AssetType::Collection, &owner_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &collection_permission).await?; + insert_test_permission(&collection_permission).await?; // Fetch dashboard file with permissions let result = fetch_dashboard_file_with_permission(&dashboard_id, &owner_id).await?; @@ -404,7 +393,7 @@ async fn test_dashboard_file_permission_precedence() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanEdit)); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; // Delete collection and associations diesel::delete(database::schema::collections_to_assets::table) @@ -446,7 +435,7 @@ async fn test_dashboard_file_public_and_collection_access() -> Result<()> { dashboard_file.public_expiry_date = Some(Utc::now() + chrono::Duration::days(1)); let dashboard_id = dashboard_file.id; - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create collection let collection_id = Uuid::new_v4(); @@ -496,7 +485,7 @@ async fn test_dashboard_file_public_and_collection_access() -> Result<()> { let collection_permission = test_db .create_asset_permission(&collection_id, AssetType::Collection, &viewer_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &collection_permission).await?; + insert_test_permission(&collection_permission).await?; let result2 = fetch_dashboard_file_with_permission(&dashboard_id, &viewer_id).await?; assert!(result2.is_some(), "Dashboard file should be found"); @@ -504,7 +493,7 @@ async fn test_dashboard_file_public_and_collection_access() -> Result<()> { assert_eq!(file_with_permission2.permission, Some(AssetPermissionRole::CanEdit)); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; // Delete collection and associations diesel::delete(database::schema::collections_to_assets::table) @@ -530,7 +519,6 @@ async fn test_dashboard_file_public_and_collection_access() -> Result<()> { async fn test_deleted_dashboard_file_not_returned() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -543,13 +531,13 @@ async fn test_deleted_dashboard_file_not_returned() -> Result<()> { let dashboard_id = dashboard_file.id; // Insert the deleted dashboard file - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create permission let permission = test_db .create_asset_permission(&dashboard_id, AssetType::DashboardFile, &owner_id, AssetPermissionRole::Owner) .await?; - insert_test_permission(&mut conn, &permission).await?; + insert_test_permission(&permission).await?; // Fetch file with permissions let result = fetch_dashboard_file_with_permission(&dashboard_id, &owner_id).await?; @@ -558,7 +546,7 @@ async fn test_deleted_dashboard_file_not_returned() -> Result<()> { assert!(result.is_none(), "Deleted dashboard file should not be found"); // Clean up - cleanup_test_data(&mut conn, &[dashboard_id]).await?; + cleanup_test_data(&[dashboard_id]).await?; Ok(()) } \ No newline at end of file diff --git a/api/libs/database/tests/helpers/metric_files_test.rs b/api/libs/database/tests/helpers/metric_files_test.rs index 4d8bd7ad6..48684b0fa 100644 --- a/api/libs/database/tests/helpers/metric_files_test.rs +++ b/api/libs/database/tests/helpers/metric_files_test.rs @@ -10,14 +10,13 @@ use diesel_async::RunQueryDsl; use tokio; use uuid::Uuid; -use super::test_utils::{cleanup_test_data, insert_test_metric_file, insert_test_dashboard_file, insert_test_permission, TestDb}; +use database::test_utils::{TestDb, insert_test_metric_file, insert_test_dashboard_file, insert_test_permission, cleanup_test_data}; /// Tests the fetch_metric_file_with_permissions function with direct permission #[tokio::test] async fn test_metric_file_direct_permission() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -26,7 +25,7 @@ async fn test_metric_file_direct_permission() -> Result<()> { let metric_id = metric_file.id; // Insert the test metric file - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Test cases with different permission roles for role in [ @@ -40,7 +39,7 @@ async fn test_metric_file_direct_permission() -> Result<()> { let permission = test_db .create_asset_permission(&metric_id, AssetType::MetricFile, &owner_id, role) .await?; - insert_test_permission(&mut conn, &permission).await?; + insert_test_permission(&permission).await?; // Fetch file with permissions let result = fetch_metric_file_with_permissions(&metric_id, &owner_id).await?; @@ -50,16 +49,10 @@ async fn test_metric_file_direct_permission() -> Result<()> { let file_with_permission = result.unwrap(); assert_eq!(file_with_permission.metric_file.id, metric_id); assert_eq!(file_with_permission.permission, Some(role)); - - // Clean up permission for next test - diesel::delete(database::schema::asset_permissions::table) - .filter(database::schema::asset_permissions::asset_id.eq(metric_id)) - .execute(&mut conn) - .await?; } // Clean up - cleanup_test_data(&mut conn, &[metric_id]).await?; + cleanup_test_data(&[metric_id]).await?; Ok(()) } @@ -69,7 +62,6 @@ async fn test_metric_file_direct_permission() -> Result<()> { async fn test_metric_file_no_permission() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -78,7 +70,7 @@ async fn test_metric_file_no_permission() -> Result<()> { let metric_id = metric_file.id; // Insert the test metric file - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Fetch file with permissions (no permission exists) let result = fetch_metric_file_with_permissions(&metric_id, &Uuid::new_v4()).await?; @@ -90,7 +82,7 @@ async fn test_metric_file_no_permission() -> Result<()> { assert_eq!(file_with_permission.permission, None); // Clean up - cleanup_test_data(&mut conn, &[metric_id]).await?; + cleanup_test_data(&[metric_id]).await?; Ok(()) } @@ -100,7 +92,6 @@ async fn test_metric_file_no_permission() -> Result<()> { async fn test_metric_file_public_access() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -115,7 +106,7 @@ async fn test_metric_file_public_access() -> Result<()> { let metric_id = metric_file.id; // Insert the test metric file - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Fetch file with permissions for a random user (no direct permission) let random_user_id = Uuid::new_v4(); @@ -128,7 +119,7 @@ async fn test_metric_file_public_access() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanView)); // Clean up - cleanup_test_data(&mut conn, &[metric_id]).await?; + cleanup_test_data(&[metric_id]).await?; Ok(()) } @@ -138,7 +129,6 @@ async fn test_metric_file_public_access() -> Result<()> { async fn test_metric_file_expired_public_access() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and file let user = test_db.create_test_user().await?; @@ -153,7 +143,7 @@ async fn test_metric_file_expired_public_access() -> Result<()> { let metric_id = metric_file.id; // Insert the test metric file - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Fetch file with permissions for a random user (no direct permission) let random_user_id = Uuid::new_v4(); @@ -166,7 +156,7 @@ async fn test_metric_file_expired_public_access() -> Result<()> { assert_eq!(file_with_permission.permission, None); // Clean up - cleanup_test_data(&mut conn, &[metric_id]).await?; + cleanup_test_data(&[metric_id]).await?; Ok(()) } @@ -176,7 +166,6 @@ async fn test_metric_file_expired_public_access() -> Result<()> { async fn test_fetch_multiple_metric_files() -> Result<()> { // Initialize test environment let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; // Create test user and files let user = test_db.create_test_user().await?; @@ -185,28 +174,28 @@ async fn test_fetch_multiple_metric_files() -> Result<()> { // Create and insert three test metric files with different permissions let metric_file1 = test_db.create_test_metric_file(&owner_id).await?; let metric_id1 = metric_file1.id; - insert_test_metric_file(&mut conn, &metric_file1).await?; + insert_test_metric_file(&metric_file1).await?; let mut metric_file2 = test_db.create_test_metric_file(&owner_id).await?; metric_file2.publicly_accessible = true; metric_file2.public_expiry_date = Some(Utc::now() + chrono::Duration::days(1)); let metric_id2 = metric_file2.id; - insert_test_metric_file(&mut conn, &metric_file2).await?; + insert_test_metric_file(&metric_file2).await?; let metric_file3 = test_db.create_test_metric_file(&owner_id).await?; let metric_id3 = metric_file3.id; - insert_test_metric_file(&mut conn, &metric_file3).await?; + insert_test_metric_file(&metric_file3).await?; // Create and insert permissions let permission1 = test_db .create_asset_permission(&metric_id1, AssetType::MetricFile, &owner_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &permission1).await?; + insert_test_permission(&permission1).await?; let permission3 = test_db .create_asset_permission(&metric_id3, AssetType::MetricFile, &owner_id, AssetPermissionRole::CanView) .await?; - insert_test_permission(&mut conn, &permission3).await?; + insert_test_permission(&permission3).await?; // Fetch multiple files with permissions let ids = vec![metric_id1, metric_id2, metric_id3]; @@ -229,7 +218,7 @@ async fn test_fetch_multiple_metric_files() -> Result<()> { } // Clean up - cleanup_test_data(&mut conn, &ids).await?; + cleanup_test_data(&ids).await?; Ok(()) } @@ -251,11 +240,11 @@ async fn test_metric_file_dashboard_access() -> Result<()> { // Create metric and dashboard files let metric_file = test_db.create_test_metric_file(&owner_id).await?; let metric_id = metric_file.id; - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; let dashboard_file = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id = dashboard_file.id; - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create association between metric and dashboard let metric_to_dashboard = MetricFileToDashboardFile { @@ -277,7 +266,7 @@ async fn test_metric_file_dashboard_access() -> Result<()> { let dashboard_permission = test_db .create_asset_permission(&dashboard_id, AssetType::DashboardFile, &viewer_id, AssetPermissionRole::CanView) .await?; - insert_test_permission(&mut conn, &dashboard_permission).await?; + insert_test_permission(&dashboard_permission).await?; // Fetch metric file with permissions as viewer let result = fetch_metric_file_with_permissions(&metric_id, &viewer_id).await?; @@ -289,7 +278,7 @@ async fn test_metric_file_dashboard_access() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanView)); // Clean up - cleanup_test_data(&mut conn, &[metric_id, dashboard_id]).await?; + cleanup_test_data(&[metric_id, dashboard_id]).await?; // Delete metric-to-dashboard association diesel::delete(database::schema::metric_files_to_dashboard_files::table) @@ -315,7 +304,7 @@ async fn test_metric_file_collection_access() -> Result<()> { // Create metric file let metric_file = test_db.create_test_metric_file(&owner_id).await?; let metric_id = metric_file.id; - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Create collection let collection_id = Uuid::new_v4(); @@ -359,7 +348,7 @@ async fn test_metric_file_collection_access() -> Result<()> { let collection_permission = test_db .create_asset_permission(&collection_id, AssetType::Collection, &viewer_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &collection_permission).await?; + insert_test_permission(&collection_permission).await?; // Fetch metric file with permissions as viewer let result = fetch_metric_file_with_permissions(&metric_id, &viewer_id).await?; @@ -371,7 +360,7 @@ async fn test_metric_file_collection_access() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanEdit)); // Clean up - cleanup_test_data(&mut conn, &[metric_id]).await?; + cleanup_test_data(&[metric_id]).await?; // Delete collection and associations diesel::delete(database::schema::collections_to_assets::table) @@ -406,7 +395,7 @@ async fn test_metric_file_permission_hierarchy() -> Result<()> { // Create metric file let metric_file = test_db.create_test_metric_file(&owner_id).await?; let metric_id = metric_file.id; - insert_test_metric_file(&mut conn, &metric_file).await?; + insert_test_metric_file(&metric_file).await?; // Create collection let collection_id = Uuid::new_v4(); @@ -449,7 +438,7 @@ async fn test_metric_file_permission_hierarchy() -> Result<()> { // Create dashboard file let dashboard_file = test_db.create_test_dashboard_file(&owner_id).await?; let dashboard_id = dashboard_file.id; - insert_test_dashboard_file(&mut conn, &dashboard_file).await?; + insert_test_dashboard_file(&dashboard_file).await?; // Create association between metric and dashboard let metric_to_dashboard = MetricFileToDashboardFile { @@ -473,19 +462,19 @@ async fn test_metric_file_permission_hierarchy() -> Result<()> { let direct_permission = test_db .create_asset_permission(&metric_id, AssetType::MetricFile, &owner_id, AssetPermissionRole::CanFilter) .await?; - insert_test_permission(&mut conn, &direct_permission).await?; + insert_test_permission(&direct_permission).await?; // Collection permission - CanEdit (higher than direct) let collection_permission = test_db .create_asset_permission(&collection_id, AssetType::Collection, &owner_id, AssetPermissionRole::CanEdit) .await?; - insert_test_permission(&mut conn, &collection_permission).await?; + insert_test_permission(&collection_permission).await?; // Dashboard permission - CanView (lower than others) let dashboard_permission = test_db .create_asset_permission(&dashboard_id, AssetType::DashboardFile, &owner_id, AssetPermissionRole::CanView) .await?; - insert_test_permission(&mut conn, &dashboard_permission).await?; + insert_test_permission(&dashboard_permission).await?; // Fetch metric file with permissions let result = fetch_metric_file_with_permissions(&metric_id, &owner_id).await?; @@ -497,7 +486,7 @@ async fn test_metric_file_permission_hierarchy() -> Result<()> { assert_eq!(file_with_permission.permission, Some(AssetPermissionRole::CanEdit)); // Clean up - cleanup_test_data(&mut conn, &[metric_id, dashboard_id]).await?; + cleanup_test_data(&[metric_id, dashboard_id]).await?; // Delete collections and associations diesel::delete(database::schema::collections_to_assets::table) diff --git a/api/libs/handlers/tests/dashboards/get_dashboard_handler_permission_test.rs b/api/libs/handlers/tests/dashboards/get_dashboard_handler_permission_test.rs index 694a972e9..ae809222f 100644 --- a/api/libs/handlers/tests/dashboards/get_dashboard_handler_permission_test.rs +++ b/api/libs/handlers/tests/dashboards/get_dashboard_handler_permission_test.rs @@ -1,6 +1,6 @@ use anyhow::{Result, Context}; use chrono::Utc; -use database::enums::{AssetPermissionRole, AssetType}; +use database::enums::{AssetPermissionRole, AssetType, UserOrganizationRole}; use database::test_utils::{TestDb, cleanup_test_data, insert_test_dashboard_file, insert_test_permission}; use handlers::dashboards::get_dashboard_handler; use middleware::{AuthenticatedUser, OrganizationMembership}; @@ -31,10 +31,9 @@ fn create_test_auth_user(user_id: Uuid, organization_id: Option) -> Authen #[tokio::test] async fn test_get_dashboard_no_permission_private() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let dashboard = test_db.create_test_dashboard_file(&owner.id).await?; - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); // User not in org, no share @@ -43,18 +42,17 @@ async fn test_get_dashboard_no_permission_private() -> Result<()> { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("don't have permission")); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } #[tokio::test] async fn test_get_dashboard_no_permission_public_no_password() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&owner.id).await?; dashboard.publicly_accessible = true; - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); @@ -64,20 +62,19 @@ async fn test_get_dashboard_no_permission_public_no_password() -> Result<()> { let response = result.unwrap(); assert_eq!(response.permission, AssetPermissionRole::CanView); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } #[tokio::test] async fn test_get_dashboard_no_permission_public_correct_password() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&owner.id).await?; dashboard.publicly_accessible = true; let password = "testpassword".to_string(); dashboard.public_password = Some(password.clone()); - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); @@ -87,19 +84,18 @@ async fn test_get_dashboard_no_permission_public_correct_password() -> Result<() let response = result.unwrap(); assert_eq!(response.permission, AssetPermissionRole::CanView); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } #[tokio::test] async fn test_get_dashboard_no_permission_public_incorrect_password() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&owner.id).await?; dashboard.publicly_accessible = true; dashboard.public_password = Some("correctpassword".to_string()); - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); @@ -108,19 +104,18 @@ async fn test_get_dashboard_no_permission_public_incorrect_password() -> Result< assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Incorrect password")); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } #[tokio::test] async fn test_get_dashboard_no_permission_public_missing_password() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&owner.id).await?; dashboard.publicly_accessible = true; dashboard.public_password = Some("correctpassword".to_string()); - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); @@ -129,19 +124,18 @@ async fn test_get_dashboard_no_permission_public_missing_password() -> Result<() assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("public_password required")); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } #[tokio::test] async fn test_get_dashboard_no_permission_public_expired() -> Result<()> { let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let owner = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&owner.id).await?; dashboard.publicly_accessible = true; dashboard.public_expiry_date = Some(Utc::now() - chrono::Duration::days(1)); // Expired - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let random_user = create_test_auth_user(Uuid::new_v4(), None); @@ -150,7 +144,7 @@ async fn test_get_dashboard_no_permission_public_expired() -> Result<()> { assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("expired")); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } @@ -158,15 +152,14 @@ async fn test_get_dashboard_no_permission_public_expired() -> Result<()> { async fn test_get_dashboard_direct_permission_public_password() -> Result<()> { // User has direct CanEdit permission, should bypass public password check let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let user = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&user.id).await?; dashboard.publicly_accessible = true; dashboard.public_password = Some("testpassword".to_string()); - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let permission = test_db.create_asset_permission(&dashboard.id, AssetType::DashboardFile, &user.id, AssetPermissionRole::CanEdit).await?; - insert_test_permission(&mut conn, &permission).await?; + insert_test_permission(&permission).await?; let auth_user = create_test_auth_user(user.id, Some(test_db.organization_id)); @@ -176,7 +169,7 @@ async fn test_get_dashboard_direct_permission_public_password() -> Result<()> { let response = result.unwrap(); assert_eq!(response.permission, AssetPermissionRole::CanEdit); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } @@ -184,12 +177,11 @@ async fn test_get_dashboard_direct_permission_public_password() -> Result<()> { async fn test_get_dashboard_admin_role_public_password() -> Result<()> { // User is WorkspaceAdmin, should bypass public password check let test_db = TestDb::new().await?; - let mut conn = test_db.get_conn().await?; let admin_user = test_db.create_test_user().await?; let mut dashboard = test_db.create_test_dashboard_file(&admin_user.id).await?; dashboard.publicly_accessible = true; dashboard.public_password = Some("testpassword".to_string()); - insert_test_dashboard_file(&mut conn, &dashboard).await?; + insert_test_dashboard_file(&dashboard).await?; let auth_user = AuthenticatedUser { id: admin_user.id, @@ -214,7 +206,7 @@ async fn test_get_dashboard_admin_role_public_password() -> Result<()> { let response = result.unwrap(); assert_eq!(response.permission, AssetPermissionRole::CanView); - cleanup_test_data(&mut conn, &[dashboard.id]).await?; + cleanup_test_data(&[dashboard.id]).await?; Ok(()) } diff --git a/api/libs/handlers/tests/dashboards/permission_field_test.rs b/api/libs/handlers/tests/dashboards/permission_field_test.rs index 9fa9db7b5..0ad85e452 100644 --- a/api/libs/handlers/tests/dashboards/permission_field_test.rs +++ b/api/libs/handlers/tests/dashboards/permission_field_test.rs @@ -1,112 +1,44 @@ use anyhow::Result; -use database::enums::{AssetPermissionRole, AssetType, IdentityType}; -use database::models::{AssetPermission, DashboardFile}; +use database::enums::{AssetPermissionRole, AssetType, IdentityType, UserOrganizationRole}; +use database::models::{DashboardFile, UserToOrganization}; use database::pool::get_pg_pool; -use database::schema::{asset_permissions, dashboard_files}; -use database::models::UserToOrganization; +use database::schema::{asset_permissions, dashboard_files, users_to_organizations}; use database::types::{DashboardYml, VersionHistory}; use diesel::prelude::*; use diesel_async::RunQueryDsl; use handlers::dashboards::get_dashboard_handler; +use middleware::{AuthenticatedUser, OrganizationMembership}; use std::collections::HashMap; use uuid::Uuid; -/// Helper function to create a test dashboard file -async fn create_test_dashboard( - organization_id: Uuid, - user_id: Uuid, - name: &str, -) -> Result { - let mut conn = get_pg_pool().get().await?; - let dashboard_id = Uuid::new_v4(); - - // Create a simple dashboard content - let content = DashboardYml { - name: name.to_string(), - description: Some(format!("Test dashboard description for {}", name)), - rows: Vec::new(), - }; - - let dashboard_file = DashboardFile { - id: dashboard_id, - name: name.to_string(), - file_name: format!("{}.yml", name.to_lowercase().replace(" ", "_")), - content, - filter: None, - organization_id, - created_by: user_id, - created_at: chrono::Utc::now(), - updated_at: chrono::Utc::now(), - deleted_at: None, - publicly_accessible: false, - publicly_enabled_by: None, - public_expiry_date: None, - version_history: VersionHistory(HashMap::new()), - public_password: None, - }; - - diesel::insert_into(dashboard_files::table) - .values(&dashboard_file) - .execute(&mut conn) - .await?; - - Ok(dashboard_file) -} - -/// Helper function to add permission for a dashboard -async fn add_permission( - asset_id: Uuid, - user_id: Uuid, - role: AssetPermissionRole, - created_by: Uuid, -) -> Result<()> { - let mut conn = get_pg_pool().get().await?; - - let permission = AssetPermission { - identity_id: user_id, - identity_type: IdentityType::User, - asset_id, - asset_type: AssetType::DashboardFile, - role, - created_at: chrono::Utc::now(), - updated_at: chrono::Utc::now(), - deleted_at: None, - created_by, - updated_by: created_by, - }; - - diesel::insert_into(asset_permissions::table) - .values(&permission) - .execute(&mut conn) - .await?; - - Ok(()) -} +// Use test_utils helpers +use database::test_utils::{TestDb, insert_test_dashboard_file, insert_test_permission, cleanup_test_data}; /// Test to ensure permission fields in dashboard response match the permission used for access control #[tokio::test] async fn test_dashboard_permission_field_consistency() -> Result<()> { - // Create user and organization for testing - let user_id = Uuid::new_v4(); - let org_id = Uuid::new_v4(); + // Use TestDb for setup + let test_db = TestDb::new().await?; + let user_id = test_db.user_id; + let org_id = test_db.organization_id; - // Create test dashboard - let dashboard = create_test_dashboard( - org_id, - user_id, - "Test Permission Dashboard" - ).await?; + // Create test dashboard using TestDb helper + let dashboard = test_db.create_test_dashboard_file(&user_id).await?; + // Insert dashboard using helper + insert_test_dashboard_file(&dashboard).await?; - // Add permission for asset - add_permission( - dashboard.id, - user_id, - AssetPermissionRole::Owner, - user_id + // Add permission for asset using TestDb helper + let permission = test_db.create_asset_permission( + &dashboard.id, + AssetType::DashboardFile, + &user_id, + AssetPermissionRole::Owner ).await?; + // Insert permission using helper + insert_test_permission(&permission).await?; // Create middleware user - let middleware_user = middleware::AuthenticatedUser { + let middleware_user = AuthenticatedUser { id: user_id, email: "test@example.com".to_string(), name: Some("Test User".to_string()), @@ -116,58 +48,55 @@ async fn test_dashboard_permission_field_consistency() -> Result<()> { attributes: serde_json::json!({}), avatar_url: None, organizations: vec![ - middleware::OrganizationMembership { + OrganizationMembership { id: org_id, - role: database::enums::UserOrganizationRole::WorkspaceAdmin, + role: UserOrganizationRole::WorkspaceAdmin, }, ], teams: vec![], }; // Get dashboard with the user who has owner permission - let dashboard_response = get_dashboard_handler(&dashboard.id, &middleware_user, None).await?; + let dashboard_response = get_dashboard_handler(&dashboard.id, &middleware_user, None, None).await?; // Check if permission fields are consistent assert_eq!(dashboard_response.permission, AssetPermissionRole::Owner); assert_eq!(dashboard_response.access, AssetPermissionRole::Owner); + // Clean up using helper + cleanup_test_data(&[dashboard.id]).await?; + Ok(()) } /// Test to ensure public dashboards grant CanView permission to users without direct permissions #[tokio::test] async fn test_public_dashboard_permission_field() -> Result<()> { - // Create user and organization for testing - let owner_id = Uuid::new_v4(); - let org_id = Uuid::new_v4(); + // Use TestDb for setup + let test_db = TestDb::new().await?; + let owner_id = test_db.user_id; + let org_id = test_db.organization_id; - // Create test dashboard - let dashboard = create_test_dashboard( - org_id, - owner_id, - "Public Dashboard" - ).await?; + // Create test dashboard using TestDb helper + let mut dashboard = test_db.create_test_dashboard_file(&owner_id).await?; // Make dashboard public - let mut conn = get_pg_pool().get().await?; - diesel::update(dashboard_files::table) - .filter(dashboard_files::id.eq(dashboard.id)) - .set(( - dashboard_files::publicly_accessible.eq(true), - dashboard_files::publicly_enabled_by.eq(Some(owner_id)), - dashboard_files::public_expiry_date.eq(Some(chrono::Utc::now() + chrono::Duration::days(7))), - )) - .execute(&mut conn) - .await?; + dashboard.publicly_accessible = true; + dashboard.publicly_enabled_by = Some(owner_id); + dashboard.public_expiry_date = Some(chrono::Utc::now() + chrono::Duration::days(7)); + + // Insert dashboard using helper + insert_test_dashboard_file(&dashboard).await?; - // Create another user + // Create another user (just need the ID) let other_user_id = Uuid::new_v4(); - // Add user to organization with viewer role + // Add user to organization with viewer role (manual insert, no helper yet) + let mut conn = get_pg_pool().get().await?; let user_org = UserToOrganization { user_id: other_user_id, organization_id: org_id, - role: database::enums::UserOrganizationRole::Viewer, + role: UserOrganizationRole::Viewer, sharing_setting: database::enums::SharingSetting::None, edit_sql: true, upload_csv: true, @@ -182,13 +111,13 @@ async fn test_public_dashboard_permission_field() -> Result<()> { status: database::enums::UserOrganizationStatus::Active, }; - diesel::insert_into(database::schema::users_to_organizations::table) + diesel::insert_into(users_to_organizations::table) .values(&user_org) .execute(&mut conn) .await?; // Create middleware user for other user - let other_middleware_user = middleware::AuthenticatedUser { + let other_middleware_user = AuthenticatedUser { id: other_user_id, email: "other@example.com".to_string(), name: Some("Other User".to_string()), @@ -198,44 +127,53 @@ async fn test_public_dashboard_permission_field() -> Result<()> { attributes: serde_json::json!({}), avatar_url: None, organizations: vec![ - middleware::OrganizationMembership { + OrganizationMembership { id: org_id, - role: database::enums::UserOrganizationRole::Viewer, + role: UserOrganizationRole::Viewer, }, ], teams: vec![], }; // Get dashboard with the other user who doesn't have direct permission - let dashboard_response = get_dashboard_handler(&dashboard.id, &other_middleware_user, None).await?; + let dashboard_response = get_dashboard_handler(&dashboard.id, &other_middleware_user, None, None).await?; // Public assets should have CanView permission by default assert_eq!(dashboard_response.permission, AssetPermissionRole::CanView); assert_eq!(dashboard_response.access, AssetPermissionRole::CanView); + // Clean up using helper (also removes permissions) + cleanup_test_data(&[dashboard.id]).await?; + + // Manual cleanup for user_org association + diesel::delete(users_to_organizations::table) + .filter(users_to_organizations::user_id.eq(other_user_id)) + .filter(users_to_organizations::organization_id.eq(org_id)) + .execute(&mut conn) + .await?; + Ok(()) } /// Test to ensure access is denied for users without permissions and non-public dashboards #[tokio::test] async fn test_dashboard_permission_denied() -> Result<()> { - // Create user and organization for testing - let owner_id = Uuid::new_v4(); - let org_id = Uuid::new_v4(); + // Use TestDb for setup + let test_db = TestDb::new().await?; + let owner_id = test_db.user_id; + let org_id = test_db.organization_id; - // Create a private test dashboard - let dashboard = create_test_dashboard( - org_id, - owner_id, - "Private Dashboard" - ).await?; + // Create a private test dashboard using TestDb helper + let dashboard = test_db.create_test_dashboard_file(&owner_id).await?; + // Insert dashboard using helper + insert_test_dashboard_file(&dashboard).await?; // Create another user in a different organization let other_user_id = Uuid::new_v4(); let other_org_id = Uuid::new_v4(); // Create middleware user for other user - let other_middleware_user = middleware::AuthenticatedUser { + let other_middleware_user = AuthenticatedUser { id: other_user_id, email: "other@example.com".to_string(), name: Some("Other User".to_string()), @@ -245,21 +183,24 @@ async fn test_dashboard_permission_denied() -> Result<()> { attributes: serde_json::json!({}), avatar_url: None, organizations: vec![ - middleware::OrganizationMembership { + OrganizationMembership { id: other_org_id, - role: database::enums::UserOrganizationRole::Viewer, + role: UserOrganizationRole::Viewer, }, ], teams: vec![], }; // Try to get dashboard with a user who has no permissions - let result = get_dashboard_handler(&dashboard.id, &other_middleware_user, None).await; + let result = get_dashboard_handler(&dashboard.id, &other_middleware_user, None, None).await; // Should be denied access assert!(result.is_err()); let error = result.unwrap_err(); assert!(error.to_string().contains("You don't have permission")); + // Clean up using helper + cleanup_test_data(&[dashboard.id]).await?; + Ok(()) } \ No newline at end of file diff --git a/api/migrations/2025-04-07-222456_add_public_password_to_files/up.sql b/api/migrations/2025-04-07-222456_add_public_password_to_files/up.sql index 5afc44da0..c83e14db4 100644 --- a/api/migrations/2025-04-07-222456_add_public_password_to_files/up.sql +++ b/api/migrations/2025-04-07-222456_add_public_password_to_files/up.sql @@ -1,7 +1,25 @@ -- Your SQL goes here +-- Add public_password column to metric_files table if it doesn't exist +DO $$ +BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM information_schema.columns + WHERE table_name = 'metric_files' + AND column_name = 'public_password' + ) THEN + ALTER TABLE metric_files + ADD COLUMN public_password TEXT; + END IF; -ALTER TABLE metric_files -ADD COLUMN public_password TEXT; - -ALTER TABLE dashboard_files -ADD COLUMN public_password TEXT; + -- Add public_password column to dashboard_files table if it doesn't exist + IF NOT EXISTS ( + SELECT 1 + FROM information_schema.columns + WHERE table_name = 'dashboard_files' + AND column_name = 'public_password' + ) THEN + ALTER TABLE dashboard_files + ADD COLUMN public_password TEXT; + END IF; +END $$; diff --git a/api/prds/active/enhancement_collection_asset_permissions.md b/api/prds/active/enhancement_collection_asset_permissions.md index 49220c84c..3c06e334d 100644 --- a/api/prds/active/enhancement_collection_asset_permissions.md +++ b/api/prds/active/enhancement_collection_asset_permissions.md @@ -9,6 +9,8 @@ This PRD details the modifications required for the `get_collection_handler` to incorporate granular permission checks for each asset (Metric, Dashboard, etc.) contained within a collection. It introduces a `has_access` flag to the `CollectionAsset` type to indicate whether the requesting user has permission to view the specific asset. +**Note on Concurrency:** This work depends on the completion of the [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). Once the helper is available, this task can potentially be performed concurrently with the enhancements for the dashboard and data execution handlers. + ## 2. Problem Statement The current `get_collection_handler` lists assets associated with a collection but doesn't verify the user's permission for each *individual* asset. This can lead to users seeing assets in the list that they cannot actually access, creating a confusing user experience. diff --git a/api/prds/active/enhancement_dashboard_metric_permissions.md b/api/prds/active/enhancement_dashboard_metric_permissions.md index e56fde67e..18c73cbe3 100644 --- a/api/prds/active/enhancement_dashboard_metric_permissions.md +++ b/api/prds/active/enhancement_dashboard_metric_permissions.md @@ -9,6 +9,8 @@ This PRD describes the modifications needed for the `get_dashboard_handler` to implement granular permission checks for each metric included in a dashboard. It involves leveraging the central permission helper and adjusting how metric data is fetched and represented in the response, potentially by modifying `get_metric_handler` or how its results are processed. +**Note on Concurrency:** This work depends on the completion of the [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). Once the helper is available, this task can potentially be performed concurrently with the enhancements for the collection and data execution handlers. + ## 2. Problem Statement Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_handler`. If `get_metric_handler` fails (potentially due to permissions), the metric is silently filtered out from the dashboard response's `metrics` map. Users aren't informed *why* a metric configured in the dashboard isn't visible, and they can't distinguish between a metric failing to load due to an error vs. lack of permissions. @@ -19,6 +21,7 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ - Ensure that if a user lacks `CanView` permission for a metric configured in a dashboard, the metric is still included in the response's `metrics` map, but represented minimally with `has_access: false`. - Add the `has_access: bool` field to the `BusterMetric` type. - Leverage the `check_specific_asset_access` helper for permission verification. +- **Note:** This handler modification primarily affects the *metadata* served about the metric. Blocking the actual *execution* of the metric's SQL query for data retrieval will be handled by adding permission checks to the relevant data execution handler (as per the project PRD). ## 4. Non-Goals @@ -30,9 +33,13 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ **Option A: Modify `get_metric_handler` (Recommended)** 1. **Type Modification:** - - Add `has_access: bool` to `BusterMetric` struct (likely in `libs/handlers/src/metrics/types.rs`). Also ensure relevant nested types like `MetricConfig` are adjusted if necessary or handled during minimal object creation. + - Add `has_access: bool` to `BusterMetric` struct (likely in `libs/handlers/src/metrics/types.rs`). Also ensure relevant nested types like `ChartConfig` (note: it's from `database::types::ChartConfig`) are handled during minimal object creation. ```rust // libs/handlers/src/metrics/types.rs (adjust path as needed) + use database::types::ChartConfig; // Import the correct ChartConfig + use database::enums::Verification; + use chrono::{DateTime, Utc, TimeZone}; + #[derive(Serialize, Deserialize, Debug, Clone)] // Add necessary derives pub struct BusterMetric { // --- Fields always present, even if no access --- @@ -57,6 +64,9 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ } // Add default implementation if needed for minimal construction + // Note: Default cannot be easily derived due to non-optional fields. + // We will construct the minimal object manually. + /* impl Default for BusterMetric { fn default() -> Self { Self { @@ -72,6 +82,7 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ } } } + */ ``` 2. **Modify `get_metric_handler.rs`:** @@ -86,12 +97,12 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ let basic_metric_info = metric_files::table .filter(metric_files::id.eq(metric_id)) .filter(metric_files::deleted_at.is_null()) - .select((metric_files::id, metric_files::name, metric_files::organization_id)) - .first::<(Uuid, String, Uuid)>(&mut conn) + .select((metric_files::id, metric_files::name, metric_files::organization_id, metric_files::created_by)) + .first::<(Uuid, String, Uuid, Uuid)>(&mut conn) // Fetch created_by too .await .optional()?; // Use optional() to handle not found gracefully - let (id, name, org_id) = match basic_metric_info { + let (id, name, org_id, created_by_id) = match basic_metric_info { Some(info) => info, None => return Err(anyhow!("Metric not found")), // Return Err if metric doesn't exist }; @@ -114,11 +125,43 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ }) } else { // Construct minimal BusterMetric with has_access: false + // Need to provide defaults for non-optional fields. + let default_time = Utc.timestamp_opt(0, 0).unwrap(); // Use epoch for timestamps Ok(BusterMetric { id, + metric_type: "metric".to_string(), name, // Use fetched name + version_number: 0, // Default version + description: None, // Optional + file_name: "".to_string(), // Default empty + time_frame: "".to_string(), // Default empty + datasets: vec![], // Default empty + data_source_id: "".to_string(), // Default empty + error: None, // Optional + chart_config: None, // Optional + data_metadata: None, // Optional + status: Verification::Unverified, // Default status + evaluation_score: None, // Optional + evaluation_summary: "".to_string(), // Default empty + file: "".to_string(), // Default empty YAML + created_at: default_time, // Default timestamp + updated_at: default_time, // Default timestamp + sent_by_id: created_by_id, // Use fetched creator ID + sent_by_name: "(Restricted Access)".to_string(), // Placeholder name + sent_by_avatar_url: None, // Optional + code: None, // Optional (SQL query) + dashboards: vec![], // Default empty + collections: vec![], // Default empty + versions: vec![], // Default empty + permission: AssetPermissionRole::CanView, // Technically viewable, but content restricted + sql: "-- Restricted Access --".to_string(), // Placeholder SQL + individual_permissions: None, // Optional + public_expiry_date: None, // Optional + public_enabled_by: None, // Optional + publicly_accessible: false, // Default + public_password: None, // Optional + // **Crucially set has_access to false** has_access: false, - ..Default::default() // Use default for other fields }) } ``` diff --git a/api/prds/active/enhancement_data_execution_permissions.md b/api/prds/active/enhancement_data_execution_permissions.md new file mode 100644 index 000000000..45a53a601 --- /dev/null +++ b/api/prds/active/enhancement_data_execution_permissions.md @@ -0,0 +1,112 @@ +# Sub-PRD: Enhance Data Execution Handler Permissions + +**Author:** AI Assistant (Pair Programming with User) +**Date:** 2023-10-27 +**Status:** Proposed +**Parent PRD:** [Project: Granular Asset Permission Checks](mdc:prds/active/project_granular_asset_permissions.md) + +## 1. Overview + +This PRD outlines the necessary modifications to the handler responsible for executing metric SQL queries (the "data execution handler"). The goal is to ensure that a permission check is performed *before* any query is executed against the database, preventing users from retrieving data for metrics they are not authorized to view, even if they somehow obtained the metric ID. + +**Note on Concurrency:** This work depends on the completion of the [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). Once the helper is available, this task can potentially be performed concurrently with the enhancements for the collection and dashboard handlers. + +## 2. Problem Statement + +While other handlers (`get_metric_handler`, `get_dashboard_handler`) are being modified to control the visibility and metadata of metrics based on permissions, the handler that actually runs the metric's SQL query might currently execute it based solely on receiving a valid metric ID. This could allow unauthorized data access if a user bypasses the standard UI flows or if the metadata handlers fail to properly restrict access information. + +## 3. Goals + +- Identify the primary handler(s) responsible for taking a metric ID (and potentially parameters) and executing its SQL query. +- Integrate a call to the `check_specific_asset_access` helper function at the beginning of this handler. +- Ensure the handler returns a clear permission denied error if the check fails. +- Prevent unauthorized execution of metric queries and subsequent data leakage. + +## 4. Non-Goals + +- Modifying the query execution engine itself. +- Changing how metric SQL queries are stored or constructed. +- Implementing rate limiting or query cost analysis (though permission checks are a prerequisite for such features). + +## 5. Technical Design + +1. **Identify Target Handler:** The primary handler needs to be identified. Assuming a hypothetical handler like `execute_metric_query_handler(metric_id: Uuid, user: AuthenticatedUser, params: Option) -> Result` exists (the actual name and signature might differ). +2. **Fetch Asset Metadata:** Before executing the query, the handler must fetch basic metadata for the given `metric_id`, specifically its `organization_id`, to pass to the permission checker. This might already be part of the handler logic to retrieve the SQL. + ```rust + // Inside the data execution handler... + let mut conn = get_pg_pool().get().await?; + let metric_id = /* from request */; + let user = /* from request context */; + + // Fetch necessary info for permission check (and potentially SQL) + let metric_info = metric_files::table + .filter(metric_files::id.eq(metric_id)) + .filter(metric_files::deleted_at.is_null()) + .select((metric_files::organization_id, metric_files::sql /* or content */)) // Select org_id and SQL/content + .first::<(Uuid, String /* or appropriate type */)>(&mut conn) + .await + .map_err(|_| anyhow!("Metric not found or basic info inaccessible"))?; // Handle not found + + let (organization_id, _sql_or_content) = metric_info; + ``` +3. **Permission Check:** Call `check_specific_asset_access` immediately after retrieving the necessary metadata and before executing any query based on the metric's definition. The required role is typically `CanView`. + ```rust + // Still inside the data execution handler... + + let required_roles = [AssetPermissionRole::CanView]; + let has_permission = sharing::check_specific_asset_access( + &mut conn, + user, + &metric_id, + AssetType::MetricFile, // Assuming metrics are the target here + organization_id, + &required_roles, + ) + .await?; // Propagate DB errors + + if !has_permission { + tracing::warn!(user_id = %user.id, metric_id = %metric_id, "Permission denied for metric query execution."); + // Return a specific, user-friendly permission error + return Err(anyhow!("Permission denied: You do not have access to view data for this metric.")); + } + + // --- Permission Granted: Proceed with Query Execution --- + // ... existing logic to prepare and execute the SQL query ... + // let sql = _sql_or_content; // Get the SQL from fetched info + // ... run query ... + // return Ok(query_result); + ``` +4. **Error Handling:** Ensure that a distinct and user-understandable error is returned upon permission failure, separate from "metric not found" or query execution errors. + +5. **File Changes:** + - Modify: The identified data execution handler file (e.g., `libs/handlers/src/query_execution/execute_metric_handler.rs` - **Actual path needs confirmation**). + +## 6. Implementation Plan + +1. Confirm the exact handler(s) and file path(s) responsible for executing metric queries. +2. Ensure the handler fetches the metric's `organization_id`. +3. Insert the call to `check_specific_asset_access` near the beginning of the handler. +4. Add error handling for permission denial. +5. Add/update unit and integration tests for this handler. + +## 7. Testing Strategy + +- **Unit Tests:** + - Mock the `check_specific_asset_access` function and database interactions. + - Test case: `check_specific_asset_access` returns `Ok(true)` -> Verify query execution proceeds. + - Test case: `check_specific_asset_access` returns `Ok(false)` -> Verify a specific permission error is returned and the query execution logic is *not* called. + - Test case: `check_specific_asset_access` returns `Err` -> Verify the error is propagated correctly. + - Test case: Metric ID not found during initial metadata fetch -> Verify appropriate error is returned. +- **Integration Tests:** + - Setup: User, Metric A (user has CanView), Metric B (user has no CanView). + - Scenario 1: Call the data execution endpoint/handler for Metric A -> Verify success and data return. + - Scenario 2: Call the data execution endpoint/handler for Metric B -> Verify a clear "Permission Denied" error is returned. + +## 8. Rollback Plan + +- Revert the changes to the identified data execution handler file(s). + +## 9. Dependencies + +- Completion of [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). +- Identification of the correct data execution handler file path. \ No newline at end of file diff --git a/api/prds/active/project_granular_asset_permissions.md b/api/prds/active/project_granular_asset_permissions.md index 65d2c343a..a924c258c 100644 --- a/api/prds/active/project_granular_asset_permissions.md +++ b/api/prds/active/project_granular_asset_permissions.md @@ -25,13 +25,14 @@ This project aims to implement fine-grained permission checks for assets (like m ## 4. Implementation Plan -The implementation will be broken down into three sub-PRDs, executed in the specified order due to dependencies: +The implementation will be broken down into four sub-PRDs, executed in the specified order due to dependencies: 1. **Upcoming:** [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md) - Create/Enhance a centralized helper function in `libs/sharing` for checking specific asset permissions. 2. **Upcoming:** [Enhance Collection Asset Permissions](mdc:prds/active/enhancement_collection_asset_permissions.md) - Modify `get_collection_handler` and related types to use the new helper and include the `has_access` flag. (Depends on #1) 3. **Upcoming:** [Enhance Dashboard Metric Permissions](mdc:prds/active/enhancement_dashboard_metric_permissions.md) - Modify `get_dashboard_handler`, potentially `get_metric_handler`, and related types to use the new helper and include the `has_access` flag. (Depends on #1) +4. **Upcoming:** Enhance Data Execution Handler - Modify the handler responsible for executing metric SQL queries to call the permission helper before execution. (Depends on #1) -**Concurrency:** Sub-PRDs #2 and #3 can potentially be worked on concurrently *after* Sub-PRD #1 is completed and merged, as they modify different handlers but depend on the same shared helper. +**Concurrency:** Sub-PRDs #2 and #3 can potentially be worked on concurrently *after* Sub-PRD #1 is completed and merged, as they modify different handlers but depend on the same shared helper. Sub-PRD #4 also depends on #1 and can likely be done concurrently with #2/#3. ## 5. High-Level Technical Design @@ -39,6 +40,7 @@ The implementation will be broken down into three sub-PRDs, executed in the spec - Develop a reusable function `check_specific_asset_access` in `libs/sharing` to determine if a user has the required permission level for a given asset ID and type. - Modify `get_collection_handler` to fetch all associated asset IDs, use the helper function (potentially in batch) to check permissions, and populate the `has_access` field in the response. - Modify `get_dashboard_handler` (and potentially underlying asset fetchers like `get_metric_handler`) to use the helper function. For assets where the user lacks permission, return a minimal representation of the asset with `has_access: false` instead of filtering it out or returning a hard error solely due to permissions. +- **Crucially, modify the handler responsible for executing metric SQL queries to call `check_specific_asset_access` before running the query. If the check returns `false`, the handler must return a permission error instead of executing the query.** ## 6. Testing Strategy diff --git a/api/prds/active/refactor_sharing_permission_helper.md b/api/prds/active/refactor_sharing_permission_helper.md index a58b557f7..a1790fffc 100644 --- a/api/prds/active/refactor_sharing_permission_helper.md +++ b/api/prds/active/refactor_sharing_permission_helper.md @@ -7,18 +7,18 @@ ## 1. Overview -This PRD details the creation or enhancement of a centralized helper function within the `libs/sharing` crate. This function will encapsulate the logic for checking if a given user has at least one of a set of required permission roles for a specific asset (identified by ID and type). This promotes consistency and simplifies permission checking in various handlers. +This PRD details the creation or enhancement of a centralized helper function within the `libs/sharing` crate. This function will encapsulate the logic for checking if a given user has at least one of a set of required permission roles for a specific asset (identified by ID and type). This promotes consistency and simplifies permission checking in various handlers, including those retrieving asset metadata (like `get_metric_handler`, `get_collection_handler`) and those executing actions based on assets (like the handler running metric SQL queries). ## 2. Problem Statement -Currently, permission checking logic might be duplicated or slightly varied across different handlers (e.g., `get_collection_handler`, `get_dashboard_handler`, `get_metric_handler`). Checking permissions for assets *contained within* other assets requires a standardized approach that considers direct user permissions, organization roles, and potentially public access settings of the specific asset. +Currently, permission checking logic might be duplicated or slightly varied across different handlers (e.g., `get_collection_handler`, `get_dashboard_handler`, `get_metric_handler`, and potentially data execution handlers). Checking permissions for assets *contained within* other assets, or *before executing* an action related to an asset, requires a standardized, testable approach that considers direct user permissions, organization roles, and potentially public access settings of the specific asset. ## 3. Goals -- Create a reusable function `check_specific_asset_access` within `libs/sharing`. +- Create a reusable, modular, and easily testable function `check_specific_asset_access` within `libs/sharing`. - This function should accept user context, asset details (ID, type, org ID), and required permission levels. - It should return `Ok(true)` if the user meets the requirements, `Ok(false)` if they don't, and `Err` only for database or unexpected errors. -- Consolidate permission checking logic for specific assets into this helper. +- Consolidate permission checking logic for specific assets into this single helper, callable from multiple contexts (metadata retrieval, action execution). ## 4. Non-Goals @@ -56,7 +56,37 @@ Currently, permission checking logic might be duplicated or slightly varied acro asset_organization_id: Uuid, required_roles: &[AssetPermissionRole], ) -> Result { - // --- 1. Check Direct Permissions --- + // --- 1. Check High-Level Organization Permissions First --- + // Check if the user is WorkspaceAdmin or DataAdmin in the asset's organization + let high_level_org_roles = [ + database::enums::UserOrganizationRole::WorkspaceAdmin, + database::enums::UserOrganizationRole::DataAdmin, + ]; + + let has_high_level_org_role = select(exists( + users_to_organizations::table + .filter(users_to_organizations::user_id.eq(&user.id)) + .filter(users_to_organizations::organization_id.eq(asset_organization_id)) + .filter(users_to_organizations::deleted_at.is_null()) + .filter(users_to_organizations::status.eq(database::enums::UserOrganizationStatus::Active)) + .filter(users_to_organizations::role.eq_any(high_level_org_roles)) + )) + .get_result::(conn) + .await; + + match has_high_level_org_role { + Ok(true) => return Ok(true), // User is Org Admin/Data Admin, grant access + Ok(false) => { /* Continue to check direct permissions */ } + Err(e) => { + tracing::error!( + "DB error checking high-level organization permissions for user {} in org {}: {}", + user.id, asset_organization_id, e + ); + return Err(anyhow!("Failed to check organization permissions: {}", e)); + } + } + + // --- 2. Check Direct Permissions --- let direct_permission_exists = select(exists( asset_permissions::table .filter(asset_permissions::identity_id.eq(&user.id)) @@ -82,44 +112,41 @@ Currently, permission checking logic might be duplicated or slightly varied acro } } - // --- 2. Check Organization Permissions --- - // Check if the user is part of the asset's organization AND has an Admin/Owner role - // (Adjust roles based on specific requirements - maybe Members also get view?) - let org_roles_to_check = [ - database::enums::UserOrganizationRole::Admin, - database::enums::UserOrganizationRole::Owner, - // Add database::enums::UserOrganizationRole::Member if members should have view access - ]; + // --- 3. Check Other Organization Permissions (e.g., Member role granting CanView) --- + // If specific roles like Member should grant CanView, add that check here. + // This check is only relevant if high-level admin check and direct permission check failed. + // Example (If Member grants CanView): + /* + if required_roles.contains(&AssetPermissionRole::CanView) { + let is_org_member = select(exists( + users_to_organizations::table + .filter(users_to_organizations::user_id.eq(&user.id)) + .filter(users_to_organizations::organization_id.eq(asset_organization_id)) + .filter(users_to_organizations::deleted_at.is_null()) + .filter(users_to_organizations::status.eq(database::enums::UserOrganizationStatus::Active)) + .filter(users_to_organizations::role.eq(database::enums::UserOrganizationRole::Member)) // Check for Member + )) + .get_result::(conn) + .await; - let sufficient_org_role_exists = select(exists( - users_to_organizations::table - .filter(users_to_organizations::user_id.eq(&user.id)) - .filter(users_to_organizations::organization_id.eq(asset_organization_id)) - .filter(users_to_organizations::deleted_at.is_null()) - .filter(users_to_organizations::status.eq(database::enums::UserOrganizationStatus::Active)) - .filter(users_to_organizations::role.eq_any(org_roles_to_check)) - // We implicitly assume org roles grant at least 'CanView' equivalent. - // If finer control is needed, we might need a mapping from OrgRole -> AssetPermissionRole. - // For now, if any required_role is <= CanView and user has sufficient org role, grant access. - // This simplifies logic: if user needs CanView/Edit/FullAccess/Owner and is Org Admin/Owner, they likely have it. - )) - // Only check org permissions if required_roles includes something an org admin might have (e.g., CanView) - .filter(required_roles.iter().any(|r| *r <= AssetPermissionRole::CanView)) - .get_result::(conn) - .await; + match is_org_member { + Ok(true) => return Ok(true), // Org Member grants CanView + Ok(false) => { /* Continue */ } + Err(e) => { + tracing::error!( + "DB error checking Member organization permissions for asset {} type {:?} in org {}: {}", + asset_id, asset_type, asset_organization_id, e + ); + // Don't return Err here, as failure to check Member role shouldn't block access + // if other permissions might exist (though they were already checked). + // Fall through to return false. + } + } + } + */ - - match sufficient_org_role_exists { - Ok(true) => Ok(true), // Found sufficient org permission - Ok(false) => Ok(false), // No sufficient direct or org permission found - Err(e) => { - tracing::error!( - "DB error checking organization permissions for asset {} type {:?} in org {}: {}", - asset_id, asset_type, asset_organization_id, e - ); - Err(anyhow!("Failed to check organization asset permissions: {}", e)) - } - } + // If none of the above checks granted access, return false + Ok(false) } ``` 3. **File Changes:** @@ -142,7 +169,7 @@ Currently, permission checking logic might be duplicated or slightly varied acro - User has direct `Owner` permission -> returns `Ok(true)` when `CanView` or `Owner` is required. - User has direct `CanView` permission -> returns `Ok(false)` when `Owner` is required. - User has no direct permission but Org Admin role -> returns `Ok(true)` when `CanView` is required. - - User has no direct permission and Org Member role -> returns `Ok(false)` (unless Member is added to `org_roles_to_check`). + - User has no direct permission and Org Member role -> returns `Ok(false)` (unless Member role check is added and CanView is required). - User has no relevant direct or org permission -> returns `Ok(false)`. - Database error during direct permission check -> returns `Err`. - Database error during org permission check -> returns `Err`. diff --git a/api/server/src/routes/rest/routes/chats/duplicate_chat.rs b/api/server/src/routes/rest/routes/chats/duplicate_chat.rs index e6a46b897..d3da978af 100644 --- a/api/server/src/routes/rest/routes/chats/duplicate_chat.rs +++ b/api/server/src/routes/rest/routes/chats/duplicate_chat.rs @@ -98,7 +98,7 @@ mod tests { let org_id = Uuid::new_v4(); let organizations = vec![OrganizationMembership { id: org_id, - role: UserOrganizationRole::Owner, + role: UserOrganizationRole::WorkspaceAdmin, }]; let auth_user = AuthenticatedUser { @@ -203,6 +203,7 @@ mod tests { id: Uuid::new_v4(), message_id: *message_id, file_id, + version_number: 1, created_at: now, updated_at: now, deleted_at: None, diff --git a/api/server/src/routes/rest/routes/dashboards/get_dashboard_test.rs b/api/server/src/routes/rest/routes/dashboards/get_dashboard_test.rs deleted file mode 100644 index 9df2bb169..000000000 --- a/api/server/src/routes/rest/routes/dashboards/get_dashboard_test.rs +++ /dev/null @@ -1,111 +0,0 @@ -#[cfg(test)] -mod tests { - use anyhow::Result; - use axum::extract::{Extension, Path, Query}; - use axum::http::StatusCode; - use uuid::Uuid; - use database::enums::{AssetPermissionRole, AssetType, UserOrganizationRole}; - use database::tests::common::db::TestSetup; - use database::tests::common::assets::AssetTestHelpers; - use database::tests::common::permissions::PermissionTestHelpers; - use middleware::AuthenticatedUser; - - use super::GetDashboardQueryParams; - use super::get_dashboard_rest_handler; - - #[tokio::test] - async fn test_dashboard_not_found() -> Result<()> { - // Setup test environment - let setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - - // Test with a non-existent dashboard ID - let non_existent_id = Uuid::new_v4(); - let params = GetDashboardQueryParams { version_number: None }; - - // Call the handler directly - let result = get_dashboard_rest_handler( - Extension(setup.user), - Path(non_existent_id), - Query(params) - ).await; - - // Verify we get the correct 404 status code - assert!(matches!(result, Err((StatusCode::NOT_FOUND, _))), - "Expected NOT_FOUND status code for non-existent dashboard"); - - setup.db.cleanup().await?; - Ok(()) - } - - #[tokio::test] - async fn test_dashboard_permission_denied() -> Result<()> { - // Setup test environment with two users - let admin_setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - let viewer_setup = TestSetup::new(Some(UserOrganizationRole::Viewer)).await?; - - // Admin creates a dashboard - let dashboard = AssetTestHelpers::create_test_dashboard(&admin_setup.db, "Test Dashboard").await?; - - // Set permission for the admin but not for the viewer - PermissionTestHelpers::create_user_permission( - &admin_setup.db, - dashboard.id, - AssetType::DashboardFile, - admin_setup.user.id, - AssetPermissionRole::Owner - ).await?; - - // Try to access with viewer (who doesn't have permission) - let params = GetDashboardQueryParams { version_number: None }; - - // Call the handler directly with viewer - let result = get_dashboard_rest_handler( - Extension(viewer_setup.user), - Path(dashboard.id), - Query(params) - ).await; - - // Verify we get the correct 403 status code - assert!(matches!(result, Err((StatusCode::FORBIDDEN, _))), - "Expected FORBIDDEN status code for unauthorized access"); - - admin_setup.db.cleanup().await?; - viewer_setup.db.cleanup().await?; - Ok(()) - } - - #[tokio::test] - async fn test_dashboard_version_not_found() -> Result<()> { - // Setup test environment - let setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - - // Create a dashboard - let dashboard = AssetTestHelpers::create_test_dashboard_with_permission( - &setup.db, - "Test Dashboard", - setup.user.id, - AssetPermissionRole::Owner - ).await?; - - // Try to access with a non-existent version - let params = GetDashboardQueryParams { version_number: Some(999) }; - - // Call the handler directly - let result = get_dashboard_rest_handler( - Extension(setup.user), - Path(dashboard.id), - Query(params) - ).await; - - // Verify we get the correct 404 status code - assert!(matches!(result, Err((StatusCode::NOT_FOUND, _))), - "Expected NOT_FOUND status code for non-existent version"); - - setup.db.cleanup().await?; - Ok(()) - } - - // Note: We can't easily test the public_password required case - // in unit tests since it requires configuring the dashboard with a public password, - // which would need additional setup. This would be better tested at the API level. -} \ No newline at end of file diff --git a/api/server/src/routes/rest/routes/dashboards/mod.rs b/api/server/src/routes/rest/routes/dashboards/mod.rs index eacd49f8a..9e7a4a801 100644 --- a/api/server/src/routes/rest/routes/dashboards/mod.rs +++ b/api/server/src/routes/rest/routes/dashboards/mod.rs @@ -8,8 +8,6 @@ use axum::{ mod create_dashboard; mod delete_dashboard; mod get_dashboard; -#[cfg(test)] -mod get_dashboard_test; mod list_dashboards; mod sharing; mod update_dashboard; diff --git a/api/server/src/routes/rest/routes/metrics/get_metric_test.rs b/api/server/src/routes/rest/routes/metrics/get_metric_test.rs deleted file mode 100644 index 1eb429c25..000000000 --- a/api/server/src/routes/rest/routes/metrics/get_metric_test.rs +++ /dev/null @@ -1,189 +0,0 @@ -#[cfg(test)] -mod tests { - use anyhow::Result; - use axum::extract::{Extension, Path, Query}; - use axum::http::StatusCode; - use uuid::Uuid; - use database::enums::{AssetPermissionRole, AssetType, UserOrganizationRole}; - use database::tests::common::db::TestSetup; - use database::tests::common::assets::AssetTestHelpers; - use database::tests::common::permissions::PermissionTestHelpers; - use middleware::AuthenticatedUser; - use diesel::prelude::*; - use diesel::ExpressionMethods; - - use super::GetMetricQueryParams; - use super::get_metric_rest_handler; - - #[tokio::test] - async fn test_metric_not_found() -> Result<()> { - // Setup test environment - let setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - - // Test with a non-existent metric ID - let non_existent_id = Uuid::new_v4(); - let params = GetMetricQueryParams { version_number: None }; - - // Call the handler directly - let result = get_metric_rest_handler( - Extension(setup.user), - Path(non_existent_id), - Query(params) - ).await; - - // Verify we get the correct 404 status code - assert!(matches!(result, Err((StatusCode::NOT_FOUND, _))), - "Expected NOT_FOUND status code for non-existent metric"); - - setup.db.cleanup().await?; - Ok(()) - } - - #[tokio::test] - async fn test_metric_permission_denied() -> Result<()> { - // Setup test environment with two users - let admin_setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - let viewer_setup = TestSetup::new(Some(UserOrganizationRole::Viewer)).await?; - - // Admin creates a metric - let metric = AssetTestHelpers::create_test_metric(&admin_setup.db, "Test Metric").await?; - - // Set permission for the admin but not for the viewer - PermissionTestHelpers::create_user_permission( - &admin_setup.db, - metric.id, - AssetType::MetricFile, - admin_setup.user.id, - AssetPermissionRole::Owner - ).await?; - - // Try to access with viewer (who doesn't have permission) - let params = GetMetricQueryParams { version_number: None }; - - // Call the handler directly with viewer - let result = get_metric_rest_handler( - Extension(viewer_setup.user), - Path(metric.id), - Query(params) - ).await; - - // Verify we get the correct 403 status code - assert!(matches!(result, Err((StatusCode::FORBIDDEN, _))), - "Expected FORBIDDEN status code for unauthorized access"); - - admin_setup.db.cleanup().await?; - viewer_setup.db.cleanup().await?; - Ok(()) - } - - #[tokio::test] - async fn test_metric_version_not_found() -> Result<()> { - // Setup test environment - let setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - - // Create a metric - let metric = AssetTestHelpers::create_test_metric_with_permission( - &setup.db, - "Test Metric", - setup.user.id, - AssetPermissionRole::Owner - ).await?; - - // Try to access with a non-existent version - let params = GetMetricQueryParams { version_number: Some(999) }; - - // Call the handler directly - let result = get_metric_rest_handler( - Extension(setup.user), - Path(metric.id), - Query(params) - ).await; - - // Verify we get the correct 404 status code - assert!(matches!(result, Err((StatusCode::NOT_FOUND, _))), - "Expected NOT_FOUND status code for non-existent version"); - - setup.db.cleanup().await?; - Ok(()) - } - - #[tokio::test] - async fn test_metric_password_required() -> Result<()> { - // Setup test environment - let setup = TestSetup::new(Some(UserOrganizationRole::WorkspaceAdmin)).await?; - - // Create a metric - let mut metric = AssetTestHelpers::create_test_metric_with_permission( - &setup.db, - "Password Protected Metric", - setup.user.id, - AssetPermissionRole::Owner - ).await?; - - // Create another user who won't have direct permission - let other_setup = TestSetup::new(Some(UserOrganizationRole::Viewer)).await?; - - // Make the metric public with password protection - let mut conn = setup.db.pool.get().await?; - diesel::update(database::schema::metric_files::table) - .filter(database::schema::metric_files::id.eq(metric.id)) - .set(( - database::schema::metric_files::publicly_accessible.eq(true), - database::schema::metric_files::publicly_enabled_by.eq(Some(setup.user.id)), - database::schema::metric_files::public_password.eq(Some("secret123".to_string())), - )) - .execute(&mut conn) - .await?; - - // Try to access without providing password - let params = GetMetricQueryParams { - version_number: None, - password: None - }; - - // Call the handler directly with the other user - let result = get_metric_rest_handler( - Extension(other_setup.user), - Path(metric.id), - Query(params) - ).await; - - // Verify we get the correct 418 IM_A_TEAPOT status code - assert!(matches!(result, Err((StatusCode::IM_A_TEAPOT, _))), - "Expected IM_A_TEAPOT status code for missing password"); - - // Try with wrong password - let params = GetMetricQueryParams { - version_number: None, - password: Some("wrong".to_string()) - }; - - let result = get_metric_rest_handler( - Extension(other_setup.user.clone()), - Path(metric.id), - Query(params) - ).await; - - // Should still get error - assert!(result.is_err()); - - // Now try with correct password - let params = GetMetricQueryParams { - version_number: None, - password: Some("secret123".to_string()) - }; - - let result = get_metric_rest_handler( - Extension(other_setup.user), - Path(metric.id), - Query(params) - ).await; - - // Should succeed - assert!(result.is_ok()); - - setup.db.cleanup().await?; - other_setup.db.cleanup().await?; - Ok(()) - } -} \ No newline at end of file diff --git a/api/server/src/routes/rest/routes/metrics/mod.rs b/api/server/src/routes/rest/routes/metrics/mod.rs index b5459062a..34f14d5e1 100644 --- a/api/server/src/routes/rest/routes/metrics/mod.rs +++ b/api/server/src/routes/rest/routes/metrics/mod.rs @@ -7,8 +7,6 @@ use axum::{ mod bulk_update_metrics; mod delete_metric; mod get_metric; -#[cfg(test)] -mod get_metric_test; mod get_metric_data; mod list_metrics; mod sharing; diff --git a/api/server/tests/metrics/bulk_update_metrics_test.rs b/api/server/tests/metrics/bulk_update_metrics_test.rs deleted file mode 100644 index 898cd943a..000000000 --- a/api/server/tests/metrics/bulk_update_metrics_test.rs +++ /dev/null @@ -1,261 +0,0 @@ -use anyhow::Result; -use axum::http::StatusCode; -use database::enums::{AssetPermissionRole, AssetType, UserOrganizationRole, Verification}; -use database::tests::common::assets::AssetTestHelpers; -use database::tests::common::db::DbTestHelpers; -use database::tests::common::permissions::PermissionTestHelpers; -use database::tests::common::users::UserTestHelpers; -use futures::future::try_join_all; -use handlers::metrics::{BulkUpdateMetricsRequest, BulkUpdateMetricsResponse, MetricStatusUpdate}; -use middleware::{AuthenticatedUser, OrganizationMembership}; -use uuid::Uuid; -use database::types::VersionHistory; -use diesel::prelude::*; -use diesel_async::RunQueryDsl; -use chrono::Utc; - -/// Test the bulk update endpoint with authorization -#[tokio::test] -async fn test_bulk_update_metrics_endpoint() -> Result<()> { - // Initialize test app with auth - let (app, test_db, _auth_token, user) = DbTestHelpers::init_test_app_with_auth().await?; - - // Create authenticated user with admin role - let _admin_authenticated_user = AuthenticatedUser { - id: user.id, - email: user.email.clone(), - name: user.name.clone(), - organizations: vec![OrganizationMembership { - id: test_db.organization_id, - role: UserOrganizationRole::WorkspaceAdmin, - }], - config: serde_json::Value::Null, - created_at: Utc::now(), - updated_at: Utc::now(), - attributes: serde_json::Value::Null, - avatar_url: None, - teams: Vec::new(), - }; - - // Create test metrics - let metric_count = 5; - let metric_ids = try_join_all((0..metric_count).map(|i| { - AssetTestHelpers::create_test_metric( - &test_db, - &format!("Test Metric {}", i), - Some(user.id), - Some(test_db.organization_id), - ) - })).await?; - - // Add permissions for the user - for metric_id in &metric_ids { - PermissionTestHelpers::create_permission( - &test_db, - *metric_id, - AssetType::MetricFile, - user.id, - AssetPermissionRole::Owner, - ).await?; - } - - // Create update request - let updates = metric_ids - .iter() - .map(|id| MetricStatusUpdate { - id: *id, - verification: Verification::Verified, - }) - .collect(); - - // The request is now just the vector of updates - let request: BulkUpdateMetricsRequest = updates; - - // Test successful update - let response = reqwest::Client::new() - .put(format!("{}/metrics", app.address)) - .header("Authorization", format!("Bearer {}", _auth_token)) - .json(&request) - .send() - .await?; - - assert_eq!(response.status(), StatusCode::OK); - - let body: BulkUpdateMetricsResponse = response.json().await?; - assert_eq!(body.success_count, metric_count); - assert_eq!(body.failure_count, 0); - assert!(body.failed_updates.is_empty()); - - // Verify database state - let mut conn = test_db.get_conn().await?; - for id in &metric_ids { - use database::schema::metric_files::dsl::*; - let metric_file = metric_files - .filter(database::schema::metric_files::id.eq(id)) - .first::(&mut conn) - .await?; - - assert_eq!(metric_file.verification, Verification::Verified); - } - - // Test unauthorized access - let other_user = UserTestHelpers::create_test_user(&test_db).await?; - let other_metric = AssetTestHelpers::create_test_metric( - &test_db, - "Other User's Metric", - Some(other_user.id), - Some(test_db.organization_id), - ).await?; - - // Try to update a mix of allowed and forbidden metrics - let request: BulkUpdateMetricsRequest = vec![ - MetricStatusUpdate { - id: metric_ids[0], - verification: Verification::InReview, - }, - MetricStatusUpdate { - id: other_metric, - verification: Verification::InReview, - }, - ]; - - let response = reqwest::Client::new() - .put(format!("{}/metrics", app.address)) - .header("Authorization", format!("Bearer {}", _auth_token)) - .json(&request) - .send() - .await?; - - assert_eq!(response.status(), StatusCode::OK); - - let body: BulkUpdateMetricsResponse = response.json().await?; - assert_eq!(body.success_count, 1, "Should only update the authorized metric"); - assert_eq!(body.failure_count, 1, "Should fail to update the unauthorized metric"); - assert_eq!(body.failed_updates.len(), 1); - assert_eq!(body.failed_updates[0].metric_id, other_metric); - assert_eq!(body.failed_updates[0].error_code, "PERMISSION_DENIED"); - - // Test with nonexistent metrics - let request: BulkUpdateMetricsRequest = vec![ - MetricStatusUpdate { - id: Uuid::new_v4(), - verification: Verification::Verified, - }, - ]; - - let response = reqwest::Client::new() - .put(format!("{}/metrics", app.address)) - .header("Authorization", format!("Bearer {}", _auth_token)) - .json(&request) - .send() - .await?; - - assert_eq!(response.status(), StatusCode::OK); - - let body: BulkUpdateMetricsResponse = response.json().await?; - assert_eq!(body.success_count, 0); - assert_eq!(body.failure_count, 1); - assert_eq!(body.failed_updates[0].error_code, "NOT_FOUND"); - - // Test with empty updates list - let request: BulkUpdateMetricsRequest = vec![]; - - let response = reqwest::Client::new() - .put(format!("{}/metrics", app.address)) - .header("Authorization", format!("Bearer {}", _auth_token)) - .json(&request) - .send() - .await?; - - assert_eq!(response.status(), StatusCode::BAD_REQUEST); - - // Cleanup - drop(conn); - test_db.cleanup().await?; - - Ok(()) -} - -/// Test for rate limiting of the bulk update endpoint -#[tokio::test] -async fn test_bulk_update_concurrency() -> Result<()> { - // Initialize test app with auth - let (app, test_db, _auth_token, user) = DbTestHelpers::init_test_app_with_auth().await?; - - // Create authenticated user with admin role - let _admin_authenticated_user = AuthenticatedUser { - id: user.id, - email: user.email.clone(), - name: user.name.clone(), - organizations: vec![OrganizationMembership { - id: test_db.organization_id, - role: UserOrganizationRole::WorkspaceAdmin, - }], - config: serde_json::Value::Null, - created_at: Utc::now(), - updated_at: Utc::now(), - attributes: serde_json::Value::Null, - avatar_url: None, - teams: Vec::new(), - }; - - // Create test metrics (a larger batch) - let metric_count = 25; - let metric_ids = try_join_all((0..metric_count).map(|i| { - AssetTestHelpers::create_test_metric( - &test_db, - &format!("Test Metric {}", i), - Some(user.id), - Some(test_db.organization_id), - ) - })).await?; - - // Add permissions for the user - for metric_id in &metric_ids { - PermissionTestHelpers::create_permission( - &test_db, - *metric_id, - AssetType::MetricFile, - user.id, - AssetPermissionRole::Owner, - ).await?; - } - - // Create update request - let updates = metric_ids - .iter() - .map(|id| MetricStatusUpdate { - id: *id, - verification: Verification::Verified, - }) - .collect(); - - // Test different batch sizes - REMOVED as batch size is not controllable via the request anymore. - // We just send the request once. - // let batch_sizes = vec![5, 10, 25]; - - // for batch_size in batch_sizes { - let request: BulkUpdateMetricsRequest = updates.clone(); - - let start = std::time::Instant::now(); - let response = reqwest::Client::new() - .put(format!("{}/metrics", app.address)) - .header("Authorization", format!("Bearer {}", _auth_token)) - .json(&request) - .send() - .await?; - let duration = start.elapsed(); - - assert_eq!(response.status(), StatusCode::OK); - - println!("Bulk update took {:?} for {} metrics (using default batching)", duration, metric_count); - - let body: BulkUpdateMetricsResponse = response.json().await?; - assert_eq!(body.success_count, metric_count); - // } - - // Cleanup - test_db.cleanup().await?; - - Ok(()) -} \ No newline at end of file diff --git a/api/server/tests/metrics/mod.rs b/api/server/tests/metrics/mod.rs deleted file mode 100644 index a0215b782..000000000 --- a/api/server/tests/metrics/mod.rs +++ /dev/null @@ -1 +0,0 @@ -mod bulk_update_metrics_test; \ No newline at end of file diff --git a/api/server/tests/mod.rs b/api/server/tests/mod.rs index 13fa3c58c..e69de29bb 100644 --- a/api/server/tests/mod.rs +++ b/api/server/tests/mod.rs @@ -1 +0,0 @@ -mod metrics; \ No newline at end of file