From af7e969af8dbe53ad794fc7994800eb829802597 Mon Sep 17 00:00:00 2001 From: dal Date: Thu, 23 Jan 2025 15:22:07 -0800 Subject: [PATCH] fix permission check on post_dataset rest (#59) * fix permission check on post_dataset rest * refactor: enhance dataset overview access lineage and permission checks - Updated the `get_dataset_overview` function to conditionally add default access lineage based on user roles and existing access paths. - Simplified the logic for adding user roles to the lineage, ensuring clarity and maintainability. - Improved handling for the `RestrictedQuerier` role to include checks for existing access before adding default lineage, enhancing permission accuracy. - Streamlined code by removing redundant checks and consolidating role handling, optimizing overall readability. * feat: Enhance permission group handling and data retrieval - Introduced a new `PermissionGroupInfo` struct to encapsulate detailed information about permission groups, including user and dataset counts. - Updated the `get_permission_group` and `list_permission_groups` functions to improve data retrieval and error handling. - Refactored SQL queries in `list_permission_groups` to include additional joins for counting users and datasets associated with permission groups, enhancing the overall functionality and clarity of the API. - Streamlined code for better readability and maintainability, ensuring consistent handling of user and permission group data. * refactor: Improve dataset access handling and permission checks - Enhanced the `get_restricted_user_datasets` and `get_restricted_user_datasets_with_metadata` functions to include additional permission checks for dataset groups and permission groups. - Consolidated SQL queries to ensure proper filtering of deleted records and improved clarity in dataset retrieval logic. - Introduced new joins and filters to handle dataset group permissions, ensuring accurate access control for users. - Streamlined code for better readability and maintainability, enhancing overall functionality in dataset access management. * fix: Update SQL migration and seed data for user attributes - Modified the SQL migration to specify the schema for the `users` table, ensuring clarity in the update statement. - Adjusted the seed data for `users_to_organizations` to change the `organization_id` from 'public' to 'none', reflecting a more accurate state for user roles and organization associations. - Ensured consistency in the formatting of SQL insert statements for better readability. * fix: Prevent users from updating their own profiles - Added a check in the `update_user_handler` to prevent users from updating their own information, returning an error if they attempt to do so. - This change enhances security by ensuring that users cannot modify their own records, which could lead to unauthorized changes. * refactor: Simplify dashboard permission queries by removing team-based joins - Removed left joins with `teams_to_users` table in dashboard permission queries - Simplified permission checks to only filter by direct user ID - Updated queries in `get_user_dashboard_permission`, `get_bulk_user_dashboard_permission`, and `list_dashboards_handler` - Streamlined SQL query logic for more direct and efficient permission checks --- .../2025-01-17-182615_user_info_cache/up.sql | 2 +- api/src/database/seed.sql | 32 +++++++++---------- .../routes/rest/routes/users/update_user.rs | 4 +++ .../routes/ws/dashboards/dashboard_utils.rs | 18 ++--------- .../routes/ws/dashboards/list_dashboards.rs | 13 +------- 5 files changed, 24 insertions(+), 45 deletions(-) diff --git a/api/migrations/2025-01-17-182615_user_info_cache/up.sql b/api/migrations/2025-01-17-182615_user_info_cache/up.sql index b11f6af5b..1aa7e3908 100644 --- a/api/migrations/2025-01-17-182615_user_info_cache/up.sql +++ b/api/migrations/2025-01-17-182615_user_info_cache/up.sql @@ -23,7 +23,7 @@ CREATE TRIGGER sync_user_org_attributes EXECUTE FUNCTION update_user_org_attributes(); -- Update existing records - UPDATE users u + UPDATE public.users u SET attributes = jsonb_set( jsonb_set( COALESCE(attributes, '{}'::jsonb), diff --git a/api/src/database/seed.sql b/api/src/database/seed.sql index a86509513..aa9b9d21a 100644 --- a/api/src/database/seed.sql +++ b/api/src/database/seed.sql @@ -982,25 +982,25 @@ INSERT INTO public.terms (id, name, definition, sql_snippet, organization_id, cr -- INSERT INTO public.users_to_organizations ( - user_id, - organization_id, - role, + user_id, + organization_id, + role, status, - sharing_setting, - edit_sql, - upload_csv, - export_assets, - email_slack_enabled, - created_at, - updated_at, - deleted_at, - created_by, - updated_by, + sharing_setting, + edit_sql, + upload_csv, + export_assets, + email_slack_enabled, + created_at, + updated_at, + deleted_at, + created_by, + updated_by, deleted_by ) VALUES -('c2dd64cd-f7f3-4884-bc91-d46ae431901e', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'workspace_admin', 'active', 'public', false, false, false, false, '2024-11-05 15:41:13.958254+00', '2024-11-05 15:41:13.958254+00', NULL, 'c2dd64cd-f7f3-4884-bc91-d46ae431901e', 'c2dd64cd-f7f3-4884-bc91-d46ae431901e', NULL), -('1fe85021-e799-471b-8837-953e9ae06e4c', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'querier', 'active', 'team', false, false, false, false, '2024-11-05 15:41:13.958255+00', '2024-11-05 15:41:13.958255+00', NULL, '1fe85021-e799-471b-8837-953e9ae06e4c', '1fe85021-e799-471b-8837-953e9ae06e4c', NULL), -('6840fa04-c0d7-4e0e-8d3d-ea9190d93874', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'data_admin', 'active', 'public', false, false, false, false, '2024-11-05 15:41:13.958256+00', '2024-11-05 15:41:13.958256+00', NULL, '6840fa04-c0d7-4e0e-8d3d-ea9190d93874', '6840fa04-c0d7-4e0e-8d3d-ea9190d93874', NULL); +('c2dd64cd-f7f3-4884-bc91-d46ae431901e', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'workspace_admin', 'active', 'none', false, false, false, false, '2024-11-05 15:41:13.958254+00', '2024-11-05 15:41:13.958254+00', NULL, 'c2dd64cd-f7f3-4884-bc91-d46ae431901e', 'c2dd64cd-f7f3-4884-bc91-d46ae431901e', NULL), +('1fe85021-e799-471b-8837-953e9ae06e4c', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'querier', 'active', 'none', false, false, false, false, '2024-11-05 15:41:13.958255+00', '2024-11-05 15:41:13.958255+00', NULL, '1fe85021-e799-471b-8837-953e9ae06e4c', '1fe85021-e799-471b-8837-953e9ae06e4c', NULL), +('6840fa04-c0d7-4e0e-8d3d-ea9190d93874', 'bf58d19a-8bb9-4f1d-a257-2d2105e7f1ce', 'data_admin', 'active', 'none', false, false, false, false, '2024-11-05 15:41:13.958256+00', '2024-11-05 15:41:13.958256+00', NULL, '6840fa04-c0d7-4e0e-8d3d-ea9190d93874', '6840fa04-c0d7-4e0e-8d3d-ea9190d93874', NULL); diff --git a/api/src/routes/rest/routes/users/update_user.rs b/api/src/routes/rest/routes/users/update_user.rs index d1f4b53b1..ddf8d22a7 100644 --- a/api/src/routes/rest/routes/users/update_user.rs +++ b/api/src/routes/rest/routes/users/update_user.rs @@ -73,6 +73,10 @@ pub async fn update_user_handler( } }; + if &auth_user.id == user_id { + return Err(anyhow::anyhow!("Cannot update self")); + }; + match is_user_workspace_admin_or_data_admin(auth_user, &user_organization_id).await { Ok(true) => (), Ok(false) => return Err(anyhow::anyhow!("Insufficient permissions")), diff --git a/api/src/routes/ws/dashboards/dashboard_utils.rs b/api/src/routes/ws/dashboards/dashboard_utils.rs index 4d2a814cd..c56551066 100644 --- a/api/src/routes/ws/dashboards/dashboard_utils.rs +++ b/api/src/routes/ws/dashboards/dashboard_utils.rs @@ -271,15 +271,8 @@ pub async fn get_user_dashboard_permission( }; let permissions = match asset_permissions::table - .left_join( - teams_to_users::table.on(asset_permissions::identity_id.eq(teams_to_users::team_id)), - ) .select(asset_permissions::role) - .filter( - asset_permissions::identity_id - .eq(&user_id) - .or(teams_to_users::user_id.eq(&user_id)), - ) + .filter(asset_permissions::identity_id.eq(&user_id)) .filter(asset_permissions::asset_id.eq(&dashboard_id)) .filter(asset_permissions::deleted_at.is_null()) .load::(&mut conn) @@ -322,15 +315,8 @@ pub async fn get_bulk_user_dashboard_permission( }; let permissions = match asset_permissions::table - .left_join( - teams_to_users::table.on(asset_permissions::identity_id.eq(teams_to_users::team_id)), - ) .select((asset_permissions::asset_id, asset_permissions::role)) - .filter( - asset_permissions::identity_id - .eq(&user_id) - .or(teams_to_users::user_id.eq(&user_id)), - ) + .filter(asset_permissions::identity_id.eq(&user_id)) .filter(asset_permissions::asset_id.eq_any(dashboard_ids)) .filter(asset_permissions::deleted_at.is_null()) .load::<(Uuid, AssetPermissionRole)>(&mut conn) diff --git a/api/src/routes/ws/dashboards/list_dashboards.rs b/api/src/routes/ws/dashboards/list_dashboards.rs index 871f4f040..170500956 100644 --- a/api/src/routes/ws/dashboards/list_dashboards.rs +++ b/api/src/routes/ws/dashboards/list_dashboards.rs @@ -115,13 +115,6 @@ async fn list_dashboards_handler( .and(asset_permissions::asset_type.eq(AssetType::Dashboard)) .and(asset_permissions::deleted_at.is_null())), ) - .left_join( - teams_to_users::table.on(asset_permissions::identity_id - .eq(teams_to_users::user_id) - .and(asset_permissions::identity_type.eq(IdentityType::Team)) - .and(teams_to_users::deleted_at.is_null()) - .and(asset_permissions::deleted_at.is_null())), - ) .inner_join(users::table.on(users::id.eq(dashboards::created_by))) .select(( dashboards::id, @@ -133,11 +126,7 @@ async fn list_dashboards_handler( users::name.nullable(), )) .filter(dashboards::deleted_at.is_null()) - .filter( - asset_permissions::identity_id - .eq(user_id) - .or(teams_to_users::user_id.eq(user_id)), - ) + .filter(asset_permissions::identity_id.eq(user_id)) .distinct() .order((dashboards::updated_at.desc(), dashboards::id.asc())) .offset(page * page_size)