Merge #65010

65010: sql: fix Usage privilege on Tables/DBs after upgrading from 20.1 r=RichardJCai a=RichardJCai

In 20.2, I made a mistake when introducing USAGE privilege by
adding it before ZONECONFIG privilege in the bitfield.
Due to this, when updating from 20.1 to 20.2, ZONECONFIG privilege will be
interpretted as USAGE.

Fortunately, ZONECONFIG was only a valid privilege on tables and databases
while USAGE is invalid on both those objects. Due to this, whenever we encounter
USAGE on a privilege descriptor versioned 20.1 (InitialVersion) we can fix
this by removing the bit for USAGE and adding the bit for ZONECONFIG on the
privilege descriptor.

Release note: None

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

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

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

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;

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
 }
 

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