catalog: add descriptor repair to remove missing roles

Previously, we had a bug that could lead to descriptors having
privileages to roles that no longer exist. This could lead to certain
commands like SHOW GRANTS breaking. To address this, this patch will add
descirptor repair logic to automatically clean up oprhaned privileges.

Fixes: #122552

Release note (bug fix): Add automated clean up / validation for dropped
roles inside descriptors.

pkg/sql/catalog/BUILD.bazel

@@ -27,6 +27,7 @@ go_library(
         "//pkg/keys",
         "//pkg/kv",
         "//pkg/roachpb",
+        "//pkg/security/username",
         "//pkg/server/telemetry",
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/catalog/catpb",

pkg/sql/catalog/dbdesc/database_desc_builder.go

@@ -183,6 +183,25 @@ func (ddb *databaseDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (ddb *databaseDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(ddb.maybeModified.Privileges.Users))
+	for _, priv := range ddb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(ddb.maybeModified.Privileges.Users) {
+		ddb.maybeModified.Privileges.Users = newPrivs
+		ddb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (ddb *databaseDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	ddb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/dbdesc/database_test.go

@@ -389,13 +389,18 @@ func TestMaybeConvertIncompatibleDBPrivilegesToDefaultPrivileges(t *testing.T) {
 	}
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.DatabaseDescriptor
 		validIDs              catalog.DescriptorIDSet
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "schema",
@@ -406,13 +411,15 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					"bar": {ID: 101},
 					"baz": {ID: 12345},
 				},
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.DatabaseDescriptor{
 				Name: "foo",
 				ID:   100,
 				Schemas: map[string]descpb.DatabaseDescriptor_SchemaInfo{
 					"bar": {ID: 101},
 				},
+				Privileges: goodPrivilege,
 			},
 			validIDs: catalog.MakeDescriptorIDSet(100, 101),
 		},
@@ -427,8 +434,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 			require.NoError(t, b.StripDanglingBackReferences(test.validIDs.Contains, func(id jobspb.JobID) bool {
 				return false
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableDatabase()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableDatabase().DatabaseDesc(), desc.DatabaseDesc())
 		})
 	}

pkg/sql/catalog/descriptor.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -108,6 +109,10 @@ type DescriptorBuilder interface {
 		nonTerminalJobIDMightExist func(id jobspb.JobID) bool,
 	) error
 
+	// StripNonExistentRoles removes any privileges granted to roles that
+	// don't exist.
+	StripNonExistentRoles(roleExists func(role username.SQLUsername) bool) error
+
 	// SetRawBytesInStorage sets `rawBytesInStorage` field by deep-copying `rawBytes`.
 	SetRawBytesInStorage(rawBytes []byte)
 

pkg/sql/catalog/funcdesc/func_desc_builder.go

@@ -151,6 +151,25 @@ func (fdb *functionDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (fdb *functionDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(fdb.maybeModified.Privileges.Users))
+	for _, priv := range fdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(fdb.maybeModified.Privileges.Users) {
+		fdb.maybeModified.Privileges.Users = newPrivs
+		fdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (fdb *functionDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	fdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/funcdesc/func_desc_test.go

@@ -804,13 +804,18 @@ func TestToOverload(t *testing.T) {
 	}
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.FunctionDescriptor
 		validIDs              catalog.DescriptorIDSet
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "depended on by",
@@ -827,9 +832,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.FunctionDescriptor{
 				Name: "foo",
@@ -840,9 +843,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 						ColumnIDs: []descpb.ColumnID{1},
 					},
 				},
-				Privileges: &catpb.PrivilegeDescriptor{
-					Version: catpb.Version23_2,
-				},
+				Privileges: goodPrivilege,
 			},
 			validIDs: catalog.MakeDescriptorIDSet(104, 105),
 		},
@@ -857,8 +858,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 			require.NoError(t, b.StripDanglingBackReferences(test.validIDs.Contains, func(id jobspb.JobID) bool {
 				return false
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableFunction()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableFunction().FuncDesc(), desc.FuncDesc())
 		})
 	}

pkg/sql/catalog/post_deserialization_changes.go

@@ -140,4 +140,8 @@ const (
 	// FixedIncorrectForeignKeyOrigins indicates that foreign key origin /
 	// reference IDs that should point to the current descriptor were fixed.
 	FixedIncorrectForeignKeyOrigins
+
+	// StrippedNonExistentRoles indicates that at least one role identified did
+	// not exist.
+	StrippedNonExistentRoles
 )

pkg/sql/catalog/schemadesc/BUILD.bazel

@@ -16,6 +16,7 @@ go_library(
         "//pkg/clusterversion",
         "//pkg/jobs/jobspb",
         "//pkg/keys",
+        "//pkg/security/username",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catpb",
         "//pkg/sql/catalog/catprivilege",

pkg/sql/catalog/schemadesc/schema_desc_builder.go

@@ -13,7 +13,9 @@ package schemadesc
 import (
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -138,6 +140,23 @@ func (sdb *schemaDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+func (sdb *schemaDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(sdb.maybeModified.Privileges.Users))
+	for _, priv := range sdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(sdb.maybeModified.Privileges.Users) {
+		sdb.maybeModified.Privileges.Users = newPrivs
+		sdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (sdb *schemaDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	sdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/schemadesc/schema_desc_test.go

@@ -243,3 +243,42 @@ func TestValidateCrossSchemaReferences(t *testing.T) {
 		}
 	}
 }
+
+func TestStripNonExistentRoles(t *testing.T) {
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
+	tests := []struct {
+		desc    descpb.SchemaDescriptor
+		expDesc descpb.SchemaDescriptor
+	}{
+		{ // 0
+			desc: descpb.SchemaDescriptor{
+				ID:         52,
+				ParentID:   51,
+				Name:       "schema1",
+				Privileges: badPrivilege,
+			},
+			expDesc: descpb.SchemaDescriptor{
+				ID:         52,
+				ParentID:   51,
+				Name:       "schema1",
+				Privileges: goodPrivilege,
+			},
+		},
+	}
+	for _, test := range tests {
+		b := schemadesc.NewBuilder(&test.desc)
+		require.NoError(t, b.RunPostDeserializationChanges())
+		out := schemadesc.NewBuilder(&test.expDesc)
+		require.NoError(t, out.RunPostDeserializationChanges())
+		require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+			return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+		}))
+		desc := b.BuildCreatedMutableSchema()
+		require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
+		require.Equal(t, out.BuildCreatedMutableSchema().SchemaDesc(), desc.SchemaDesc())
+	}
+}

pkg/sql/catalog/tabledesc/BUILD.bazel

@@ -25,6 +25,7 @@ go_library(
         "//pkg/jobs/jobspb",
         "//pkg/keys",
         "//pkg/roachpb",
+        "//pkg/security/username",
         "//pkg/settings",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/catenumpb",

pkg/sql/catalog/tabledesc/table_desc_builder.go

@@ -14,8 +14,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catprivilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/seqexpr"
@@ -264,6 +266,25 @@ func (tdb *tableDescriptorBuilder) StripDanglingBackReferences(
 	return nil
 }
 
+// StripNonExistentRoles implements the catalog.DescriptorBuilder
+// interface.
+func (tdb *tableDescriptorBuilder) StripNonExistentRoles(
+	roleExists func(role username.SQLUsername) bool,
+) error {
+	newPrivs := make([]catpb.UserPrivileges, 0, len(tdb.maybeModified.Privileges.Users))
+	for _, priv := range tdb.maybeModified.Privileges.Users {
+		exists := roleExists(priv.UserProto.Decode())
+		if exists {
+			newPrivs = append(newPrivs, priv)
+		}
+	}
+	if len(newPrivs) != len(tdb.maybeModified.Privileges.Users) {
+		tdb.maybeModified.Privileges.Users = newPrivs
+		tdb.changes.Add(catalog.StrippedNonExistentRoles)
+	}
+	return nil
+}
+
 // SetRawBytesInStorage implements the catalog.DescriptorBuilder interface.
 func (tdb *tableDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) {
 	tdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy

pkg/sql/catalog/tabledesc/table_desc_test.go

@@ -14,7 +14,9 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/stretchr/testify/require"
 )
@@ -58,14 +60,19 @@ func (desc *Mutable) TestingSetClusterVersion(d descpb.TableDescriptor) {
 	desc.original = makeImmutable(&d)
 }
 
-func TestStripDanglingBackReferences(t *testing.T) {
+func TestStripDanglingBackReferencesAndRoles(t *testing.T) {
 	type testCase struct {
 		name                  string
 		input, expectedOutput descpb.TableDescriptor
 		validDescIDs          catalog.DescriptorIDSet
 		validJobIDs           map[jobspb.JobID]struct{}
 	}
 
+	badPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	goodPrivilege := catpb.NewBaseDatabasePrivilegeDescriptor(username.RootUserName())
+	badPrivilege.Users = append(badPrivilege.Users, catpb.UserPrivileges{
+		UserProto: username.TestUserName().EncodeProto(),
+	})
 	testData := []testCase{
 		{
 			name: "descriptor IDs",
@@ -86,6 +93,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{OriginTableID: 105, ReferencedTableID: 104},
 				},
 				ReplacementOf: descpb.TableDescriptor_Replacement{ID: 12345},
+				Privileges:    badPrivilege,
 			},
 			expectedOutput: descpb.TableDescriptor{
 				Name: "foo",
@@ -97,6 +105,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 				InboundFKs: []descpb.ForeignKeyConstraint{
 					{OriginTableID: 105},
 				},
+				Privileges: goodPrivilege,
 			},
 			validDescIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105),
 			validJobIDs:  map[jobspb.JobID]struct{}{},
@@ -115,7 +124,8 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{MutationID: 1},
 					{MutationID: 2},
 				},
-				DropJobID: 1,
+				DropJobID:  1,
+				Privileges: badPrivilege,
 			},
 			expectedOutput: descpb.TableDescriptor{
 				Name: "foo",
@@ -127,6 +137,7 @@ func TestStripDanglingBackReferences(t *testing.T) {
 					{MutationID: 1},
 					{MutationID: 2},
 				},
+				Privileges: goodPrivilege,
 			},
 			validDescIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105),
 			validJobIDs:  map[jobspb.JobID]struct{}{111222333444: {}},
@@ -143,8 +154,12 @@ func TestStripDanglingBackReferences(t *testing.T) {
 				_, ok := test.validJobIDs[id]
 				return ok
 			}))
+			require.NoError(t, b.StripNonExistentRoles(func(role username.SQLUsername) bool {
+				return role.IsAdminRole() || role.IsPublicRole() || role.IsRootUser()
+			}))
 			desc := b.BuildCreatedMutableTable()
 			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences))
+			require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles))
 			require.Equal(t, out.BuildCreatedMutableTable().TableDesc(), desc.TableDesc())
 		})
 	}