diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index eb18c16bbe79..50a4f124b85e 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -1772,6 +1772,13 @@ func TestTenantLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestTenantLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestTenantLogic_system( t *testing.T, ) { @@ -1793,13 +1800,6 @@ func TestTenantLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestTenantLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestTenantLogic_table( t *testing.T, ) { diff --git a/pkg/roachpb/app_stats.go b/pkg/roachpb/app_stats.go index a7c7cfcc1265..0c0471d10b33 100644 --- a/pkg/roachpb/app_stats.go +++ b/pkg/roachpb/app_stats.go @@ -19,15 +19,15 @@ import ( // StmtFingerprintID is the type of a Statement's fingerprint ID. type StmtFingerprintID uint64 -// ConstructStatementFingerprintID constructs an ID by hashing an anonymized query, its database -// and failure status, and if it was part of an implicit txn. At the time of writing, -// these are the axis' we use to bucket queries for stats collection -// (see stmtKey). +// ConstructStatementFingerprintID constructs an ID by hashing query with +// constants redacted, its database and failure status, and if it was part of an +// implicit txn. At the time of writing, these are the axis' we use to bucket +// queries for stats collection (see stmtKey). func ConstructStatementFingerprintID( - anonymizedStmt string, failed bool, implicitTxn bool, database string, + stmtNoConstants string, failed bool, implicitTxn bool, database string, ) StmtFingerprintID { fnv := util.MakeFNV64() - for _, c := range anonymizedStmt { + for _, c := range stmtNoConstants { fnv.Add(uint64(c)) } for _, c := range database { diff --git a/pkg/server/drain_test.go b/pkg/server/drain_test.go index fe09b10a2e87..4c855532f7ab 100644 --- a/pkg/server/drain_test.go +++ b/pkg/server/drain_test.go @@ -23,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/grpcutil" @@ -37,6 +38,7 @@ import ( // TestDrain tests the Drain RPC. func TestDrain(t *testing.T) { defer leaktest.AfterTest(t)() + skip.UnderRaceWithIssue(t, 86974, "flaky test") defer log.Scope(t).Close(t) doTestDrain(t) } diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go index 79cd95f1b1e9..948a537039bb 100644 --- a/pkg/sql/drop_role.go +++ b/pkg/sql/drop_role.go @@ -25,8 +25,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/roleoption" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sessioninit" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" @@ -73,21 +75,12 @@ func (p *planner) DropRoleNode( }, nil } -type objectType string - -const ( - database objectType = "database" - table objectType = "table" - schema objectType = "schema" - typeObject objectType = "type" - function objectType = "function" - defaultPrivilege objectType = "default_privilege" -) - type objectAndType struct { - ObjectType objectType - ObjectName string - ErrorMessage error + ObjectType privilege.ObjectType + ObjectName string + IsDefaultPrivilege bool + IsGlobalPrivilege bool + ErrorMessage error } func (n *DropRoleNode) startExec(params runParams) error { @@ -137,7 +130,7 @@ func (n *DropRoleNode) startExec(params runParams) error { userNames[db.GetPrivileges().Owner()] = append( userNames[db.GetPrivileges().Owner()], objectAndType{ - ObjectType: database, + ObjectType: privilege.Database, ObjectName: db.GetName(), }) } @@ -181,7 +174,7 @@ func (n *DropRoleNode) startExec(params runParams) error { userNames[tableDescriptor.GetPrivileges().Owner()] = append( userNames[tableDescriptor.GetPrivileges().Owner()], objectAndType{ - ObjectType: table, + ObjectType: privilege.Table, ObjectName: tn.String(), }) } @@ -209,7 +202,7 @@ func (n *DropRoleNode) startExec(params runParams) error { userNames[schemaDesc.GetPrivileges().Owner()] = append( userNames[schemaDesc.GetPrivileges().Owner()], objectAndType{ - ObjectType: schema, + ObjectType: privilege.Schema, ObjectName: schemaDesc.GetName(), }) } @@ -235,7 +228,7 @@ func (n *DropRoleNode) startExec(params runParams) error { userNames[typDesc.GetPrivileges().Owner()] = append( userNames[typDesc.GetPrivileges().Owner()], objectAndType{ - ObjectType: typeObject, + ObjectType: privilege.Type, ObjectName: tn.String(), }) } @@ -252,7 +245,7 @@ func (n *DropRoleNode) startExec(params runParams) error { userNames[fnDesc.GetPrivileges().Owner()] = append( userNames[fnDesc.GetPrivileges().Owner()], objectAndType{ - ObjectType: function, + ObjectType: privilege.Function, ObjectName: name.String(), }, ) @@ -273,6 +266,10 @@ func (n *DropRoleNode) startExec(params runParams) error { } } + if err := addDependentPrivilegesFromSystemPrivileges(params.ctx, n.roleNames, params.p, privilegeObjectFormatter, userNames); err != nil { + return err + } + // Was there any object depending on that user? if privilegeObjectFormatter.Len() > 0 { fnl := tree.NewFmtCtx(tree.FmtSimple) @@ -311,13 +308,14 @@ func (n *DropRoleNode) startExec(params runParams) error { if len(dependentObjects) > 0 { objectsMsg := tree.NewFmtCtx(tree.FmtSimple) for _, obj := range dependentObjects { - switch obj.ObjectType { - case database, table, schema, typeObject, function: - objectsMsg.WriteString(fmt.Sprintf("\nowner of %s %s", obj.ObjectType, obj.ObjectName)) - case defaultPrivilege: + if obj.IsDefaultPrivilege { hasDependentDefaultPrivilege = true objectsMsg.WriteString(fmt.Sprintf("\n%s", obj.ErrorMessage)) hints = append(hints, errors.GetAllHints(obj.ErrorMessage)...) + } else if obj.IsGlobalPrivilege { + objectsMsg.WriteString(fmt.Sprintf("\n%s", obj.ErrorMessage)) + } else { + objectsMsg.WriteString(fmt.Sprintf("\nowner of %s %s", obj.ObjectType, obj.ObjectName)) } } objects := objectsMsg.CloseAndGetString() @@ -543,7 +541,7 @@ func addDependentPrivileges( hint := createHint(role, grantee) userNames[role.Role] = append(userNames[role.Role], objectAndType{ - ObjectType: defaultPrivilege, + IsDefaultPrivilege: true, ErrorMessage: errors.WithHint( errors.Newf( "owner of default privileges on new %s belonging to role %s in database %s%s", @@ -568,9 +566,69 @@ func addDependentPrivileges( } userNames[grantee] = append(userNames[grantee], objectAndType{ - ObjectType: defaultPrivilege, - ErrorMessage: errors.WithHint(err, hint), + IsDefaultPrivilege: true, + ErrorMessage: errors.WithHint(err, hint), }) } } } + +func addDependentPrivilegesFromSystemPrivileges( + ctx context.Context, + usernames []username.SQLUsername, + p *planner, + privilegeObjectFormatter *tree.FmtCtx, + userNamesToDependentPrivileges map[username.SQLUsername][]objectAndType, +) (retErr error) { + names := make([]string, len(usernames)) + for i, username := range usernames { + names[i] = username.Normalized() + } + rows, err := p.QueryIteratorEx(ctx, `drop-role-get-system-privileges`, sessiondata.NodeUserSessionDataOverride, + `SELECT DISTINCT username, path, privileges FROM system.privileges WHERE username = ANY($1) ORDER BY 1, 2`, names) + if err != nil { + return err + } + defer func() { + retErr = errors.CombineErrors(retErr, rows.Close()) + }() + for { + ok, err := rows.Next(ctx) + if err != nil { + return err + } + if !ok { + break + } + name := tree.MustBeDString(rows.Cur()[0]) + sqlUsername := username.MakeSQLUsernameFromPreNormalizedString(string(name)) + path := tree.MustBeDString(rows.Cur()[1]) + privileges := tree.MustBeDArray(rows.Cur()[2]) + obj, err := syntheticprivilege.Parse(string(path)) + if err != nil { + return err + } + + if obj.GetObjectType() == privilege.Global { + for _, priv := range privileges.Array { + userNamesToDependentPrivileges[sqlUsername] = append( + userNamesToDependentPrivileges[sqlUsername], + objectAndType{ + IsGlobalPrivilege: true, + ErrorMessage: errors.Newf( + "%s has global %s privilege", sqlUsername, priv.String(), + )}) + } + continue + } + if privilegeObjectFormatter.Len() > 0 { + privilegeObjectFormatter.WriteString(", ") + } + privilegeObjectFormatter.WriteString(string(obj.GetObjectType())) + if obj.GetName() != "" { + privilegeObjectFormatter.WriteString(" ") + privilegeObjectFormatter.FormatName(obj.GetName()) + } + } + return nil +} diff --git a/pkg/sql/logictest/testdata/logic_test/system_privileges b/pkg/sql/logictest/testdata/logic_test/synthetic_privileges similarity index 82% rename from pkg/sql/logictest/testdata/logic_test/system_privileges rename to pkg/sql/logictest/testdata/logic_test/synthetic_privileges index a89fcea738bd..6a00481f1679 100644 --- a/pkg/sql/logictest/testdata/logic_test/system_privileges +++ b/pkg/sql/logictest/testdata/logic_test/synthetic_privileges @@ -243,3 +243,43 @@ query TTTT SELECT * FROM system.privileges ---- root /global/ {MODIFYCLUSTERSETTING} {} + +statement ok +DROP USER testuser + +statement ok +CREATE USER testuser + +statement ok +GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser + +statement ok +GRANT SELECT ON crdb_internal.tables TO testuser + +statement ok +GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser + +statement error pq: cannot drop role/user testuser: grants still exist on external_connection foo, virtual_table "crdb_internal.tables" +DROP USER testuser + +statement ok +GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser2 + +statement ok +GRANT USAGE ON EXTERNAL CONNECTION foo TO testuser2 + +statement error pq: cannot drop roles/users testuser, testuser2: grants still exist on external_connection foo, virtual_table "crdb_internal.tables", external_connection foo +DROP USER testuser, testuser2 + +# Check the error message for a combination of global and default privileges. +statement ok +CREATE USER testuser3 + +statement ok +GRANT SYSTEM MODIFYCLUSTERSETTING, EXTERNALCONNECTION TO testuser3 + +statement ok +ALTER DEFAULT PRIVILEGES GRANT SELECT ON TABLES TO testuser3 + +statement error pq: role testuser3 cannot be dropped because some objects depend on it\nprivileges for default privileges on new relations belonging to role root in database test\ntestuser3 has global 'EXTERNALCONNECTION' privilege\ntestuser3 has global 'MODIFYCLUSTERSETTING' privilege +DROP USER testuser3 diff --git a/pkg/sql/logictest/testdata/logic_test/system_privileges_mixed b/pkg/sql/logictest/testdata/logic_test/synthetic_privileges_mixed similarity index 100% rename from pkg/sql/logictest/testdata/logic_test/system_privileges_mixed rename to pkg/sql/logictest/testdata/logic_test/synthetic_privileges_mixed diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index 2479f5054d10..4dc7cd2c5a99 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -1759,6 +1759,13 @@ func TestLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestLogic_system( t *testing.T, ) { @@ -1780,13 +1787,6 @@ func TestLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestLogic_table( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index ed01a5772178..fe534b0d8407 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -1759,6 +1759,13 @@ func TestLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestLogic_system( t *testing.T, ) { @@ -1780,13 +1787,6 @@ func TestLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestLogic_table( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 4906e5585551..a146fe79ca3f 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -1773,6 +1773,13 @@ func TestLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestLogic_system( t *testing.T, ) { @@ -1794,13 +1801,6 @@ func TestLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestLogic_table( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-mixed-22.1-22.2/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.1-22.2/generated_test.go index 56f7418f017d..bebf6e6a1498 100644 --- a/pkg/sql/logictest/tests/local-mixed-22.1-22.2/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-22.1-22.2/generated_test.go @@ -79,9 +79,9 @@ func TestLogic_new_schema_changer_mixed( runLogicTest(t, "new_schema_changer_mixed") } -func TestLogic_system_privileges_mixed( +func TestLogic_synthetic_privileges_mixed( t *testing.T, ) { defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges_mixed") + runLogicTest(t, "synthetic_privileges_mixed") } diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index f6791f2bd8c9..b88c81f9e44f 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -1766,6 +1766,13 @@ func TestLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestLogic_system( t *testing.T, ) { @@ -1787,13 +1794,6 @@ func TestLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestLogic_table( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 4b225f118be6..d0dda9818fe1 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -1920,6 +1920,13 @@ func TestLogic_subquery_correlated( runLogicTest(t, "subquery_correlated") } +func TestLogic_synthetic_privileges( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "synthetic_privileges") +} + func TestLogic_system( t *testing.T, ) { @@ -1941,13 +1948,6 @@ func TestLogic_system_namespace( runLogicTest(t, "system_namespace") } -func TestLogic_system_privileges( - t *testing.T, -) { - defer leaktest.AfterTest(t)() - runLogicTest(t, "system_privileges") -} - func TestLogic_table( t *testing.T, ) {