From fbf9f04e107c674474f61c1559a2f1e56dfeeaba Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 28 Jul 2026 14:29:06 +0200 Subject: [PATCH] fix: surface the real postgres error when data table migrations fail (#10371) * fix: surface the real postgres error when data table migrations fail Co-Authored-By: Claude Opus 5 (1M context) * fix: address review nits on the data table migration error fix Co-Authored-By: Claude Opus 5 (1M context) * fix: name the exact grant a data table migration needs Co-Authored-By: Claude Opus 5 (1M context) * fix: quote both identifiers in the data table grant hint Co-Authored-By: Claude Opus 5 (1M context) * feat: add a data table connection and privilege check to workspace settings Co-Authored-By: Claude Opus 5 (1M context) * fix: report data table privileges from the capability fields, not the grant list Co-Authored-By: Claude Opus 5 (1M context) * fix: read grant targets from the server and drop the public schema guess Co-Authored-By: Claude Opus 5 (1M context) * fix: render the search_path suggestion server-side and pin the granted database Co-Authored-By: Claude Opus 5 (1M context) * fix: key the connection check on request identity, not the data table name Co-Authored-By: Claude Opus 5 (1M context) * fix: declare the data table check schema field nullable and required Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/tests/datatable_migrations_grants.rs | 300 ++++++++++++++++++ .../fixtures/datatable_migrations_grants.sql | 35 ++ backend/windmill-api-settings/src/lib.rs | 4 +- .../src/datatable_migrations.rs | 125 +++++++- .../windmill-api-workspaces/src/workspaces.rs | 156 ++++++++- backend/windmill-api/openapi.yaml | 46 +++ backend/windmill-common/src/error.rs | 87 +++++ backend/windmill-common/src/lib.rs | 2 +- backend/windmill-common/src/query_builders.rs | 8 +- .../DataTableSettings.svelte | 116 ++++++- 10 files changed, 849 insertions(+), 30 deletions(-) create mode 100644 backend/tests/datatable_migrations_grants.rs create mode 100644 backend/tests/fixtures/datatable_migrations_grants.sql diff --git a/backend/tests/datatable_migrations_grants.rs b/backend/tests/datatable_migrations_grants.rs new file mode 100644 index 0000000000..ebe9aeba4d --- /dev/null +++ b/backend/tests/datatable_migrations_grants.rs @@ -0,0 +1,300 @@ +//! Regression test for running data table migrations against a database whose +//! role only holds DML grants. +//! +//! Two failure modes are pinned here: +//! - the Postgres message must reach the caller. `tokio_postgres::Error`'s +//! `Display` renders only the error kind, so interpolating it with `{}` +//! produced a bare `Failed to ensure _wm_migrations table: db error`. +//! - `CREATE TABLE IF NOT EXISTS` checks CREATE on the schema *before* it +//! checks existence, so the run must probe for `_wm_migrations` first or an +//! unprivileged role can never migrate, even against a pre-created table. +//! +//! Plus the privilege report that surfaces the same state from workspace +//! settings before anyone reaches a migration. + +use serde_json::{json, Value}; +use sqlx::{Pool, Postgres}; + +use windmill_test_utils::*; + +const ROLE: &str = "wm_dtmig_test_role"; +const ROLE_PASSWORD: &str = "wm_dtmig_test_pwd"; +/// Deliberately hyphenated: it only parses inside double quotes, so it pins that +/// the emitted recovery statement quotes the role rather than interpolating it. +const NOSCHEMA_ROLE: &str = "wm-dtmig-noschema"; + +fn authed(b: reqwest::RequestBuilder) -> reqwest::RequestBuilder { + b.header("Authorization", "Bearer DTMIG_ADMIN_TOKEN") +} + +/// Point the fixture's data table at this test's own database, connecting as a +/// role that may read and write but not create: `GRANT USAGE` without `CREATE`, +/// and the schema's own CREATE revoked from PUBLIC so the outcome does not +/// depend on the server's default `public` grants (relaxed before Postgres 15). +async fn setup_unprivileged_datatable_role(db: &Pool) -> anyhow::Result<()> { + let opts = (*db.connect_options()).clone(); + let dbname = opts.get_database().expect("test database name").to_string(); + + sqlx::query(&format!( + // Roles are cluster objects, not per-test-database ones. A previous run + // leaving the role behind raises duplicate_object; the tests in this + // binary run in parallel, so two sessions can also clear that check + // together and collide on pg_authid's unique index instead. + "DO $$ BEGIN \ + CREATE ROLE {ROLE} LOGIN PASSWORD '{ROLE_PASSWORD}'; \ + EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; \ + END $$" + )) + .execute(db) + .await?; + sqlx::raw_sql(&format!( + "REVOKE CREATE ON SCHEMA public FROM PUBLIC; \ + GRANT CONNECT ON DATABASE \"{dbname}\" TO {ROLE}; \ + GRANT USAGE ON SCHEMA public TO {ROLE};" + )) + .execute(db) + .await?; + + sqlx::query( + "INSERT INTO resource (workspace_id, path, value, resource_type, created_by) \ + VALUES ('dtmig-ws', 'u/dtmig-admin/pg', $1, 'postgresql', 'dtmig-admin')", + ) + .bind(json!({ + "host": opts.get_host(), + "port": opts.get_port(), + "dbname": dbname, + "user": ROLE, + "password": ROLE_PASSWORD, + "sslmode": "disable", + })) + .execute(db) + .await?; + + Ok(()) +} + +#[sqlx::test(fixtures("datatable_migrations_grants"))] +async fn test_run_migrations_without_create_privilege(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + setup_unprivileged_datatable_role(&db).await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let url = + format!("http://localhost:{port}/api/w/dtmig-ws/workspaces/run_datatable_migrations/main"); + + // No `_wm_migrations` yet and no way to create one: the caller must be told + // what Postgres actually refused, not "db error". + let resp = authed(reqwest::Client::new().post(&url)).send().await?; + assert_eq!(resp.status(), 500); + let body = resp.text().await?; + assert!( + body.contains("permission denied for schema"), + "the Postgres message should reach the caller, got: {body}" + ); + // The suggested statement must be complete and quoted, not a placeholder. + assert!( + body.contains(&format!("GRANT CREATE ON SCHEMA \"public\" TO \"{ROLE}\"")), + "the hint should name the actual role and schema, got: {body}" + ); + + // Once an operator has created the bookkeeping table and granted DML on it, + // migrations run even though the role still cannot create tables. + sqlx::raw_sql(&format!( + "CREATE TABLE _wm_migrations ( \ + datatable TEXT NOT NULL, \ + version BIGINT NOT NULL, \ + installed_at TIMESTAMPTZ NOT NULL DEFAULT now(), \ + PRIMARY KEY (datatable, version)); \ + GRANT SELECT, INSERT, UPDATE, DELETE ON _wm_migrations TO {ROLE};" + )) + .execute(&db) + .await?; + + let resp = authed(reqwest::Client::new().post(&url)).send().await?; + let status = resp.status(); + let body = resp.text().await?; + assert_eq!( + status, 200, + "run should succeed on a pre-created table: {body}" + ); + + Ok(()) +} + +#[sqlx::test(fixtures("datatable_migrations_grants"))] +async fn test_datatable_connection_report(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + setup_unprivileged_datatable_role(&db).await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let url = + format!("http://localhost:{port}/api/w/dtmig-ws/workspaces/test_datatable_connection/main"); + + // The report is a privilege disclosure about the data table's database, so + // it stays behind the same bar as editing the data table config. + let resp = reqwest::Client::new() + .get(&url) + .header("Authorization", "Bearer DTMIG_USER_TOKEN") + .send() + .await?; + assert_eq!(resp.status(), 403, "non-admins must not get the report"); + + let report: Value = authed(reqwest::Client::new().get(&url)) + .send() + .await? + .json() + .await?; + assert_eq!(report["user"], ROLE); + assert_eq!(report["schema"], "public"); + assert_eq!(report["can_create_table"], false); + assert_eq!(report["can_create_schema"], false); + let grants = report["suggested_grants"].as_array().unwrap(); + assert!( + grants + .iter() + .any(|g| g.as_str().unwrap() + == format!("GRANT CREATE ON SCHEMA \"public\" TO \"{ROLE}\"")), + "missing schema grant: {report}" + ); + // Pin the name, not just the shape: the endpoint reads it from + // `current_database()` rather than the resource, and a prefix assertion + // would pass either way. + let dbname = (*db.connect_options()) + .clone() + .get_database() + .expect("test database name") + .to_string(); + assert!( + grants.iter().any(|g| g.as_str().unwrap() + == format!("GRANT CREATE ON DATABASE \"{dbname}\" TO \"{ROLE}\"")), + "missing database grant for {dbname}: {report}" + ); + + // A pre-created bookkeeping table lets migration *tracking* work, but the + // role still cannot create anything: the report must keep saying so rather + // than falling silent because nothing needs creating right now. + sqlx::raw_sql(&format!( + "CREATE TABLE _wm_migrations ( \ + datatable TEXT NOT NULL, \ + version BIGINT NOT NULL, \ + installed_at TIMESTAMPTZ NOT NULL DEFAULT now(), \ + PRIMARY KEY (datatable, version)); \ + GRANT SELECT, INSERT, UPDATE, DELETE ON _wm_migrations TO {ROLE};" + )) + .execute(&db) + .await?; + + let report: Value = authed(reqwest::Client::new().get(&url)) + .send() + .await? + .json() + .await?; + assert_eq!(report["migrations_table_exists"], true); + assert_eq!(report["can_create_table"], false); + assert!( + report["suggested_grants"] + .as_array() + .unwrap() + .iter() + .any(|g| g.as_str().unwrap().contains("ON SCHEMA")), + "an existing bookkeeping table must not suppress the schema grant: {report}" + ); + + // Granting the privileges clears the suggestions. + sqlx::raw_sql(&format!( + "GRANT CREATE ON SCHEMA public TO {ROLE}; \ + GRANT CREATE ON DATABASE \"{dbname}\" TO {ROLE};" + )) + .execute(&db) + .await?; + + let report: Value = authed(reqwest::Client::new().get(&url)) + .send() + .await? + .json() + .await?; + assert_eq!(report["can_create_table"], true); + assert_eq!(report["can_create_schema"], true); + assert_eq!(report["suggested_grants"].as_array().unwrap().len(), 0); + + Ok(()) +} + +/// Point the fixture's second data table at a role whose `search_path` resolves +/// to nothing, the one state where no grant helps. +async fn setup_schemaless_datatable_role(db: &Pool) -> anyhow::Result<()> { + let opts = (*db.connect_options()).clone(); + let dbname = opts.get_database().expect("test database name").to_string(); + + sqlx::query(&format!( + "DO $$ BEGIN \ + CREATE ROLE \"{NOSCHEMA_ROLE}\" LOGIN PASSWORD '{ROLE_PASSWORD}'; \ + EXCEPTION WHEN duplicate_object OR unique_violation THEN NULL; \ + END $$" + )) + .execute(db) + .await?; + // Cluster-wide for this role, which is why it gets one of its own rather + // than sharing the role the other assertions connect with. + sqlx::raw_sql(&format!( + "ALTER ROLE \"{NOSCHEMA_ROLE}\" SET search_path = wm_dtmig_absent_schema; \ + GRANT CONNECT ON DATABASE \"{dbname}\" TO \"{NOSCHEMA_ROLE}\";" + )) + .execute(db) + .await?; + + sqlx::query( + "INSERT INTO resource (workspace_id, path, value, resource_type, created_by) \ + VALUES ('dtmig-ws', 'u/dtmig-admin/pg_noschema', $1, 'postgresql', 'dtmig-admin')", + ) + .bind(json!({ + "host": opts.get_host(), + "port": opts.get_port(), + "dbname": dbname, + "user": NOSCHEMA_ROLE, + "password": ROLE_PASSWORD, + "sslmode": "disable", + })) + .execute(db) + .await?; + + Ok(()) +} + +#[sqlx::test(fixtures("datatable_migrations_grants"))] +async fn test_datatable_connection_without_a_resolvable_schema( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + setup_schemaless_datatable_role(&db).await?; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let report: Value = authed(reqwest::Client::new().get(format!( + "http://localhost:{port}/api/w/dtmig-ws/workspaces/test_datatable_connection/noschema" + ))) + .send() + .await? + .json() + .await?; + + assert!(report["schema"].is_null(), "expected no schema: {report}"); + // No grant fixes an empty search_path, so suggesting one would send the + // reader after a statement that changes nothing. + assert!( + !report["suggested_grants"] + .as_array() + .unwrap() + .iter() + .any(|g| g.as_str().unwrap().contains("ON SCHEMA")), + "an empty search_path must not yield a schema grant: {report}" + ); + assert_eq!( + report["suggested_search_path"], + format!("ALTER ROLE \"{NOSCHEMA_ROLE}\" SET search_path = public") + ); + + Ok(()) +} diff --git a/backend/tests/fixtures/datatable_migrations_grants.sql b/backend/tests/fixtures/datatable_migrations_grants.sql new file mode 100644 index 0000000000..81feb9ce91 --- /dev/null +++ b/backend/tests/fixtures/datatable_migrations_grants.sql @@ -0,0 +1,35 @@ +-- Fixture for the data table migration bookkeeping-grants regression test. +-- Workspace + admin token + a data table pointing at a postgres resource; the +-- test fills that resource in with credentials for a deliberately unprivileged +-- role, since the database name is allocated per test run. + +INSERT INTO workspace (id, name, owner) VALUES + ('dtmig-ws', 'DTMIG WS', 'dtmig-admin'); + +INSERT INTO workspace_key (workspace_id, kind, key) VALUES + ('dtmig-ws', 'cloud', 'dtmig-key'); + +INSERT INTO workspace_settings (workspace_id, datatable) VALUES + ('dtmig-ws', '{"datatables": {"main": {"database": {"resource_type": "postgresql", "resource_path": "u/dtmig-admin/pg"}, "migrations_enabled": true}, "noschema": {"database": {"resource_type": "postgresql", "resource_path": "u/dtmig-admin/pg_noschema"}, "migrations_enabled": true}}}'); + +INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('dtmig-ws', 'all', 'All users', '{}'); + +INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('dtmig-admin@windmill.dev', 'x', 'password', true, true, 'DTMIG Admin', 'dtmig-admin'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('dtmig-ws', 'dtmig-admin@windmill.dev', 'dtmig-admin', true, 'Admin'); + +INSERT INTO token(token_hash, token_prefix, token, email, label, super_admin) + VALUES (encode(sha256('DTMIG_ADMIN_TOKEN'::bytea), 'hex'), 'DTMIG_ADM', 'DTMIG_ADMIN_TOKEN', 'dtmig-admin@windmill.dev', 't', true); + +-- Non-admin member, to pin that the privilege report stays admin-only. +INSERT INTO password(email, password_hash, login_type, super_admin, verified, name, username) + VALUES ('dtmig-user@windmill.dev', 'x', 'password', false, true, 'DTMIG User', 'dtmig-user'); + +INSERT INTO usr(workspace_id, email, username, is_admin, role) VALUES + ('dtmig-ws', 'dtmig-user@windmill.dev', 'dtmig-user', false, 'User'); + +INSERT INTO token(token_hash, token_prefix, token, email, label) + VALUES (encode(sha256('DTMIG_USER_TOKEN'::bytea), 'hex'), 'DTMIG_USR', 'DTMIG_USER_TOKEN', 'dtmig-user@windmill.dev', 't'); diff --git a/backend/windmill-api-settings/src/lib.rs b/backend/windmill-api-settings/src/lib.rs index bd34ca35ef..eca0001eab 100644 --- a/backend/windmill-api-settings/src/lib.rs +++ b/backend/windmill-api-settings/src/lib.rs @@ -53,7 +53,7 @@ use windmill_common::{ auth::is_super_admin_email, ee_oss::{get_license_plan, LicensePlan}, email_oss::send_email_plain_text, - error::{self, JsonResult, Result}, + error::{self, pg_error_message, JsonResult, Result}, get_database_url, global_settings::{ AI_CONFIG_SETTING, APP_WORKSPACED_ROUTE_SETTING, AUTOMATE_USERNAME_CREATION_SETTING, @@ -1705,7 +1705,7 @@ async fn setup_custom_instance_pg_database_inner( .map_err(|e| { error::Error::ExecutionErr(format!( "Failed to grant permissions to custom_instance_user: {}", - e.to_string(), + pg_error_message(&e), )) })?; diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index fef9963933..73e2fb85cc 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -21,14 +21,16 @@ use chrono::Utc; use serde::{Deserialize, Serialize}; use sqlx::{Postgres, Transaction}; use std::collections::{HashMap, HashSet}; +use tokio_postgres::error::SqlState; use windmill_api_auth::{require_super_admin, ApiAuthed}; use windmill_api_jobs::run_wait_result_internal; use windmill_audit::audit_oss::audit_log; use windmill_audit::ActionKind; use windmill_common::db::UserDB; -use windmill_common::error::{Error, JsonResult, Result}; +use windmill_common::error::{pg_error_message, Error, JsonResult, Result}; use windmill_common::jobs::{JobPayload, RawCode}; +use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::runnable_settings::{ConcurrencySettingsWithCustom, DebouncingSettings}; use windmill_common::scripts::ScriptLang; use windmill_common::users::username_to_permissioned_as; @@ -222,7 +224,31 @@ async fn run_datatable_migration_job( /// key would let one data table's migration mark another's same-version /// migration as already applied (and rollback could touch the wrong row). async fn ensure_wm_migrations_schema(client: &tokio_postgres::Client) -> Result<()> { - client + // `CREATE TABLE IF NOT EXISTS` checks CREATE on the schema before it checks + // existence, so probing first is what lets a data table whose role only holds + // DML grants keep migrating against an already-created bookkeeping table. + // `to_regclass` resolves through search_path, like the unqualified statements + // the rest of this module runs against it. Takes no parameters, so it goes + // through the simple protocol: a named prepared statement is what stalls + // behind a transaction-pooling proxy (see `pg_get_full_schema`). + let rows = client + .simple_query("SELECT to_regclass('_wm_migrations') IS NOT NULL AS present") + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to look up _wm_migrations table: {}", + pg_error_message(&e) + )) + })?; + let exists = rows.iter().any(|msg| match msg { + tokio_postgres::SimpleQueryMessage::Row(row) => row.get("present") == Some("t"), + _ => false, + }); + if exists { + return Ok(()); + } + + let Err(e) = client .batch_execute( "CREATE TABLE IF NOT EXISTS _wm_migrations (\ datatable TEXT NOT NULL, \ @@ -231,10 +257,53 @@ async fn ensure_wm_migrations_schema(client: &tokio_postgres::Client) -> Result< PRIMARY KEY (datatable, version))", ) .await - .map_err(|e| { - Error::internal_err(format!("Failed to ensure _wm_migrations table: {}", e)) - })?; - Ok(()) + else { + return Ok(()); + }; + + let mut msg = format!( + "Failed to ensure _wm_migrations table: {}", + pg_error_message(&e) + ); + // A role with only table-level grants cannot create it: since Postgres 15 the + // `public` schema no longer grants CREATE to PUBLIC, so this is the usual + // failure on a bring-your-own database. + if e.code() == Some(&SqlState::INSUFFICIENT_PRIVILEGE) { + // Windmill connects as the role that lacks the privilege, so it cannot + // grant it: hand over the statement a schema owner has to run instead. + // Keep it ahead of the explanation below — the UI collapses everything + // past the first couple of lines behind a "Show more". + if let Some((user, schema)) = connection_identity(client).await { + // Both come back unquoted, so a mixed-case or hyphenated name would + // otherwise render a statement that targets a different schema. + msg.push_str(&format!( + ". Run: GRANT CREATE ON SCHEMA {} TO {}", + render_db_quoted_identifier(&schema, DbType::Postgresql), + render_db_quoted_identifier(&user, DbType::Postgresql), + )); + } + msg.push_str( + ". Applied migrations are recorded in a `_wm_migrations` table in the data \ + table's own database, so its user needs to be able to create it", + ); + } + Err(Error::internal_err(msg)) +} + +/// The role and default schema of a data table connection, for grant hints. +/// Both come from the server so the statement we suggest names what the +/// connection actually resolves to, not what the resource happens to say. +async fn connection_identity(client: &tokio_postgres::Client) -> Option<(String, String)> { + let rows = client + .simple_query("SELECT current_user AS usr, current_schema() AS sch") + .await + .ok()?; + rows.iter().find_map(|msg| match msg { + tokio_postgres::SimpleQueryMessage::Row(row) => { + Some((row.get("usr")?.to_string(), row.get("sch")?.to_string())) + } + _ => None, + }) } /// Open a connection to a data table's own database and hold the session-level @@ -265,7 +334,12 @@ async fn lock_datatable_migration_runs( client .batch_execute("SELECT pg_advisory_lock(hashtext('windmill_datatable_migrations')::int8)") .await - .map_err(|e| Error::internal_err(format!("Failed to acquire migration lock: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!( + "Failed to acquire migration lock: {}", + pg_error_message(&e) + )) + })?; Ok(client) } @@ -287,7 +361,7 @@ async fn read_applied_versions_on_client( Err(e) if e.as_db_error().map(|d| d.code().code()) == Some("42P01") => Ok(HashSet::new()), Err(e) => Err(Error::internal_err(format!( "Failed to read _wm_migrations: {}", - e + pg_error_message(&e) ))), } } @@ -366,7 +440,12 @@ async fn run_datatable_migrations( &[&datatable_name, &m.timestamp], ) .await - .map_err(|e| Error::internal_err(format!("Failed to record migration: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!( + "Failed to record migration: {}", + pg_error_message(&e) + )) + })?; applied.push(AppliedMigration { version: m.timestamp, name: m.name }); } @@ -433,7 +512,12 @@ async fn rollback_datatable_migrations( &[&datatable_name, &only], ) .await - .map_err(|e| Error::internal_err(format!("Failed to read _wm_migrations: {}", e)))?, + .map_err(|e| { + Error::internal_err(format!( + "Failed to read _wm_migrations: {}", + pg_error_message(&e) + )) + })?, None => client .query_opt( "SELECT version FROM _wm_migrations WHERE datatable = $1 \ @@ -441,7 +525,12 @@ async fn rollback_datatable_migrations( &[&datatable_name], ) .await - .map_err(|e| Error::internal_err(format!("Failed to read _wm_migrations: {}", e)))?, + .map_err(|e| { + Error::internal_err(format!( + "Failed to read _wm_migrations: {}", + pg_error_message(&e) + )) + })?, }; let version: i64 = match target { @@ -492,7 +581,12 @@ async fn rollback_datatable_migrations( &[&datatable_name, &version], ) .await - .map_err(|e| Error::internal_err(format!("Failed to drop migration record: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!( + "Failed to drop migration record: {}", + pg_error_message(&e) + )) + })?; Ok(Json(RollbackDatatableMigrationsResult { rolled_back: vec![RolledBackMigration { version, name: definition.name }], @@ -940,7 +1034,10 @@ async fn mark_datatable_version_installed( ) .await .map_err(|e| { - Error::internal_err(format!("Failed to mark initial migration installed: {}", e)) + Error::internal_err(format!( + "Failed to mark initial migration installed: {}", + pg_error_message(&e) + )) })?; Ok(()) } @@ -1378,7 +1475,7 @@ fn ignore_missing_wm_migrations(e: tokio_postgres::Error) -> Result<()> { Some("42P01") => Ok(()), _ => Err(Error::internal_err(format!( "Failed to update _wm_migrations: {}", - e + pg_error_message(&e) ))), } } diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index c8f67c9d2e..b7e2ab72c1 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -34,6 +34,7 @@ use windmill_audit::audit_oss::{audit_log, AuditAuthorable}; use windmill_audit::ActionKind; use windmill_common::db::UserDB; use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; +use windmill_common::query_builders::{render_db_quoted_identifier, DbType}; use windmill_common::users::username_to_permissioned_as; use windmill_common::variables::{ build_crypt, decrypt, encrypt, SECRET_SALT, WORKSPACE_CRYPT_CACHE, @@ -51,7 +52,7 @@ use windmill_common::workspaces::{ use windmill_common::workspaces::{Ducklake, DucklakeCatalogResourceType}; use windmill_common::PgDatabase; use windmill_common::{ - error::{Error, JsonResult, Result}, + error::{pg_error_message, Error, JsonResult, Result}, global_settings::{ AUTOMATE_USERNAME_CREATION_SETTING, DISABLE_WORKSPACE_INVITE_EMAILS_SETTING, }, @@ -133,6 +134,10 @@ pub fn workspaced_service() -> Router { get(get_datatable_table_schema), ) .route("/edit_datatable_config", post(edit_datatable_config)) + .route( + "/test_datatable_connection/{datatable_name}", + get(test_datatable_connection), + ) .merge(crate::datatable_migrations::routes()) .route("/git_sync_enabled", get(get_git_sync_enabled)) .route("/git_sync_deploy_mode", get(get_git_sync_deploy_mode)) @@ -2023,6 +2028,127 @@ struct DataTableTableSchema { columns: ColumnMap, } +#[derive(Serialize, Debug)] +struct DataTableConnectionCheck { + /// The role the data table actually connects as, and the schema its + /// unqualified statements resolve to. Both are read from the server rather + /// than the resource, which need not spell either of them out. + user: String, + schema: Option, + /// Whether that role can create tables in `schema` / schemas in the database. + can_create_table: bool, + can_create_schema: bool, + /// Whether the migration bookkeeping table is already present. Informative + /// only: it explains why migration *tracking* can work without CREATE, and + /// grants nothing beyond that. + migrations_table_exists: bool, + /// Statements to run for the privileges that are missing, empty when there + /// are none. Windmill connects as the role that lacks them, so it can only + /// name them for a schema owner to run. + suggested_grants: Vec, + /// Statement that gives the session a schema to work in, when `search_path` + /// resolves to none. Rendered here rather than by the caller so identifier + /// quoting stays in one place. + #[serde(skip_serializing_if = "Option::is_none")] + suggested_search_path: Option, +} + +/// Report what the data table's own database lets its role do. Surfacing this +/// from the settings page is the difference between finding out here and finding +/// out on a first schema change, when the failure reads as a Postgres refusal +/// deep inside a migration. +async fn test_datatable_connection( + authed: ApiAuthed, + Extension(db): Extension, + Path((w_id, datatable_name)): Path<(String, String)>, +) -> JsonResult { + require_admin(authed.is_admin, &authed.username)?; + + let db_resource = get_datatable_resource_from_db_unchecked(&db, &w_id, &datatable_name).await?; + let pg_db: PgDatabase = serde_json::from_value(db_resource) + .map_err(|e| Error::internal_err(format!("Failed to parse database credentials: {}", e)))?; + let (client, connection) = pg_db.connect(Some(&db)).await?; + let join_handle = tokio::spawn(async move { connection.await }); + + // One round trip, no side effects: `has_*_privilege` answers for the + // connected role without attempting the operation. + let rows = client + .simple_query( + "SELECT current_user AS usr, \ + current_schema() AS sch, \ + current_database() AS db, \ + has_schema_privilege(current_schema(), 'CREATE') AS can_create_table, \ + has_database_privilege(current_database(), 'CREATE') AS can_create_schema, \ + to_regclass('_wm_migrations') IS NOT NULL AS has_migrations_table", + ) + .await + .map_err(|e| { + Error::internal_err(format!( + "Failed to inspect data table privileges: {}", + pg_error_message(&e) + )) + }); + + drop(client); + let _ = windmill_common::shutdown_pg_connection(join_handle).await; + + let row = rows? + .into_iter() + .find_map(|msg| match msg { + tokio_postgres::SimpleQueryMessage::Row(row) => Some(row), + _ => None, + }) + .ok_or_else(|| Error::internal_err("Privilege query returned no row".to_string()))?; + + let user = row.get("usr").unwrap_or_default().to_string(); + let schema = row.get("sch").map(str::to_string); + let can_create_table = row.get("can_create_table") == Some("t"); + let can_create_schema = row.get("can_create_schema") == Some("t"); + let migrations_table_exists = row.get("has_migrations_table") == Some("t"); + + let quoted_user = render_db_quoted_identifier(&user, DbType::Postgresql); + let mut suggested_grants = Vec::new(); + // Suggest on the capability alone: an existing `_wm_migrations` spares only + // that one table, and says nothing about the tables a migration will create. + // A NULL `current_schema()` means search_path resolves to nothing, and no + // grant fixes that — an unqualified CREATE fails with `no schema has been + // selected to create in` whoever holds the privilege — so suggest nothing + // and let `schema: null` carry the diagnosis. + if let (false, Some(target)) = (can_create_table, schema.as_deref()) { + suggested_grants.push(format!( + "GRANT CREATE ON SCHEMA {} TO {}", + render_db_quoted_identifier(target, DbType::Postgresql), + quoted_user + )); + } + if !can_create_schema { + // Named from the server like every other identifier here: behind a + // pooler the resource's dbname can be an alias for another database. + let dbname = row.get("db").unwrap_or(pg_db.dbname.as_str()); + suggested_grants.push(format!( + "GRANT CREATE ON DATABASE {} TO {}", + render_db_quoted_identifier(dbname, DbType::Postgresql), + quoted_user + )); + } + + // An empty search_path is not a privilege problem, so it gets a statement of + // its own rather than a grant. + let suggested_search_path = schema + .is_none() + .then(|| format!("ALTER ROLE {quoted_user} SET search_path = public")); + + Ok(Json(DataTableConnectionCheck { + user, + schema, + can_create_table, + can_create_schema, + migrations_table_exists, + suggested_grants, + suggested_search_path, + })) +} + async fn list_datatable_schemas( _authed: ApiAuthed, Extension(db): Extension, @@ -2139,7 +2265,9 @@ async fn get_datatable_schema(db: &DB, w_id: &str, datatable_name: &str) -> Resu &[], ) .await - .map_err(|e| Error::internal_err(format!("Failed to query schemas: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!("Failed to query schemas: {}", pg_error_message(&e))) + })?; // Build hierarchical structure: schema -> table -> column -> compact_type let mut schema_map: SchemaMap = HashMap::new(); @@ -2173,7 +2301,9 @@ async fn get_datatable_schema(db: &DB, w_id: &str, datatable_name: &str) -> Resu &[&schema_names], ) .await - .map_err(|e| Error::internal_err(format!("Failed to query columns: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!("Failed to query columns: {}", pg_error_message(&e))) + })?; for row in rows { let table_schema: String = row.get(0); @@ -2221,7 +2351,9 @@ async fn get_datatable_tables(db: &DB, w_id: &str, datatable_name: &str) -> Resu &[], ) .await - .map_err(|e| Error::internal_err(format!("Failed to query schemas: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!("Failed to query schemas: {}", pg_error_message(&e))) + })?; let mut table_map: TableListMap = HashMap::new(); let schema_names: Vec = schema_rows @@ -2247,7 +2379,9 @@ async fn get_datatable_tables(db: &DB, w_id: &str, datatable_name: &str) -> Resu &[&schema_names], ) .await - .map_err(|e| Error::internal_err(format!("Failed to query tables: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!("Failed to query tables: {}", pg_error_message(&e))) + })?; for row in rows { let table_schema: String = row.get(0); @@ -2299,7 +2433,9 @@ async fn get_datatable_table_columns( &[&schema_name, &table_name], ) .await - .map_err(|e| Error::internal_err(format!("Failed to query columns: {}", e)))?; + .map_err(|e| { + Error::internal_err(format!("Failed to query columns: {}", pg_error_message(&e))) + })?; if rows.is_empty() { return Err(Error::NotFound(format!( @@ -2671,7 +2807,10 @@ async fn create_pg_database( ) .await .map_err(|e| { - Error::internal_err(format!("Failed to check database existence: {}", e)) + Error::internal_err(format!( + "Failed to check database existence: {}", + pg_error_message(&e) + )) })?; let db_exists: bool = row.get(0); @@ -2690,7 +2829,8 @@ async fn create_pg_database( .map_err(|e| { Error::internal_err(format!( "Failed to create database '{}': {}", - req.target_dbname, e + req.target_dbname, + pg_error_message(&e) )) })?; diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 7927a7f00d..1dd692752f 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -4633,6 +4633,52 @@ paths: items: $ref: "#/components/schemas/DataTableSchema" + /w/{workspace}/workspaces/test_datatable_connection/{datatable_name}: + get: + summary: check what the data table's database lets its role do + operationId: testDataTableConnection + tags: + - workspace + parameters: + - $ref: "#/components/parameters/WorkspaceId" + - name: datatable_name + in: path + required: true + schema: + type: string + responses: + "200": + description: connection and privilege report + content: + application/json: + schema: + type: object + properties: + user: + type: string + schema: + type: string + nullable: true + can_create_table: + type: boolean + can_create_schema: + type: boolean + migrations_table_exists: + type: boolean + suggested_grants: + type: array + items: + type: string + suggested_search_path: + type: string + required: + - user + - schema + - can_create_table + - can_create_schema + - migrations_table_exists + - suggested_grants + /w/{workspace}/workspaces/list_datatable_tables: get: summary: list tables of all connected Datatables diff --git a/backend/windmill-common/src/error.rs b/backend/windmill-common/src/error.rs index f202bf27f9..0d5ebdc2c3 100644 --- a/backend/windmill-common/src/error.rs +++ b/backend/windmill-common/src/error.rs @@ -247,6 +247,41 @@ pub fn to_anyhow(e: T) -> anyhow:: From::from(e) } +/// Render a `tokio_postgres` error for a user-facing message. +/// +/// The pinned rust-postgres build prints only the error *kind* in its `Display` +/// impl, so `format!("{e}")` on one of these yields the useless `db error` and +/// drops the Postgres message. Interpolate errors from a `tokio_postgres::Client` +/// through this instead. +pub fn pg_error_message(e: &tokio_postgres::Error) -> String { + match e.as_db_error() { + Some(db_err) => format_db_error(db_err.message(), db_err.detail(), db_err.hint()), + // Non-database failures (io, tls, protocol) keep their message in the cause. + None => error_source_chain(e), + } +} + +fn format_db_error(message: &str, detail: Option<&str>, hint: Option<&str>) -> String { + let mut msg = message.to_string(); + if let Some(detail) = detail { + msg.push_str(&format!(" ({detail})")); + } + if let Some(hint) = hint { + msg.push_str(&format!(". Hint: {hint}")); + } + msg +} + +fn error_source_chain(e: &dyn std::error::Error) -> String { + let mut msg = e.to_string(); + let mut source = e.source(); + while let Some(cause) = source { + msg.push_str(&format!(": {cause}")); + source = cause.source(); + } + msg +} + impl IntoResponse for Error { fn into_response(self) -> axum::response::Response { let status = match self { @@ -419,4 +454,56 @@ mod tests { let rendered = Error::JsonErr(v).to_string(); assert_eq!(rendered, "[\n 1,\n 2,\n 3\n]"); } + + #[test] + fn db_error_renders_message_with_detail_and_hint() { + assert_eq!( + super::format_db_error("permission denied for schema public", None, None), + "permission denied for schema public" + ); + assert_eq!( + super::format_db_error("insert violates foreign key", Some("Key (id)=(1)"), None), + "insert violates foreign key (Key (id)=(1))" + ); + assert_eq!( + super::format_db_error( + "column does not exist", + None, + Some("Perhaps you meant \"b\"") + ), + "column does not exist. Hint: Perhaps you meant \"b\"" + ); + } + + #[test] + fn non_db_error_walks_the_source_chain() { + #[derive(Debug)] + struct Layer(&'static str, Option>); + impl std::fmt::Display for Layer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } + } + impl std::error::Error for Layer { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.1 + .as_ref() + .map(|c| c.as_ref() as &(dyn std::error::Error + 'static)) + } + } + + // The pinned rust-postgres build renders only the kind, so everything + // actionable is in the causes: they must all reach the message. + let err = Layer( + "error connecting to server", + Some(Box::new(Layer( + "tcp connect error", + Some(Box::new(Layer("timed out", None))), + ))), + ); + assert_eq!( + super::error_source_chain(&err), + "error connecting to server: tcp connect error: timed out" + ); + } } diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index 1eda7503af..30a5edad32 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -1257,7 +1257,7 @@ pub async fn create_custom_instance_database( tracing::warn!( "Failed to grant permissions on '{}': {}. Continuing.", dbname, - e + crate::error::pg_error_message(&e) ); } diff --git a/backend/windmill-common/src/query_builders.rs b/backend/windmill-common/src/query_builders.rs index c0eedadbc4..c315a564af 100644 --- a/backend/windmill-common/src/query_builders.rs +++ b/backend/windmill-common/src/query_builders.rs @@ -1,6 +1,8 @@ use serde::{Deserialize, Deserializer, Serialize}; use windmill_types::scripts::ScriptLang; +use crate::error::pg_error_message; + fn deserialize_bool_from_null<'de, D>(deserializer: D) -> Result where D: Deserializer<'de>, @@ -4848,7 +4850,7 @@ fn required_str<'a>( column: &str, ) -> Result<&'a str, String> { row.try_get(column) - .map_err(|e| format!("Failed to read column {}: {}", column, e))? + .map_err(|e| format!("Failed to read column {}: {}", column, pg_error_message(&e)))? .ok_or_else(|| format!("Unexpected NULL in column {}", column)) } @@ -4894,7 +4896,7 @@ pub async fn pg_get_full_schema( ) .await .map(simple_query_rows) - .map_err(|e| format!("Failed to query columns: {}", e))?; + .map_err(|e| format!("Failed to query columns: {}", pg_error_message(&e)))?; let fk_rows = client .simple_query( @@ -4922,7 +4924,7 @@ pub async fn pg_get_full_schema( ) .await .map(simple_query_rows) - .map_err(|e| format!("Failed to query foreign keys: {}", e))?; + .map_err(|e| format!("Failed to query foreign keys: {}", pg_error_message(&e)))?; let mut result: FullDatabaseSchema = std::collections::HashMap::new(); diff --git a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte index f652ed8704..069ef6093d 100644 --- a/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte +++ b/frontend/src/lib/components/workspaceSettings/DataTableSettings.svelte @@ -51,7 +51,7 @@