diff --git a/pkg/sql/alter_default_privileges.go b/pkg/sql/alter_default_privileges.go index 64683b9c36ae..d47c020fc020 100644 --- a/pkg/sql/alter_default_privileges.go +++ b/pkg/sql/alter_default_privileges.go @@ -134,10 +134,12 @@ 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") } @@ -145,7 +147,7 @@ func (n *alterDefaultPrivilegesNode) startExec(params runParams) error { // 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 @@ -153,7 +155,7 @@ func (n *alterDefaultPrivilegesNode) startExec(params runParams) error { 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()) } } } diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_schema b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_schema index f4f6ed67e2af..5df8fb612911 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_schema +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_sequence b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_sequence index ec5bfb44bfca..78e876b93d37 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_sequence +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_table b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_table index e18e3d67db18..a244e0a753b5 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_table +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_type b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_type index 3f7dc6982703..dd3062f0ee9f 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_type +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema index 4bfbdd1038b9..22d74fd6a92a 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_in_schema +++ b/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. diff --git a/pkg/sql/logictest/testdata/logic_test/reassign_owned_by b/pkg/sql/logictest/testdata/logic_test/reassign_owned_by index 0646f2101caf..4e8756a81530 100644 --- a/pkg/sql/logictest/testdata/logic_test/reassign_owned_by +++ b/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 diff --git a/pkg/sql/logictest/testdata/logic_test/show_default_privileges b/pkg/sql/logictest/testdata/logic_test/show_default_privileges index 779ff3b6297a..36dcbddbd83c 100644 --- a/pkg/sql/logictest/testdata/logic_test/show_default_privileges +++ b/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 diff --git a/pkg/sql/reassign_owned_by.go b/pkg/sql/reassign_owned_by.go index 89e1d0d14cd0..c00c63e47618 100644 --- a/pkg/sql/reassign_owned_by.go +++ b/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 }