mirror of https://github.com/buster-so/buster.git
global and prds
This commit is contained in:
parent
1e020bb94c
commit
7e588d23b3
|
@ -62,7 +62,7 @@ While these files contain best practices for writing tests, REST patterns, etc.,
|
|||
|
||||
- ](./CLAUDE-LIBRARY-INDEX.md) - Comprehensive index of all functries
|
||||
- [**Library Template**](./libs/CLAUDE-TEMPLATE.ing new library documentation
|
||||
- [**Database Test Guide**](./libs/database/tests/CLAUDE.md) - Detailed guide for using the database test infrastructure
|
||||
- [**Database Test Guide**](mdc:libs/database/tests/CLAUDE.md) - Detailed guide for using the database test infrastructure
|
||||
|
||||
## Library Relationships
|
||||
- **agents** → depends on → **litellm**, **database**, **braintrust**
|
||||
|
|
|
@ -1,143 +0,0 @@
|
|||
# Sub-PRD: Enhance Collection Asset Permissions
|
||||
|
||||
**Author:** AI Assistant (Pair Programming with User)
|
||||
**Date:** 2023-10-27
|
||||
**Status:** Proposed
|
||||
**Parent PRD:** [Project: Granular Asset Permission Checks](mdc:prds/active/project_granular_asset_permissions.md)
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This PRD details the modifications required for the `get_collection_handler` to incorporate granular permission checks for each asset (Metric, Dashboard, etc.) contained within a collection. It introduces a `has_access` flag to the `CollectionAsset` type to indicate whether the requesting user has permission to view the specific asset.
|
||||
|
||||
**Note on Concurrency:** This work depends on the completion of the [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). Once the helper is available, this task can potentially be performed concurrently with the enhancements for the dashboard and data execution handlers.
|
||||
|
||||
## 2. Problem Statement
|
||||
|
||||
The current `get_collection_handler` lists assets associated with a collection but doesn't verify the user's permission for each *individual* asset. This can lead to users seeing assets in the list that they cannot actually access, creating a confusing user experience.
|
||||
|
||||
## 3. Goals
|
||||
|
||||
- Modify `get_collection_handler` to check permissions for each contained asset using the `check_specific_asset_access` helper.
|
||||
- Add a `has_access: bool` field to the `CollectionAsset` struct in `libs/handlers/src/collections/types.rs`.
|
||||
- Populate the `has_access` field based on the permission check result for each asset.
|
||||
- Ensure the API response includes all associated assets, clearly marking accessible vs. inaccessible ones.
|
||||
|
||||
## 4. Non-Goals
|
||||
|
||||
- Changing the permission check logic for the collection itself.
|
||||
- Modifying how assets are associated with collections.
|
||||
- Implementing checks for asset types not currently queried (e.g., Chats).
|
||||
|
||||
## 5. Technical Design
|
||||
|
||||
1. **Type Modification:**
|
||||
- Add `has_access: bool` to `CollectionAsset` struct:
|
||||
```rust
|
||||
// libs/handlers/src/collections/types.rs
|
||||
use database::enums::{AssetType, AssetPermissionRole};
|
||||
use chrono::{DateTime, Utc};
|
||||
use uuid::Uuid;
|
||||
|
||||
pub struct CollectionAsset {
|
||||
pub id: Uuid,
|
||||
pub name: String,
|
||||
pub created_by: AssetUser,
|
||||
pub created_at: DateTime<Utc>,
|
||||
pub updated_at: DateTime<Utc>,
|
||||
pub asset_type: AssetType,
|
||||
pub has_access: bool,
|
||||
}
|
||||
```
|
||||
|
||||
2. **Handler Modification (`get_collection_handler.rs`):**
|
||||
- **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)]
|
||||
struct AssetQueryResult {
|
||||
id: Uuid,
|
||||
name: String,
|
||||
user_name: Option<String>,
|
||||
email: Option<String>,
|
||||
created_at: DateTime<Utc>,
|
||||
updated_at: DateTime<Utc>,
|
||||
asset_type: AssetType,
|
||||
organization_id: Uuid,
|
||||
}
|
||||
|
||||
// Hypothetical efficient fetch result
|
||||
struct FetchedAssetWithPermission {
|
||||
asset: AssetQueryResult,
|
||||
base_permission: Option<AssetPermissionRole> // From direct/collection checks
|
||||
}
|
||||
|
||||
// Fetch assets and their base permissions efficiently
|
||||
// let fetched_assets_with_perms: Vec<FetchedAssetWithPermission> = /* Call new/adapted helper */ ;
|
||||
```
|
||||
- **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
|
||||
let admin_roles = [
|
||||
database::enums::UserOrganizationRole::WorkspaceAdmin,
|
||||
database::enums::UserOrganizationRole::DataAdmin,
|
||||
];
|
||||
|
||||
for item in fetched_assets_with_perms {
|
||||
let asset = item.asset;
|
||||
let base_permission = item.base_permission;
|
||||
|
||||
// Check org admin override from cache
|
||||
let is_org_admin = user.organizations.iter().any(|org| {
|
||||
org.id == asset.organization_id && admin_roles.contains(&org.role)
|
||||
});
|
||||
|
||||
let has_access = if is_org_admin {
|
||||
true
|
||||
} else {
|
||||
// Check if base permission (direct/collection) is sufficient
|
||||
base_permission.map_or(false, |role| role >= required_role)
|
||||
};
|
||||
|
||||
// Construct the CollectionAsset object
|
||||
final_assets.push(CollectionAsset {
|
||||
id: asset.id,
|
||||
name: asset.name,
|
||||
created_by: AssetUser { /* ... from asset ... */ },
|
||||
created_at: asset.created_at,
|
||||
updated_at: asset.updated_at,
|
||||
asset_type: asset.asset_type,
|
||||
has_access, // Set the final flag
|
||||
});
|
||||
}
|
||||
```
|
||||
- **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` (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
|
||||
|
||||
1. Modify `CollectionAsset` struct.
|
||||
2. Implement or adapt the efficient batch asset fetching logic (including base permissions).
|
||||
3. Integrate the access determination logic using fetched permissions and cached org roles.
|
||||
4. Ensure minimal `CollectionAsset` details are always included, regardless of `has_access`.
|
||||
5. Add/update integration tests.
|
||||
|
||||
## 7. Testing Strategy
|
||||
|
||||
- **Unit Tests:** Not directly applicable to the handler itself, focus on integration tests. Test modifications to `format_assets` logic if separated.
|
||||
- **Integration Tests:**
|
||||
- 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
|
||||
|
||||
- Revert changes to the handler and types files.
|
||||
|
||||
## 9. Dependencies
|
||||
|
||||
- Completion of Phase 1 (Type modifications for `has_access`). The simplified helper from the revised `refactor_sharing_permission_helper.md` is *not* directly used here.
|
|
@ -1,221 +0,0 @@
|
|||
# Sub-PRD: Enhance Dashboard Metric Permissions
|
||||
|
||||
**Author:** AI Assistant (Pair Programming with User)
|
||||
**Date:** 2023-10-27
|
||||
**Status:** Proposed
|
||||
**Parent PRD:** [Project: Granular Asset Permission Checks](mdc:prds/active/project_granular_asset_permissions.md)
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This PRD describes the modifications needed for the `get_dashboard_handler` to implement granular permission checks for each metric included in a dashboard. It involves leveraging the central permission helper and adjusting how metric data is fetched and represented in the response, potentially by modifying `get_metric_handler` or how its results are processed.
|
||||
|
||||
**Note on Concurrency:** This work depends on the completion of the [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md). Once the helper is available, this task can potentially be performed concurrently with the enhancements for the collection and data execution handlers.
|
||||
|
||||
## 2. Problem Statement
|
||||
|
||||
Currently, `get_dashboard_handler` fetches associated metrics using `get_metric_handler`. If `get_metric_handler` fails (potentially due to permissions), the metric is silently filtered out from the dashboard response's `metrics` map. Users aren't informed *why* a metric configured in the dashboard isn't visible, and they can't distinguish between a metric failing to load due to an error vs. lack of permissions.
|
||||
|
||||
## 3. Goals
|
||||
|
||||
- Modify `get_dashboard_handler`'s metric fetching logic to handle permission denials gracefully.
|
||||
- Ensure that if a user lacks `CanView` permission for a metric configured in a dashboard, the metric is still included in the response's `metrics` map, but represented minimally with `has_access: false`.
|
||||
- Add the `has_access: bool` field to the `BusterMetric` type.
|
||||
- Leverage the `check_specific_asset_access` helper for permission verification.
|
||||
- **Note:** This handler modification primarily affects the *metadata* served about the metric. Blocking the actual *execution* of the metric's SQL query for data retrieval will be handled by adding permission checks to the relevant data execution handler (as per the project PRD).
|
||||
|
||||
## 4. Non-Goals
|
||||
|
||||
- Changing the permission check logic for the dashboard itself.
|
||||
- Modifying how metrics are associated with dashboards (i.e., the dashboard config).
|
||||
|
||||
## 5. Technical Design
|
||||
|
||||
**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
|
||||
// libs/handlers/src/metrics/types.rs (adjust path as needed)
|
||||
use database::types::ChartConfig; // Import the correct ChartConfig
|
||||
use database::enums::Verification;
|
||||
use chrono::{DateTime, Utc, TimeZone};
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone)] // Add necessary derives
|
||||
pub struct BusterMetric {
|
||||
// --- Fields always present, even if no access ---
|
||||
pub id: Uuid,
|
||||
#[serde(default)] // Use default if loading minimal object
|
||||
pub name: String,
|
||||
pub has_access: bool, // New field
|
||||
|
||||
// --- Fields only present if has_access = true ---
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub description: Option<String>,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub config: Option<MetricConfig>, // Or appropriate type
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub data: Option<Value>, // Or appropriate type
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub created_at: Option<DateTime<Utc>>,
|
||||
// ... other optional fields ...
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub version_number: Option<i32>,
|
||||
// ... etc ...
|
||||
}
|
||||
|
||||
// Add default implementation if needed for minimal construction
|
||||
// Note: Default cannot be easily derived due to non-optional fields.
|
||||
// We will construct the minimal object manually.
|
||||
/*
|
||||
impl Default for BusterMetric {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
id: Uuid::nil(), // Or handle differently
|
||||
name: String::new(),
|
||||
has_access: false,
|
||||
description: None,
|
||||
config: None,
|
||||
data: None,
|
||||
created_at: None,
|
||||
version_number: None,
|
||||
// ... initialize other optional fields to None ...
|
||||
}
|
||||
}
|
||||
}
|
||||
*/
|
||||
```
|
||||
|
||||
2. **Modify `get_metric_handler.rs`:**
|
||||
- **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 */;
|
||||
let user = /* get user */;
|
||||
|
||||
// Fetch metric and its base permission level efficiently
|
||||
let metric_file_with_permission_option = fetch_metric_file_with_permissions(metric_id, &user.id)
|
||||
.await
|
||||
.map_err(|e| anyhow!("Failed to fetch metric file with permissions: {}", e))?;
|
||||
|
||||
let metric_file_with_permission = if let Some(mf) = metric_file_with_permission_option {
|
||||
mf
|
||||
} else {
|
||||
tracing::warn!(metric_id = %metric_id, "Metric file not found during fetch");
|
||||
return Err(anyhow!("Metric file not found"));
|
||||
};
|
||||
|
||||
let metric_file = metric_file_with_permission.metric_file;
|
||||
let base_permission = metric_file_with_permission.permission; // Direct or Collection permission
|
||||
```
|
||||
- **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...
|
||||
|
||||
// Check org admin override from cache
|
||||
let admin_roles = [
|
||||
database::enums::UserOrganizationRole::WorkspaceAdmin,
|
||||
database::enums::UserOrganizationRole::DataAdmin,
|
||||
];
|
||||
let is_org_admin = user.organizations.iter().any(|org| {
|
||||
org.id == metric_file.organization_id && admin_roles.contains(&org.role)
|
||||
});
|
||||
|
||||
// Check public access rules (simplified example, adapt from existing handler logic)
|
||||
let is_publicly_viewable = metric_file.publicly_accessible
|
||||
&& metric_file.public_expiry_date.map_or(true, |expiry| expiry > chrono::Utc::now())
|
||||
&& metric_file.public_password.is_none(); // Simplification: ignore password case for now
|
||||
|
||||
let required_role = AssetPermissionRole::CanView; // Minimum requirement
|
||||
|
||||
let has_access = if is_org_admin {
|
||||
true
|
||||
} else if base_permission.map_or(false, |role| role >= required_role) {
|
||||
true // Has sufficient direct/collection permission
|
||||
} else {
|
||||
is_publicly_viewable // Fallback to public access check
|
||||
};
|
||||
|
||||
// Determine effective permission role to return (for display/context)
|
||||
let effective_permission = if is_org_admin {
|
||||
base_permission.unwrap_or(AssetPermissionRole::Owner) // Admins often treated as Owners
|
||||
} else if let Some(role) = base_permission {
|
||||
role
|
||||
} else if is_publicly_viewable {
|
||||
AssetPermissionRole::CanView
|
||||
} else {
|
||||
// This case should ideally not be reached if has_access logic is correct
|
||||
// but provide a default if needed, maybe linked to has_access=false outcome.
|
||||
// If has_access is false here, maybe return a specific 'NoAccess' pseudo-role?
|
||||
// For now, align with has_access outcome:
|
||||
if has_access { AssetPermissionRole::CanView } else { /* Need a way to signify no access */ AssetPermissionRole::CanView } // Placeholder!
|
||||
};
|
||||
|
||||
// If has_access is false, construct minimal object. Otherwise, construct full object.
|
||||
if has_access {
|
||||
// ... (fetch version content, datasets, user info, dashboards, collections etc. as before) ...
|
||||
// ... (construct FULL BusterMetric) ...
|
||||
let full_metric = BusterMetric {
|
||||
id: metric_file.id,
|
||||
// ... all fields populated ...
|
||||
has_access: true,
|
||||
permission: effective_permission, // Use determined effective permission
|
||||
// ...
|
||||
};
|
||||
Ok(full_metric)
|
||||
} else {
|
||||
// ... (Construct MINIMAL BusterMetric, setting has_access: false, permission: appropriate_indicator) ...
|
||||
let minimal_metric = BusterMetric {
|
||||
id: metric_file.id,
|
||||
name: metric_file.name,
|
||||
has_access: false,
|
||||
permission: AssetPermissionRole::CanView, // Or a 'NoAccess' pseudo-role if defined
|
||||
// ... provide defaults for other non-optional fields ...
|
||||
..Default::default() // Use defaults where possible, but override required fields
|
||||
};
|
||||
Ok(minimal_metric)
|
||||
}
|
||||
```
|
||||
- **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`:**
|
||||
- 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)**
|
||||
|
||||
- Keep `get_metric_handler` mostly as-is (returning `Err` on permission denied).
|
||||
- In `get_dashboard_handler`, after `join_all`, iterate through results. If a result is `Err`, attempt a *separate* minimal query to fetch just the `id` and `name` for that metric ID. If successful, create a minimal `BusterMetric { has_access: false, ... }` and add it to the map. If the minimal query fails (e.g., metric truly doesn't exist), log and omit.
|
||||
- *Drawback:* Less efficient (potential extra queries), splits logic.
|
||||
|
||||
**Decision:** Proceed with **Option A**.
|
||||
|
||||
4. **File Changes:**
|
||||
- Modify: `libs/handlers/src/metrics/get_metric_handler.rs` (or similar)
|
||||
- Modify: `libs/handlers/src/metrics/types.rs` (or similar, for `BusterMetric`)
|
||||
- Modify: `libs/handlers/src/dashboards/get_dashboard_handler.rs` (minor changes to error handling/filtering logic after `join_all`)
|
||||
|
||||
## 6. Implementation Plan
|
||||
|
||||
1. Modify `BusterMetric` type definition.
|
||||
2. Refactor `get_metric_handler` to perform permission check upfront and return minimal object on denial.
|
||||
3. Adjust result processing loop in `get_dashboard_handler`.
|
||||
4. Add/update integration tests.
|
||||
|
||||
## 7. Testing Strategy
|
||||
|
||||
- **Unit Tests (`get_metric_handler`):**
|
||||
- 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: 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
|
||||
|
||||
- Revert changes to the modified handler and type files.
|
||||
|
||||
## 9. Dependencies
|
||||
|
||||
- Completion of Phase 1 (Type modifications for `has_access`). The simplified helper from the revised `refactor_sharing_permission_helper.md` is *not* directly used here.
|
|
@ -1,82 +0,0 @@
|
|||
# Project PRD: Granular Asset Permission Checks in Collections and Dashboards
|
||||
|
||||
**Author:** AI Assistant (Pair Programming with User)
|
||||
**Date:** 2023-10-27
|
||||
**Status:** Proposed
|
||||
|
||||
## 1. Overview
|
||||
|
||||
This project aims to implement fine-grained permission checks for assets (like metrics and dashboards) contained within collections and dashboards. Currently, access checks primarily focus on the container, potentially exposing the existence of contained assets the user cannot access or hiding them inconsistently. This project will ensure that access to each contained asset is individually verified against the user's permissions, and the accessibility is clearly communicated in the API response.
|
||||
|
||||
## 2. Goals
|
||||
|
||||
- Implement fine-grained access control checks for individual assets (metrics, dashboards) listed within collections returned by `get_collection_handler`.
|
||||
- Implement fine-grained access control checks for individual metrics listed within dashboards returned by `get_dashboard_handler`.
|
||||
- Ensure users can see basic identifying information (ID, name, type) of assets within containers even if they lack `CanView` permission for those specific assets.
|
||||
- Clearly indicate assets the user lacks permission to view using a `has_access: bool` flag in the response structures.
|
||||
- Promote consistency by centralizing asset permission checking logic.
|
||||
|
||||
## 3. Non-Goals
|
||||
|
||||
- Changing the existing permission model (roles, identity types).
|
||||
- Implementing access checks for asset types beyond metrics and dashboards initially.
|
||||
- Modifying the frontend UI to visually represent the `has_access` status.
|
||||
- Changing how permissions for the *container* (collection/dashboard itself) are checked.
|
||||
|
||||
## 4. Implementation Plan
|
||||
|
||||
The implementation is divided into the following phases and corresponding sub-PRDs. Phase 1 must be completed before Phases 2 and 3 can begin. Phases 2 and 3 can be implemented concurrently after Phase 1 is complete.
|
||||
|
||||
**Phase 1: Foundational Permission Helper & Type Modifications (Blocking)**
|
||||
|
||||
* **Task:** Implement the simplified permission checking helper and add `has_access` flags to relevant types.
|
||||
* **Sub-PRD:** [Refactor Sharing Permission Helper](mdc:prds/active/refactor_sharing_permission_helper.md) (Note: This PRD will be updated to reflect the simplified helper design).
|
||||
* **Also includes:** Modifying `BusterMetric` and `CollectionAsset` types to add the `has_access: bool` field.
|
||||
* **Status:** Upcoming
|
||||
* **Details:** Create a helper primarily for contexts needing checks based only on ID, using cached org roles. Add the boolean flag to response types.
|
||||
|
||||
**Phase 2: Concurrent Handler Enhancements (Requires Phase 1 Completion)**
|
||||
|
||||
* **Task A (Concurrent):** Enhance Collection Handler
|
||||
* **Sub-PRD:** [Enhance Collection Asset Permissions](mdc:prds/active/enhancement_collection_asset_permissions.md)
|
||||
* **Status:** Upcoming
|
||||
* **Details:** Modify `get_collection_handler` to use efficient permission fetching (like `fetch_..._with_permissions`), check cached org roles, set `has_access` flag, and return minimal `CollectionAsset` if access denied.
|
||||
* **Task B (Concurrent):** Enhance Dashboard/Metric Handlers
|
||||
* **Sub-PRD:** [Enhance Dashboard Metric Permissions](mdc:prds/active/enhancement_dashboard_metric_permissions.md)
|
||||
* **Status:** Upcoming
|
||||
* **Details:** Modify `get_metric_handler` (and `get_dashboard_handler` processing) to use efficient permission fetching, check cached org roles, set `has_access` flag, and return minimal `BusterMetric` if access denied. Downstream handlers like `get_metric_data_handler` will check this flag.
|
||||
|
||||
**Phase 3: Integration Testing & Rollout**
|
||||
|
||||
* **Task:** Perform end-to-end testing covering all enhanced handlers and scenarios involving mixed permissions.
|
||||
* **Details:** Ensure collections, dashboards, and direct data execution requests correctly reflect and enforce the granular permissions.
|
||||
* **Rollout:** Deploy changes once all phases are complete and tested.
|
||||
|
||||
## 5. High-Level Technical Design
|
||||
|
||||
- Introduce a `has_access: bool` field to `CollectionAsset` and `BusterMetric` response types.
|
||||
- Develop a reusable helper function in `libs/sharing` primarily for pre-execution checks, using cached org roles and querying only direct `asset_permissions`.
|
||||
- Modify `get_collection_handler` and `get_metric_handler` to efficiently fetch assets *with* their base permissions (handling `deleted_at`), then check cached org admin roles, and finally determine the `has_access` status.
|
||||
- Ensure handlers return minimal, non-sensitive object representations when `has_access` is false.
|
||||
- Ensure handlers like `get_metric_data_handler` check the `has_access` flag before proceeding with sensitive operations (like query execution).
|
||||
|
||||
## 6. Testing Strategy
|
||||
|
||||
- Each sub-PRD will define specific unit and integration tests.
|
||||
- End-to-end tests should cover scenarios involving collections and dashboards containing a mix of accessible and inaccessible assets for a given user.
|
||||
|
||||
## 7. Rollout Plan
|
||||
|
||||
- Implement and merge sub-PRDs sequentially as defined in the Implementation Plan.
|
||||
- Feature can be rolled out once all sub-PRDs are completed and merged. No feature flag is deemed necessary as this is primarily a backend enhancement improving correctness.
|
||||
|
||||
## 8. Dependencies
|
||||
|
||||
- Requires understanding of the existing permission system (`asset_permissions`, `users_to_organizations`).
|
||||
- Modifies core handlers for collections and dashboards.
|
||||
|
||||
## 9. Open Questions/Decisions Made
|
||||
|
||||
- **Public Container vs. Private Asset:** Decided: Access to a public container *does not* grant implicit access to contained assets. The user's specific permissions for the contained asset will always be checked.
|
||||
- **Metadata for Inaccessible Assets:** Decided: Assets marked `has_access: false` will include non-sensitive identifiers like `id`, `name`, and `asset_type`. Sensitive data (like metric `content` or dashboard `config`) will not be included.
|
||||
- **Error Handling for Permission Checks:** Decided: If checking permission for a specific asset fails due to a database error (not lack of permission), that asset will be omitted from the results, and the error will be logged. Lack of permission results in `has_access: false`.
|
|
@ -1,115 +0,0 @@
|
|||
# Sub-PRD: Simplified Asset Permission Helper
|
||||
|
||||
**Author:** AI Assistant (Pair Programming with User)
|
||||
**Date:** 2023-10-27
|
||||
**Status:** Proposed
|
||||
**Parent PRD:** [Project: Granular Asset Permission Checks](mdc:prds/active/project_granular_asset_permissions.md)
|
||||
|
||||
## 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 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 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
|
||||
|
||||
- Create a reusable, modular, and testable function `check_permission_for_asset_id` within `libs/sharing`.
|
||||
- This function should accept user context (`AuthenticatedUser` containing cached org roles), asset details (ID, type, org ID), and required permission levels.
|
||||
- It should first check the user's cached organization roles (`user.organizations`) for high-level admin privileges (`WorkspaceAdmin`, `DataAdmin`).
|
||||
- If no admin role applies, it should query the `asset_permissions` table for direct permissions for the specific asset ID and user.
|
||||
- It should return `Ok(true)` if the user meets the requirements (either via org admin role or direct permission), `Ok(false)` if they don't, and `Err` only for database query errors.
|
||||
|
||||
## 4. Non-Goals
|
||||
|
||||
- Replacing the efficient fetch helpers like `fetch_metric_file_with_permissions` used by metadata retrieval handlers.
|
||||
- Querying the `users_to_organizations` table (relies on cached roles in `AuthenticatedUser`).
|
||||
- Handling complex permission inheritance beyond direct permissions and high-level org roles (unless explicitly added).
|
||||
|
||||
## 5. Technical Design
|
||||
|
||||
1. **Location:** `libs/sharing/src/permissions.rs` (or similar).
|
||||
2. **Function Signature & Implementation:**
|
||||
```rust
|
||||
// libs/sharing/src/permissions.rs
|
||||
use anyhow::{anyhow, Result};
|
||||
use diesel::prelude::*;
|
||||
use diesel::dsl::exists;
|
||||
use diesel_async::AsyncPgConnection;
|
||||
use middleware::AuthenticatedUser;
|
||||
use uuid::Uuid;
|
||||
use database::enums::{AssetPermissionRole, AssetType, UserOrganizationRole, UserOrganizationStatus};
|
||||
use database::schema::asset_permissions;
|
||||
|
||||
/// Checks if a user has the required permission level for a specific asset ID,
|
||||
/// leveraging cached organization roles and querying direct permissions.
|
||||
/// Intended primarily for pre-execution checks.
|
||||
pub async fn check_permission_for_asset_id(
|
||||
conn: &mut AsyncPgConnection,
|
||||
user: &AuthenticatedUser,
|
||||
asset_id: &Uuid,
|
||||
asset_type: AssetType,
|
||||
asset_organization_id: Uuid, // Org ID of the asset itself
|
||||
required_roles: &[AssetPermissionRole],
|
||||
) -> Result<bool> {
|
||||
// --- 1. Check Cached High-Level Organization Roles First ---
|
||||
let high_level_org_roles = [
|
||||
UserOrganizationRole::WorkspaceAdmin,
|
||||
UserOrganizationRole::DataAdmin,
|
||||
];
|
||||
|
||||
let has_high_level_org_role = user.organizations.iter().any(|org| {
|
||||
org.id == asset_organization_id && high_level_org_roles.contains(&org.role)
|
||||
// Assuming AuthenticatedUser.organizations only contains active memberships
|
||||
});
|
||||
|
||||
if has_high_level_org_role {
|
||||
return Ok(true); // User is Org Admin/Data Admin for the asset's org, grant access
|
||||
}
|
||||
|
||||
// --- 2. Check Direct Permissions (Database Query) ---
|
||||
let direct_permission_exists = select(exists(
|
||||
asset_permissions::table
|
||||
.filter(asset_permissions::asset_id.eq(asset_id))
|
||||
.filter(asset_permissions::asset_type.eq(asset_type))
|
||||
.filter(asset_permissions::identity_id.eq(user.id))
|
||||
.filter(asset_permissions::identity_type.eq(database::enums::IdentityType::User))
|
||||
.filter(asset_permissions::deleted_at.is_null()) // Ignore deleted permissions
|
||||
.filter(asset_permissions::role.eq_any(required_roles)),
|
||||
))
|
||||
.get_result::<bool>(conn)
|
||||
.await;
|
||||
|
||||
match direct_permission_exists {
|
||||
Ok(true) => Ok(true), // Found sufficient direct permission
|
||||
Ok(false) => Ok(false), // No sufficient direct permission found
|
||||
Err(e) => {
|
||||
tracing::error!(
|
||||
"DB error checking direct asset permissions for user {} asset {} type {:?}: {}",
|
||||
user.id, asset_id, asset_type, e
|
||||
);
|
||||
Err(anyhow!("Failed to check direct asset permissions: {}", e))
|
||||
}
|
||||
}
|
||||
// Note: No check for Member role here unless specifically required
|
||||
// for action execution contexts, as metadata handlers use different logic.
|
||||
}
|
||||
```
|
||||
3. **File Changes:**
|
||||
- Create/Modify: `libs/sharing/src/permissions.rs`
|
||||
- Modify: `libs/sharing/src/lib.rs` (to export the function)
|
||||
|
||||
## 6. Implementation Plan
|
||||
|
||||
1. Implement the `check_permission_for_asset_id` function as defined above.
|
||||
2. Add comprehensive unit tests.
|
||||
3. Export the function.
|
||||
|
||||
## 7. Testing Strategy
|
||||
|
||||
- **Unit Tests:**
|
||||
- Mock `AuthenticatedUser` with various `organizations` states.
|
||||
- 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 `
|
Loading…
Reference in New Issue