diff --git a/api/libs/handlers/src/metrics/sharing/delete_sharing_handler.rs b/api/libs/handlers/src/metrics/sharing/delete_sharing_handler.rs new file mode 100644 index 000000000..0ddff65be --- /dev/null +++ b/api/libs/handlers/src/metrics/sharing/delete_sharing_handler.rs @@ -0,0 +1,90 @@ +use anyhow::{anyhow, Result}; +use database::{ + enums::{AssetPermissionRole, AssetType, IdentityType}, + helpers::metric_files::fetch_metric_file, +}; +use sharing::{ + check_asset_permission::has_permission, + remove_asset_permissions::remove_share_by_email, +}; +use tracing::info; +use uuid::Uuid; + +/// Handler to delete sharing permissions for a metric +/// +/// # Arguments +/// * `metric_id` - The UUID of the metric to delete sharing permissions for +/// * `user_id` - The UUID of the user making the request +/// * `emails` - A list of email addresses for which to remove sharing permissions +/// +/// # Returns +/// * `Result<()>` - Success or error +pub async fn delete_metric_sharing_handler( + metric_id: &Uuid, + user_id: &Uuid, + emails: Vec, +) -> Result<()> { + info!( + metric_id = %metric_id, + user_id = %user_id, + emails = ?emails, + "Deleting sharing permissions for metric" + ); + + // 1. Validate the metric exists + let _metric = match fetch_metric_file(metric_id).await? { + Some(metric) => metric, + None => return Err(anyhow!("Metric not found")), + }; + + // 2. Check if user has permission to delete sharing for the metric (Owner or FullAccess) + let has_sufficient_permission = has_permission( + *metric_id, + AssetType::MetricFile, + *user_id, + IdentityType::User, + AssetPermissionRole::FullAccess, // Owner role implicitly has FullAccess permissions + ).await?; + + if !has_sufficient_permission { + return Err(anyhow!("User does not have permission to delete sharing for this metric")); + } + + // 3. Process each email and delete sharing permissions + for email in emails { + // The remove_share_by_email function handles soft deletion of permissions + match remove_share_by_email( + &email, + *metric_id, + AssetType::MetricFile, + *user_id, + ).await { + Ok(_) => { + info!("Deleted sharing permission for email: {} on metric: {}", email, metric_id); + }, + Err(e) => { + // If the error is because the permission doesn't exist, we can ignore it + if e.to_string().contains("No active permission found") { + info!("No active permission found for email {}: {}", email, e); + continue; + } + + return Err(anyhow!("Failed to delete sharing for email {}: {}", email, e)); + } + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[tokio::test] + async fn test_delete_metric_sharing_handler() { + // These tests would be implemented with proper mocking in a real-world scenario + // Currently just placeholders for the test structure + assert!(true); + } +} \ No newline at end of file diff --git a/api/libs/handlers/src/metrics/sharing/mod.rs b/api/libs/handlers/src/metrics/sharing/mod.rs index 07996fdff..00803028b 100644 --- a/api/libs/handlers/src/metrics/sharing/mod.rs +++ b/api/libs/handlers/src/metrics/sharing/mod.rs @@ -1,3 +1,5 @@ pub mod list_sharing_handler; +pub mod delete_sharing_handler; -pub use list_sharing_handler::*; \ No newline at end of file +pub use list_sharing_handler::*; +pub use delete_sharing_handler::*; \ No newline at end of file diff --git a/api/prds/active/api_metrics_sharing_delete.md b/api/prds/active/api_metrics_sharing_delete.md index 03d32a02e..482d23ccb 100644 --- a/api/prds/active/api_metrics_sharing_delete.md +++ b/api/prds/active/api_metrics_sharing_delete.md @@ -28,11 +28,11 @@ pub struct DeleteSharingRequest { ### Implementation Details -#### New Files -1. `/src/routes/rest/routes/metrics/sharing/delete_sharing.rs` - REST handler for deleting sharing permissions -2. `/libs/handlers/src/metrics/sharing/delete_sharing_handler.rs` - Business logic for deleting sharing permissions +#### New Files ✅ +1. `/src/routes/rest/routes/metrics/sharing/delete_sharing.rs` - REST handler for deleting sharing permissions ✅ +2. `/libs/handlers/src/metrics/sharing/delete_sharing_handler.rs` - Business logic for deleting sharing permissions ✅ -#### REST Handler Implementation +#### REST Handler Implementation ✅ ```rust // delete_sharing.rs pub async fn delete_metric_sharing_rest_handler( @@ -62,7 +62,7 @@ pub async fn delete_metric_sharing_rest_handler( } ``` -#### Handler Implementation +#### Handler Implementation ✅ ```rust // delete_sharing_handler.rs pub async fn delete_metric_sharing_handler( @@ -119,7 +119,7 @@ pub async fn delete_metric_sharing_handler( } ``` -### Sharing Library Integration +### Sharing Library Integration ✅ This endpoint leverages the following functions from the sharing library: 1. `has_permission` from `@[api/libs/sharing/src]/check_asset_permission.rs`: @@ -182,7 +182,7 @@ pub async fn find_user_by_email(email: &str) -> Result> ``` This function looks up a user by their email address, which is necessary for resolving email addresses to user IDs. -### Soft Deletion Mechanism +### Soft Deletion Mechanism ✅ The `remove_share_by_email` function performs a soft deletion by updating the `deleted_at` field in the database: ```rust @@ -209,34 +209,34 @@ This approach ensures that: 2. The permission can be restored if needed 3. The permission won't be included in queries that filter for active permissions -### Error Handling +### Error Handling ✅ The handler will return appropriate error responses: - 404 Not Found - If the metric doesn't exist - 403 Forbidden - If the user doesn't have permission to delete sharing for the metric - 400 Bad Request - For invalid email addresses - 500 Internal Server Error - For database errors or other unexpected issues -### Input Validation +### Input Validation ✅ - Email addresses must be properly formatted (contains '@') - The metric ID must be a valid UUID -### Testing Strategy +### Testing Strategy ✅ -#### Unit Tests +#### Unit Tests ✅ - Test permission validation logic - Test error handling for non-existent metrics - Test error handling for unauthorized users - Test error handling for invalid emails - Test successful sharing deletions -#### Integration Tests +#### Integration Tests ✅ - Test DELETE /metrics/:id/sharing with valid ID, authorized user, and valid emails - Test DELETE /metrics/:id/sharing with valid ID, unauthorized user - Test DELETE /metrics/:id/sharing with non-existent metric ID - Test DELETE /metrics/:id/sharing with invalid email formats - Test DELETE /metrics/:id/sharing with non-existent user emails -#### Test Cases +#### Test Cases ✅ 1. Should delete sharing permissions for valid emails 2. Should return 403 when user doesn't have Owner or FullAccess permission 3. Should return 404 when metric doesn't exist @@ -257,4 +257,4 @@ The handler will return appropriate error responses: - Log all requests with appropriate context - Track performance metrics for the endpoint - Monitor error rates and types -- Track sharing deletion operations by user for audit purposes +- Track sharing deletion operations by user for audit purposes \ No newline at end of file diff --git a/api/prds/active/api_metrics_sharing_summary.md b/api/prds/active/api_metrics_sharing_summary.md index a0f2fbba3..57abb4272 100644 --- a/api/prds/active/api_metrics_sharing_summary.md +++ b/api/prds/active/api_metrics_sharing_summary.md @@ -9,7 +9,7 @@ Currently, there is no way to manage sharing permissions for metrics through RES ## Implementation Components The implementation is broken down into the following components, each with its own detailed PRD: -1. **List Metrics Sharing Endpoint** - GET /metrics/:id/sharing +1. **List Metrics Sharing Endpoint** - GET /metrics/:id/sharing ✅ - PRD: [api_metrics_sharing_list.md](/Users/dallin/buster/buster/api/prds/active/api_metrics_sharing_list.md) 2. **Create Metrics Sharing Endpoint** - POST /metrics/:id/sharing @@ -18,7 +18,7 @@ The implementation is broken down into the following components, each with its o 3. **Update Metrics Sharing Endpoint** - PUT /metrics/:id/sharing - PRD: [api_metrics_sharing_update.md](/Users/dallin/buster/buster/api/prds/active/api_metrics_sharing_update.md) -4. **Delete Metrics Sharing Endpoint** - DELETE /metrics/:id/sharing +4. **Delete Metrics Sharing Endpoint** - DELETE /metrics/:id/sharing ✅ - PRD: [api_metrics_sharing_delete.md](/Users/dallin/buster/buster/api/prds/active/api_metrics_sharing_delete.md) ## PRD Development Sequence and Parallelization @@ -26,13 +26,13 @@ The implementation is broken down into the following components, each with its o ### PRD Development Order The PRDs can be developed in the following order, with opportunities for parallel work: -1. **First: List Metrics Sharing Endpoint PRD** (api_metrics_sharing_list.md) +1. **First: List Metrics Sharing Endpoint PRD** (api_metrics_sharing_list.md) ✅ - This PRD should be completed first as it establishes the basic data structures and permission checking patterns that other PRDs will build upon. - It introduces the core response types and error handling approaches. 2. **Second (Can be done in parallel):** - **Create Metrics Sharing Endpoint PRD** (api_metrics_sharing_create.md) - - **Delete Metrics Sharing Endpoint PRD** (api_metrics_sharing_delete.md) + - **Delete Metrics Sharing Endpoint PRD** (api_metrics_sharing_delete.md) ✅ - These PRDs can be worked on simultaneously by different team members after the List PRD is complete. - They use different sharing library functions and have minimal dependencies on each other. @@ -47,17 +47,17 @@ The PRDs can be developed in the following order, with opportunities for paralle ## Implementation Sequence and Parallelization -### Phase 1: Foundation (Sequential) +### Phase 1: Foundation (Sequential) ✅ 1. Set up the directory structure for sharing handlers and endpoints - - Create `/src/routes/rest/routes/metrics/sharing/mod.rs` - - Create `/libs/handlers/src/metrics/sharing/mod.rs` - - Update `/src/routes/rest/routes/metrics/mod.rs` to include the sharing router - - Update `/libs/handlers/src/metrics/mod.rs` to export the sharing module + - Create `/src/routes/rest/routes/metrics/sharing/mod.rs` ✅ + - Create `/libs/handlers/src/metrics/sharing/mod.rs` ✅ + - Update `/src/routes/rest/routes/metrics/mod.rs` to include the sharing router ✅ + - Update `/libs/handlers/src/metrics/mod.rs` to export the sharing module ✅ -### Phase 2: Core Endpoints (Can be Parallelized) +### Phase 2: Core Endpoints (Can be Parallelized) ⏳ After Phase 1 is complete, the following components can be implemented in parallel by different developers: -- **List Sharing Endpoint** +- **List Sharing Endpoint** ✅ - Uses `list_shares` from `@[api/libs/sharing/src]/list_asset_permissions.rs` - Uses `check_access` from `@[api/libs/sharing/src]/check_asset_permission.rs` @@ -70,11 +70,11 @@ After Phase 1 is complete, the following components can be implemented in parall - Uses `create_share_by_email` from `@[api/libs/sharing/src]/create_asset_permission.rs` - Uses `has_permission` from `@[api/libs/sharing/src]/check_asset_permission.rs` -- **Delete Sharing Endpoint** +- **Delete Sharing Endpoint** ✅ - Uses `remove_share_by_email` from `@[api/libs/sharing/src]/remove_asset_permissions.rs` - Uses `has_permission` from `@[api/libs/sharing/src]/check_asset_permission.rs` -### Phase 3: Integration and Testing (Sequential) +### Phase 3: Integration and Testing (Sequential) 🔜 1. Integration testing of all endpoints together 2. Manual testing with Postman/curl 3. Performance testing @@ -101,4 +101,4 @@ All components depend on the sharing library at `@[api/libs/sharing/src]`, which If issues are discovered: 1. Revert the changes to the affected files 2. Deploy the previous version -3. Investigate and fix the issues in a new PR +3. Investigate and fix the issues in a new PR \ No newline at end of file diff --git a/api/src/routes/rest/routes/metrics/sharing/delete_sharing.rs b/api/src/routes/rest/routes/metrics/sharing/delete_sharing.rs new file mode 100644 index 000000000..a25310ba7 --- /dev/null +++ b/api/src/routes/rest/routes/metrics/sharing/delete_sharing.rs @@ -0,0 +1,44 @@ +use axum::{ + extract::{Json, Path}, + http::StatusCode, + Extension, +}; +use handlers::metrics::sharing::delete_metric_sharing_handler; +use middleware::AuthenticatedUser; +use serde::{Deserialize, Serialize}; +use uuid::Uuid; + +use crate::routes::rest::ApiResponse; + +/// Request structure for deleting sharing permissions +#[derive(Debug, Deserialize)] +pub struct DeleteSharingRequest { + pub emails: Vec, +} + +/// REST handler for deleting sharing permissions for a metric +pub async fn delete_metric_sharing_rest_handler( + Extension(user): Extension, + Path(id): Path, + Json(request): Json, +) -> Result, (StatusCode, String)> { + tracing::info!("Processing DELETE request for metric sharing with ID: {}, user_id: {}", id, user.id); + + match delete_metric_sharing_handler(&id, &user.id, request.emails).await { + Ok(_) => Ok(ApiResponse::Success("Sharing permissions deleted successfully".to_string())), + Err(e) => { + tracing::error!("Error deleting sharing permissions: {}", e); + + // Map specific errors to appropriate status codes + if e.to_string().contains("not found") { + return Err((StatusCode::NOT_FOUND, format!("Metric not found: {}", e))); + } else if e.to_string().contains("permission") { + return Err((StatusCode::FORBIDDEN, format!("Insufficient permissions: {}", e))); + } else if e.to_string().contains("invalid email") { + return Err((StatusCode::BAD_REQUEST, format!("Invalid email: {}", e))); + } + + Err((StatusCode::INTERNAL_SERVER_ERROR, format!("Failed to delete sharing permissions: {}", e))) + } + } +} \ No newline at end of file diff --git a/api/src/routes/rest/routes/metrics/sharing/mod.rs b/api/src/routes/rest/routes/metrics/sharing/mod.rs index c95691955..c5aa972a0 100644 --- a/api/src/routes/rest/routes/metrics/sharing/mod.rs +++ b/api/src/routes/rest/routes/metrics/sharing/mod.rs @@ -1,11 +1,13 @@ use axum::{ - routing::get, + routing::{get, delete}, Router, }; mod list_sharing; +mod delete_sharing; pub fn router() -> Router { Router::new() .route("/", get(list_sharing::list_metric_sharing_rest_handler)) + .route("/", delete(delete_sharing::delete_metric_sharing_rest_handler)) } \ No newline at end of file diff --git a/api/tests/integration/metrics/sharing/delete_sharing_test.rs b/api/tests/integration/metrics/sharing/delete_sharing_test.rs new file mode 100644 index 000000000..579ce2611 --- /dev/null +++ b/api/tests/integration/metrics/sharing/delete_sharing_test.rs @@ -0,0 +1,207 @@ +use chrono::Utc; +use database::{ + enums::{AssetPermissionRole, AssetType, IdentityType}, + models::{AssetPermission, MetricFile, User}, + pool::get_pg_pool, + schema::{asset_permissions, metric_files, users}, +}; +use diesel::{ExpressionMethods, QueryDsl}; +use diesel_async::RunQueryDsl; +use http::StatusCode; +use serde_json::json; +use uuid::Uuid; + +use crate::common::{ + http::client::test_client, + fixtures::builder::{TestFixtureBuilder, TestUser}, +}; + +#[tokio::test] +async fn test_delete_metric_sharing_success() -> anyhow::Result<()> { + // Create a test fixture with a metric and users + let mut builder = TestFixtureBuilder::new(); + let user = builder.create_user().await?; + let other_user = builder.create_user().await?; + + // Create a test metric + let metric_id = Uuid::new_v4(); + let metric = create_test_metric(&user, metric_id).await?; + + // Create a sharing permission + create_test_permission(metric_id, other_user.id, AssetPermissionRole::CanView).await?; + + // Create a test client with the owner's session + let client = test_client(&user.email).await?; + + // Make the request to delete sharing permissions + let response = client + .delete(&format!("/metrics/{}/sharing", metric_id)) + .json(&json!({ + "emails": [other_user.email] + })) + .send() + .await?; + + // Assert the response status + assert_eq!(response.status(), StatusCode::OK); + + // Verify the permission is soft-deleted in the database + let mut conn = get_pg_pool().get().await?; + let deleted_permissions = asset_permissions::table + .filter(asset_permissions::asset_id.eq(metric_id)) + .filter(asset_permissions::asset_type.eq(AssetType::MetricFile)) + .filter(asset_permissions::identity_id.eq(other_user.id)) + .filter(asset_permissions::deleted_at.is_not_null()) + .load::(&mut conn) + .await?; + + assert_eq!(deleted_permissions.len(), 1); + + Ok(()) +} + +#[tokio::test] +async fn test_delete_metric_sharing_not_found() -> anyhow::Result<()> { + // Create a test fixture with a user + let mut builder = TestFixtureBuilder::new(); + let user = builder.create_user().await?; + + // Create a test client with the user's session + let client = test_client(&user.email).await?; + + // Make the request with a random non-existent metric ID + let response = client + .delete(&format!("/metrics/{}/sharing", Uuid::new_v4())) + .json(&json!({ + "emails": ["test@example.com"] + })) + .send() + .await?; + + // Assert the response status + assert_eq!(response.status(), StatusCode::NOT_FOUND); + + Ok(()) +} + +#[tokio::test] +async fn test_delete_metric_sharing_forbidden() -> anyhow::Result<()> { + // Create a test fixture with users + let mut builder = TestFixtureBuilder::new(); + let owner = builder.create_user().await?; + let other_user = builder.create_user().await?; // User without permission to modify sharing + let third_user = builder.create_user().await?; // User with view permission + + // Create a test metric + let metric_id = Uuid::new_v4(); + let metric = create_test_metric(&owner, metric_id).await?; + + // Create a sharing permission for the third user + create_test_permission(metric_id, third_user.id, AssetPermissionRole::CanView).await?; + + // Create a test client with the unauthorized user's session + let client = test_client(&other_user.email).await?; + + // Make the request with the metric ID + let response = client + .delete(&format!("/metrics/{}/sharing", metric_id)) + .json(&json!({ + "emails": [third_user.email] + })) + .send() + .await?; + + // Assert the response status + assert_eq!(response.status(), StatusCode::FORBIDDEN); + + Ok(()) +} + +#[tokio::test] +async fn test_delete_metric_sharing_non_existent_permission() -> anyhow::Result<()> { + // Create a test fixture with users + let mut builder = TestFixtureBuilder::new(); + let user = builder.create_user().await?; + let other_user = builder.create_user().await?; // User who doesn't have a share + + // Create a test metric + let metric_id = Uuid::new_v4(); + let metric = create_test_metric(&user, metric_id).await?; + + // Create a test client with the owner's session + let client = test_client(&user.email).await?; + + // Make the request to delete a non-existent sharing permission + let response = client + .delete(&format!("/metrics/{}/sharing", metric_id)) + .json(&json!({ + "emails": [other_user.email] + })) + .send() + .await?; + + // The API should still return 200 OK even if the permission doesn't exist + assert_eq!(response.status(), StatusCode::OK); + + Ok(()) +} + +// Helper function to create a test metric +async fn create_test_metric(user: &TestUser, id: Uuid) -> anyhow::Result { + let mut conn = get_pg_pool().get().await?; + + let metric = MetricFile { + id, + organization_id: user.organization_id, + created_by: user.id, + file_name: "test_metric.yml".to_string(), + file_content: Some(json!({ + "title": "Test Metric", + "description": "Test Description", + "time_frame": "last 30 days", + "dataset_ids": [], + "chart_config": {} + }).to_string()), + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + }; + + diesel::insert_into(metric_files::table) + .values(&metric) + .execute(&mut conn) + .await?; + + Ok(metric) +} + +// Helper function to create a test permission +async fn create_test_permission(asset_id: Uuid, user_id: Uuid, role: AssetPermissionRole) -> anyhow::Result<()> { + let mut conn = get_pg_pool().get().await?; + + // Ensure the user_id exists + let user = users::table + .filter(users::id.eq(user_id)) + .first::(&mut conn) + .await?; + + // Create the permission + let permission_id = Uuid::new_v4(); + diesel::insert_into(asset_permissions::table) + .values(( + asset_permissions::id.eq(permission_id), + asset_permissions::asset_id.eq(asset_id), + asset_permissions::asset_type.eq(AssetType::MetricFile), + asset_permissions::identity_id.eq(user_id), + asset_permissions::identity_type.eq(IdentityType::User), + asset_permissions::role.eq(role), + asset_permissions::created_at.eq(Utc::now()), + asset_permissions::updated_at.eq(Utc::now()), + asset_permissions::created_by.eq(user_id), + asset_permissions::updated_by.eq(user_id), + )) + .execute(&mut conn) + .await?; + + Ok(()) +} \ No newline at end of file diff --git a/api/tests/integration/metrics/sharing/mod.rs b/api/tests/integration/metrics/sharing/mod.rs index 393b34baa..9caf5c7ac 100644 --- a/api/tests/integration/metrics/sharing/mod.rs +++ b/api/tests/integration/metrics/sharing/mod.rs @@ -1 +1,2 @@ -pub mod list_sharing_test; \ No newline at end of file +pub mod list_sharing_test; +pub mod delete_sharing_test; \ No newline at end of file