Merge pull request #523 from buster-so/staging

Bugfix on cascading permissions for dashboards -> metrics

apps/api/libs/handlers/src/metrics/get_metric_data_handler.rs

@@ -1,11 +1,10 @@
 use anyhow::{anyhow, Result};
-use chrono::Utc;
 use database::{
     pool::get_pg_pool,
-    schema::{dashboard_files, metric_files, metric_files_to_dashboard_files},
+    schema::metric_files,
     types::{data_metadata::DataMetadata, MetricYml},
 };
-use diesel::{BoolExpressionMethods, ExpressionMethods, JoinOnDsl, QueryDsl};
+use diesel::{ExpressionMethods, QueryDsl};
 use diesel_async::RunQueryDsl;
 use indexmap::IndexMap;
 use middleware::AuthenticatedUser;
@@ -68,41 +67,18 @@ pub async fn get_metric_data_handler(
 
             if is_permission_error {
                 tracing::warn!(
-                    "Initial metric access failed due to potential permission issue: {}. Checking public dashboard access.",
+                    "Initial metric access failed due to potential permission issue: {}. Checking dashboard access.",
                     e
                 );
 
-                // --- Step 3: Check if metric belongs to a valid public dashboard ---
-                let mut conn_check = get_pg_pool().get().await?;
-                let now = Utc::now();
-
-                let public_dashboard_exists = match metric_files_to_dashboard_files::table
-                    .inner_join(dashboard_files::table.on(
-                        dashboard_files::id.eq(metric_files_to_dashboard_files::dashboard_file_id),
-                    ))
-                    .filter(metric_files_to_dashboard_files::metric_file_id.eq(request.metric_id))
-                    .filter(dashboard_files::publicly_accessible.eq(true))
-                    .filter(dashboard_files::deleted_at.is_null())
-                    .filter(
-                        dashboard_files::public_expiry_date
-                            .is_null()
-                            .or(dashboard_files::public_expiry_date.gt(now)),
-                    )
-                    .select(dashboard_files::id) // Select any column to check existence
-                    .first::<Uuid>(&mut conn_check) // Try to get the first matching ID
+                // Check if user has access to ANY dashboard containing this metric (including public dashboards)
+                let has_dashboard_access = sharing::check_metric_dashboard_access(&request.metric_id, &user.id)
                     .await
-                {
-                    Ok(id) => Some(id),
-                    Err(diesel::NotFound) => None,
-                    Err(e) => {
-                        tracing::error!("Error checking if public dashboard exists: {}", e);
-                        return Err(anyhow!("Error checking if public dashboard exists: {}", e));
-                    }
-                };
+                    .unwrap_or(false);
 
-                if public_dashboard_exists.is_some() {
-                    // --- Step 4: Public dashboard found, fetch metric bypassing permissions ---
-                    tracing::info!("Found associated public dashboard. Fetching metric definition without direct permissions.");
+                if has_dashboard_access {
+                    // User has access to a dashboard containing this metric
+                    tracing::info!("Found associated dashboard with user access. Fetching metric with dashboard context.");
                     match get_metric_for_dashboard_handler(
                         &request.metric_id,
                         &user,
@@ -113,19 +89,19 @@ pub async fn get_metric_data_handler(
                     {
                         Ok(metric_via_dashboard) => {
                             tracing::debug!(
-                                "Successfully retrieved metric via public dashboard association."
+                                "Successfully retrieved metric via dashboard association."
                             );
                             metric_via_dashboard // Use this metric definition
                         }
                         Err(fetch_err) => {
                             // If fetching via dashboard fails unexpectedly, return that error
-                            tracing::error!("Failed to fetch metric via dashboard context even though public dashboard exists: {}", fetch_err);
+                            tracing::error!("Failed to fetch metric via dashboard context: {}", fetch_err);
                             return Err(fetch_err);
                         }
                     }
                 } else {
-                    // No public dashboard association found, return the original permission error
-                    tracing::warn!("No valid public dashboard association found for metric. Returning original error.");
+                    // No dashboard access, return the original permission error
+                    tracing::warn!("No dashboard association found for metric. Returning original error.");
                     return Err(e);
                 }
             } else {

apps/api/libs/handlers/src/metrics/get_metric_for_dashboard_handler.rs

@@ -157,10 +157,22 @@ pub async fn get_metric_for_dashboard_handler(
             tracing::debug!(metric_id = %metric_id, user_id = %user.id, ?permission, "Granting access via direct permission.");
         }
     } else {
-        // No sufficient direct/admin permission, check public access rules
-        tracing::debug!(metric_id = %metric_id, "Insufficient direct/admin permission. Checking public access rules.");
+        // No sufficient direct/admin permission, check if user has access via a dashboard
+        tracing::debug!(metric_id = %metric_id, "Insufficient direct/admin permission. Checking dashboard access.");
+        
+        let has_dashboard_access = sharing::check_metric_dashboard_access(metric_id, &user.id)
+            .await
+            .unwrap_or(false);
+            
+        if has_dashboard_access {
+            // User has access to a dashboard containing this metric, grant CanView
+            tracing::debug!(metric_id = %metric_id, user_id = %user.id, "User has access via dashboard. Granting CanView.");
+            permission = AssetPermissionRole::CanView;
+        } else {
+            // No dashboard access, check public access rules
+            tracing::debug!(metric_id = %metric_id, "No dashboard access. Checking public access rules.");
             if !metric_file.publicly_accessible {
-            tracing::warn!(metric_id = %metric_id, user_id = %user.id, "Permission denied (not public, insufficient direct permission).");
+                tracing::warn!(metric_id = %metric_id, user_id = %user.id, "Permission denied (not public, no dashboard access, insufficient direct permission).");
                 return Err(anyhow!("You don't have permission to view this metric"));
             }
             tracing::debug!(metric_id = %metric_id, "Metric is publicly accessible.");
@@ -201,6 +213,7 @@ pub async fn get_metric_for_dashboard_handler(
                 permission = AssetPermissionRole::CanView;
             }
         }
+    }
 
     // Declare variables to hold potentially versioned data
     let resolved_name: String;

apps/api/libs/handlers/src/metrics/get_metric_handler.rs

@@ -155,10 +155,22 @@ pub async fn get_metric_handler(
             tracing::debug!(metric_id = %metric_id, user_id = %user.id, ?permission, "Granting access via direct permission.");
         }
     } else {
-        // No sufficient direct/admin permission, check public access rules
-        tracing::debug!(metric_id = %metric_id, "Insufficient direct/admin permission. Checking public access rules.");
+        // No sufficient direct/admin permission, check if user has access via a dashboard
+        tracing::debug!(metric_id = %metric_id, "Insufficient direct/admin permission. Checking dashboard access.");
+        
+        let has_dashboard_access = sharing::check_metric_dashboard_access(metric_id, &user.id)
+            .await
+            .unwrap_or(false);
+            
+        if has_dashboard_access {
+            // User has access to a dashboard containing this metric, grant CanView
+            tracing::debug!(metric_id = %metric_id, user_id = %user.id, "User has access via dashboard. Granting CanView.");
+            permission = AssetPermissionRole::CanView;
+        } else {
+            // No dashboard access, check public access rules
+            tracing::debug!(metric_id = %metric_id, "No dashboard access. Checking public access rules.");
             if !metric_file.publicly_accessible {
-            tracing::warn!(metric_id = %metric_id, user_id = %user.id, "Permission denied (not public, insufficient direct permission).");
+                tracing::warn!(metric_id = %metric_id, user_id = %user.id, "Permission denied (not public, no dashboard access, insufficient direct permission).");
                 return Err(anyhow!("You don't have permission to view this metric"));
             }
             tracing::debug!(metric_id = %metric_id, "Metric is publicly accessible.");
@@ -199,6 +211,7 @@ pub async fn get_metric_handler(
                 permission = AssetPermissionRole::CanView;
             }
         }
+    }
 
     // Declare variables to hold potentially versioned data
     let resolved_name: String;

apps/api/libs/sharing/src/asset_access_checks.rs

@@ -1,4 +1,8 @@
-use database::enums::{AssetPermissionRole, UserOrganizationRole};
+use database::enums::{AssetPermissionRole, AssetType, IdentityType, UserOrganizationRole};
+use database::pool::get_pg_pool;
+use database::schema::{asset_permissions, dashboard_files, metric_files_to_dashboard_files};
+use diesel::{BoolExpressionMethods, ExpressionMethods, JoinOnDsl, QueryDsl, OptionalExtension};
+use diesel_async::RunQueryDsl;
 use middleware::OrganizationMembership;
 use uuid::Uuid;
 
@@ -39,6 +43,80 @@ pub fn check_permission_access(
     false
 }
 
+/// Checks if a user has access to a metric through any associated dashboard.
+///
+/// This function is used to implement permission cascading from dashboards to metrics.
+/// If a user has access to any dashboard containing the metric (either through direct permissions
+/// or if the dashboard is public), they get at least CanView permission.
+///
+/// # Arguments
+/// * `metric_id` - UUID of the metric to check
+/// * `user_id` - UUID of the user to check permissions for
+///
+/// # Returns
+/// * `Result<bool>` - True if the user has access to any dashboard containing the metric, false otherwise
+pub async fn check_metric_dashboard_access(
+    metric_id: &Uuid,
+    user_id: &Uuid,
+) -> Result<bool, diesel::result::Error> {
+    let mut conn = get_pg_pool().get().await.map_err(|e| {
+        diesel::result::Error::DatabaseError(
+            diesel::result::DatabaseErrorKind::UnableToSendCommand,
+            Box::new(e.to_string()),
+        )
+    })?;
+
+    // First check if user has direct access to any dashboard containing this metric
+    let has_direct_access = metric_files_to_dashboard_files::table
+        .inner_join(
+            dashboard_files::table
+                .on(dashboard_files::id.eq(metric_files_to_dashboard_files::dashboard_file_id)),
+        )
+        .inner_join(
+            asset_permissions::table.on(
+                asset_permissions::asset_id.eq(dashboard_files::id)
+                    .and(asset_permissions::asset_type.eq(AssetType::DashboardFile))
+                    .and(asset_permissions::identity_id.eq(user_id))
+                    .and(asset_permissions::identity_type.eq(IdentityType::User))
+                    .and(asset_permissions::deleted_at.is_null())
+            ),
+        )
+        .filter(metric_files_to_dashboard_files::metric_file_id.eq(metric_id))
+        .filter(dashboard_files::deleted_at.is_null())
+        .filter(metric_files_to_dashboard_files::deleted_at.is_null())
+        .select(dashboard_files::id)
+        .first::<Uuid>(&mut conn)
+        .await
+        .optional()?;
+
+    if has_direct_access.is_some() {
+        return Ok(true);
+    }
+
+    // Now check if metric belongs to any PUBLIC dashboard (not expired)
+    let now = chrono::Utc::now();
+    let has_public_access = metric_files_to_dashboard_files::table
+        .inner_join(
+            dashboard_files::table
+                .on(dashboard_files::id.eq(metric_files_to_dashboard_files::dashboard_file_id)),
+        )
+        .filter(metric_files_to_dashboard_files::metric_file_id.eq(metric_id))
+        .filter(dashboard_files::deleted_at.is_null())
+        .filter(metric_files_to_dashboard_files::deleted_at.is_null())
+        .filter(dashboard_files::publicly_accessible.eq(true))
+        .filter(
+            dashboard_files::public_expiry_date
+                .is_null()
+                .or(dashboard_files::public_expiry_date.gt(now)),
+        )
+        .select(dashboard_files::id)
+        .first::<Uuid>(&mut conn)
+        .await
+        .optional()?;
+
+    Ok(has_public_access.is_some())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

apps/api/libs/sharing/src/lib.rs

@@ -19,4 +19,4 @@ pub use types::{
     SerializableAssetPermission, UserInfo,
 };
 pub use user_lookup::find_user_by_email;
-pub use asset_access_checks::check_permission_access;
+pub use asset_access_checks::{check_permission_access, check_metric_dashboard_access};

apps/web/src/api/createAxiosInstance.ts

@@ -51,6 +51,10 @@ export const defaultAxiosRequestHandler = async (
     token = (await options?.checkTokenValidity()?.then((res) => res?.access_token || '')) || '';
   }
 
+  if (!token) {
+    throw new Error('User authentication error - no token found');
+  }
+
   (config.headers as AxiosRequestHeaders).Authorization = `Bearer ${token}`;
 
   return config;

apps/web/src/app/app/layout.tsx

@@ -28,7 +28,7 @@ export default async function Layout({
 
   const userInfoState = queryClient.getQueryState(queryKeys.userGetUserMyself.queryKey);
 
-  const is402Error = userInfoState?.status === 'error' && userInfoState?.error?.status === 402;
+  const is402Error = userInfoState?.status === 'error' && userInfoState?.error?.status === 402; //402 is the payment required error code
 
   if (is402Error) {
     return <ClientRedirect to={createBusterRoute({ route: BusterRoutes.INFO_GETTING_STARTED })} />;

apps/web/src/context/Supabase/SupabaseContextProvider.tsx

@@ -7,6 +7,9 @@ import { checkTokenValidityFromServer } from '@/api/buster_rest/nextjs/auth';
 import { useMemoizedFn } from '@/hooks/useMemoizedFn';
 import { millisecondsFromUnixTimestamp } from '@/lib/timestamp';
 import type { UseSupabaseUserContextType } from '@/lib/supabase';
+import { timeout } from '@/lib/timeout';
+import { useBusterNotifications } from '../BusterNotifications';
+import { flushSync } from 'react-dom';
 
 const PREEMTIVE_REFRESH_MINUTES = 5;
 
@@ -16,6 +19,7 @@ const useSupabaseContextInternal = ({
   supabaseContext: UseSupabaseUserContextType;
 }) => {
   const refreshTimerRef = useRef<ReturnType<typeof setTimeout> | undefined>(undefined);
+  const { openErrorNotification, openInfoMessage } = useBusterNotifications();
   const [accessToken, setAccessToken] = useState(supabaseContext.accessToken || '');
 
   const isAnonymousUser = !supabaseContext.user?.id || supabaseContext.user?.is_anonymous === true;
@@ -45,7 +49,8 @@ const useSupabaseContextInternal = ({
           accessToken,
           preemptiveRefreshMinutes: PREEMTIVE_REFRESH_MINUTES
         });
-        onUpdateToken({ accessToken: res.access_token, expiresAt: res.expires_at });
+        await onUpdateToken({ accessToken: res.access_token, expiresAt: res.expires_at });
+        await timeout(25);
         return res;
       }
 
@@ -55,6 +60,11 @@ const useSupabaseContextInternal = ({
       };
     } catch (e) {
       console.error(e);
+      openErrorNotification({
+        title: 'Error checking user authentication',
+        description: 'Please try again later',
+        duration: 120 * 1000 //2 minutes
+      });
       throw e;
     }
   });
@@ -62,6 +72,9 @@ const useSupabaseContextInternal = ({
   const onUpdateToken = useMemoizedFn(
     async ({ accessToken, expiresAt: _expiresAt }: { accessToken: string; expiresAt: number }) => {
       setAccessToken(accessToken);
+      flushSync(() => {
+        openInfoMessage('Token refreshed');
+      });
     }
   );
 

apps/web/src/lib/supabase/getSupabaseUserContext.ts

@@ -11,7 +11,11 @@ export const getSupabaseUserContext = async (preemptiveRefreshMinutes = 5) => {
   const supabase = await createClient();
 
   // Get the session first
-  let { data: sessionData } = await supabase.auth.getSession();
+  let { data: sessionData, error: sessionError } = await supabase.auth.getSession();
+
+  if (sessionError) {
+    console.error('Error getting session:', sessionError);
+  }
 
   // Check if we need to refresh the session
   if (sessionData.session) {
@@ -29,10 +33,15 @@ export const getSupabaseUserContext = async (preemptiveRefreshMinutes = 5) => {
   }
 
   // Get user data
-  const { data: userData } = await supabase.auth.getUser();
+  const { data: userData, error: userError } = await supabase.auth.getUser();
+
+  if (userError) {
+    console.error('Error getting user:', userData, userError);
+  }
 
   if (!userData.user) {
     const { session: anonSession } = await signInWithAnonymousUser();
+    console.info('created anon session', anonSession);
     return {
       user: anonSession?.user || null,
       accessToken: anonSession?.access_token
@@ -42,6 +51,11 @@ export const getSupabaseUserContext = async (preemptiveRefreshMinutes = 5) => {
   const user = userData.user;
   const accessToken = sessionData.session?.access_token;
   const refreshToken = sessionData.session?.refresh_token;
+
+  if (!accessToken) {
+    console.error('No access token found for user:', user);
+  }
+
   return { user, accessToken, refreshToken };
 };