diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go index fdbed1d47ee4..424d394c5ff8 100644 --- a/pkg/ccl/backupccl/restore_job.go +++ b/pkg/ccl/backupccl/restore_job.go @@ -327,6 +327,8 @@ func WriteDescriptors( desc.GetID(), desc) } } + privilegeDesc := desc.GetPrivileges() + descpb.MaybeFixUsagePrivForTablesAndDBs(&privilegeDesc) wroteDBs[desc.GetID()] = desc if err := descsCol.WriteDescToBatch( ctx, false /* kvTrace */, desc.(catalog.MutableDescriptor), b, @@ -378,6 +380,8 @@ func WriteDescriptors( table.GetID(), table) } } + privilegeDesc := table.GetPrivileges() + descpb.MaybeFixUsagePrivForTablesAndDBs(&privilegeDesc) // If the table descriptor is being written to a multi-region database and // the table does not have a locality config setup, set one up here. The // table's locality config will be set to the default locality - REGIONAL diff --git a/pkg/ccl/backupccl/restore_old_versions_test.go b/pkg/ccl/backupccl/restore_old_versions_test.go index e3e51bf80f89..431c7bd51dbc 100644 --- a/pkg/ccl/backupccl/restore_old_versions_test.go +++ b/pkg/ccl/backupccl/restore_old_versions_test.go @@ -57,6 +57,7 @@ func TestRestoreOldVersions(t *testing.T) { fkRevDirs = testdataBase + "/fk-rev-history" clusterDirs = testdataBase + "/cluster" exceptionalDirs = testdataBase + "/exceptional" + privilegeDirs = testdataBase + "/privileges" ) t.Run("table-restore", func(t *testing.T) { @@ -150,6 +151,17 @@ ORDER BY object_type, object_name`, [][]string{ }) }) }) + + t.Run("zoneconfig_privilege_restore", func(t *testing.T) { + dirs, err := ioutil.ReadDir(privilegeDirs) + require.NoError(t, err) + for _, dir := range dirs { + require.True(t, dir.IsDir()) + exportDir, err := filepath.Abs(filepath.Join(privilegeDirs, dir.Name())) + require.NoError(t, err) + t.Run(dir.Name(), restoreV201ZoneconfigPrivilegeTest(exportDir)) + } + }) } func restoreOldVersionTest(exportDir string) func(t *testing.T) { @@ -182,6 +194,46 @@ func restoreOldVersionTest(exportDir string) func(t *testing.T) { } } +// restoreV201ZoneconfigPrivilegeTest checks that privilege descriptors with +// ZONECONFIG from tables and databases are correctly restored. +// The ZONECONFIG bit was overwritten to be USAGE in 20.2 onwards. +// We only need to test restoring with full cluster backup / restore as +// it is the only form of restore that restores privileges. +func restoreV201ZoneconfigPrivilegeTest(exportDir string) func(t *testing.T) { + return func(t *testing.T) { + const numAccounts = 1000 + _, _, _, tmpDir, cleanupFn := BackupRestoreTestSetup(t, MultiNode, numAccounts, InitManualReplication) + defer cleanupFn() + + _, _, sqlDB, cleanup := backupRestoreTestSetupEmpty(t, singleNode, tmpDir, + InitManualReplication, base.TestClusterArgs{}) + defer cleanup() + err := os.Symlink(exportDir, filepath.Join(tmpDir, "foo")) + require.NoError(t, err) + sqlDB.Exec(t, `RESTORE FROM $1`, LocalFoo) + testDBGrants := [][]string{ + {"test", "admin", "ALL"}, + {"test", "root", "ALL"}, + {"test", "testuser", "ZONECONFIG"}, + } + sqlDB.CheckQueryResults(t, `show grants on database test`, testDBGrants) + + testTableGrants := [][]string{ + {"test", "public", "test_table", "admin", "ALL"}, + {"test", "public", "test_table", "root", "ALL"}, + {"test", "public", "test_table", "testuser", "ZONECONFIG"}, + } + sqlDB.CheckQueryResults(t, `show grants on test.test_table`, testTableGrants) + + testTable2Grants := [][]string{ + {"test", "public", "test_table2", "admin", "ALL"}, + {"test", "public", "test_table2", "root", "ALL"}, + {"test", "public", "test_table2", "testuser", "ALL"}, + } + sqlDB.CheckQueryResults(t, `show grants on test.test_table2`, testTable2Grants) + } +} + func restoreOldVersionFKRevTest(exportDir string) func(t *testing.T) { return func(t *testing.T) { params := base.TestServerArgs{} diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/create_with_privileges.sql b/pkg/ccl/backupccl/testdata/restore_old_versions/create_with_privileges.sql new file mode 100644 index 000000000000..da39f0dede03 --- /dev/null +++ b/pkg/ccl/backupccl/testdata/restore_old_versions/create_with_privileges.sql @@ -0,0 +1,20 @@ +-- The below SQL is used to create the data that is then exported with BACKUP. +-- This should be run on a v20.1 cluster, the ZONECONFIG bit is incorrectly +-- updated to be interpreted as USAGE in versions of 20.2 and onward. +-- The restore test should ensure that ZONECONFIG stays as ZONECONFIG. + +CREATE DATABASE test; + +SET database = test; + +CREATE USER testuser; + +CREATE TABLE test_table(); + +CREATE TABLE test_table2(); + +GRANT ZONECONFIG ON DATABASE test TO testuser; + +GRANT ZONECONFIG ON test_table TO testuser; + +GRANT ALL ON test_table2 TO testuser; diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481816066.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481816066.sst new file mode 100644 index 000000000000..a9878518f097 Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481816066.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481848836.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481848836.sst new file mode 100644 index 000000000000..571ed1380a27 Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481848836.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481881604.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481881604.sst new file mode 100644 index 000000000000..fa68149d7e22 Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481881604.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481914372.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481914372.sst new file mode 100644 index 000000000000..f521799b4946 Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117481914372.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482045442.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482045442.sst new file mode 100644 index 000000000000..7b2f468a506c Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482045442.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482078210.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482078210.sst new file mode 100644 index 000000000000..400956ef6afe Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117482078210.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485420546.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485420546.sst new file mode 100644 index 000000000000..72119886c1ee Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485420546.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485879300.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485879300.sst new file mode 100644 index 000000000000..3adfb047113f Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117485879300.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486469124.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486469124.sst new file mode 100644 index 000000000000..780aa8a82a06 Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486469124.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486829572.sst b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486829572.sst new file mode 100644 index 000000000000..cad6a9ec293b Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/657900117486829572.sst differ diff --git a/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/BACKUP b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/BACKUP new file mode 100644 index 000000000000..37904764a85a Binary files /dev/null and b/pkg/ccl/backupccl/testdata/restore_old_versions/privileges/v20.1.6/BACKUP differ diff --git a/pkg/sql/catalog/dbdesc/database_desc_builder.go b/pkg/sql/catalog/dbdesc/database_desc_builder.go index af087e51c90a..840e37c5c534 100644 --- a/pkg/sql/catalog/dbdesc/database_desc_builder.go +++ b/pkg/sql/catalog/dbdesc/database_desc_builder.go @@ -59,6 +59,7 @@ func (ddb *databaseDescriptorBuilder) RunPostDeserializationChanges( // run again and mixed-version clusters always write "good" descriptors. ddb.maybeModified = protoutil.Clone(ddb.original).(*descpb.DatabaseDescriptor) descpb.MaybeFixPrivileges(ddb.maybeModified.ID, &ddb.maybeModified.Privileges) + descpb.MaybeFixUsagePrivForTablesAndDBs(&ddb.maybeModified.Privileges) return nil } diff --git a/pkg/sql/catalog/descpb/privilege.go b/pkg/sql/catalog/descpb/privilege.go index ae835d49c71c..c1a3b5c6f96c 100644 --- a/pkg/sql/catalog/descpb/privilege.go +++ b/pkg/sql/catalog/descpb/privilege.go @@ -208,6 +208,47 @@ func (p *PrivilegeDescriptor) Revoke( } } +// MaybeFixUsagePrivForTablesAndDBs fixes cases where privilege descriptors +// with ZONECONFIG were corrupted after upgrading from 20.1 to 20.2. +// USAGE was mistakenly added in the privilege bitfield above ZONECONFIG +// causing privilege descriptors with ZONECONFIG in 20.1 to have USAGE privilege +// instead of ZONECONFIG. +// Fortunately ZONECONFIG was only valid on TABLES/DB while USAGE is not valid +// on either so we know if the descriptor was corrupted. +func MaybeFixUsagePrivForTablesAndDBs(ptr **PrivilegeDescriptor) bool { + if *ptr == nil { + *ptr = &PrivilegeDescriptor{} + } + p := *ptr + + if p.Version > InitialVersion { + // InitialVersion is for descriptors that were created in versions 20.1 and + // earlier. If the privilege descriptor was created after 20.1, then we + // do not have to fix it. Furthermore privilege descriptor versions are + // currently never updated so we're guaranteed to only have this issue + // on privilege descriptors that are on "InitialVersion". + return false + } + + modified := false + for i := range p.Users { + // Users is a slice of values, we need pointers to make them mutable. + userPrivileges := &p.Users[i] + // Tables and Database should not have USAGE privilege in 20.2 onwards. + // The only reason they would have USAGE privilege is because they had + // ZoneConfig in 20.1 and upgrading to 20.2 where USAGE was added + // in the privilege bitfield where ZONECONFIG previously was. + if privilege.USAGE.Mask()&userPrivileges.Privileges != 0 { + // Remove USAGE privilege and add ZONECONFIG. The privilege was + // originally ZONECONFIG in 20.1 but got changed to USAGE in 20.2 + // due to changing the bitfield values. + userPrivileges.Privileges = (userPrivileges.Privileges - privilege.USAGE.Mask()) | privilege.ZONECONFIG.Mask() + modified = true + } + } + return modified +} + // MaybeFixPrivileges fixes the privilege descriptor if needed, including: // * adding default privileges for the "admin" role // * fixing default privileges for the "root" user diff --git a/pkg/sql/catalog/descpb/privilege_test.go b/pkg/sql/catalog/descpb/privilege_test.go index e5192c7d3f48..c69f4123aace 100644 --- a/pkg/sql/catalog/descpb/privilege_test.go +++ b/pkg/sql/catalog/descpb/privilege_test.go @@ -651,3 +651,241 @@ func TestValidateOwnership(t *testing.T) { t.Fatal(err) } } + +// TestMaybeFixUsageAndZoneConfigPrivilege checks that Table and DB descriptors +// on created on v20.1 or prior (PrivilegeDescVersion InitialVersion) with +// USAGE privilege have its privilege correctly updated. +// The bit representing USAGE privilege in 20.2 for Tables/DBs should actually +// be ZONECONFIG privilege and should be updated. +func TestMaybeFixUsageAndZoneConfigPrivilege(t *testing.T) { + + fooUser := security.MakeSQLUsernameFromPreNormalizedString("foo") + barUser := security.MakeSQLUsernameFromPreNormalizedString("bar") + bazUser := security.MakeSQLUsernameFromPreNormalizedString("baz") + + type userPrivileges map[security.SQLUsername]privilege.List + + testCases := []struct { + input userPrivileges + modified bool + output userPrivileges + objectType privilege.ObjectType + privDescVersion PrivilegeDescVersion + description string + isValid bool + }{ + // Cases for Tables and Databases. + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + privilege.Table, + InitialVersion, + "A privilege descriptor from a table created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + privilege.Database, + InitialVersion, + "A privilege descriptor from a database created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.ALL}, + }, + false, + userPrivileges{ + fooUser: privilege.List{privilege.ALL}, + }, + privilege.Table, + InitialVersion, + "ALL should stay as ALL", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + false, + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + privilege.Table, + OwnerVersion, + "A privilege descriptor from a table created in v20.2 onwards " + + "(OwnerVersion) should not be modified.", + false, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + false, + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + }, + privilege.Database, + OwnerVersion, + "A privilege descriptor from a Database created in v20.2 onwards " + + "(OwnerVersion) should not be modified.", + false, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + false, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + privilege.Table, + OwnerVersion, + "A privilege descriptor from a table created in v20.2 onwards " + + "(OwnerVersion) should not be modified.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + false, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + privilege.Database, + OwnerVersion, + "A privilege descriptor from a Database created in v20.2 onwards " + + "(OwnerVersion) should not be modified.", + true, + }, + // Fix privileges for multiple users. + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + barUser: privilege.List{privilege.USAGE, privilege.CREATE, privilege.SELECT}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + barUser: privilege.List{privilege.ZONECONFIG, privilege.CREATE, privilege.SELECT}, + }, + privilege.Table, + InitialVersion, + "A privilege descriptor from a table created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + barUser: privilege.List{privilege.USAGE, privilege.CREATE, privilege.SELECT}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + barUser: privilege.List{privilege.ZONECONFIG, privilege.CREATE, privilege.SELECT}, + }, + privilege.Database, + InitialVersion, + "A privilege descriptor from a table created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + barUser: privilege.List{privilege.USAGE, privilege.CREATE, privilege.GRANT}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + barUser: privilege.List{privilege.ZONECONFIG, privilege.CREATE, privilege.GRANT}, + }, + privilege.Database, + InitialVersion, + "A privilege descriptor from a database created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE}, + barUser: privilege.List{privilege.USAGE, privilege.CREATE, privilege.SELECT}, + bazUser: privilege.List{privilege.ALL, privilege.USAGE}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + barUser: privilege.List{privilege.ZONECONFIG, privilege.CREATE, privilege.SELECT}, + bazUser: privilege.List{privilege.ALL}, + }, + privilege.Database, + InitialVersion, + "A privilege descriptor from a table created in v20.1 or prior " + + "(InitialVersion) with USAGE should have the privilege converted to ZONECONFIG.", + true, + }, + // Test case where the privilege descriptor has ZONECONFIG and USAGE. + { + userPrivileges{ + fooUser: privilege.List{privilege.USAGE, privilege.ZONECONFIG}, + }, + true, + userPrivileges{ + fooUser: privilege.List{privilege.ZONECONFIG}, + }, + privilege.Table, + InitialVersion, + "If the descriptor has USAGE and ZONECONFIG, it should become just " + + "ZONECONFIG", + true, + }, + } + + for num, tc := range testCases { + desc := &PrivilegeDescriptor{Version: tc.privDescVersion} + for u, p := range tc.input { + desc.Grant(u, p) + } + modified := MaybeFixUsagePrivForTablesAndDBs(&desc) + + if tc.modified != modified { + t.Errorf("expected modifed to be %v, was %v", tc.modified, modified) + } + + for u, p := range tc.output { + outputUser, ok := desc.findUser(u) + if !ok { + t.Errorf("#%d: expected user %s in output, but not found (%v)\n%s", + num, u, desc.Users, tc.description, + ) + } + if a, e := privilege.ListFromBitField(outputUser.Privileges, privilege.Any), p; a.ToBitField() != e.ToBitField() { + t.Errorf("#%d: user %s: expected privileges %v, got %v\n%s", + num, u, e, a, tc.description, + ) + } + + err := privilege.ValidatePrivileges(p, tc.objectType) + if tc.isValid && err != nil { + t.Errorf("%s\n%s", err.Error(), tc.description) + } + } + + } + +} diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go index 91844111e4c0..91d017bc1a27 100644 --- a/pkg/sql/catalog/tabledesc/table_desc_builder.go +++ b/pkg/sql/catalog/tabledesc/table_desc_builder.go @@ -190,6 +190,10 @@ func maybeFillInDescriptor( // run again and mixed-version clusters always write "good" descriptors. changes.FixedPrivileges = descpb.MaybeFixPrivileges(desc.ID, &desc.Privileges) + fixedUsagePrivilege := descpb.MaybeFixUsagePrivForTablesAndDBs(&desc.Privileges) + + changes.FixedPrivileges = changes.FixedPrivileges || fixedUsagePrivilege + if dg != nil { changes.UpgradedForeignKeyRepresentation, err = maybeUpgradeForeignKeyRepresentation( ctx, dg, skipFKsWithNoMatchingTable /* skipFKsWithNoMatchingTable*/, desc)