buster/api/prds/active/api_public_sharing_fix.md

title	author	date	status	parent_prd	ticket
Public Sharing Parameters Fix	Claude	2024-04-07	Draft	project_bug_fixes_and_testing.md	BUS-1064

Public Sharing Parameters Fix

Problem Statement

Current behavior:

• Public sharing parameters using Option<Option<T>> are confusing and error-prone
• Diesel treats None as "don't update" vs Some(None) as "update to null"
• No clear distinction between "remove value" and "don't change value"
• Inconsistent handling between metrics and dashboards

Expected behavior:

• Clear enum for handling null vs none cases
• Consistent parameter handling across assets
• Proper diesel changeset updates
• Type-safe handling of public sharing fields

Technical Design

Parameter Types

/// Represents a field that can be either kept unchanged, set to null, or updated with a value
#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum UpdateField<T> {
    NoChange,
    SetNull,
    Update(T),
}

/// Request for updating sharing settings
#[derive(Debug, Serialize, Deserialize)]
pub struct UpdateSharingRequest {
    /// List of users to share with
    pub users: Option<Vec<ShareRecipient>>,
    /// Whether the asset should be publicly accessible
    pub publicly_accessible: Option<bool>,
    /// Password for public access
    pub public_password: UpdateField<String>,
    /// Expiration date for public access
    pub public_expiry_date: UpdateField<DateTime<Utc>>,
}

impl<T> UpdateField<T> {
    /// Converts the UpdateField into an Option<Option<T>> for diesel
    pub fn into_option(self) -> Option<Option<T>> {
        match self {
            UpdateField::NoChange => None,
            UpdateField::SetNull => Some(None),
            UpdateField::Update(value) => Some(Some(value)),
        }
    }
}

Handler Update

pub async fn update_sharing_handler(
    asset_id: &Uuid,
    user: &AuthenticatedUser,
    request: UpdateSharingRequest,
) -> Result<()> {
    // ... permission checks ...

    let mut changeset = diesel::update(dsl::asset_files)
        .filter(dsl::id.eq(asset_id));

    // Only include fields in changeset if they should be updated
    if let Some(publicly_accessible) = request.publicly_accessible {
        changeset = changeset.set(dsl::publicly_accessible.eq(publicly_accessible));
        
        // Update publicly_enabled_by based on publicly_accessible
        let enabled_by = if publicly_accessible {
            Some(user.id)
        } else {
            None
        };
        changeset = changeset.set(dsl::publicly_enabled_by.eq(enabled_by));
    }

    // Handle public_password using UpdateField
    match request.public_password {
        UpdateField::Update(password) => {
            changeset = changeset.set(dsl::public_password.eq(Some(password)));
        }
        UpdateField::SetNull => {
            changeset = changeset.set(dsl::public_password.eq(None::<String>));
        }
        UpdateField::NoChange => {}
    }

    // Handle public_expiry_date using UpdateField
    match request.public_expiry_date {
        UpdateField::Update(date) => {
            changeset = changeset.set(dsl::public_expiry_date.eq(Some(date)));
        }
        UpdateField::SetNull => {
            changeset = changeset.set(dsl::public_expiry_date.eq(None::<DateTime<Utc>>));
        }
        UpdateField::NoChange => {}
    }

    // Execute the update
    changeset.execute(&mut conn).await?;

    Ok(())
}

Test Cases

// libs/handlers/tests/sharing/public_update_tests.rs

use chrono::{Duration, Utc};
use database::tests::common::{TestSetup, UserOrganizationRole};
use database::tests::common::assets::AssetTestHelpers;

#[tokio::test]
async fn test_metric_public_updates() -> Result<()> {
    let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?;
    
    // Create test metric
    let metric_id = AssetTestHelpers::create_test_metric(
        &setup.db,
        "Test Metric",
        setup.organization.id
    ).await?;

    // Test 1: Make public with password and expiry
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::Update("secretpass123".to_string()),
        public_expiry_date: UpdateField::Update(Utc::now() + Duration::days(7)),
    };

    let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify updates
    let metric = fetch_metric_with_permission(&metric_id, &setup.user.id).await?;
    assert!(metric.publicly_accessible);
    assert_eq!(metric.publicly_enabled_by, Some(setup.user.id));
    assert_eq!(metric.public_password, Some("secretpass123".to_string()));
    assert!(metric.public_expiry_date.is_some());

    // Test 2: Remove password but keep public
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: None, // Don't change
        public_password: UpdateField::SetNull,
        public_expiry_date: UpdateField::NoChange,
    };

    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_with_permission(&metric_id, &setup.user.id).await?;
    assert!(metric.publicly_accessible); // Still public
    assert_eq!(metric.publicly_enabled_by, Some(setup.user.id));
    assert_eq!(metric.public_password, None); // Password removed
    assert!(metric.public_expiry_date.is_some()); // Expiry unchanged

    // Test 3: Make private
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(false),
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::NoChange,
    };

    let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify made private
    let metric = fetch_metric_with_permission(&metric_id, &setup.user.id).await?;
    assert!(!metric.publicly_accessible);
    assert_eq!(metric.publicly_enabled_by, None);
    assert_eq!(metric.public_password, None);
    assert!(metric.public_expiry_date.is_some()); // Unchanged since NoChange

    // Test 4: Clear all public settings
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(false),
        public_password: UpdateField::SetNull,
        public_expiry_date: UpdateField::SetNull,
    };

    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_with_permission(&metric_id, &setup.user.id).await?;
    assert!(!metric.publicly_accessible);
    assert_eq!(metric.publicly_enabled_by, None);
    assert_eq!(metric.public_password, None);
    assert_eq!(metric.public_expiry_date, None);

    Ok(())
}

#[tokio::test]
async fn test_dashboard_public_updates() -> Result<()> {
    let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?;
    
    // Create test dashboard
    let dashboard_id = AssetTestHelpers::create_test_dashboard(
        &setup.db,
        "Test Dashboard",
        setup.organization.id
    ).await?;

    // Test 1: Make public with expiry but no password
    let request = UpdateDashboardSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::Update(Utc::now() + Duration::days(7)),
    };

    let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify updates
    let dashboard = fetch_dashboard_with_permission(&dashboard_id, &setup.user.id).await?;
    assert!(dashboard.publicly_accessible);
    assert_eq!(dashboard.publicly_enabled_by, Some(setup.user.id));
    assert_eq!(dashboard.public_password, None);
    assert!(dashboard.public_expiry_date.is_some());

    // Test 2: Add password to public dashboard
    let request = UpdateDashboardSharingRequest {
        users: None,
        publicly_accessible: None, // Don't change
        public_password: UpdateField::Update("dashpass123".to_string()),
        public_expiry_date: UpdateField::NoChange,
    };

    let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify password added
    let dashboard = fetch_dashboard_with_permission(&dashboard_id, &setup.user.id).await?;
    assert!(dashboard.publicly_accessible); // Still public
    assert_eq!(dashboard.publicly_enabled_by, Some(setup.user.id));
    assert_eq!(dashboard.public_password, Some("dashpass123".to_string()));
    assert!(dashboard.public_expiry_date.is_some()); // Unchanged

    // Test 3: Update expiry only
    let new_expiry = Utc::now() + Duration::days(14);
    let request = UpdateDashboardSharingRequest {
        users: None,
        publicly_accessible: None,
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::Update(new_expiry),
    };

    let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify only expiry updated
    let dashboard = fetch_dashboard_with_permission(&dashboard_id, &setup.user.id).await?;
    assert!(dashboard.publicly_accessible); // Unchanged
    assert_eq!(dashboard.publicly_enabled_by, Some(setup.user.id));
    assert_eq!(dashboard.public_password, Some("dashpass123".to_string())); // Unchanged
    assert!(dashboard.public_expiry_date.unwrap().timestamp() == new_expiry.timestamp());

    // Test 4: Make private but keep password and expiry
    let request = UpdateDashboardSharingRequest {
        users: None,
        publicly_accessible: Some(false),
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::NoChange,
    };

    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_with_permission(&dashboard_id, &setup.user.id).await?;
    assert!(!dashboard.publicly_accessible);
    assert_eq!(dashboard.publicly_enabled_by, None);
    assert_eq!(dashboard.public_password, Some("dashpass123".to_string())); // Unchanged
    assert!(dashboard.public_expiry_date.is_some()); // Unchanged

    // Test 5: Clear all public settings
    let request = UpdateDashboardSharingRequest {
        users: None,
        publicly_accessible: Some(false),
        public_password: UpdateField::SetNull,
        public_expiry_date: UpdateField::SetNull,
    };

    let result = update_dashboard_sharing_handler(&dashboard_id, &setup.user, request).await;
    assert!(result.is_ok());

    // Verify all public settings cleared
    let dashboard = fetch_dashboard_with_permission(&dashboard_id, &setup.user.id).await?;
    assert!(!dashboard.publicly_accessible);
    assert_eq!(dashboard.publicly_enabled_by, None);
    assert_eq!(dashboard.public_password, None);
    assert_eq!(dashboard.public_expiry_date, None);

    Ok(())
}

#[tokio::test]
async fn test_public_update_edge_cases() -> Result<()> {
    let setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?;
    
    // Create test metric
    let metric_id = AssetTestHelpers::create_test_metric(
        &setup.db,
        "Test Metric",
        setup.organization.id
    ).await?;

    // Test 1: Make public with expired date (should fail)
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::Update(Utc::now() - Duration::days(1)),
    };

    let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await;
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("expiry date must be in the future"));

    // Test 2: Update with empty password (should fail)
    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::Update("".to_string()),
        public_expiry_date: UpdateField::NoChange,
    };

    let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await;
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("password cannot be empty"));

    // Test 3: Concurrent updates
    let concurrent_setup = TestSetup::new(Some(UserOrganizationRole::Admin)).await?;

    let request1 = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::Update("pass1".to_string()),
        public_expiry_date: UpdateField::NoChange,
    };

    let request2 = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::Update("pass2".to_string()),
        public_expiry_date: UpdateField::NoChange,
    };

    let (result1, result2) = tokio::join!(
        update_metric_sharing_handler(&metric_id, &setup.user, request1),
        update_metric_sharing_handler(&metric_id, &concurrent_setup.user, request2)
    );

    assert!(result1.is_ok() || result2.is_ok()); // At least one should succeed
    
    // Verify final state (last write wins)
    let metric = fetch_metric_with_permission(&metric_id, &setup.user.id).await?;
    assert!(metric.publicly_accessible);
    assert!(metric.public_password == Some("pass1".to_string()) || 
           metric.public_password == Some("pass2".to_string()));

    Ok(())
}

#[tokio::test]
async fn test_public_update_permissions() -> Result<()> {
    // Test with viewer role (should not be able to update public settings)
    let setup = TestSetup::new(Some(UserOrganizationRole::Viewer)).await?;
    
    let metric_id = AssetTestHelpers::create_test_metric(
        &setup.db,
        "Test Metric",
        setup.organization.id
    ).await?;

    let request = UpdateMetricSharingRequest {
        users: None,
        publicly_accessible: Some(true),
        public_password: UpdateField::NoChange,
        public_expiry_date: UpdateField::NoChange,
    };

    let result = update_metric_sharing_handler(&metric_id, &setup.user, request).await;
    assert!(result.is_err());
    assert!(result.unwrap_err().to_string().contains("permission"));

    Ok(())
}

These tests cover:

1. Metric Public Updates:

   • Making public with password and expiry
   • Removing password while keeping public
   • Making private
   • Clearing all public settings

2. Dashboard Public Updates:

   • Making public with expiry only
   • Adding password to public dashboard
   • Updating expiry only
   • Making private while keeping settings
   • Clearing all public settings

3. Edge Cases:

   • Expired dates
   • Empty passwords
   • Concurrent updates
   • Permission checks

Each test verifies:

• Correct field updates
• Field preservation when using NoChange
• Proper null handling with SetNull
• Proper enabled_by updates
• Permission enforcement

Would you like me to implement any additional test cases or modify the existing ones?

Implementation Plan

Phase 1: Type Updates

1. Add UpdateField enum
2. Update sharing request structs
3. Add helper methods for diesel conversion

Phase 2: Handler Updates

1. Update metric sharing handler
2. Update dashboard sharing handler
3. Add changeset logic
4. Update tests

Phase 3: REST Updates

1. Update REST handlers for new types
2. Add request validation
3. Update error handling

Success Criteria

1. Clear distinction between null and no-change cases
2. Proper handling of public sharing fields
3. Consistent behavior across assets
4. All tests passing

References

Related Files

• libs/handlers/src/metrics/sharing/update_sharing_handler.rs
• libs/handlers/src/dashboards/sharing/update_sharing_handler.rs
• server/src/routes/rest/routes/metrics/sharing/update_sharing.rs
• server/src/routes/rest/routes/dashboards/sharing/update_sharing.rs

Appendix

Related Files

• libs/handlers/src/sharing/validate.rs
• libs/handlers/src/sharing/update_sharing.rs
• libs/handlers/tests/sharing/sharing_params_test.rs
• libs/models/src/sharing.rs

Sharing Parameter Reference

Valid parameter combinations:

• Public sharing must allow anonymous access
• Public sharing limited to read-only access
• Expiration date must be in the future
• Non-public sharing can have any access level