diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index fea7de2a35..042e86a92f 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -2f0450cf3d5c79575bbb47f1a2dd7564e42a8b69 +923564cebc131396ea47d9af76d02d15bd61ca43 diff --git a/backend/windmill-api-workspaces/src/datatable_acl.rs b/backend/windmill-api-workspaces/src/datatable_acl.rs index 7467c06aac..538af12275 100644 --- a/backend/windmill-api-workspaces/src/datatable_acl.rs +++ b/backend/windmill-api-workspaces/src/datatable_acl.rs @@ -980,16 +980,16 @@ async fn build_acl_plan( AclChange::Grant { role, .. } | AclChange::Revoke { role, .. } => role, }; let pg_role = pg_role_of(&roles, role_name)?; - // `admin` is the login the data table itself reaches Postgres through, so a - // revoke on the database would take away what every role here connects with - // — including the one that would have to grant it back. - if matches!(req.change, AclChange::Revoke { .. }) - && matches!(req.target, AclTarget::Database) - && role_name == ADMIN_DATATABLE_ROLE - { + // `admin` is the login the data table itself reaches Postgres through — the + // one migrations, schema browsing and every other role's creation run as. Its + // own access is not the drawer's to take away, on any target: the grants an + // instance database is provisioned with cover the database and schema + // `public` alike, and a revoke that lands leaves nothing able to grant it + // back. + if matches!(req.change, AclChange::Revoke { .. }) && role_name == ADMIN_DATATABLE_ROLE { return Err(Error::BadRequest(format!( "'{ADMIN_DATATABLE_ROLE}' is how this data table reaches its database; \ - revoking on the database itself would lock every role out of it" + its own access is not revocable from here" ))); } // The roles a plan may also write about: what a schema's new owner is kept diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 41a288e8f5..b94cd400c1 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -1111,6 +1111,66 @@ pub fn redact_datatable_settings_for_export( Some(datatable) } +/// Refuse a resource edit that would move a permissioned data table onto another +/// database. +/// +/// The roles of such a data table live in the database it points at: their logins +/// were created there and every grant they hold is recorded there. The path in +/// the config staying the same says nothing — the resource behind it can be +/// edited — so the identity the connection resolves to is what has to hold +/// still. A password rotation is not an identity change and stays allowed. +pub async fn ensure_resource_identity_change_allowed( + db: &DB, + w_id: &str, + path: &str, + old_value: Option<&serde_json::Value>, + new_value: Option<&serde_json::Value>, +) -> Result<()> { + let (Some(old_value), Some(new_value)) = (old_value, new_value) else { + return Ok(()); + }; + let identity = |v: &serde_json::Value| { + ( + v.get("host").cloned(), + v.get("port").cloned(), + v.get("dbname").cloned(), + v.get("user").cloned(), + ) + }; + if identity(old_value) == identity(new_value) { + return Ok(()); + } + + let datatables: std::collections::HashMap = sqlx::query_scalar!( + "SELECT ws.datatable->'datatables' FROM workspace_settings ws WHERE ws.workspace_id = $1", + w_id + ) + .fetch_optional(db) + .await? + .flatten() + .and_then(|v| serde_json::from_value(v).ok()) + .unwrap_or_default(); + + let permissioned: Vec<&str> = datatables + .iter() + .filter(|(_, dt)| { + dt.database.resource_type == DataTableCatalogResourceType::Postgresql + && dt.database.resource_path == path + && dt.permissions.as_ref().is_some_and(|p| p.enabled) + }) + .map(|(name, _)| name.as_str()) + .collect(); + if permissioned.is_empty() { + return Ok(()); + } + Err(Error::BadRequest(format!( + "This resource backs data table {} with permissions enabled, so the database it points \ + at cannot be changed: disable them first, which drops the roles from the database they \ + were created in.", + permissioned.join(", ") + ))) +} + /// The data table settings as one audit parameter, which is stored and traced /// in the clear — so it goes through the same redaction as any other export. pub fn datatable_settings_for_audit(settings: &impl Serialize) -> String { diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index b107d34be4..9283c04c90 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -1811,6 +1811,29 @@ async fn update_resource( return Err(Error::PermissionDenied(msg)); } + // Same as `set_resource_value`: the identity a permissioned data table's + // roles were created against is not free to move underneath them. + if let Some(nvalue) = ns.value.as_ref() { + let previous = sqlx::query_scalar!( + "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", + path, + &w_id + ) + .fetch_optional(&db) + .await? + .flatten(); + let nvalue: serde_json::Value = serde_json::from_str(nvalue.get()) + .map_err(|e| Error::BadRequest(format!("Invalid resource value: {e}")))?; + windmill_common::workspaces::ensure_resource_identity_change_allowed( + &db, + &w_id, + path, + previous.as_ref(), + Some(&nvalue), + ) + .await?; + } + let mut sqlb = SqlBuilder::update_table("resource"); sqlb.and_where_eq("path", "?".bind(&path)); sqlb.and_where_eq("workspace_id", "?".bind(&w_id)); @@ -2123,6 +2146,25 @@ async fn set_resource_value( } authorize_azure_devops_reference(authed, db, user_db, w_id, value.as_ref()).await?; + // A data table's roles live in the database its resource points at, so the + // identity behind that path is not free to move while they exist. + let previous = sqlx::query_scalar!( + "SELECT value FROM resource WHERE path = $1 AND workspace_id = $2", + path, + w_id + ) + .fetch_optional(db) + .await? + .flatten(); + windmill_common::workspaces::ensure_resource_identity_change_allowed( + db, + w_id, + path, + previous.as_ref(), + value.as_ref(), + ) + .await?; + let mut tx = user_db.clone().begin(authed).await?; // `RETURNING resource_type` rather than a second lookup: the advisory below has to know the diff --git a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte index 7ff3fd41f0..9e77e9b41c 100644 --- a/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte +++ b/frontend/src/lib/components/datatableAcl/PgAclEditor.svelte @@ -193,10 +193,10 @@ {@const revokeScope = revokeScopeOf(grant)} - {#if info.roles.includes(grant.grantee) && revokeScope && info.can_manage && !(target.kind === 'database' && grant.grantee === ADMIN_DATATABLE_ROLE)} + on one the caller does not own has nothing to offer. And what + `admin` holds is what every role here connects through, so it + is not the drawer's to take away wherever it appears. --> + {#if info.roles.includes(grant.grantee) && revokeScope && info.can_manage && grant.grantee !== ADMIN_DATATABLE_ROLE}