Merge #86499 #87052 #87070

86499: sql: prevent dropping roles with synthetic privileges r=rafiss a=RichardJCai

Followup work to be done to support DROP OWNED BY and
REASSIGN OWNED BY here.

Release justification: Bug fix to newly introduced feature
Release note: None

87052: server: skip TestDrain under testrace r=srosenberg a=adityamaru

Informs: #86974

Release note: None

Release justification: test only change

87070: roachpb: clarify parameter name in ConstructStatementFingerprintID r=rafiss a=rafiss

"anonymizedStmt" is incorrect here -- the value passed in should be a
query with constants removed, but identifiers present.

Release note: None

Release justification: low risk change

Co-authored-by: richardjcai <[email protected]>
Co-authored-by: adityamaru <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

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 {

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

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

pkg/sql/logictest/testdata/logic_test/system_privileges → 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