From 9ae9c2d5cce1ca2d3a370cd2b5f94bd3dfc4706d Mon Sep 17 00:00:00 2001 From: dal Date: Mon, 7 Apr 2025 16:36:53 -0600 Subject: [PATCH] api_public_sharing_fix --- .../chats/sharing/update_sharing_handler.rs | 30 +- .../tests/sharing/public_update_tests.rs | 414 +++--------------- .../routes/chats/sharing/update_sharing.rs | 25 +- 3 files changed, 102 insertions(+), 367 deletions(-) diff --git a/api/libs/handlers/src/chats/sharing/update_sharing_handler.rs b/api/libs/handlers/src/chats/sharing/update_sharing_handler.rs index eafcb44fb..deb2a1d73 100644 --- a/api/libs/handlers/src/chats/sharing/update_sharing_handler.rs +++ b/api/libs/handlers/src/chats/sharing/update_sharing_handler.rs @@ -6,7 +6,7 @@ use database::{ }; use middleware::AuthenticatedUser; use serde::{Deserialize, Serialize}; -use sharing::{check_permission_access, create_share_by_email}; +use sharing::{check_permission_access, create_share_by_email, types::UpdateField}; use tracing::info; use uuid::Uuid; @@ -24,10 +24,12 @@ pub struct UpdateChatSharingRequest { pub users: Option>, /// Whether the chat should be publicly accessible pub publicly_accessible: Option, - /// Password for public access (if null, will clear existing password) - pub public_password: Option>, - /// Expiration date for public access (if null, will clear existing expiration) - pub public_expiration: Option>>, + /// Password for public access + #[serde(default)] + pub public_password: UpdateField, + /// Expiration date for public access + #[serde(default)] + pub public_expiry_date: UpdateField>, } /// Updates sharing permissions for a chat @@ -107,8 +109,10 @@ pub async fn update_chat_sharing_handler( } } - // Note: Chat doesn't have password_secret_id in its database model - // If public_password becomes needed in the future, additional implementation will be required + // Note: Currently, chats don't support public sharing + // The public_* fields are included for API consistency but are ignored + // If public sharing for chats is implemented in the future, this section will need to be updated + // Following the pattern from metric_sharing_handler.rs and dashboard_sharing_handler.rs Ok(()) } @@ -148,8 +152,8 @@ mod tests { let request = UpdateChatSharingRequest { users: None, publicly_accessible: None, - public_password: None, - public_expiration: None, + public_password: UpdateField::NoChange, + public_expiry_date: UpdateField::NoChange, }; // Call handler - should fail because the chat doesn't exist @@ -191,8 +195,8 @@ mod tests { role: AssetPermissionRole::CanView, }]), publicly_accessible: None, - public_password: None, - public_expiration: None, + public_password: UpdateField::NoChange, + public_expiry_date: UpdateField::NoChange, }; // This test will fail in isolation as we can't easily mock the database @@ -228,8 +232,8 @@ mod tests { let request = UpdateChatSharingRequest { users: None, publicly_accessible: Some(true), - public_password: None, - public_expiration: Some(Some(Utc::now())), + public_password: UpdateField::NoChange, + public_expiry_date: UpdateField::Update(Utc::now()), }; // This test will fail in isolation as we can't easily mock the database diff --git a/api/libs/handlers/tests/sharing/public_update_tests.rs b/api/libs/handlers/tests/sharing/public_update_tests.rs index 208b00f14..5621efb9e 100644 --- a/api/libs/handlers/tests/sharing/public_update_tests.rs +++ b/api/libs/handlers/tests/sharing/public_update_tests.rs @@ -1,401 +1,129 @@ // Tests for public sharing parameter fixes -// This test file contains integration tests that require a database connection -// This test cannot be run directly through cargo test because it requires database fixtures -// It serves primarily as a reference implementation that would be run in a proper -// integration test environment where database test helpers are available - -// The test cases follow the specifications from the PRD and demonstrate how -// the UpdateField enum should be used in production code. - +// Integration tests for public sharing parameters use anyhow::Result; use chrono::{Duration, Utc}; -use database::enums::AssetPermissionRole; +use database::{ + enums::AssetPermissionRole, + helpers::{ + metric_files::fetch_metric_file_with_permissions, + dashboard_files::fetch_dashboard_file_with_permission, + }, + pool::get_pg_pool, +}; use handlers::metrics::sharing::{update_metric_sharing_handler, UpdateMetricSharingRequest, ShareRecipient as MetricShareRecipient}; use handlers::dashboards::sharing::{update_dashboard_sharing_handler, UpdateDashboardSharingRequest}; +use middleware::AuthenticatedUser; use sharing::types::UpdateField; +use uuid::Uuid; #[tokio::test] -#[ignore] +#[ignore = "Integration test requires database test infrastructure"] async fn test_metric_public_updates() -> Result<()> { - // NOTE: This would be used in integration tests with DB access - // let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?; - // let metric_id = AssetTestHelpers::create_test_metric( - // &setup.db, - // "Test Metric", - // setup.organization.id - // ).await?; + // This test verifies that public sharing updates work correctly for metrics + // It's marked as ignored since it requires the database test infrastructure - // For this sample test, we'll return Ok since we can't run the actual test - // The important part is showing how the UpdateField would be used - - // Example 1: Make public with password and expiry + // Test cases include: + // 1. Make public with password and expiry + // 2. Remove password but keep public + // 3. Make private + // 4. Clear all public settings + + // Since we're not running the actual test, just make sure the request structs work correctly let expiry_date = Utc::now() + Duration::days(7); - let request = UpdateMetricSharingRequest { + let _request = UpdateMetricSharingRequest { users: None, publicly_accessible: Some(true), public_password: UpdateField::Update("secretpass123".to_string()), public_expiry_date: UpdateField::Update(expiry_date), }; - - // This is how the request would be constructed for making a metric public - // with both password and expiry date - // In a real test we would: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Then verify the fields were updated properly - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(metric.metric_file.publicly_accessible); - // assert_eq!(metric.metric_file.publicly_enabled_by, Some(setup.user.id)); - // assert_eq!(metric.metric_file.public_password, Some("secretpass123".to_string())); - // assert!(metric.metric_file.public_expiry_date.is_some()); - - // Example 2: Remove password but keep public - let request = UpdateMetricSharingRequest { - users: None, - publicly_accessible: None, // Don't change - public_password: UpdateField::SetNull, // Remove password - public_expiry_date: UpdateField::NoChange, // Keep existing expiry - }; - - // This demonstrates removing only the password while keeping - // the metric public and preserving the expiry date - - // In a real test: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify only password was removed - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(metric.metric_file.publicly_accessible); - // assert_eq!(metric.metric_file.publicly_enabled_by, Some(setup.user.id)); - // assert_eq!(metric.metric_file.public_password, None); - // assert!(metric.metric_file.public_expiry_date.is_some()); - - // Example 3: Make private but keep other settings - let request = UpdateMetricSharingRequest { - users: None, - publicly_accessible: Some(false), // Make private - public_password: UpdateField::NoChange, // Keep password setting - public_expiry_date: UpdateField::NoChange, // Keep expiry setting - }; - - // This demonstrates making a metric private while keeping the password and expiry - // settings in the database (they won't be used while private but will be preserved) - - // In a real test: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify made private but kept other settings - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(!metric.metric_file.publicly_accessible); - // assert_eq!(metric.metric_file.publicly_enabled_by, None); - // assert_eq!(metric.metric_file.public_password, None); - // assert!(metric.metric_file.public_expiry_date.is_some()); - - // Example 4: Clear all public settings - let request = UpdateMetricSharingRequest { - users: None, - publicly_accessible: Some(false), // Make private - public_password: UpdateField::SetNull, // Clear password - public_expiry_date: UpdateField::SetNull, // Clear expiry - }; - - // This demonstrates completely clearing all public access settings - - // In a real test: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify all public settings cleared - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(!metric.metric_file.publicly_accessible); - // assert_eq!(metric.metric_file.publicly_enabled_by, None); - // assert_eq!(metric.metric_file.public_password, None); - // assert_eq!(metric.metric_file.public_expiry_date, None); - - // Since this is a mock test, just return Ok - Ok() + Ok(()) } #[tokio::test] -#[ignore] +#[ignore = "Integration test requires database test infrastructure"] async fn test_dashboard_public_updates() -> Result<()> { - // NOTE: This would be used in integration tests with DB access - // let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?; - // let dashboard_id = AssetTestHelpers::create_test_dashboard( - // &setup.db, - // "Test Dashboard", - // setup.organization.id - // ).await?; + // This test verifies that public sharing updates work correctly for dashboards + // It's marked as ignored since it requires the database test infrastructure - // For this sample test, we'll return Ok since we can't run the actual test - // The important part is showing how the UpdateField would be used - - // Example 1: Make public with expiry but no password + // Test cases include: + // 1. Make public with expiry but no password + // 2. Add password to public dashboard + // 3. Update expiry only + // 4. Make private but keep password and expiry + // 5. Clear all public settings + + // Since we're not running the actual test, just make sure the request structs work correctly let expiry_date = Utc::now() + Duration::days(7); - let request = UpdateDashboardSharingRequest { + let _request = UpdateDashboardSharingRequest { users: None, publicly_accessible: Some(true), // Make public public_password: UpdateField::NoChange, // Don't set password public_expiry_date: UpdateField::Update(expiry_date), // Set expiry }; - - // This demonstrates making a dashboard public with an expiry date but no password - // In a real test we would: - // let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify updates - // let dashboard = fetch_dashboard_file_with_permission(&dashboard_id, &setup.user.id).await?.unwrap(); - // assert!(dashboard.dashboard_file.publicly_accessible); - // assert_eq!(dashboard.dashboard_file.publicly_enabled_by, Some(setup.user.id)); - // assert_eq!(dashboard.dashboard_file.public_password, None); - // assert!(dashboard.dashboard_file.public_expiry_date.is_some()); - - // Example 2: Add password to public dashboard - let request = UpdateDashboardSharingRequest { - users: None, - publicly_accessible: None, // Don't change - public_password: UpdateField::Update("dashpass123".to_string()), // Set password - public_expiry_date: UpdateField::NoChange, // Keep existing expiry - }; - - // This demonstrates adding a password to an already public dashboard - - // In a real test we would: - // let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify password added - // let dashboard = fetch_dashboard_file_with_permission(&dashboard_id, &setup.user.id).await?.unwrap(); - // assert!(dashboard.dashboard_file.publicly_accessible); - // assert_eq!(dashboard.dashboard_file.publicly_enabled_by, Some(setup.user.id)); - // assert_eq!(dashboard.dashboard_file.public_password, Some("dashpass123".to_string())); - // assert!(dashboard.dashboard_file.public_expiry_date.is_some()); - - // Example 3: Update expiry only - let new_expiry = Utc::now() + Duration::days(14); - let request = UpdateDashboardSharingRequest { - users: None, - publicly_accessible: None, // Don't change - public_password: UpdateField::NoChange, // Don't change - public_expiry_date: UpdateField::Update(new_expiry), // Update expiry - }; - - // This demonstrates updating only the expiry date on a public dashboard - - // In a real test we would: - // let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify only expiry updated - // let dashboard = fetch_dashboard_file_with_permission(&dashboard_id, &setup.user.id).await?.unwrap(); - // assert!(dashboard.dashboard_file.publicly_accessible); - // assert_eq!(dashboard.dashboard_file.publicly_enabled_by, Some(setup.user.id)); - // assert_eq!(dashboard.dashboard_file.public_password, Some("dashpass123".to_string())); - // assert!(dashboard.dashboard_file.public_expiry_date.is_some()); - // - // // Timestamps might be slightly different due to database rounding - // // So we compare just the date parts - // if let Some(saved_date) = dashboard.dashboard_file.public_expiry_date { - // assert!(saved_date.date_naive() == new_expiry.date_naive()); - // } - - // Example 4: Make private but keep password and expiry - let request = UpdateDashboardSharingRequest { - users: None, - publicly_accessible: Some(false), // Make private - public_password: UpdateField::NoChange, // Don't change password - public_expiry_date: UpdateField::NoChange, // Don't change expiry - }; - - // This demonstrates making a dashboard private while preserving password and expiry - // settings for potential future re-enabling - - // In a real test we would: - // let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify made private but kept other settings - // let dashboard = fetch_dashboard_file_with_permission(&dashboard_id, &setup.user.id).await?.unwrap(); - // assert!(!dashboard.dashboard_file.publicly_accessible); - // assert_eq!(dashboard.dashboard_file.publicly_enabled_by, None); - // assert_eq!(dashboard.dashboard_file.public_password, Some("dashpass123".to_string())); - // assert!(dashboard.dashboard_file.public_expiry_date.is_some()); - - // Example 5: Clear all public settings - let request = UpdateDashboardSharingRequest { - users: None, - publicly_accessible: Some(false), // Make private - public_password: UpdateField::SetNull, // Clear password - public_expiry_date: UpdateField::SetNull, // Clear expiry - }; - - // COMMENTED OUT: let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await; - // COMMENTED OUT: assert!(result.is_ok(), "Failed to update dashboard: {:?}", result); - // COMMENTED OUT: - // COMMENTED OUT: // Verify all public settings cleared - // COMMENTED OUT: let dashboard = database::helpers::dashboard_files::fetch_dashboard_file_with_permission( - // COMMENTED OUT: &dashboard_id, &setup.user.id).await?.unwrap(); - // COMMENTED OUT: assert!(!dashboard.dashboard_file.publicly_accessible, "Dashboard should remain private"); - // COMMENTED OUT: assert_eq!(dashboard.dashboard_file.publicly_enabled_by, None, "User ID should remain cleared"); - // COMMENTED OUT: assert_eq!(dashboard.dashboard_file.public_password, None, "Password should be cleared"); - // COMMENTED OUT: assert_eq!(dashboard.dashboard_file.public_expiry_date, None, "Expiry date should be cleared"); - // COMMENTED OUT: Ok(()) } #[tokio::test] -#[ignore] +#[ignore = "Integration test requires database test infrastructure"] async fn test_public_update_edge_cases() -> Result<()> { - // NOTE: This would be used in integration tests with DB access - // let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?; - // let metric_id = AssetTestHelpers::create_test_metric( - // &setup.db, - // "Test Metric", - // setup.organization.id - // ).await?; + // This test verifies edge cases for public sharing updates + // It's marked as ignored since it requires the database test infrastructure - // For this sample test, we'll return Ok since we can't run the actual test - // The important part is showing how the UpdateField would be used - - // Example 1: Handle validation error - expired date - let request = UpdateMetricSharingRequest { + // Test cases include: + // 1. Make public with expired date (should fail) + // 2. Update with empty password (should fail) + + // Since we're not running the actual test, just make sure the request structs work correctly + let _past_date_request = UpdateMetricSharingRequest { users: None, publicly_accessible: Some(true), public_password: UpdateField::NoChange, public_expiry_date: UpdateField::Update(Utc::now() - Duration::days(1)), // Past date }; - - // This demonstrates validation handling for expired dates - // The handler will validate that expiry dates are in the future - // In a real test we would: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_err()); - // let error = result.unwrap_err().to_string(); - // assert!(error.contains("expiry date must be in the future")); - - // Example 2: Handle validation error - empty password - let request = UpdateMetricSharingRequest { + let _empty_password_request = UpdateMetricSharingRequest { users: None, publicly_accessible: Some(true), public_password: UpdateField::Update("".to_string()), // Empty password public_expiry_date: UpdateField::NoChange, }; - - // This demonstrates validation handling for empty passwords - // The handler will validate that passwords are not empty - // In a real test we would: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_err()); - // let error = result.unwrap_err().to_string(); - // assert!(error.contains("password cannot be empty")); - - // Example 3: Concurrent updates - // This would demonstrate handling concurrent updates - - // In a real test: - // First make it public - // let setup_request = UpdateMetricSharingRequest { - // users: None, - // publicly_accessible: Some(true), - // public_password: UpdateField::NoChange, - // public_expiry_date: UpdateField::NoChange, - // }; - // let _ = update_metric_sharing_handler(&metric_id, &setup.user, setup_request).await?; - - // Now concurrent updates with different passwords - let request1 = UpdateMetricSharingRequest { - users: None, - publicly_accessible: Some(true), // Keep public - public_password: UpdateField::Update("pass1".to_string()), // Set password 1 - public_expiry_date: UpdateField::NoChange, - }; - - let request2 = UpdateMetricSharingRequest { - users: None, - publicly_accessible: Some(true), // Keep public - public_password: UpdateField::Update("pass2".to_string()), // Set password 2 - public_expiry_date: UpdateField::NoChange, - }; - - // In a real test we would: - // let (result1, result2) = tokio::join!( - // update_metric_sharing_handler(&metric_id, &setup.user, request1), - // update_metric_sharing_handler(&metric_id, &setup.user, request2) - // ); - // - // // At least one should succeed - // assert!(result1.is_ok() || result2.is_ok()); - // - // // Verify final state (last write wins) - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(metric.metric_file.publicly_accessible); - // assert!( - // metric.metric_file.public_password == Some("pass1".to_string()) || - // metric.metric_file.public_password == Some("pass2".to_string()) - // ); - Ok(()) } #[tokio::test] -#[ignore] +#[ignore = "Integration test requires database test infrastructure"] async fn test_public_update_permissions() -> Result<()> { - // This test would check that users with insufficient permissions - // cannot update public sharing settings - // - // In a real test: - // // Test with viewer role (should not be able to update public settings) - // let setup = TestSetup::new(Some(UserOrganizationRole::Viewer)).await?; - // - // // Create test metric (viewer cannot create, so admin creates it) - // let admin_setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?; - // let metric_id = AssetTestHelpers::create_test_metric( - // &admin_setup.db, - // "Test Metric", - // setup.organization.id - // ).await?; - - // Example of trying to make public with viewer account (insufficient permission) - let request = UpdateMetricSharingRequest { + // This test verifies permission checks for public sharing updates + // It's marked as ignored since it requires the database test infrastructure + + // Test cases include: + // 1. Try to make public with viewer account (should fail) + + // Since we're not running the actual test, just make sure the request structs work correctly + let _request = UpdateMetricSharingRequest { users: None, publicly_accessible: Some(true), // Attempt to make public public_password: UpdateField::NoChange, public_expiry_date: UpdateField::NoChange, }; - - // In a real test we would: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_err()); - // let error = result.unwrap_err().to_string(); - // assert!(error.contains("permission") || error.contains("not found")); - + Ok(()) } #[tokio::test] -#[ignore] +#[ignore = "Integration test requires database test infrastructure"] async fn test_sharing_with_users() -> Result<()> { - // This test would demonstrate sharing with users while also updating public settings - // - // In a real test: - // let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?; - // - // let metric_id = AssetTestHelpers::create_test_metric( - // &setup.db, - // "Test Metric", - // setup.organization.id - // ).await?; - - // Example of sharing with users while also updating public settings - let request = UpdateMetricSharingRequest { + // This test verifies that sharing with users and public settings can be updated together + // It's marked as ignored since it requires the database test infrastructure + + // Test cases include: + // 1. Share with users while also updating public settings + + // Since we're not running the actual test, just make sure the request structs work correctly + let _request = UpdateMetricSharingRequest { users: Some(vec![ MetricShareRecipient { email: "test-user1@example.com".to_string(), @@ -410,20 +138,6 @@ async fn test_sharing_with_users() -> Result<()> { public_password: UpdateField::Update("shared-password".to_string()), // With password public_expiry_date: UpdateField::Update(Utc::now() + Duration::days(30)), // And expiry }; - - // This demonstrates that user sharing and public settings can be updated in a single call - - // In a real test we would: - // let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await; - // assert!(result.is_ok()); - // - // // Verify public settings are updated - // let metric = fetch_metric_file_with_permissions(&metric_id, &setup.user.id).await?.unwrap(); - // assert!(metric.metric_file.publicly_accessible); - // assert_eq!(metric.metric_file.public_password, Some("shared-password".to_string())); - // assert!(metric.metric_file.public_expiry_date.is_some()); - // - // // And verify user shares were created (using the list_shares functionality) Ok(()) } \ No newline at end of file diff --git a/api/server/src/routes/rest/routes/chats/sharing/update_sharing.rs b/api/server/src/routes/rest/routes/chats/sharing/update_sharing.rs index 5695cad0c..d08f8aa8d 100644 --- a/api/server/src/routes/rest/routes/chats/sharing/update_sharing.rs +++ b/api/server/src/routes/rest/routes/chats/sharing/update_sharing.rs @@ -18,15 +18,32 @@ use uuid::Uuid; /// "users": [ /// { /// "email": "user@example.com", -/// "role": "Viewer" +/// "role": "CanView" /// } /// ], /// "publicly_accessible": true, -/// "public_password": "password", -/// "public_expiration": "2023-12-31T23:59:59Z" +/// "public_password": { +/// "update": "password" +/// }, +/// "public_expiry_date": { +/// "update": "2023-12-31T23:59:59Z" +/// } /// } /// ``` -/// All fields are optional. If a field is not provided, it won't be updated. +/// All fields are optional. Field update options: +/// - `users`: List of users to share with. If omitted, existing shares are not changed. +/// - `publicly_accessible`: Boolean flag. If `true`, asset becomes public; if `false`, asset becomes private. +/// - `public_password`: Object with one of these values: +/// - `"no_change"`: Keep existing password (default if field omitted) +/// - `"set_null"`: Remove existing password +/// - `{"update": "new_password"}`: Set a new password +/// - `public_expiry_date`: Object with one of these values: +/// - `"no_change"`: Keep existing expiry date (default if field omitted) +/// - `"set_null"`: Remove existing expiry date +/// - `{"update": "2023-12-31T23:59:59Z"}`: Set a new expiry date (ISO-8601 format) +/// +/// Note: Currently, chats don't support public sharing. The `public_*` fields are included for API consistency +/// but are ignored in the implementation. ///