From 54543077a2d768725814782226cbe4d1abb948d4 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Thu, 24 Feb 2022 18:17:49 -0500 Subject: [PATCH] sql: dropping user with default privilege's error message is more clear Release note (sql change): When dropping a user that has default privileges, the error message now includes which database and schema the default privileges are defined in. Additionally a hint is given to show exactly how to remove the default privileges. Example: role testuser2 cannot be dropped because some objects depend on it owner of default privileges on new sequences belonging to role testuser2 in database testdb2 privileges for default privileges on new sequences belonging to role testuser3 in database testdb2 privileges for default privileges on new sequences for all roles in database test privileges for default privileges on new sequences for all roles in database testdb2 --- pkg/sql/drop_role.go | 151 +++++++++++------- .../drop_role_with_default_privileges | 114 +++++++------ pkg/sql/logictest/testdata/logic_test/owner | 4 +- 3 files changed, 163 insertions(+), 106 deletions(-) diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index 75b298d0886a..b059ee713e5d 100644 --- a/pkg/sql/drop_role.go +++ b/pkg/sql/drop_role.go @@ -14,6 +14,7 @@ import ( "context" "fmt" "sort" + "strings" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/security" @@ -154,7 +155,7 @@ func (n *DropRoleNode) startExec(params runParams) error { break } } - return accumulateDependentDefaultPrivileges(db, userNames) + return accumulateDependentDefaultPrivileges(db.GetDefaultPrivilegeDescriptor(), userNames, db.GetName()) }); err != nil { return err } @@ -259,6 +260,20 @@ func (n *DropRoleNode) startExec(params runParams) error { name := security.MakeSQLUsernameFromPreNormalizedString(names[i]) // Did the user own any objects? dependentObjects := userNames[name] + + // Sort the slice so we're guaranteed the same ordering on errors. + sort.SliceStable(dependentObjects, func(i int, j int) bool { + if dependentObjects[i].ObjectType != dependentObjects[j].ObjectType { + return dependentObjects[i].ObjectType < dependentObjects[j].ObjectType + } + + if dependentObjects[i].ObjectName != dependentObjects[j].ObjectName { + return dependentObjects[i].ObjectName < dependentObjects[j].ObjectName + } + + return dependentObjects[i].ErrorMessage.Error() < dependentObjects[j].ErrorMessage.Error() + }) + var hints []string if len(dependentObjects) > 0 { objectsMsg := tree.NewFmtCtx(tree.FmtSimple) for _, obj := range dependentObjects { @@ -268,6 +283,7 @@ func (n *DropRoleNode) startExec(params runParams) error { case defaultPrivilege: hasDependentDefaultPrivilege = true objectsMsg.WriteString(fmt.Sprintf("\n%s", obj.ErrorMessage)) + hints = append(hints, errors.GetAllHints(obj.ErrorMessage)...) } } objects := objectsMsg.CloseAndGetString() @@ -276,10 +292,7 @@ func (n *DropRoleNode) startExec(params runParams) error { name, objects) if hasDependentDefaultPrivilege { err = errors.WithHint(err, - "use SHOW DEFAULT PRIVILEGES FOR ROLE to find existing default privileges"+ - " and execute ALTER DEFAULT PRIVILEGES {FOR ROLE ... / FOR ALL ROLES} "+ - "REVOKE ... ON ... FROM ... to remove them"+ - "\nsee: SHOW DEFAULT PRIVILEGES and ALTER DEFAULT PRIVILEGES", + strings.Join(hints, "\n"), ) } return err @@ -427,58 +440,12 @@ func (*DropRoleNode) Close(context.Context) {} // accumulateDependentDefaultPrivileges checks for any default privileges // that the users in userNames have and append them to the objectAndType array. func accumulateDependentDefaultPrivileges( - db catalog.DatabaseDescriptor, userNames map[security.SQLUsername][]objectAndType, + defaultPrivilegeDescriptor catalog.DefaultPrivilegeDescriptor, + userNames map[security.SQLUsername][]objectAndType, + dbName string, ) error { - addDependentPrivileges := func(object tree.AlterDefaultPrivilegesTargetObject, defaultPrivs descpb.PrivilegeDescriptor, role descpb.DefaultPrivilegesRole) { - var objectType string - switch object { - case tree.Tables: - objectType = "relations" - case tree.Sequences: - objectType = "sequences" - case tree.Types: - objectType = "types" - case tree.Schemas: - objectType = "schemas" - } - - for _, privs := range defaultPrivs.Users { - if !role.ForAllRoles { - if _, ok := userNames[role.Role]; ok { - userNames[role.Role] = append(userNames[role.Role], - objectAndType{ - ObjectType: defaultPrivilege, - ErrorMessage: errors.Newf( - "owner of default privileges on new %s belonging to role %s", - objectType, role.Role, - ), - }) - } - } - grantee := privs.User() - if _, ok := userNames[grantee]; ok { - var err error - if role.ForAllRoles { - err = errors.Newf( - "privileges for default privileges on new %s for all roles", - objectType, - ) - } else { - err = errors.Newf( - "privileges for default privileges on new %s belonging to role %s", - objectType, role.Role, - ) - } - userNames[grantee] = append(userNames[grantee], - objectAndType{ - ObjectType: defaultPrivilege, - ErrorMessage: err, - }) - } - } - } // No error is returned. - return db.GetDefaultPrivilegeDescriptor().ForEachDefaultPrivilegeForRole(func( + return defaultPrivilegeDescriptor.ForEachDefaultPrivilegeForRole(func( defaultPrivilegesForRole descpb.DefaultPrivilegesForRole) error { role := descpb.DefaultPrivilegesRole{} if defaultPrivilegesForRole.IsExplicitRole() { @@ -487,8 +454,80 @@ func accumulateDependentDefaultPrivileges( role.ForAllRoles = true } for object, defaultPrivs := range defaultPrivilegesForRole.DefaultPrivilegesPerObject { - addDependentPrivileges(object, defaultPrivs, role) + addDependentPrivileges(object, defaultPrivs, role, userNames, dbName) } return nil }) } + +func addDependentPrivileges( + object tree.AlterDefaultPrivilegesTargetObject, + defaultPrivs descpb.PrivilegeDescriptor, + role descpb.DefaultPrivilegesRole, + userNames map[security.SQLUsername][]objectAndType, + dbName string, +) { + var objectType string + switch object { + case tree.Tables: + objectType = "relations" + case tree.Sequences: + objectType = "sequences" + case tree.Types: + objectType = "types" + case tree.Schemas: + objectType = "schemas" + } + + createHint := func( + role descpb.DefaultPrivilegesRole, + grantee security.SQLUsername, + ) string { + + roleString := "ALL ROLES" + if !role.ForAllRoles { + roleString = fmt.Sprintf("ROLE %s", role.Role.SQLIdentifier()) + } + + return fmt.Sprintf("USE %s; ALTER DEFAULT PRIVILEGES FOR %s REVOKE ALL ON %s FROM %s;", + dbName, roleString, strings.ToUpper(object.String()), grantee.SQLIdentifier()) + } + + for _, privs := range defaultPrivs.Users { + grantee := privs.User() + if !role.ForAllRoles { + if _, ok := userNames[role.Role]; ok { + hint := createHint(role, grantee) + userNames[role.Role] = append(userNames[role.Role], + objectAndType{ + ObjectType: defaultPrivilege, + ErrorMessage: errors.WithHint( + errors.Newf( + "owner of default privileges on new %s belonging to role %s in database %s", + objectType, role.Role, dbName, + ), hint), + }) + } + } + if _, ok := userNames[grantee]; ok { + hint := createHint(role, grantee) + var err error + if role.ForAllRoles { + err = errors.Newf( + "privileges for default privileges on new %s for all roles in database %s", + objectType, dbName, + ) + } else { + err = errors.Newf( + "privileges for default privileges on new %s belonging to role %s in database %s", + objectType, role.Role, dbName, + ) + } + userNames[grantee] = append(userNames[grantee], + objectAndType{ + ObjectType: defaultPrivilege, + ErrorMessage: errors.WithHint(err, hint), + }) + } + } +} diff --git a/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges b/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges index 793dcc781def..b5d56a8a0707 100644 --- a/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges +++ b/pkg/sql/logictest/testdata/logic_test/drop_role_with_default_privileges @@ -1,85 +1,103 @@ statement ok -CREATE USER test1; -CREATE USER test2; -GRANT test1 TO ROOT; +CREATE USER testuser1; +CREATE USER testuser2; +GRANT testuser1 TO ROOT; statement ok -ALTER DEFAULT PRIVILEGES FOR ROLE test1 GRANT SELECT ON TABLES TO test2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 GRANT SELECT ON TABLES TO testuser2; -statement error pq: role test1 cannot be dropped because some objects depend on it\nowner of default privileges on new relations belonging to role test1 -DROP ROLE test1 +statement error pq: role testuser1 cannot be dropped because some objects depend on it\nowner of default privileges on new relations belonging to role testuser1 in database test +DROP ROLE testuser1 -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new relations belonging to role test1 -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new relations belonging to role testuser1 in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE SELECT ON TABLES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 GRANT USAGE ON SCHEMAS TO test2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE SELECT ON TABLES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 GRANT USAGE ON SCHEMAS TO testuser2; -statement error pq: role test1 cannot be dropped because some objects depend on it\nowner of default privileges on new schemas belonging to role test1 -DROP ROLE test1 +statement error pq: role testuser1 cannot be dropped because some objects depend on it\nowner of default privileges on new schemas belonging to role testuser1 in database test +DROP ROLE testuser1 -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new schemas belonging to role test1 -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new schemas belonging to role testuser1 in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE USAGE ON SCHEMAS FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 GRANT USAGE ON TYPES TO test2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE USAGE ON SCHEMAS FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 GRANT USAGE ON TYPES TO testuser2; -statement error pq: role test1 cannot be dropped because some objects depend on it\nowner of default privileges on new types belonging to role test1 -DROP ROLE test1 +statement error pq: role testuser1 cannot be dropped because some objects depend on it\nowner of default privileges on new types belonging to role testuser1 in database test +DROP ROLE testuser1 -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new types belonging to role test1 -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new types belonging to role testuser1 in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE USAGE ON TYPES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 GRANT SELECT ON SEQUENCES TO test2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE USAGE ON TYPES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 GRANT SELECT ON SEQUENCES TO testuser2; -statement error pq: role test1 cannot be dropped because some objects depend on it\nowner of default privileges on new sequences belonging to role test1 -DROP ROLE test1 +statement error pq: role testuser1 cannot be dropped because some objects depend on it\nowner of default privileges on new sequences belonging to role testuser1 in database test +DROP ROLE testuser1 -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new sequences belonging to role test1 -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new sequences belonging to role testuser1 in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE SELECT ON TABLES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE USAGE ON SCHEMAS FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE USAGE ON TYPES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ROLE test1 REVOKE SELECT ON SEQUENCES FROM test2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE SELECT ON TABLES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE USAGE ON SCHEMAS FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE USAGE ON TYPES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser1 REVOKE SELECT ON SEQUENCES FROM testuser2; statement ok -DROP ROLE test1; +DROP ROLE testuser1; statement ok -DROP ROLE test2; +DROP ROLE testuser2; statement ok -CREATE USER test2 +CREATE USER testuser2 statement ok -ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON TABLES TO test2 +ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON TABLES TO testuser2 -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new relations for all roles -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new relations for all roles in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT USAGE ON SCHEMAS TO test2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE SELECT ON TABLES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT USAGE ON SCHEMAS TO testuser2; -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new schemas for all roles -DROP ROLE test2; +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new schemas for all roles in database test +DROP ROLE testuser2; statement ok -ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE USAGE ON SCHEMAS FROM test2; -ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT USAGE ON TYPES TO test2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE USAGE ON SCHEMAS FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT USAGE ON TYPES TO testuser2; -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new types for all roles -DROP ROLE test2 +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new types for all roles in database test +DROP ROLE testuser2 statement ok -ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE USAGE ON TYPES FROM test2; -ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON SEQUENCES TO test2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE USAGE ON TYPES FROM testuser2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON SEQUENCES TO testuser2; -statement error pq: role test2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new sequences for all roles -DROP ROLE test2 +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nprivileges for default privileges on new sequences for all roles in database test +DROP ROLE testuser2 + +# Grant default privileges to testuser2 in a second database. +statement ok +CREATE ROLE testuser3; +GRANT testuser2 TO root; +GRANT testuser3 TO root; +CREATE DATABASE testdb2; +USE testdb2; +ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON SEQUENCES TO testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser3 GRANT SELECT ON SEQUENCES TO testuser2; +ALTER DEFAULT PRIVILEGES FOR ROLE testuser2 GRANT SELECT ON SEQUENCES TO testuser3; + +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nowner of default privileges on new sequences belonging to role testuser2 in database testdb2\nprivileges for default privileges on new sequences belonging to role testuser3 in database testdb2\nprivileges for default privileges on new sequences for all roles in database test\nprivileges for default privileges on new sequences for all roles in database testdb2 +DROP ROLE testuser2 + +# Check the hint output. +statement error pq: role testuser2 cannot be dropped because some objects depend on it\nowner of default privileges on new sequences belonging to role testuser2 in database testdb2\nprivileges for default privileges on new sequences belonging to role testuser3 in database testdb2\nprivileges for default privileges on new sequences for all roles in database test\nprivileges for default privileges on new sequences for all roles in database testdb2\nHINT: USE testdb2; ALTER DEFAULT PRIVILEGES FOR ROLE testuser2 REVOKE ALL ON SEQUENCES FROM testuser3;\nUSE testdb2; ALTER DEFAULT PRIVILEGES FOR ROLE testuser3 REVOKE ALL ON SEQUENCES FROM testuser2;\nUSE test; ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE ALL ON SEQUENCES FROM testuser2;\nUSE testdb2; ALTER DEFAULT PRIVILEGES FOR ALL ROLES REVOKE ALL ON SEQUENCES FROM testuser2; +DROP ROLE testuser2 diff --git a/pkg/sql/logictest/testdata/logic_test/owner b/pkg/sql/logictest/testdata/logic_test/owner index 8992363cba4a..56fc7dc7ff1f 100644 --- a/pkg/sql/logictest/testdata/logic_test/owner +++ b/pkg/sql/logictest/testdata/logic_test/owner @@ -202,7 +202,7 @@ statement ok REVOKE ALL ON DATABASE test FROM testuser; REVOKE ALL ON TABLE d.t FROM testuser; -statement error pq: role testuser cannot be dropped because some objects depend on it.*\n.*owner of database d.*\n.*owner of table test.public.t.*\n.*owner of table d.public.t +statement error pq: role testuser cannot be dropped because some objects depend on it\nowner of database d\nowner of table d.public.t\nowner of table test.public.t DROP ROLE testuser # Cannot drop object due to owned objects message should only show the owned @@ -246,5 +246,5 @@ REVOKE ALL ON TABLE d.s.t FROM testuser; user testuser -statement error pq: role testuser cannot be dropped because some objects depend on it.*\n.*owner of database d.*\n.*owner of table test.public.t.*\n.*owner of table d.public.t.*\n.*owner of table d.s.t.*\n.*owner of schema s.*\n.*owner of type d.public._?typ.*\n.*owner of type d.public._?typ +statement error pq: role testuser cannot be dropped because some objects depend on it\nowner of database d\nowner of schema s DROP ROLE testuser