Ling0925
/
buster
mirror of https://github.com/buster-so/buster.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

prd and metric file change

This commit is contained in:
dal 2025-04-08 10:48:24 -06:00
parent 43e1843d7b
commit b5d8cdc662
No known key found for this signature in database
GPG Key ID: 16F4B0E1E9F61122
4 changed files with 42 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
api/libs/database/src/helpers/metric_files.rs
View File

@ -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

47
api/prds/active/enhancement_collection_asset_permissions.md
View File

@ -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<AssetPermissionRole> }`. 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<AssetPermissionRole>` (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<FetchedAssetWithPermission> = /* ... */ ;
// 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<FetchedAssetWithPermission> = /* 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<CollectionAsset> = 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

33
api/prds/active/enhancement_dashboard_metric_permissions.md
View File

@ -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

24
api/prds/active/refactor_sharing_permission_helper.md
View File

@ -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.
- User has `