From af7e969af8dbe53ad794fc7994800eb829802597 Mon Sep 17 00:00:00 2001 From: dal Date: Thu, 23 Jan 2025 15:22:07 -0800 Subject: [PATCH 1/3] 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) From ee58c05b54b616b928ca9b2d8f9afc82f68b9a53 Mon Sep 17 00:00:00 2001 From: dal Date: Thu, 23 Jan 2025 16:54:09 -0700 Subject: [PATCH 2/3] dashboard permissions fix --- .../routes/ws/dashboards/dashboard_utils.rs | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/api/src/routes/ws/dashboards/dashboard_utils.rs b/api/src/routes/ws/dashboards/dashboard_utils.rs index c56551066..91ea6573a 100644 --- a/api/src/routes/ws/dashboards/dashboard_utils.rs +++ b/api/src/routes/ws/dashboards/dashboard_utils.rs @@ -14,7 +14,7 @@ use crate::{ enums::{AssetPermissionRole, AssetType}, lib::get_pg_pool, models::{Dashboard, Message}, - schema::{asset_permissions, dashboards, messages, teams_to_users, threads_to_dashboards}, + schema::{asset_permissions, dashboards, messages, teams_to_users, threads_to_dashboards, users_to_organizations}, }, utils::{ clients::{sentry_utils::send_sentry_error, supabase_vault::read_secret}, @@ -270,6 +270,40 @@ pub async fn get_user_dashboard_permission( } }; + // First get the dashboard's organization + let dashboard_org = match dashboards::table + .select(dashboards::organization_id) + .filter(dashboards::id.eq(dashboard_id)) + .filter(dashboards::deleted_at.is_null()) + .first::(&mut conn) + .await { + Ok(org_id) => org_id, + Err(diesel::result::Error::NotFound) => return Ok(None), + Err(e) => { + tracing::error!("Error querying dashboard organization: {}", e); + return Err(anyhow!("Error querying dashboard organization: {}", e)); + } + }; + + // Then get the user's organization + let user_org = match users_to_organizations::table + .select(users_to_organizations::organization_id) + .filter(users_to_organizations::user_id.eq(user_id)) + .first::(&mut conn) + .await { + Ok(org_id) => org_id, + Err(diesel::result::Error::NotFound) => return Ok(None), + Err(e) => { + tracing::error!("Error querying user organization: {}", e); + return Err(anyhow!("Error querying user organization: {}", e)); + } + }; + + // If organizations don't match, deny access + if dashboard_org != user_org { + return Ok(None); + } + let permissions = match asset_permissions::table .select(asset_permissions::role) .filter(asset_permissions::identity_id.eq(&user_id)) @@ -314,11 +348,27 @@ pub async fn get_bulk_user_dashboard_permission( } }; + // Get user's organization + let user_org = match users_to_organizations::table + .select(users_to_organizations::organization_id) + .filter(users_to_organizations::user_id.eq(user_id)) + .first::(&mut conn) + .await { + Ok(org_id) => org_id, + Err(diesel::result::Error::NotFound) => return Ok(HashMap::new()), + Err(e) => { + tracing::error!("Error querying user organization: {}", e); + return Err(anyhow!("Error querying user organization: {}", e)); + } + }; + let permissions = match asset_permissions::table + .inner_join(dashboards::table.on(asset_permissions::asset_id.eq(dashboards::id))) .select((asset_permissions::asset_id, asset_permissions::role)) .filter(asset_permissions::identity_id.eq(&user_id)) .filter(asset_permissions::asset_id.eq_any(dashboard_ids)) .filter(asset_permissions::deleted_at.is_null()) + .filter(dashboards::organization_id.eq(user_org)) .load::<(Uuid, AssetPermissionRole)>(&mut conn) .await { From 504a3360b6549ec183234b506124bbc96e459908 Mon Sep 17 00:00:00 2001 From: Nate Kelley Date: Thu, 23 Jan 2025 17:01:52 -0700 Subject: [PATCH 3/3] set cookies --- web/src/api/createServerInstance.ts | 4 ++-- web/src/context/Supabase/server.ts | 26 ++++++++++++------------ web/src/middleware/supabaseMiddleware.ts | 3 ++- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/web/src/api/createServerInstance.ts b/web/src/api/createServerInstance.ts index c23e7d470..1487cbc1a 100644 --- a/web/src/api/createServerInstance.ts +++ b/web/src/api/createServerInstance.ts @@ -2,7 +2,7 @@ import { BASE_URL } from './buster_rest/instances'; import type { RequestInit } from 'next/dist/server/web/spec-extension/request'; -import { createClient } from '../context/Supabase/server'; +import { createServerSupabaseClient } from '../context/Supabase/server'; export interface FetchConfig extends RequestInit { baseURL?: string; @@ -10,7 +10,7 @@ export interface FetchConfig extends RequestInit { } export const serverFetch = async (url: string, config: FetchConfig = {}): Promise => { - const supabase = await createClient(); + const supabase = await createServerSupabaseClient(); const sessionData = await supabase.auth.getSession(); const accessToken = sessionData.data?.session?.access_token; diff --git a/web/src/context/Supabase/server.ts b/web/src/context/Supabase/server.ts index 409d53e31..5077e0564 100644 --- a/web/src/context/Supabase/server.ts +++ b/web/src/context/Supabase/server.ts @@ -1,21 +1,21 @@ -import { createServerClient } from '@supabase/ssr'; +import { CookieOptions, createServerClient } from '@supabase/ssr'; import { cookies } from 'next/headers'; -export async function createClient() { +export const COOKIE_OPTIONS: CookieOptions = { + path: '/', + secure: process.env.NODE_ENV === 'production', // Only use secure in production + sameSite: 'lax', // Type assertion to fix the error + httpOnly: true, // Make cookies HttpOnly + maxAge: 60 * 60 * 24 * 7 // 1 week +}; + +export const createServerSupabaseClient = async () => { const cookieStore = await cookies(); - return await createServerClient( + return createServerClient( process.env.NEXT_PUBLIC_SUPABASE_URL!, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!, { - cookieOptions: { - secure: true, - httpOnly: true - }, - auth: { - autoRefreshToken: true, - persistSession: true - }, cookies: { getAll() { return cookieStore.getAll(); @@ -23,7 +23,7 @@ export async function createClient() { setAll(cookiesToSet) { try { cookiesToSet.forEach(({ name, value, options }) => - cookieStore.set(name, value, options) + cookieStore.set(name, value, { ...options, ...COOKIE_OPTIONS }) ); } catch { // The `setAll` method was called from a Server Component. @@ -34,4 +34,4 @@ export async function createClient() { } } ); -} +}; diff --git a/web/src/middleware/supabaseMiddleware.ts b/web/src/middleware/supabaseMiddleware.ts index c51ae1b4f..edd6d0bde 100644 --- a/web/src/middleware/supabaseMiddleware.ts +++ b/web/src/middleware/supabaseMiddleware.ts @@ -1,3 +1,4 @@ +import { COOKIE_OPTIONS } from '@/context/Supabase/server'; import { createServerClient } from '@supabase/ssr'; import { User } from '@supabase/supabase-js'; import { NextResponse, type NextRequest } from 'next/server'; @@ -23,7 +24,7 @@ export async function updateSession(request: NextRequest) { request }); cookiesToSet.forEach(({ name, value, options }) => - supabaseResponse.cookies.set(name, value, options) + supabaseResponse.cookies.set(name, value, { ...options, ...COOKIE_OPTIONS }) ); } }