sql: fix Usage privilege on Tables/DBs after upgrading from 20.1

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

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,41 @@ 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
+	modified := false
+	// InitialVersion is for descriptors that were created in versions 20.1 and
+	// earlier.
+	if p.Version == InitialVersion {
+		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 == privilege.USAGE.Mask() {
+				// 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

pkg/sql/catalog/descpb/privilege_test.go

@@ -651,3 +651,228 @@ 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.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)
+			}
+		}
+
+	}
+
+}

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)