From c6042c31bdfc29f7c6ea08be62642669c47426dc Mon Sep 17 00:00:00 2001 From: Sang Jun Bak Date: Tue, 27 Jan 2026 19:04:05 -0500 Subject: [PATCH] Fix invalid retraction for builtin role relations By having a system variable as an input to a addition/retraction, we were able to get in a state of getting invalid retractions by flipping the system variable. We circumvent this by removing the system variable from the equation. This should be okay since by default, the `login` and `superuser` attribute in Cloud should be null, effectively doing the same thing as the code before. --- .../src/catalog/builtin_table_updates.rs | 45 ++++++++++++------- test/sqllogictest/pg_catalog_user.slt | 2 + test/sqllogictest/role.slt | 45 +++++++++++++++++++ 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/src/adapter/src/catalog/builtin_table_updates.rs b/src/adapter/src/catalog/builtin_table_updates.rs index 4cd0333313c07..beb036949eedc 100644 --- a/src/adapter/src/catalog/builtin_table_updates.rs +++ b/src/adapter/src/catalog/builtin_table_updates.rs @@ -12,7 +12,6 @@ mod notice; use bytesize::ByteSize; use ipnet::IpNet; use mz_adapter_types::compaction::CompactionWindow; -use mz_adapter_types::dyncfgs::ENABLE_PASSWORD_AUTH; use mz_audit_log::{EventDetails, EventType, ObjectType, VersionedEvent, VersionedStorageUsage}; use mz_catalog::SYSTEM_CONN_ID; use mz_catalog::builtin::{ @@ -231,18 +230,11 @@ impl CatalogState { RoleId::Public => vec![], id => { let role = self.get_role(&id); - let self_managed_auth_enabled = self - .system_config() - .get(ENABLE_PASSWORD_AUTH.name()) - .ok() - .map(|v| v.value() == "on") - .unwrap_or(false); - let builtin_supers = [MZ_SYSTEM_ROLE_ID, MZ_SUPPORT_ROLE_ID]; // For self managed auth, we can get the actual login and superuser bits - // directly from the role. For cloud auth, we have to do some heuristics. - // We determine login status each time a role logs in, so there's no clean + // directly from the role. For cloud auth, the LOGIN and SUPERUSER attribute + // are nullish and determined each time a role logs in, so there's no clean // way to accurately determine this in the catalog. Instead we do something // a little gross. For system roles, we hardcode the known roles that can // log in. For user roles, we determine `rolcanlogin` based on whether the @@ -258,23 +250,42 @@ impl CatalogState { // that folks are regularly creating non-login roles with names that look // like an email address (e.g., `admins@sysops.foocorp`), we can change // course. - let cloud_login_regex = "^[^@]+@[^@]+\\.[^@]+$"; - let matches = regex::Regex::new(cloud_login_regex, true) + let matches_cloud_login_heuristic = regex::Regex::new(cloud_login_regex, true) .expect("valid regex") .is_match(&role.name); - let rolcanlogin = if self_managed_auth_enabled { - role.attributes.login.unwrap_or(false) + let rolcanlogin = if let Some(login) = role.attributes.login { + login } else { - builtin_supers.contains(&role.id) || matches + // Note: Self-managed users with email-like + // role names may have `rolcanlogin` set to true if the role + // was initialized without an explicit LOGIN attribute. + // We're comfortable with this edge case, since roles that + // have login explictly turned off via NOLOGIN will have a + // LOGIN attribute value of false. + builtin_supers.contains(&role.id) || matches_cloud_login_heuristic }; - let rolsuper = if self_managed_auth_enabled { - Datum::from(role.attributes.superuser.unwrap_or(false)) + let rolsuper = if let Some(superuser) = role.attributes.superuser { + Datum::from(superuser) + } else if let Some(_) = role.attributes.login { + // If a role has an explicit LOGIN attribute but no SUPERUSER + // attribute, we assume this is self-managed auth where + // the role was created without indicating superuser privileges. + Datum::from(false) } else if builtin_supers.contains(&role.id) { + // System roles (mz_system, mz_support) are superusers Datum::from(true) } else { + // In cloud environments, superuser status is determined + // at login, so we set `rolsuper` to `null` here because + // it cannot be known beforehand. For self-managed + // auth, roles created without an explicit LOGIN + // attribute would typically have `rolsuper` set to false. + // However, since we cannot reliably distinguish between + // cloud and self-managed here, we conservatively use NULL + // for indeterminate cases. Datum::Null }; diff --git a/test/sqllogictest/pg_catalog_user.slt b/test/sqllogictest/pg_catalog_user.slt index b8b41e41e520d..97de42323562d 100644 --- a/test/sqllogictest/pg_catalog_user.slt +++ b/test/sqllogictest/pg_catalog_user.slt @@ -28,6 +28,7 @@ query TIBBBBTTT rowsort SELECT usename, usesysid, usecreatedb, usesuper, userepl, usebypassrls, passwd, valuntil, useconfig FROM pg_user; ---- materialize@foocorp.io 20190 false NULL false false NULL NULL NULL +mz_support 16662 false true false false NULL NULL NULL mz_system 16661 true true false false NULL NULL NULL statement ok @@ -40,4 +41,5 @@ query TT rowsort SELECT usename, useconfig::text FROM pg_user; ---- materialize@foocorp.io {cluster=cluster=cluster=cluster=,"search_path=abc,␠def"} +mz_support NULL mz_system NULL diff --git a/test/sqllogictest/role.slt b/test/sqllogictest/role.slt index 45df714be124b..52afd36c7902f 100644 --- a/test/sqllogictest/role.slt +++ b/test/sqllogictest/role.slt @@ -584,3 +584,48 @@ Used Indexes: Target cluster: mz_catalog_server EOF + +# Regression test for invalid retractions in mz_roles +# when enable_password_auth changes (database-issues#10042) +# and roles are created / dropped. + +# Enable password auth +simple conn=mz_system,user=mz_system +ALTER SYSTEM SET enable_password_auth TO true; +---- +COMPLETE 0 + +# Create a role with an explicit LOGIN attribute +simple conn=mz_system,user=mz_system +CREATE ROLE test_retraction_role WITH LOGIN; +---- +COMPLETE 0 + +# Verify querying mz_roles does not error +query TB +SELECT name, rolcanlogin FROM mz_roles WHERE name = 'test_retraction_role' +---- +test_retraction_role true + +# Disable password auth +simple conn=mz_system,user=mz_system +ALTER SYSTEM SET enable_password_auth TO false; +---- +COMPLETE 0 + +# Drop the role +simple conn=mz_system,user=mz_system +DROP ROLE test_retraction_role; +---- +COMPLETE 0 + +# Query mz_roles. This should NOT cause a retraction error +query T +SELECT name FROM mz_roles WHERE name = 'test_retraction_role' +---- + +# Reset password auth to off for remaining tests +simple conn=mz_system,user=mz_system +ALTER SYSTEM SET enable_password_auth TO false; +---- +COMPLETE 0