diff --git a/api/libs/database/src/helpers/metric_files.rs b/api/libs/database/src/helpers/metric_files.rs index 13ca34911..a7e770684 100644 --- a/api/libs/database/src/helpers/metric_files.rs +++ b/api/libs/database/src/helpers/metric_files.rs @@ -180,14 +180,22 @@ pub async fn fetch_metric_file_with_permissions( // Determine effective permission (prioritizing collection over direct) // Dashboard permission is NOT considered here for the base permission level. // The handler should check dashboard access separately if direct/collection/public checks fail. - let effective_permission = match collection_permission { + let mut effective_permission = match collection_permission { Some(collection) => Some(collection), None => direct_permission, }; + // Ensure at least CanView if user has access via any dashboard containing this metric + if let Some(dashboard_view_permission) = dashboard_permission { + effective_permission = match effective_permission { + Some(current_role) => Some(current_role.max(dashboard_view_permission)), // Use max to ensure CanView is minimum + None => Some(dashboard_view_permission), // Grant CanView if no other permission exists + }; + } + Ok(Some(MetricFileWithPermission { metric_file, - permission: effective_permission, // Now only reflects direct or collection permission + permission: effective_permission, // Now reflects Direct/Collection/Dashboard logic })) } @@ -301,10 +309,10 @@ pub async fn fetch_metric_files_with_permissions( // Ensure at least CanView if user has access via any dashboard containing this metric if let Some(dashboard_view_permission) = dashboard_permission { - effective_permission = match effective_permission { - Some(current_role) => Some(current_role.max(dashboard_view_permission)), // Use max to ensure CanView is minimum - None => Some(dashboard_view_permission), // Grant CanView if no other permission exists - }; + effective_permission = match effective_permission { + Some(current_role) => Some(current_role.max(dashboard_view_permission)), // Use max to ensure CanView is minimum + None => Some(dashboard_view_permission), // Grant CanView if no other permission exists + }; } // Check if the file is publicly accessible and its expiry date hasn't passed diff --git a/api/prds/active/enhancement_collection_asset_permissions.md b/api/prds/active/enhancement_collection_asset_permissions.md index 973e4b6c5..5d94d4e9d 100644 --- a/api/prds/active/enhancement_collection_asset_permissions.md +++ b/api/prds/active/enhancement_collection_asset_permissions.md @@ -50,7 +50,7 @@ The current `get_collection_handler` lists assets associated with a collection b ``` 2. **Handler Modification (`get_collection_handler.rs`):** - - **Fetch Assets & Permissions Efficiently:** Instead of fetching assets and then checking permissions iteratively, adapt or create a batch fetch helper (similar to `fetch_metric_files_with_permissions`) that returns assets along with their pre-calculated direct/collection permission level (which internally handles `deleted_at`). Let's call the hypothetical result type `FetchedAssetWithPermission { asset: AssetQueryResult, base_permission: Option }`. The `AssetQueryResult` needs to include `id`, `name`, `asset_type`, `organization_id`, and creator info. + - **Fetch Assets & Permissions Efficiently:** This is the primary **addition**. Adapt or create a batch fetch helper (similar to `fetch_metric_files_with_permissions`) that returns assets (`AssetQueryResult` including `id`, `name`, `asset_type`, `organization_id`, etc.) along with their pre-calculated `base_permission: Option` (derived from direct/collection checks, handling `deleted_at`). ```rust // Example Query Result Struct needed #[derive(Queryable, Clone, Debug)] @@ -72,16 +72,9 @@ The current `get_collection_handler` lists assets associated with a collection b } // Fetch assets and their base permissions efficiently - // let fetched_assets_with_perms: Vec = /* ... */ ; - // This might involve joining collections_to_assets with metric_files/dashboard_files - // and LEFT JOINING asset_permissions twice (once for direct, once for collection via collections_to_assets) - // or using a helper function. + // let fetched_assets_with_perms: Vec = /* Call new/adapted helper */ ; ``` - - **Determine Final Access:** Iterate through `fetched_assets_with_perms`. For each item: - - Check if the user is a `WorkspaceAdmin` or `DataAdmin` for the `asset.organization_id` using the cached `user.organizations`. - - If they are an admin, `has_access` is `true`. - - If not an admin, check if `item.base_permission` (the pre-fetched direct/collection permission) is `Some` and meets the minimum requirement (e.g., `CanView`). If yes, `has_access` is `true`. - - Otherwise, `has_access` is `false`. + - **Determine Final Access:** This is the second main **addition**. Iterate through `fetched_assets_with_perms`. For each item, determine the final `has_access` flag by checking: (1) if the user is an Org Admin for the asset's org (using cached `user.organizations`), or (2) if the `item.base_permission` meets the `CanView` requirement. ```rust let mut final_assets: Vec = Vec::new(); let required_role = AssetPermissionRole::CanView; // Minimum requirement @@ -106,11 +99,10 @@ The current `get_collection_handler` lists assets associated with a collection b base_permission.map_or(false, |role| role >= required_role) }; - // Construct minimal or full object based on has_access - // For collections, we might always show the basic info + // Construct the CollectionAsset object final_assets.push(CollectionAsset { id: asset.id, - name: asset.name, // Assuming name is okay to show + name: asset.name, created_by: AssetUser { /* ... from asset ... */ }, created_at: asset.created_at, updated_at: asset.updated_at, @@ -118,20 +110,13 @@ The current `get_collection_handler` lists assets associated with a collection b has_access, // Set the final flag }); } - - // Use final_assets in the response - let collection_state = CollectionState { - // ... other fields ... - assets: Some(final_assets), - // ... - }; ``` - - **Populate Response:** Use the resulting list containing `CollectionAsset` objects (each with the correctly determined `has_access` flag) in the final `CollectionState` response. + - **Populate Response:** Use the `final_assets` list (now containing the `has_access` flag) in the final `CollectionState` response. The existing logic for fetching the collection itself and checking its permission remains unchanged. 3. **File Changes:** - - Modify: `libs/handlers/src/collections/get_collection_handler.rs` - - Modify: `libs/handlers/src/collections/types.rs` - - Potentially Modify/Create: A helper function in `libs/database/src/helpers/` for the efficient batch fetching of assets with permissions. + - Modify: `libs/handlers/src/collections/get_collection_handler.rs` (to add fetch call and access determination loop). + - Modify: `libs/handlers/src/collections/types.rs` (to add `has_access`). + - Potentially Modify/Create: Helper function in `libs/database/src/helpers/` for efficient batch asset+permission fetching for collections. ## 6. Implementation Plan @@ -145,17 +130,9 @@ The current `get_collection_handler` lists assets associated with a collection b - **Unit Tests:** Not directly applicable to the handler itself, focus on integration tests. Test modifications to `format_assets` logic if separated. - **Integration Tests:** - - Setup: User, Collection, Metric A (user has CanView), Dashboard B (user has no permission), Metric C (doesn't exist). - - Execute `get_collection_handler`. - - Verify: - - Response status is OK. - - `collection_state.assets` contains representations for Metric A and Dashboard B. - - Metric A has `has_access: true`. - - Dashboard B has `has_access: false`. - - Metric C is not present. - - Basic details (id, name, type) are present for both A and B. - - Test variations with different user roles (Owner, Org Admin, Member, No Access). - - Test scenario where `check_specific_asset_access` returns an `Err` (e.g., simulate DB failure during check) -> Verify asset is omitted or marked inaccessible based on error handling decision. + - Setup: Use test helpers like `TestDb` and `TestSetup` (defined in `libs/database/tests/common` or similar) to create an isolated test environment with a User, Collection, and associated assets (Metrics, Dashboards). + - Grant specific permissions using direct `asset_permissions` entries for the test user and assets. + - Example Scenario: User, Collection, Metric A (user has CanView), Dashboard B (user has no permission), Metric C (doesn't exist). ## 8. Rollback Plan diff --git a/api/prds/active/enhancement_dashboard_metric_permissions.md b/api/prds/active/enhancement_dashboard_metric_permissions.md index 5de39cebf..c96c266a9 100644 --- a/api/prds/active/enhancement_dashboard_metric_permissions.md +++ b/api/prds/active/enhancement_dashboard_metric_permissions.md @@ -32,6 +32,8 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ **Option A: Modify `get_metric_handler` (Recommended)** +This approach minimizes disruption by leveraging existing efficient helper functions. + 1. **Type Modification:** - 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 @@ -86,7 +88,7 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ ``` 2. **Modify `get_metric_handler.rs`:** - - **Initial Fetch & Check:** Use the existing efficient `fetch_metric_file_with_permissions` helper. This helper already fetches the `MetricFile` and its calculated `base_permission` (direct or collection, handling `deleted_at`). + - **Leverage Existing Fetch & Check:** Continue using the existing efficient `fetch_metric_file_with_permissions` helper. This fetches the `MetricFile` and its calculated `base_permission` (direct/collection). The subsequent logic using `check_permission_access` (or similar checks for org admin roles and public access) also remains largely the same for *determining* if access should be granted. ```rust // Inside get_metric_handler... let metric_id = /* get metric_id */; @@ -107,7 +109,7 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ let metric_file = metric_file_with_permission.metric_file; let base_permission = metric_file_with_permission.permission; // Direct or Collection permission ``` - - **Determine Final Access:** Check cached org roles and potentially public access rules (as the handler already does) to determine the final `has_access` status. + - **Determine Final Access & Modify Return:** The primary change is *after* access is determined (using existing logic involving `base_permission`, cached `user.organizations` admin roles, and public access checks). Instead of returning `Err` on access denial, calculate a final `has_access: bool` flag and conditionally construct the response. ```rust // Still inside get_metric_handler... @@ -163,7 +165,7 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ }; Ok(full_metric) } else { - // ... (Construct MINIMAL BusterMetric as defined previously) ... + // ... (Construct MINIMAL BusterMetric, setting has_access: false, permission: appropriate_indicator) ... let minimal_metric = BusterMetric { id: metric_file.id, name: metric_file.name, @@ -175,10 +177,10 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ Ok(minimal_metric) } ``` - - **Return Value:** Returns `Ok(BusterMetric)` containing either the full or minimal representation based on the final `has_access` decision. Only returns `Err` on fetch failures or other unrecoverable errors. + - **Return Value:** The key change is that the handler returns `Ok(BusterMetric)` in both access granted (full object) and access denied (minimal object) scenarios for existing metrics. `Err` is reserved for actual fetch errors (metric not found, DB issues). 3. **Modify `get_dashboard_handler.rs`:** - - The logic using `join_all` on `get_metric_handler` calls remains the same. The results will now consistently be `Ok(BusterMetric)`, with the `has_access` flag indicating accessibility. The filtering logic correctly handles potential `Err` results from fetch failures. + - Requires minimal-to-no change. It continues to call `get_metric_handler`. Since `get_metric_handler` now consistently returns `Ok` for existing metrics (with `has_access` indicating permission), the dashboard handler correctly includes all configured metrics in its response map. **Option B: Handle in `get_dashboard_handler` (Less Recommended)** @@ -203,23 +205,12 @@ Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_ ## 7. Testing Strategy - **Unit Tests (`get_metric_handler`):** - - Mock DB interactions. - - Test case: User lacks `CanView` -> returns `Ok(BusterMetric { id, name, has_access: false, ... })`. - - Test case: User has `CanView` -> returns `Ok(BusterMetric { has_access: true, ...full details... })`. - - Test case: Metric ID not found -> returns `Err`. - - Test case: DB error during permission check -> returns `Err`. - - Test case: DB error during full data fetch (when permitted) -> returns `Err`. + - Mock DB interactions (e.g., the return value of `fetch_metric_file_with_permissions`) using `mockall` or similar if the fetch logic is abstracted behind a trait, or by carefully constructing mock `MetricFileWithPermission` objects. + - Mock the `AuthenticatedUser` object with different organization roles. - **Integration Tests (`get_dashboard_handler`):** - - Setup: User, Dashboard, Metric A (user has CanView), Metric B (user has no permission), Metric C (ID in dashboard config but doesn't exist). - - Execute `get_dashboard_handler`. - - Verify: - - Response status is OK. - - `response.metrics` map contains keys for Metric A and Metric B. - - `response.metrics[&MetricA_ID]` has `has_access: true` and full details. - - `response.metrics[&MetricB_ID]` has `has_access: false` and minimal details (id, name). - - `response.metrics` does not contain a key for Metric C. - - An error related to Metric C failing to load should be logged. - - Test variations with different user roles and public dashboard access. + - Setup: Use test helpers like `TestDb` and `TestSetup` to create an isolated test environment with a User, Dashboard, and associated Metrics. + - Grant specific permissions using direct `asset_permissions` entries for the test user and metrics. + - Example Scenario: User, Dashboard, Metric A (user has CanView), Metric B (user has no permission), Metric C (ID in dashboard config but doesn't exist). ## 8. Rollback Plan diff --git a/api/prds/active/refactor_sharing_permission_helper.md b/api/prds/active/refactor_sharing_permission_helper.md index 2ea539ad5..522611a74 100644 --- a/api/prds/active/refactor_sharing_permission_helper.md +++ b/api/prds/active/refactor_sharing_permission_helper.md @@ -7,11 +7,11 @@ ## 1. Overview -This PRD details the creation of a simplified, centralized helper function within the `libs/sharing` crate. This function is primarily intended for contexts (like pre-execution checks) where only an asset's ID is available, and an efficient permission check is needed. It leverages the user's cached organization roles and performs a targeted database query only for direct asset permissions. +This PRD details the creation of a simplified, centralized helper function within the `libs/sharing` crate. This function is primarily intended for contexts (like pre-execution checks where only an asset ID is available, e.g., `get_metric_data_handler` before running SQL) and an efficient permission check is needed. It leverages the user's cached organization roles and performs a targeted database query only for direct asset permissions. It differs from fetch helpers like `fetch_metric_file_with_permissions` which retrieve the full asset alongside its base permission. ## 2. Problem Statement -While handlers retrieving full asset metadata can use efficient fetch helpers (like `fetch_..._with_permissions`), handlers performing actions based only on an asset ID (e.g., executing a query for a metric ID) need a way to check permissions without re-fetching the user's organization roles. A dedicated helper using cached roles and querying only direct permissions is needed for these scenarios. +While handlers retrieving full asset metadata use efficient fetch helpers (like `fetch_..._with_permissions`), handlers performing actions based only on an asset ID (e.g., executing a query for a metric ID) need a way to check permissions without re-fetching the user's organization roles or the full asset. A dedicated helper using cached roles and querying only direct permissions is needed for these specific scenarios. ## 3. Goals @@ -110,22 +110,6 @@ While handlers retrieving full asset metadata can use efficient fetch helpers (l - **Unit Tests:** - Mock `AuthenticatedUser` with various `organizations` states. - - Mock `AsyncPgConnection` and `asset_permissions` query results. + - Mock the `AsyncPgConnection` dependency (e.g., using `mockall` if the connection is passed via a trait, or by creating a mock connection object) to simulate different `asset_permissions` query results without hitting a real database. - Test cases: - - User has `WorkspaceAdmin` role in cache for the asset's org -> returns `Ok(true)` without DB query. - - User has `DataAdmin` role in cache -> returns `Ok(true)` without DB query. - - User has other role in cache, direct `CanView` in DB -> returns `Ok(true)` when `CanView` required. - - User has other role in cache, direct `CanView` in DB -> returns `Ok(false)` when `Owner` required. - - User has other role in cache, no direct permission in DB -> returns `Ok(false)`. - - User has no role in cache for asset's org, direct `CanView` in DB -> returns `Ok(true)` when `CanView` required. - - User has no role in cache, no direct permission -> returns `Ok(false)`. - - Direct permission exists but `deleted_at` is set -> returns `Ok(false)`. - - Database error during direct permission check -> returns `Err`. - -## 8. Rollback Plan - -- Revert the changes to `libs/sharing`. - -## 9. Dependencies - -- None. \ No newline at end of file + - User has ` \ No newline at end of file