Merge branch 'fix/53-check-db-user-permissions' into 'main'

Sarumyan · Sarumyan · commit 80da1e842f7d · 2026-04-03T18:51:53.000Z
fix: check DB user permissions on checkup startup Closes #53 See merge request postgres-ai/postgresai!232
diff --git a/cli/bin/postgres-ai.ts b/cli/bin/postgres-ai.ts
@@ -15,7 +15,7 @@ import { fetchIssues, fetchIssueComments, createIssueComment, fetchIssue, create
 import { fetchReports, fetchAllReports, fetchReportFiles, fetchReportFileData, renderMarkdownForTerminal, parseFlexibleDate } from "../lib/reports";
 import { resolveBaseUrls } from "../lib/util";
 import { uploadFile, downloadFile, buildMarkdownLink } from "../lib/storage";
-import { applyInitPlan, applyUninitPlan, buildInitPlan, buildUninitPlan, connectWithSslFallback, DEFAULT_MONITORING_USER, KNOWN_PROVIDERS, redactPasswordsInSql, resolveAdminConnection, resolveMonitoringPassword, validateProvider, verifyInitSetup } from "../lib/init";
+import { applyInitPlan, applyUninitPlan, buildInitPlan, buildUninitPlan, checkCurrentUserPermissions, connectWithSslFallback, DEFAULT_MONITORING_USER, formatPermissionCheckMessages, KNOWN_PROVIDERS, redactPasswordsInSql, resolveAdminConnection, resolveMonitoringPassword, validateProvider, verifyInitSetup } from "../lib/init";
 import { SupabaseClient, resolveSupabaseConfig, extractProjectRefFromUrl, applyInitPlanViaSupabase, verifyInitSetupViaSupabase, fetchPoolerDatabaseUrl, type PgCompatibleError } from "../lib/supabase";
 import * as pkce from "../lib/pkce";
 import * as authServer from "../lib/auth-server";
@@ -1817,6 +1817,24 @@ program
       const connResult = await connectWithSslFallback(Client, adminConn);
       client = connResult.client as Client;
 
+      // Preflight: verify the connected user has sufficient permissions
+      spinner.update("Checking database permissions");
+      const permCheck = await checkCurrentUserPermissions(client);
+      const permMessages = formatPermissionCheckMessages(permCheck);
+
+      for (const w of permMessages.warnings) {
+        console.error(w);
+      }
+
+      if (permMessages.failed) {
+        spinner.stop();
+        for (const e of permMessages.errors) {
+          console.error(e);
+        }
+        process.exitCode = 1;
+        return;
+      }
+
       // Generate reports
       let reports: Record<string, any>;
       if (checkId === "ALL") {
diff --git a/cli/lib/init.ts b/cli/lib/init.ts
@@ -671,6 +671,36 @@ export type VerifyInitResult = {
   missingOptional: string[];
 };
 
+/** A single permission check result from the preflight query. */
+export type PermissionCheckRow = {
+  permission_name: string;
+  status: "required" | "optional";
+  /**
+   * Whether the permission is granted.
+   * - `true`  — permission is granted
+   * - `false` — permission is explicitly denied
+   * - `null`  — check was skipped (e.g., object does not exist, so the privilege
+   *             check is inapplicable — such as SELECT on a view that hasn't been created)
+   */
+  granted: boolean | null;
+  fix_command: string | null;
+};
+
+/**
+ * Result of the preflight permission check for the current DB user.
+ *
+ * - `ok` is `true` when `missingRequired` is empty.
+ * - `rows` contains every check (for inspection / logging).
+ * - `missingRequired` / `missingOptional` are filtered subsets of `rows`
+ *   where the permission is not granted (`granted !== true`).
+ */
+export type PreflightPermissionResult = {
+  ok: boolean;
+  rows: PermissionCheckRow[];
+  missingRequired: PermissionCheckRow[];
+  missingOptional: PermissionCheckRow[];
+};
+
 export type UninitPlan = {
   monitoringUser: string;
   database: string;
@@ -825,7 +855,12 @@ export async function verifyInitSetup(params: {
       missingRequired.push("USAGE on schema postgres_ai");
     }
 
-    const viewExistsRes = await params.client.query("select to_regclass('postgres_ai.pg_statistic') is not null as ok");
+    const viewExistsRes = await params.client.query(`
+      select case
+        when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+        else to_regclass('postgres_ai.pg_statistic') is not null
+      end as ok
+    `);
     if (!viewExistsRes.rows?.[0]?.ok) {
       missingRequired.push("view postgres_ai.pg_statistic exists");
     } else {
@@ -948,4 +983,149 @@ export async function verifyInitSetup(params: {
   }
 }
 
+/**
+ * Check that the currently connected DB user has sufficient permissions for
+ * monitoring operations. Returns structured results with fix commands.
+ *
+ * Required permissions cause startup to fail; optional ones produce warnings.
+ *
+ * @param client  An already-connected PostgreSQL client.
+ * @returns       A {@link PreflightPermissionResult} with per-check rows and
+ *                filtered `missingRequired` / `missingOptional` arrays.
+ * @throws        Propagates database errors (network, permission denied on catalog
+ *                tables, timeout) to the caller.
+ */
+export async function checkCurrentUserPermissions(
+  client: PgClient
+): Promise<PreflightPermissionResult> {
+  const sql = `
+    with permission_checks as (
+      select
+        format('connect on database %I', current_database()) as permission_name,
+        'required' as status,
+        has_database_privilege(current_user, current_database(), 'connect') as granted
+
+      union all
+
+      select
+        'pg_monitor role membership' as permission_name,
+        'required' as status,
+        -- CASE guarantees evaluation order: pg_has_role() is only called if the
+        -- pg_monitor role exists, avoiding ERROR on PostgreSQL < 10 or when dropped.
+        case
+          when not exists (select from pg_roles where rolname = 'pg_monitor')
+          then false
+          else pg_has_role(current_user, 'pg_monitor', 'member')
+        end as granted
+
+      union all
+
+      select
+        'select on pg_catalog.pg_index' as permission_name,
+        'required' as status,
+        has_table_privilege(current_user, 'pg_catalog.pg_index', 'select') as granted
+
+      union all
+
+      select
+        'postgres_ai.pg_statistic view exists' as permission_name,
+        'optional' as status,
+        case
+          when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+          else to_regclass('postgres_ai.pg_statistic') is not null
+        end as granted
+
+      union all
+
+      select
+        'select on postgres_ai.pg_statistic' as permission_name,
+        'optional' as status,
+        case
+          when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+          when to_regclass('postgres_ai.pg_statistic') is null then null
+          else has_table_privilege(current_user, 'postgres_ai.pg_statistic', 'select')
+        end as granted
+    )
+    select
+      permission_name,
+      status,
+      granted,
+      case
+        when status = 'required' and not coalesce(granted, false) then
+          case
+            when permission_name like 'connect%' then
+              format('grant connect on database %I to %I;', current_database(), current_user)
+            when permission_name = 'pg_monitor role membership' then
+              format('grant pg_monitor to %I;', current_user)
+            when permission_name like 'select on pg_catalog.pg_index' then
+              format('grant select on pg_catalog.pg_index to %I;', current_user)
+          end
+        when permission_name = 'postgres_ai.pg_statistic view exists' and granted = false then
+          '-- create postgres_ai.pg_statistic view (see setup script)'
+        when permission_name = 'select on postgres_ai.pg_statistic' and granted = false then
+          format('grant select on postgres_ai.pg_statistic to %I;', current_user)
+        else null
+      end as fix_command
+    from permission_checks
+    order by
+      case status when 'required' then 1 else 2 end,
+      permission_name;
+  `;
+
+  const res = await client.query(sql);
+  const rows: PermissionCheckRow[] = res.rows;
+
+  // Required: treat null (skipped) as not-granted — fail safe.
+  // Optional: only explicit false counts as missing; null means the check was
+  //           skipped (e.g., view doesn't exist) and is not actionable.
+  const missingRequired = rows.filter((r) => r.status === "required" && r.granted !== true);
+  const missingOptional = rows.filter((r) => r.status === "optional" && r.granted === false);
+
+  return {
+    ok: missingRequired.length === 0,
+    rows,
+    missingRequired,
+    missingOptional,
+  };
+}
+
+/**
+ * Format permission check results into user-facing error/warning lines.
+ *
+ * @returns An object with `warnings` (for optional misses), `errors` (for
+ *          required misses including fix SQL), and `failed` (whether required
+ *          permissions are missing).
+ */
+export function formatPermissionCheckMessages(result: PreflightPermissionResult): {
+  failed: boolean;
+  warnings: string[];
+  errors: string[];
+} {
+  const warnings: string[] = [];
+  const errors: string[] = [];
+
+  for (const row of result.missingOptional) {
+    const fix = row.fix_command ? ` Fix: ${row.fix_command}` : "";
+    warnings.push(`Warning: optional permission missing — ${row.permission_name}.${fix}`);
+  }
 
+  if (!result.ok) {
+    errors.push("Error: the database user is missing required permissions.\n");
+    errors.push("Missing permissions:");
+    for (const row of result.missingRequired) {
+      errors.push(`  - ${row.permission_name}`);
+    }
+    const fixes = result.missingRequired
+      .map((r) => r.fix_command)
+      .filter(Boolean);
+    if (fixes.length > 0) {
+      errors.push("\nTo fix, run the following as a superuser:\n");
+      for (const fix of fixes) {
+        errors.push(`  ${fix}`);
+      }
+    }
+    errors.push("\nAlternatively, run 'postgresai prepare-db' to set up permissions automatically.");
+  }
+
+  return { failed: !result.ok, warnings, errors };
+}
diff --git a/cli/lib/supabase.ts b/cli/lib/supabase.ts
@@ -673,7 +673,10 @@ export async function verifyInitSetupViaSupabase(params: {
 
   // Check pg_statistic view
   const viewExistsRes = await params.client.query(
-    "SELECT to_regclass('postgres_ai.pg_statistic') IS NOT NULL as ok",
+    `SELECT CASE
+      WHEN NOT has_schema_privilege(current_user, 'postgres_ai', 'USAGE') THEN NULL
+      ELSE to_regclass('postgres_ai.pg_statistic') IS NOT NULL
+    END as ok`,
     true
   );
   if (!viewExistsRes.rows?.[0]?.ok) {
diff --git a/cli/test/PERMISSION_CHECK_TEST_SUMMARY.md b/cli/test/PERMISSION_CHECK_TEST_SUMMARY.md
@@ -0,0 +1,139 @@
+# Permission Check Test Summary
+
+## Changes Made
+
+Changed all references from `public.pg_statistic` to `postgres_ai.pg_statistic` in:
+- `cli/lib/init.ts` - Permission check SQL query
+- `cli/test/init.test.ts` - All test expectations (28 occurrences)
+
+## Key Fix: Safe Schema Checking
+
+**Before (883fa95):**
+```sql
+exists (
+  select from pg_views
+  where schemaname = 'public' and viewname = 'pg_statistic'
+) as granted
+```
+
+**After (6db79f6) - INCORRECT, caused crashes:**
+```sql
+to_regclass('postgres_ai.pg_statistic') is not null as granted
+```
+
+**Current (this fix):**
+```sql
+case
+  when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+  else to_regclass('postgres_ai.pg_statistic') is not null
+end as granted
+```
+
+### Why this fix matters
+
+**Issue with bare `to_regclass()`:**
+- Returns NULL when the schema doesn't exist ✓
+- Returns NULL when the view doesn't exist ✓
+- **Throws error** when the schema exists but user lacks USAGE privilege ✗
+
+**Fix:**
+- Check `has_schema_privilege()` first to avoid the permission error
+- Returns NULL safely in all cases where we can't check the view
+- Prevents crashes when postgres_ai schema exists but user lacks USAGE
+
+## Test Results
+
+### Unit Tests ✅
+```
+✓ 95 tests passed across 3 files
+  - 84 tests in init.test.ts (including 9 checkCurrentUserPermissions tests)
+  - 2 tests in config-consistency.test.ts
+  - 9 tests in permission-check-sql.test.ts
+```
+
+### Expected Behavior by Scenario
+
+| Scenario | User Permissions | postgres_ai Schema | Expected Result |
+|----------|-----------------|-------------------|-----------------|
+| 1. Superuser | superuser + postgres_ai.pg_statistic | ✓ Exists | ✅ PASS (clean) |
+| 2. pg_monitor, no schema access | pg_monitor only | ✗ No USAGE | ✅ PASS (warning) |
+| 3. No pg_monitor | minimal permissions | ✗ Doesn't exist | ✅ PASS (error + fix SQL) |
+| 8. After prepare-db | pg_monitor + postgres_ai grants | ✓ Exists + SELECT | ✅ PASS (clean) |
+
+### SQL Behavior Verification
+
+**Scenario 2 & 3: Schema doesn't exist or no USAGE**
+```sql
+-- Check privilege first, then to_regclass (no crash)
+case
+  when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+  else to_regclass('postgres_ai.pg_statistic') is not null
+end  → NULL
+
+-- SELECT check is skipped (returns NULL, not treated as missing optional)
+case
+  when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+  when to_regclass('postgres_ai.pg_statistic') is null then null
+  else has_table_privilege(current_user, 'postgres_ai.pg_statistic', 'select')
+end  → NULL
+```
+
+**Scenario 1 & 8: Schema exists with proper grants**
+```sql
+-- User has USAGE, to_regclass returns OID (view is visible)
+case
+  when not has_schema_privilege(current_user, 'postgres_ai', 'USAGE') then null
+  else to_regclass('postgres_ai.pg_statistic') is not null
+end  → TRUE
+
+-- SELECT check is performed
+has_table_privilege(current_user, 'postgres_ai.pg_statistic', 'select')  → TRUE/FALSE
+```
+
+## Integration Test Limitations
+
+Integration tests cannot run due to locale configuration issues with `initdb`:
+```
+error: initdb: error: invalid locale settings; check LANG and LC_* environment variables
+```
+
+However, unit tests provide comprehensive coverage of the permission check logic, including:
+- All permission scenarios (granted, denied, skipped)
+- Multiple missing permissions
+- Error propagation
+- Fix command generation
+- Message formatting
+
+## Schema Consistency
+
+The change ensures consistency across the codebase:
+- ✅ `cli/lib/init.ts` - now checks postgres_ai.pg_statistic
+- ✅ `cli/lib/supabase.ts` - already checks postgres_ai.pg_statistic
+- ✅ `cli/sql/03.permissions.sql` - creates postgres_ai.pg_statistic
+- ✅ `config/target-db/init.sql` - creates postgres_ai.pg_statistic
+- ✅ `config/pgwatch-prometheus/metrics.yml` - references postgres_ai.pg_statistic
+
+## Commits
+
+1. **955cff2** - `fix: change public.pg_statistic to postgres_ai.pg_statistic`
+   - Updated permission check queries
+   - Updated all test expectations
+
+2. **6db79f6** - `fix: use to_regclass() for safe postgres_ai.pg_statistic check`
+   - Replaced pg_views query with to_regclass()
+   - ⚠️ This introduced a bug: crashes when schema exists but user lacks USAGE
+
+3. **[current]** - `fix: wrap to_regclass() with has_schema_privilege() check`
+   - Fixed crash when postgres_ai schema exists but user lacks USAGE privilege
+   - Added privilege check before calling to_regclass() in all locations
+   - Updated in: init.ts (3 places) and supabase.ts (1 place)
+
+## Verification Command
+
+```bash
+# Run all permission-related tests
+bun test test/init.test.ts test/config-consistency.test.ts test/permission-check-sql.test.ts
+
+# Verify no public.pg_statistic references remain (except in comments)
+git grep -n 'public\.pg_statistic' cli/
+```
diff --git a/cli/test/init.test.ts b/cli/test/init.test.ts
diff --git a/cli/test/permission-check-sql.test.ts b/cli/test/permission-check-sql.test.ts
