starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-08-22 08:02:19 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(datatables): revoke owner-granted instance CONNECT before role drop, surface pg error details

Browse Source 
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6
This commit is contained in:
Diego Imbert
2026-07-23 19:15:27 +02:00
parent eae44099bc
commit cf9c33c1e9
2 changed files with 65 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/summarized_schema.txt
+2
View File
@@ -67,6 +67,8 @@ config: name(char), config(jsonb)
custom_concurrency_key_ended: key(char), ended_at(ts)
debounce_key: key(char), job_id(uuid), previous_job_id(uuid), first_started_at(ts), debounced_times(int)
debounce_stale_data: job_id(uuid), to_relock(text[])
datatable_ephemeral_role: role_name(text), workspace_id(char), datatable(char), permissioned_as(char), password(text), perms_hash(text), expires_at(ts), created_at(ts)
FK: (workspace_id) -> workspace(id)
debouncing_settings: hash(bigint), debounce_key(char), debounce_delay_s(int), max_total_debouncing_time(int), max_total_debounces_amount(int), debounce_args_to_accumulate(text[])
dependency_map: workspace_id(char), importer_path(char), importer_kind(importer_kind), imported_path(char), importer_node_id(char)
deployment_metadata: workspace_id(char), path(char), script_hash(bigint), app_version(bigint), callback_job_ids(uuid[]), deployment_msg(text), flow_version(bigint), job_id(uuid)
backend/windmill-common/src/datatable_permissions.rs
+63 -13
View File
@@ -398,11 +398,39 @@ async fn connect_target(pg: &PgDatabase, db: &DB) -> Result<tokio_postgres::Clie
Ok(client)
}
fn pg_err(context: &str, e: tokio_postgres::Error) -> Error {
// `Display` for tokio_postgres errors is just "db error" — the actual
// Postgres message lives in the DbError.
let detail = e
.as_db_error()
.map(|d| d.message().to_string())
.unwrap_or_else(|| e.to_string());
Error::internal_err(format!("{context}: {detail}"))
}
/// The revoke half of an instance-type role drop: database-level CONNECT was
/// granted by the main pool's user (the database owner), and only the grantor
/// (or a superuser) can revoke a privilege — `DROP OWNED` executed by
/// `custom_instance_user` fails on it. Best-effort: the grant may not exist.
async fn revoke_instance_connect(db: &DB, dbname: &str, role: &str) {
if let Err(e) = sqlx::query(&format!(
"REVOKE CONNECT ON DATABASE {} FROM {}",
quote_ident(dbname),
quote_ident(role)
))
.execute(db)
.await
{
tracing::warn!("revoking connect on {dbname} from {role}: {e:#}");
}
}
/// `DROP OWNED BY` + `DROP ROLE`, refusing to drop anything without the
/// reserved prefix. `DROP OWNED` is required even though ephemeral roles own
/// no objects: `DROP ROLE` fails while any privilege (or default-privilege
/// entry) is still granted to the role. The membership self-grant makes
/// `DROP OWNED` work on PG16+ where CREATEROLE no longer implies it.
/// Instance-type callers must run [`revoke_instance_connect`] first.
async fn guarded_drop_role(client: &tokio_postgres::Client, role: &str) -> Result<()> {
if !role.starts_with(DATATABLE_EPHEMERAL_ROLE_PREFIX) {
return Err(Error::internal_err(format!(
@@ -416,11 +444,11 @@ async fn guarded_drop_role(client: &tokio_postgres::Client, role: &str) -> Resul
client
.batch_execute(&format!("DROP OWNED BY {role_q}"))
.await
.map_err(|e| Error::internal_err(format!("dropping privileges of role {role}: {e:#}")))?;
.map_err(|e| pg_err(&format!("dropping privileges of role {role}"), e))?;
client
.batch_execute(&format!("DROP ROLE {role_q}"))
.await
.map_err(|e| Error::internal_err(format!("dropping role {role}: {e:#}")))?;
.map_err(|e| pg_err(&format!("dropping role {role}"), e))?;
Ok(())
}
@@ -435,7 +463,14 @@ async fn role_exists(client: &tokio_postgres::Client, role: &str) -> Result<bool
Ok(row.get(0))
}
async fn create_role(client: &tokio_postgres::Client, role: &str, password: &str) -> Result<()> {
async fn create_role(
db: &DB,
client: &tokio_postgres::Client,
role: &str,
password: &str,
is_instance: bool,
dbname: &str,
) -> Result<()> {
let create_sql = format!(
"CREATE ROLE {} LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE NOINHERIT NOREPLICATION CONNECTION LIMIT {EPHEMERAL_ROLE_CONNECTION_LIMIT} PASSWORD {}",
quote_ident(role),
@@ -446,13 +481,16 @@ async fn create_role(client: &tokio_postgres::Client, role: &str, password: &str
// No `CREATE ROLE IF NOT EXISTS` exists — an out-of-band concurrent
// creation surfaces as duplicate_object; drop it and retry once.
Err(e) if e.code() == Some(&tokio_postgres::error::SqlState::DUPLICATE_OBJECT) => {
if is_instance {
revoke_instance_connect(db, dbname, role).await;
}
guarded_drop_role(client, role).await?;
client
.batch_execute(&create_sql)
.await
.map_err(|e| Error::internal_err(format!("creating role {role}: {e:#}")))
.map_err(|e| pg_err(&format!("creating role {role}"), e))
}
Err(e) => Err(Error::internal_err(format!("creating role {role}: {e:#}"))),
Err(e) => Err(pg_err(&format!("creating role {role}"), e)),
}
}
@@ -559,9 +597,12 @@ pub async fn ensure_ephemeral_role(
let client = connect_target(owner, db).await?;
if role_exists(&client, &role).await? {
if is_instance {
revoke_instance_connect(db, &owner.dbname, &role).await;
}
guarded_drop_role(&client, &role).await?;
}
create_role(&client, &role, &password).await?;
create_role(db, &client, &role, &password, is_instance, &owner.dbname).await?;
if is_instance {
harden_instance_databases(db, &owner.dbname).await?;
@@ -601,11 +642,14 @@ pub async fn ensure_ephemeral_role(
let owner_role = owner.user.as_deref().unwrap_or("postgres");
for stmt in grant_sql_statements(&role, owner_role, matched) {
client.batch_execute(&stmt).await.map_err(|e| {
Error::internal_err(format!(
"applying data table grant `{stmt}` failed. The data table's owner role \
('{owner_role}') must own the target schemas and tables — tables created \
through Windmill migrations are. Original error: {e:#}"
))
pg_err(
&format!(
"applying data table grant `{stmt}` failed. The data table's owner role \
('{owner_role}') must own the target schemas and tables — tables created \
through Windmill migrations are. Error"
),
e,
)
})?;
}
drop(client);
@@ -713,12 +757,15 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s
&[&role],
)
.await
.map_err(|e| Error::internal_err(format!("checking active sessions: {e:#}")))?
.map_err(|e| pg_err("checking active sessions", e))?
.get(0);
if active > 0 {
return Ok(());
}
if role_exists(&client, role).await? {
if config.database.resource_type == DataTableCatalogResourceType::Instance {
revoke_instance_connect(db, &owner.dbname, role).await;
}
guarded_drop_role(&client, role).await?;
}
}
@@ -791,9 +838,12 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu
&[&role],
)
.await
.map_err(|e| Error::internal_err(format!("checking active sessions: {e:#}")))?
.map_err(|e| pg_err("checking active sessions", e))?
.get(0);
if active == 0 && role_exists(&client, role).await? {
if config.database.resource_type == DataTableCatalogResourceType::Instance {
revoke_instance_connect(db, &owner.dbname, role).await;
}
guarded_drop_role(&client, role).await?;
}
}