From fcf6978ed13fec9d95e621944904419f038c975c Mon Sep 17 00:00:00 2001 From: Faizan Qazi Date: Wed, 17 Apr 2024 15:14:54 +0000 Subject: [PATCH] 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 | 1 + .../catalog/dbdesc/database_desc_builder.go | 19 +++++++++ pkg/sql/catalog/dbdesc/database_test.go | 13 ++++++- pkg/sql/catalog/descriptor.go | 5 +++ pkg/sql/catalog/funcdesc/func_desc_builder.go | 19 +++++++++ pkg/sql/catalog/funcdesc/func_desc_test.go | 19 +++++---- .../catalog/post_deserialization_changes.go | 4 ++ pkg/sql/catalog/schemadesc/BUILD.bazel | 1 + .../catalog/schemadesc/schema_desc_builder.go | 19 +++++++++ .../catalog/schemadesc/schema_desc_test.go | 39 +++++++++++++++++++ pkg/sql/catalog/tabledesc/BUILD.bazel | 1 + .../catalog/tabledesc/table_desc_builder.go | 21 ++++++++++ pkg/sql/catalog/tabledesc/table_desc_test.go | 19 ++++++++- pkg/sql/catalog/typedesc/BUILD.bazel | 1 + pkg/sql/catalog/typedesc/type_desc_builder.go | 21 ++++++++++ pkg/sql/catalog/typedesc/type_desc_test.go | 14 ++++++- pkg/sql/catalog/validate.go | 20 ++++++++++ pkg/sql/crdb_internal.go | 21 ++++++++++ pkg/sql/evalcatalog/BUILD.bazel | 1 + pkg/sql/evalcatalog/pg_updatable.go | 5 +++ .../testdata/logic_test/crdb_internal_catalog | 2 +- pkg/sql/sem/builtins/builtins.go | 38 +++++++++++++++++- pkg/sql/sem/builtins/fixed_oids.go | 2 +- pkg/sql/sem/eval/deps.go | 2 + 24 files changed, 291 insertions(+), 16 deletions(-) diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel index 5c6eb02f4c27..105b137fcf20 100644 --- a/pkg/sql/catalog/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/dbdesc/database_desc_builder.go b/pkg/sql/catalog/dbdesc/database_desc_builder.go index 96a4fca51907..8ff64312cebf 100644 --- a/pkg/sql/catalog/dbdesc/database_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/dbdesc/database_test.go b/pkg/sql/catalog/dbdesc/database_test.go index 19677aeaf5e2..bd962c6c895b 100644 --- a/pkg/sql/catalog/dbdesc/database_test.go +++ b/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,6 +411,7 @@ func TestStripDanglingBackReferences(t *testing.T) { "bar": {ID: 101}, "baz": {ID: 12345}, }, + Privileges: badPrivilege, }, expectedOutput: descpb.DatabaseDescriptor{ Name: "foo", @@ -413,6 +419,7 @@ func TestStripDanglingBackReferences(t *testing.T) { 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()) }) } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index a53314d18823..3c913cc3907a 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/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) diff --git a/pkg/sql/catalog/funcdesc/func_desc_builder.go b/pkg/sql/catalog/funcdesc/func_desc_builder.go index fb76c80a8c99..368f8abf5b23 100644 --- a/pkg/sql/catalog/funcdesc/func_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/funcdesc/func_desc_test.go b/pkg/sql/catalog/funcdesc/func_desc_test.go index 276ca64ace9e..16d00eb0a6ae 100644 --- a/pkg/sql/catalog/funcdesc/func_desc_test.go +++ b/pkg/sql/catalog/funcdesc/func_desc_test.go @@ -814,13 +814,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", @@ -837,9 +842,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ColumnIDs: []descpb.ColumnID{1}, }, }, - Privileges: &catpb.PrivilegeDescriptor{ - Version: catpb.Version23_2, - }, + Privileges: badPrivilege, }, expectedOutput: descpb.FunctionDescriptor{ Name: "foo", @@ -850,9 +853,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ColumnIDs: []descpb.ColumnID{1}, }, }, - Privileges: &catpb.PrivilegeDescriptor{ - Version: catpb.Version23_2, - }, + Privileges: goodPrivilege, }, validIDs: catalog.MakeDescriptorIDSet(104, 105), }, @@ -867,8 +868,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()) }) } diff --git a/pkg/sql/catalog/post_deserialization_changes.go b/pkg/sql/catalog/post_deserialization_changes.go index 909cde2efe81..3d65086e0b9e 100644 --- a/pkg/sql/catalog/post_deserialization_changes.go +++ b/pkg/sql/catalog/post_deserialization_changes.go @@ -132,4 +132,8 @@ const ( // GrantExecuteOnFunctionToPublicRole indicates that EXECUTE was granted // to the public role for a function. GrantExecuteOnFunctionToPublicRole + + // StrippedNonExistentRoles indicates that at least one role identified did + // not exist. + StrippedNonExistentRoles ) diff --git a/pkg/sql/catalog/schemadesc/BUILD.bazel b/pkg/sql/catalog/schemadesc/BUILD.bazel index 5955bfcec8c1..fc1500576249 100644 --- a/pkg/sql/catalog/schemadesc/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/schemadesc/schema_desc_builder.go b/pkg/sql/catalog/schemadesc/schema_desc_builder.go index 7cb61af31c1a..cbb7572b92c3 100644 --- a/pkg/sql/catalog/schemadesc/schema_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/schemadesc/schema_desc_test.go b/pkg/sql/catalog/schemadesc/schema_desc_test.go index 916168239a47..6b3e405cfe30 100644 --- a/pkg/sql/catalog/schemadesc/schema_desc_test.go +++ b/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()) + } +} diff --git a/pkg/sql/catalog/tabledesc/BUILD.bazel b/pkg/sql/catalog/tabledesc/BUILD.bazel index be197b510e64..a1da6105dc27 100644 --- a/pkg/sql/catalog/tabledesc/BUILD.bazel +++ b/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", diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go index 02070fc8aa35..fbeb64cd5b09 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_builder.go +++ b/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 diff --git a/pkg/sql/catalog/tabledesc/table_desc_test.go b/pkg/sql/catalog/tabledesc/table_desc_test.go index 1d601a4ac88f..7bb60915eceb 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_test.go +++ b/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,7 +60,7 @@ 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 @@ -66,6 +68,11 @@ func TestStripDanglingBackReferences(t *testing.T) { 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", @@ -85,6 +92,7 @@ func TestStripDanglingBackReferences(t *testing.T) { {OriginTableID: 105}, }, ReplacementOf: descpb.TableDescriptor_Replacement{ID: 12345}, + Privileges: badPrivilege, }, expectedOutput: descpb.TableDescriptor{ Name: "foo", @@ -96,6 +104,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{}{}, @@ -114,7 +123,8 @@ func TestStripDanglingBackReferences(t *testing.T) { {MutationID: 1}, {MutationID: 2}, }, - DropJobID: 1, + DropJobID: 1, + Privileges: badPrivilege, }, expectedOutput: descpb.TableDescriptor{ Name: "foo", @@ -126,6 +136,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: {}}, @@ -142,8 +153,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()) }) } diff --git a/pkg/sql/catalog/typedesc/BUILD.bazel b/pkg/sql/catalog/typedesc/BUILD.bazel index f0b661bd4d32..69804352e277 100644 --- a/pkg/sql/catalog/typedesc/BUILD.bazel +++ b/pkg/sql/catalog/typedesc/BUILD.bazel @@ -15,6 +15,7 @@ go_library( "//pkg/clusterversion", "//pkg/jobs/jobspb", "//pkg/keys", + "//pkg/security/username", "//pkg/sql/catalog", "//pkg/sql/catalog/catpb", "//pkg/sql/catalog/catprivilege", diff --git a/pkg/sql/catalog/typedesc/type_desc_builder.go b/pkg/sql/catalog/typedesc/type_desc_builder.go index 04567f76b851..a7ac0af10e6f 100644 --- a/pkg/sql/catalog/typedesc/type_desc_builder.go +++ b/pkg/sql/catalog/typedesc/type_desc_builder.go @@ -13,7 +13,9 @@ package typedesc 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" @@ -149,6 +151,25 @@ func (tdb *typeDescriptorBuilder) StripDanglingBackReferences( return nil } +// StripNonExistentRoles implements the catalog.DescriptorBuilder +// interface. +func (tdb *typeDescriptorBuilder) 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 *typeDescriptorBuilder) SetRawBytesInStorage(rawBytes []byte) { tdb.rawBytesInStorage = append([]byte(nil), rawBytes...) // deep-copy diff --git a/pkg/sql/catalog/typedesc/type_desc_test.go b/pkg/sql/catalog/typedesc/type_desc_test.go index 4b08b2019d5f..6832a4f52fcc 100644 --- a/pkg/sql/catalog/typedesc/type_desc_test.go +++ b/pkg/sql/catalog/typedesc/type_desc_test.go @@ -848,13 +848,17 @@ func TestValidateTypeDesc(t *testing.T) { } } -func TestStripDanglingBackReferences(t *testing.T) { +func TestStripDanglingBackReferencesAndRoles(t *testing.T) { type testCase struct { name string input, expectedOutput descpb.TypeDescriptor 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: "referencing descriptor IDs", @@ -866,6 +870,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ArrayTypeID: 105, Kind: descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE, ReferencingDescriptorIDs: []descpb.ID{12345, 105, 5678}, + Privileges: badPrivilege, }, expectedOutput: descpb.TypeDescriptor{ Name: "foo", @@ -875,6 +880,7 @@ func TestStripDanglingBackReferences(t *testing.T) { ArrayTypeID: 105, Kind: descpb.TypeDescriptor_TABLE_IMPLICIT_RECORD_TYPE, ReferencingDescriptorIDs: []descpb.ID{105}, + Privileges: goodPrivilege, }, validIDs: catalog.MakeDescriptorIDSet(100, 101, 104, 105), }, @@ -889,8 +895,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.BuildCreatedMutableType() require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedDanglingBackReferences)) + require.True(t, desc.GetPostDeserializationChanges().Contains(catalog.StrippedNonExistentRoles)) require.Equal(t, out.BuildCreatedMutableType().TypeDesc(), desc.TypeDesc()) }) } diff --git a/pkg/sql/catalog/validate.go b/pkg/sql/catalog/validate.go index 47c168c15308..cca52af05707 100644 --- a/pkg/sql/catalog/validate.go +++ b/pkg/sql/catalog/validate.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/clusterversion" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/errors" @@ -196,3 +197,22 @@ func ValidateOutboundTypeRefBackReference(selfID descpb.ID, typ TypeDescriptor) return errors.AssertionFailedf("depends-on type %q (%d) has no corresponding referencing-descriptor back references", typ.GetName(), typ.GetID()) } + +// ValidateRolesInDescriptor validates roles within a descriptor. +func ValidateRolesInDescriptor( + descriptor Descriptor, RoleExists func(username username.SQLUsername) (bool, error), +) error { + for _, priv := range descriptor.GetPrivileges().Users { + exists, err := RoleExists(priv.User()) + if err != nil { + return err + } + if !exists { + return errors.AssertionFailedf("descriptor %q (%d) has privilege on a role %q that doesn't exist", + descriptor.GetName(), + descriptor.GetID(), + priv.User()) + } + } + return nil +} diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 95b30734a2d5..bef3c0ac1926 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -6176,6 +6176,21 @@ CREATE TABLE crdb_internal.invalid_objects ( for _, validationError := range ve { doError(validationError) } + doError(catalog.ValidateRolesInDescriptor(descriptor, func(username username.SQLUsername) (bool, error) { + if username.IsRootUser() || + username.IsAdminRole() || + username.IsPublicRole() { + return true, nil + } + err := p.CheckRoleExists(ctx, username) + if err != nil && pgerror.GetPGCode(err) == pgcode.UndefinedObject { + err = nil + return false, err + } else if err != nil { + return false, err + } + return true, nil + })) jobs.ValidateJobReferencesInDescriptor(descriptor, jmg, doError) return err } @@ -6622,6 +6637,12 @@ CREATE VIEW crdb_internal.kv_repairable_catalog_corruptions ( system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed') + ), + ( SELECT + array_agg(username) as username_array FROM + (SELECT username + FROM system.users UNION + SELECT 'public' as username) ) ) AS repaired_descriptor diff --git a/pkg/sql/evalcatalog/BUILD.bazel b/pkg/sql/evalcatalog/BUILD.bazel index 73ab778139e4..a8604817f49b 100644 --- a/pkg/sql/evalcatalog/BUILD.bazel +++ b/pkg/sql/evalcatalog/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//pkg/jobs/jobspb", "//pkg/keys", "//pkg/kv", + "//pkg/security/username", "//pkg/sql/catalog", "//pkg/sql/catalog/descbuilder", "//pkg/sql/catalog/descpb", diff --git a/pkg/sql/evalcatalog/pg_updatable.go b/pkg/sql/evalcatalog/pg_updatable.go index a232674fa08a..091f83253697 100644 --- a/pkg/sql/evalcatalog/pg_updatable.go +++ b/pkg/sql/evalcatalog/pg_updatable.go @@ -14,6 +14,7 @@ import ( "context" "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/descbuilder" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" @@ -64,6 +65,7 @@ func (b *Builtins) RepairedDescriptor( encodedDescriptor []byte, descIDMightExist func(id descpb.ID) bool, nonTerminalJobIDMightExist func(id jobspb.JobID) bool, + roleExists func(username username.SQLUsername) bool, ) ([]byte, error) { // Use the largest-possible timestamp as a sentinel value when decoding the // descriptor bytes. This will be used to set the modification time field in @@ -82,6 +84,9 @@ func (b *Builtins) RepairedDescriptor( if err := db.StripDanglingBackReferences(descIDMightExist, nonTerminalJobIDMightExist); err != nil { return nil, err } + if err := db.StripNonExistentRoles(roleExists); err != nil { + return nil, err + } mut := db.BuildCreatedMutable() // Strip the sentinel value from the descriptor. if mvccTimestampSentinel.Equal(mut.GetModificationTime()) { diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog index a7bf9a042b3e..fecf637261d2 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal_catalog @@ -404,7 +404,7 @@ SELECT id, strip_volatile(descriptor) FROM crdb_internal.kv_catalog_descriptor O 4294967195 {"table": {"columns": [{"id": 1, "name": "job_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "start_key", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 3, "name": "end_key", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "resolved", "nullable": true, "type": {"family": "DecimalFamily", "oid": 1700}}, {"id": 5, "name": "resolved_age", "nullable": true, "type": {"family": "IntervalFamily", "intervalDurationField": {}, "oid": 1186}}], "formatVersion": 3, "id": 4294967195, "name": "cluster_replication_spans", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH spans AS (SELECT j.id AS job_id, jsonb_array_elements(((crdb_internal.pb_to_json('progress', i.value)->'streamIngest')->'checkpoint')->'resolvedSpans') AS s FROM system.jobs AS j LEFT JOIN system.job_info AS i ON (j.id = i.job_id) AND (i.info_key = 'legacy_progress') WHERE j.job_type = 'REPLICATION STREAM INGESTION') SELECT job_id, crdb_internal.pretty_key(decode((s->'span')->>'key', 'base64'), 0) AS start_key, crdb_internal.pretty_key(decode((s->'span')->>'endKey', 'base64'), 0) AS end_key, ((((s->'timestamp')->>'wallTime') || '.') || COALESCE(((s->'timestamp')->'logical'), '0'))::DECIMAL AS resolved, date_trunc('second', ((cluster_logical_timestamp() - ((s->'timestamp')->>'wallTime')::INT8) / 1e9)::INTERVAL) AS resolved_age FROM spans"}} 4294967196 {"table": {"columns": [{"id": 1, "name": "desc_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "version", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "sql_instance_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "session_id", "type": {"family": "BytesFamily", "oid": 17}}, {"id": 5, "name": "crdb_region", "type": {"family": "BytesFamily", "oid": 17}}], "formatVersion": 3, "id": 4294967196, "name": "kv_session_based_leases", "nextColumnId": 6, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967197 {"table": {"columns": [{"id": 1, "name": "id", "type": {"family": "UuidFamily", "oid": 2950}}, {"id": 2, "name": "ts", "type": {"family": "DecimalFamily", "oid": 1700}}, {"id": 3, "name": "meta_type", "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "meta", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}, {"id": 5, "name": "num_spans", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "spans", "type": {"family": "BytesFamily", "oid": 17}}, {"id": 7, "name": "verified", "type": {"oid": 16}}, {"id": 8, "name": "target", "nullable": true, "type": {"family": "BytesFamily", "oid": 17}}, {"id": 9, "name": "decoded_meta", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 10, "name": "decoded_target", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 11, "name": "internal_meta", "nullable": true, "type": {"family": "JsonFamily", "oid": 3802}}, {"id": 12, "name": "num_ranges", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 13, "name": "last_updated", "nullable": true, "type": {"family": "DecimalFamily", "oid": 1700}}], "formatVersion": 3, "id": 4294967197, "name": "kv_protected_ts_records", "nextColumnId": 14, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} -4294967198 {"table": {"columns": [{"id": 1, "name": "parent_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "parent_schema_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "name", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 5, "name": "corruption", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294967198, "name": "kv_repairable_catalog_corruptions", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH data AS (SELECT ns.\"parentID\" AS parent_id, ns.\"parentSchemaID\" AS parent_schema_id, ns.name, COALESCE(ns.id, d.id) AS id, d.descriptor, crdb_internal.descriptor_with_post_deserialization_changes(d.descriptor) AS updated_descriptor, crdb_internal.repaired_descriptor(d.descriptor, (SELECT array_agg(id) AS desc_id_array FROM system.descriptor), (SELECT array_agg(id) AS job_id_array FROM system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed'))) AS repaired_descriptor FROM system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id), diag AS (SELECT *, CASE WHEN (descriptor IS NULL) AND (id != 29) THEN 'namespace' WHEN updated_descriptor != repaired_descriptor THEN 'descriptor' ELSE NULL END AS corruption FROM data) SELECT parent_id, parent_schema_id, name, id, corruption FROM diag WHERE corruption IS NOT NULL ORDER BY parent_id, parent_schema_id, name, id"}} +4294967198 {"table": {"columns": [{"id": 1, "name": "parent_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "parent_schema_id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "name", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}, {"id": 4, "name": "id", "nullable": true, "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 5, "name": "corruption", "nullable": true, "type": {"family": "StringFamily", "oid": 25}}], "formatVersion": 3, "id": 4294967198, "name": "kv_repairable_catalog_corruptions", "nextColumnId": 6, "nextConstraintId": 1, "nextMutationId": 1, "primaryIndex": {"foreignKey": {}, "geoConfig": {}, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1", "viewQuery": "WITH data AS (SELECT ns.\"parentID\" AS parent_id, ns.\"parentSchemaID\" AS parent_schema_id, ns.name, COALESCE(ns.id, d.id) AS id, d.descriptor, crdb_internal.descriptor_with_post_deserialization_changes(d.descriptor) AS updated_descriptor, crdb_internal.repaired_descriptor(d.descriptor, (SELECT array_agg(id) AS desc_id_array FROM system.descriptor), (SELECT array_agg(id) AS job_id_array FROM system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed')), (SELECT array_agg(username) AS username_array FROM (SELECT username FROM system.users UNION SELECT 'public' AS username))) AS repaired_descriptor FROM system.namespace AS ns FULL JOIN system.descriptor AS d ON ns.id = d.id), diag AS (SELECT *, CASE WHEN (descriptor IS NULL) AND (id != 29) THEN 'namespace' WHEN updated_descriptor != repaired_descriptor THEN 'descriptor' ELSE NULL END AS corruption FROM data) SELECT parent_id, parent_schema_id, name, id, corruption FROM diag WHERE corruption IS NOT NULL ORDER BY parent_id, parent_schema_id, name, id"}} 4294967199 {"table": {"columns": [{"id": 1, "name": "range_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "priority", "type": {"family": "StringFamily", "oid": 25}}, {"id": 5, "name": "log_term", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 6, "name": "log_index", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 7, "name": "tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967199, "indexes": [{"foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["range_id"], "name": "kv_flow_token_deductions_range_id_idx", "partitioning": {}, "sharded": {}, "storeColumnIds": [2, 3, 4, 5, 6, 7], "storeColumnNames": ["tenant_id", "store_id", "priority", "log_term", "log_index", "tokens"], "version": 3}], "name": "kv_flow_token_deductions", "nextColumnId": 8, "nextConstraintId": 2, "nextIndexId": 3, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967200 {"table": {"columns": [{"id": 1, "name": "range_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "total_tracked_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967200, "indexes": [{"foreignKey": {}, "geoConfig": {}, "id": 2, "interleave": {}, "keyColumnDirections": ["ASC"], "keyColumnIds": [1], "keyColumnNames": ["range_id"], "name": "kv_flow_control_handles_range_id_idx", "partitioning": {}, "sharded": {}, "storeColumnIds": [2, 3, 4], "storeColumnNames": ["tenant_id", "store_id", "total_tracked_tokens"], "version": 3}], "name": "kv_flow_control_handles", "nextColumnId": 5, "nextConstraintId": 2, "nextIndexId": 3, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} 4294967201 {"table": {"columns": [{"id": 1, "name": "tenant_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 2, "name": "store_id", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 3, "name": "available_regular_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}, {"id": 4, "name": "available_elastic_tokens", "type": {"family": "IntFamily", "oid": 20, "width": 64}}], "formatVersion": 3, "id": 4294967201, "name": "kv_flow_controller", "nextColumnId": 5, "nextConstraintId": 2, "nextIndexId": 2, "nextMutationId": 1, "primaryIndex": {"constraintId": 1, "foreignKey": {}, "geoConfig": {}, "id": 1, "interleave": {}, "partitioning": {}, "sharded": {}}, "privileges": {"ownerProto": "node", "users": [{"privileges": "32", "userProto": "public"}], "version": 3}, "replacementOf": {"time": {}}, "unexposedParentSchemaId": 4294967295, "version": "1"}} diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 35f1fb6b249d..dc295a65beaa 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -41,6 +41,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverbase" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security/password" + "github.com/cockroachdb/cockroach/pkg/security/username" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -5366,8 +5367,11 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, jobIDAlwaysValid := func(id jobspb.JobID) bool { return true } + roleAlwaysValid := func(username username.SQLUsername) bool { + return true + } ret, err := evalCtx.CatalogBuiltins.RepairedDescriptor( - ctx, []byte(s), descIDAlwaysValid, jobIDAlwaysValid, + ctx, []byte(s), descIDAlwaysValid, jobIDAlwaysValid, roleAlwaysValid, ) if err != nil { return nil, err @@ -5389,6 +5393,7 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, {Name: "descriptor", Typ: types.Bytes}, {Name: "valid_descriptor_ids", Typ: types.IntArray}, {Name: "valid_job_ids", Typ: types.IntArray}, + {Name: "valid_roles", Typ: types.StringArray}, }, ReturnType: tree.FixedReturnType(types.Bytes), Fn: func(ctx context.Context, evalCtx *eval.Context, args tree.Datums) (tree.Datum, error) { @@ -5434,8 +5439,31 @@ DO NOT USE -- USE 'CREATE VIRTUAL CLUSTER' INSTEAD`, return found } } + + roleMightExist := func(username username.SQLUsername) bool { + return true + } + if args[3] != tree.DNull { + roles, ok := tree.AsDArray(args[3]) + if !ok { + return nil, errors.Newf("expected array value, got %T", args[3]) + } + roleMap := make(map[username.SQLUsername]struct{}) + for _, roleDatum := range (*roles).Array { + role := tree.MustBeDString(roleDatum) + roleName, err := username.MakeSQLUsernameFromUserInput(string(role), username.PurposeValidation) + if err != nil { + return nil, err + } + roleMap[roleName] = struct{}{} + } + roleMightExist = func(username username.SQLUsername) bool { + _, ok := roleMap[username] + return ok + } + } ret, err := evalCtx.CatalogBuiltins.RepairedDescriptor( - ctx, []byte(s), descIDMightExist, nonTerminalJobIDMightExist, + ctx, []byte(s), descIDMightExist, nonTerminalJobIDMightExist, roleMightExist, ) if err != nil { return nil, err @@ -5479,6 +5507,12 @@ SELECT system.jobs WHERE status NOT IN ('failed', 'succeeded', 'canceled', 'revert-failed') + ), + ( SELECT + array_agg(username) as username_array FROM + (SELECT username + FROM system.users UNION + SELECT 'public' as username) ) ), true diff --git a/pkg/sql/sem/builtins/fixed_oids.go b/pkg/sql/sem/builtins/fixed_oids.go index bbb777b181fe..88eaee930f88 100644 --- a/pkg/sql/sem/builtins/fixed_oids.go +++ b/pkg/sql/sem/builtins/fixed_oids.go @@ -2419,7 +2419,7 @@ var builtinOidsArray = []string{ 2449: `st_asmvtgeom(geometry: geometry, bbox: box2d, extent: int, buffer: int) -> geometry`, 2450: `st_asmvtgeom(geometry: geometry, bbox: box2d, extent: int) -> geometry`, 2451: `st_asmvtgeom(geometry: geometry, bbox: box2d) -> geometry`, - 2452: `crdb_internal.repaired_descriptor(descriptor: bytes, valid_descriptor_ids: int[], valid_job_ids: int[]) -> bytes`, + 2452: `crdb_internal.repaired_descriptor(descriptor: bytes, valid_descriptor_ids: int[], valid_job_ids: int[], valid_roles: string[]) -> bytes`, 2453: `crdb_internal.reset_activity_tables() -> bool`, 2454: `crdb_internal.sstable_metrics(node_id: int, store_id: int, start_key: bytes, end_key: bytes) -> tuple{int AS node_id, int AS store_id, int AS level, int AS file_num, int AS approximate_span_bytes, jsonb AS metrics}`, 2455: `crdb_internal.repair_catalog_corruption(descriptor_id: int, corruption: string) -> bool`, diff --git a/pkg/sql/sem/eval/deps.go b/pkg/sql/sem/eval/deps.go index 9425bd484e5d..6a757eee09d2 100644 --- a/pkg/sql/sem/eval/deps.go +++ b/pkg/sql/sem/eval/deps.go @@ -127,12 +127,14 @@ type CatalogBuiltins interface { // puts it into a catalog.DescriptorBuilder, // calls RunPostDeserializationChanges, // calls StripDanglingBackReferences, + // calls StripNonExistentRoles, // and re-encodes it. RepairedDescriptor( ctx context.Context, encodedDescriptor []byte, descIDMightExist func(id descpb.ID) bool, nonTerminalJobIDMightExist func(id jobspb.JobID) bool, + roleExists func(username username.SQLUsername) bool, ) ([]byte, error) }