Merge pull request #133071 from cockroachdb/blathers/backport-release…

…-24.2-132929

release-24.2: sql: address tech debt and edge cases in ALTER DEFAULT PRIVILEGES and REASSIGN OWNED BY

pkg/sql/alter_default_privileges.go

@@ -134,26 +134,28 @@ func (n *alterDefaultPrivilegesNode) startExec(params runParams) error {
 		return err
 	}
 
+	var hasAdmin bool
+	if hasAdmin, err = params.p.HasAdminRole(params.ctx); err != nil {
+		return err
+	}
 	if n.n.ForAllRoles {
-		if hasAdmin, err := params.p.HasAdminRole(params.ctx); err != nil {
-			return err
-		} else if !hasAdmin {
+		if !hasAdmin {
 			return pgerror.Newf(pgcode.InsufficientPrivilege,
 				"only users with the admin role are allowed to ALTER DEFAULT PRIVILEGES FOR ALL ROLES")
 		}
 	} else {
 		// You can change default privileges only for objects that will be created
 		// by yourself or by roles that you are a member of.
 		for _, targetRole := range targetRoles {
-			if targetRole != params.p.User() {
+			if targetRole != params.p.User() && !hasAdmin {
 				memberOf, err := params.p.MemberOfWithAdminOption(params.ctx, params.p.User())
 				if err != nil {
 					return err
 				}
 
 				if _, found := memberOf[targetRole]; !found {
 					return pgerror.Newf(pgcode.InsufficientPrivilege,
-						"must be a member of %s", targetRole.Normalized())
+						"must be an admin or member of %s", targetRole.Normalized())
 				}
 			}
 		}

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_schema

@@ -125,10 +125,6 @@ user root
 statement ok
 USE d
 
-# root must be a member of testuser to ALTER DEFAULT PRIVILEGES FOR ROLE testuser.
-statement ok
-GRANT testuser TO root
-
 statement ok
 ALTER DEFAULT PRIVILEGES FOR ROLE testuser REVOKE ALL ON SCHEMAS FROM testuser, testuser2
 

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_sequence

@@ -140,9 +140,6 @@ user root
 statement ok
 USE d
 
-statement ok
-GRANT testuser TO root
-
 statement ok
 ALTER DEFAULT PRIVILEGES FOR ROLE testuser REVOKE ALL ON SEQUENCES FROM testuser, testuser2
 

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_table

@@ -185,9 +185,6 @@ use d
 statement ok
 GRANT CREATE ON DATABASE d TO testuser
 
-statement ok
-GRANT testuser TO root
-
 statement ok
 ALTER DEFAULT PRIVILEGES FOR ROLE testuser GRANT SELECT ON TABLES to testuser, testuser2
 
@@ -286,7 +283,7 @@ user testuser2
 statement ok
 USE d
 
-statement error pq: must be a member of root
+statement error pq: must be an admin or member of root
 ALTER DEFAULT PRIVILEGES FOR ROLE root GRANT SELECT ON TABLES TO testuser
 
 # Ensure you can ALTER DEFAULT PRIVILEGES for multiple roles.
@@ -363,3 +360,22 @@ ALTER DEFAULT PRIVILEGES FOR ROLE public REVOKE SELECT ON TABLES FROM testuser2,
 # Can specify PUBLIC as a grantee.
 statement ok
 ALTER DEFAULT PRIVILEGES REVOKE SELECT ON TABLES FROM public
+
+# Admins can ALTER DEFAULT PRIVILEGES for any role.
+user root
+
+# Confirm that root is not a member of testuser. We avoid using pg_has_role
+# to check, since that has a special case for all admin users.
+query TTB
+SELECT role, inheriting_member, member_is_explicit
+FROM crdb_internal.kv_inherited_role_members
+WHERE inheriting_member = 'root'
+ORDER BY role
+----
+admin  root  true
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE testuser GRANT ALL ON TABLES TO testuser2
+
+statement ok
+ALTER DEFAULT PRIVILEGES FOR ROLE testuser REVOKE ALL ON TABLES FROM testuser2

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_type

@@ -115,9 +115,6 @@ user root
 statement ok
 USE d
 
-statement ok
-GRANT testuser TO root
-
 statement ok
 ALTER DEFAULT PRIVILEGES FOR ROLE testuser REVOKE ALL ON TYPES FROM testuser, testuser2
 

pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema

@@ -12,9 +12,6 @@ CREATE USER testuser2
 statement ok
 GRANT CREATE ON DATABASE test TO testuser
 
-statement ok
-GRANT testuser TO root
-
 user testuser
 
 # Test on public schema.

pkg/sql/logictest/testdata/logic_test/reassign_owned_by

@@ -243,8 +243,19 @@ user root
 statement ok
 REVOKE CREATE ON DATABASE test FROM testuser, testuser2;
 DROP ROLE testuser;
+
+# Ownership of the public schema was transferred to testuser2.
+
+statement error role testuser2 cannot be dropped because some objects depend on it\nowner of schema test.public
+DROP ROLE testuser2
+
+statement ok
+REASSIGN OWNED BY testuser2 TO root
+
+statement ok
 DROP ROLE testuser2
 
+
 # ------------------------------------------------------------------------------
 # Make sure only objects in the current database are reassigned
 

pkg/sql/logictest/testdata/logic_test/show_default_privileges

@@ -224,7 +224,6 @@ use test2;
 CREATE USER testuser2;
 
 statement ok
-GRANT testuser TO root;
 ALTER DEFAULT PRIVILEGES FOR ROLE testuser GRANT DROP, ZONECONFIG ON TABLES TO foo WITH GRANT OPTION;
 
 query TBTTTB colnames,rowsort

pkg/sql/reassign_owned_by.go

@@ -8,6 +8,7 @@ package sql
 import (
 	"context"
 
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
@@ -19,7 +20,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
-	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
 	"github.com/cockroachdb/errors"
@@ -120,26 +120,27 @@ func (n *reassignOwnedByNode) startExec(params runParams) error {
 	for _, oldRole := range n.normalizedOldRoles {
 		// There should only be one database (current).
 		for _, dbID := range lCtx.dbIDs {
-			isOwner, err := isOwner(params.ctx, params.p, lCtx.dbDescs[dbID], oldRole)
+			dbDesc := lCtx.dbDescs[dbID]
+			owner, err := params.p.getOwnerOfPrivilegeObject(params.ctx, dbDesc)
 			if err != nil {
 				return err
 			}
-			if isOwner {
-				if err := n.reassignDatabaseOwner(lCtx.dbDescs[dbID], params); err != nil {
+			if owner == oldRole {
+				if err := n.reassignDatabaseOwner(dbDesc, params); err != nil {
 					return err
 				}
 			}
 		}
 		for _, schemaID := range lCtx.schemaIDs {
-			isOwner, err := isOwner(params.ctx, params.p, lCtx.schemaDescs[schemaID], oldRole)
+			schemaDesc := lCtx.schemaDescs[schemaID]
+			owner, err := params.p.getOwnerOfPrivilegeObject(params.ctx, schemaDesc)
 			if err != nil {
 				return err
 			}
-			if isOwner {
-				// Don't reassign public schema.
-				// TODO(richardjcai): revisit this in 22.2, in 22.1 we do not allow
-				// modifying the public schema.
-				if lCtx.schemaDescs[schemaID].GetName() == catconstants.PublicSchemaName {
+			if owner == oldRole {
+				// Don't reassign the descriptorless public schema for the system
+				// database.
+				if schemaID == keys.SystemPublicSchemaID {
 					continue
 				}
 				if err := n.reassignSchemaOwner(lCtx.schemaDescs[schemaID], currentDbDesc, params); err != nil {
@@ -149,33 +150,36 @@ func (n *reassignOwnedByNode) startExec(params runParams) error {
 		}
 
 		for _, tbID := range lCtx.tbIDs {
-			isOwner, err := isOwner(params.ctx, params.p, lCtx.tbDescs[tbID], oldRole)
+			tbDesc := lCtx.tbDescs[tbID]
+			owner, err := params.p.getOwnerOfPrivilegeObject(params.ctx, tbDesc)
 			if err != nil {
 				return err
 			}
-			if isOwner {
+			if owner == oldRole {
 				if err := n.reassignTableOwner(lCtx.tbDescs[tbID], params); err != nil {
 					return err
 				}
 			}
 		}
 		for _, typID := range lCtx.typIDs {
-			isOwner, err := isOwner(params.ctx, params.p, lCtx.typDescs[typID], oldRole)
+			typDesc := lCtx.typDescs[typID]
+			owner, err := params.p.getOwnerOfPrivilegeObject(params.ctx, typDesc)
 			if err != nil {
 				return err
 			}
-			if isOwner && (lCtx.typDescs[typID].AsAliasTypeDescriptor() == nil) {
+			if owner == oldRole && (lCtx.typDescs[typID].AsAliasTypeDescriptor() == nil) {
 				if err := n.reassignTypeOwner(lCtx.typDescs[typID].(catalog.NonAliasTypeDescriptor), params); err != nil {
 					return err
 				}
 			}
 		}
 		for _, fnID := range lCtx.fnIDs {
-			isOwner, err := isOwner(params.ctx, params.p, lCtx.fnDescs[fnID], oldRole)
+			fnDesc := lCtx.fnDescs[fnID]
+			owner, err := params.p.getOwnerOfPrivilegeObject(params.ctx, fnDesc)
 			if err != nil {
 				return err
 			}
-			if isOwner {
+			if owner == oldRole {
 				if err := n.reassignFunctionOwner(lCtx.fnDescs[fnID], params); err != nil {
 					return err
 				}