Fix invalid retraction for mz_roles

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
                 };
 

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

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